Repository navigation
[AI-1459] SessionStart memory index: Codex CLI adapter - #395
Conversation
Wires the shared SessionStart memory subsystem into the Codex hook, the third
harness onto that foundation after Claude and Cursor.
The fetch is started BEFORE the lifecycle POST so the two overlap, then awaited
under the budget remaining at that instant (HookBudget.Remaining minus Safety,
the headroom reserved for serialization and the write) immediately before the
stdout handshake. Codex blocks on this hook's stdout, so an unreachable memory
server degrades to the minimal handshake instead of delaying the write; the
payload is fully serialized before the first byte, so a renderer fault can never
emit a partial rich object followed by a second minimal one.
Scope safety mirrors the Cursor adapter: the git root discovered from the payload
cwd is preferred, the payload cwd is the fallback, and a blank scope root skips
injection entirely rather than letting the shared resolver fall back to the hook
process's cwd and inject an unrelated repository's memories. Nested KCAP_SKIP=1
invocations already return before this path, and excluded/disabled repos are
short-circuited above it, so neither reaches the memory subsystem.
The no-fragment path deliberately keeps the existing handshake constant rather
than the shared adapter's rendering of the same envelope: both encode
{"continue":true}, but the adapter appends a trailing newline to every envelope
(as Claude and Cursor already ship), and adopting it would change the bytes Codex
receives on every no-memory SessionStart. Byte-identity there is an acceptance
criterion, so the constant wins; the adapter still owns the fragment-bearing
shape, which is the only genuinely new output.
Tests pin the byte-identical minimal handshake, the single-JSON-value contract
(no trailing document), quote/newline/control/non-BMP escaping round-trips, the
recorded newline asymmetry, and that Stop never carries memory context.
Closes AI-1459.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR Summary by QodoCodex hook: inject SessionStart memory fragment with budgeted stdout contract
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| static async Task<string?> AwaitMemoryFragmentAsync(Task<string?> task, long processStart) { | ||
| try { | ||
| var budget = HookBudget.Remaining(processStart, "session-start") - HookBudget.Safety; | ||
|
|
||
| if (budget <= TimeSpan.Zero) | ||
| return task.IsCompletedSuccessfully ? task.Result : null; | ||
|
|
||
| return await task.WaitAsync(budget); |
There was a problem hiding this comment.
2. Safety double-subtracted 🐞 Bug ≡ Correctness
CodexHookCommand subtracts HookBudget.Safety from HookBudget.Remaining(), but Remaining() already subtracts Safety internally, shrinking both the memory-fetch budget and the WaitAsync cap by an extra 1.5s. This makes the new memory injection path time out/skip more often than intended even when time remains in the hook ceiling.
Agent Prompt
### Issue description
`HookBudget.Remaining(processStart, ...)` already returns a safety-adjusted remaining budget (it subtracts `HookBudget.Safety` internally). The Codex SessionStart memory integration subtracts `HookBudget.Safety` *again* when (a) starting the memory task and (b) awaiting it, effectively reserving safety twice and prematurely expiring the memory fetch.
### Issue Context
- `HookBudget.Remaining()` is defined as `Ceiling - elapsed - Safety`.
- Codex’s memory path uses `HookBudget.Remaining(...) - HookBudget.Safety` in two places.
- Claude’s analogous logic awaits under `HookBudget.Remaining(...)` (no extra subtraction), suggesting Codex’s extra subtraction is unintended.
### Fix
- Use `HookBudget.Remaining(processStart, "session-start")` directly (no additional `- HookBudget.Safety`) for:
- the `budget` passed into `StartMemoryIndexTask(...)`
- the `budget` used in `AwaitMemoryFragmentAsync(...)`
- If you truly need a *second* reserve distinct from `HookBudget.Safety`, introduce a separate constant (e.g., `SerializationReserve`) instead of reusing `HookBudget.Safety`.
### Fix Focus Areas
- src/Capacitor.Cli/Commands/CodexHookCommand.cs[162-173]
- src/Capacitor.Cli/Commands/CodexHookCommand.cs[365-372]
- src/Capacitor.Cli/Commands/HookBudget.cs[10-21]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
/api/memories/index is bearer-authenticated, and the shared provider hands a rejected bearer back to the client factory so it can mint a refreshed client. The production fallback was a bare new HttpClient(), so BOTH the initial request and the 401 refresh went out anonymous: the provider recorded a retryable failure and Codex silently received no memory context on every authenticated deployment. Replaced with the same auth-aware refresh factory ClaudeHookCommand uses, named (DefaultMemoryClientFactory) so it is assertable, and disposeClients now only disposes clients we created — an injected factory's client is its caller's, and may be handed back again on the refresh call. Guarded by a source-level test scoped to the factory body: a credential-attaching assertion would need KCAP_CONFIG_DIR bound before PathHelpers' static init, which a parallel shared assembly cannot guarantee. Mutation-tested — reintroducing the bare client fails the guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ment Codex (Qodo) Two Qodo findings. 1. Fail-open regression introduced by the auth fix. The authenticated-client helper funnels through EnsureAbsolute, which prints a hint and calls Environment.Exit(2) on a URL it cannot accept. From SessionStart that would kill the hook BEFORE the stdout handshake, so Codex would receive no output at all and reject the session — strictly worse than skipping an optional memory fragment. CanAttemptMemoryInjection now rejects a blank/unacceptable URL before any auth discovery, mirroring PostBestEffortAsync's guard. Tested via the predicate rather than the exit, since tripping Environment.Exit would take the test host down. 2. README documented Codex as not wired for the SessionStart team-memory index. Updated the capability-matrix row and the two prose mentions (harness list and the per-vendor envelope field), per the repo rule that user-facing CLI surface changes update README.md in the same PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
NO FINDINGS |
Review close-outCodex review flow: clean (2 rounds on the original + 2 on the delta). Qodo: both findings addressed. Findings fixed
Correction to this PR's original descriptionThe description claimed fail-open as an end-to-end property. That is accurate only for the memory branch. A codex delta review correctly showed the SessionStart handshake can still be lost on an unacceptable base URL, because That hole is pre-existing — verified on VerificationNew |
1 participant
Wires the shared SessionStart memory subsystem into the Codex hook — the third harness onto that foundation after Claude and Cursor.
What the foundation already gave us
Worth stating up front, because it shrank this change a lot: the shared subsystem already had a
SessionStartHarness.Codexoutput adapter rendering exactly the required envelope, and its orchestrator already implements the monotonic budget (aRemaining()recomputed before every await), the lease state machine, and the fail-open catch-all. So this PR is the call site, not a renderer or a budget mechanism.Ordering
The fetch starts before the lifecycle POST so the two overlap, then is awaited under the budget remaining at that instant —
HookBudget.Remaining(processStart, "session-start") - HookBudget.Safety, the headroom reserved for serialization plus the write — immediately before the stdout handshake.Codex blocks on this hook's stdout, so:
Scope safety
Mirrors the Cursor adapter's guard: the git root discovered from the payload
cwdis preferred, the payloadcwdis the fallback, and a blank scope root skips injection entirely rather than letting the shared resolver fall back to the hook process's cwd and inject an unrelated repository's memories.Eligibility comes for free from existing structure: nested
KCAP_SKIP=1invocations return before this path, and excluded/disabled repos short-circuit above it — neither reaches the memory subsystem. Codex's SessionStart payload carries no lifecyclesource, so the reason is reported asNew; re-injection on a resume of the same session id is prevented by the shared lease keyed on (harness, session id) rather than by a signal we cannot observe.One deliberate asymmetry (please sanity-check this)
The no-fragment path keeps the pre-existing handshake constant instead of the shared adapter's rendering of the same envelope. Both encode
{"continue":true}, but the adapter appends a trailing newline to every envelope it renders (json + "\n"— which Claude and Cursor already ship). Adopting it here would change the bytes Codex receives on every no-memory SessionStart (opt-out, exclusion, provider failure, budget exhaustion) for no gain, and byte-identity on that path is an acceptance criterion. So the constant wins for null; the adapter owns the fragment-bearing shape, which is the only genuinely new output. A test pins the asymmetry so it is a recorded decision, not an accident.Tests
CodexSessionStartMemoryTests(8, new) pins:continue+hookSpecificOutput.additionalContext;Verified locally: CodexSessionStartMemoryTests 8/8 · CodexStdoutContractTests 2/2 (ordering contract intact) · ClaudeHookCommandTests 35/35 · CursorHookCommandTests 38/38 · CodexHooksInstallerTests 8/8.
check-linear-ids.shclean. AOT publish generates native code with no IL/trim warnings.Spec-vs-code note
The issue's rev-2 spec asserted that lifecycle POST and repo enrichment should be post-stdout. In the code as it stands they already run before the handshake write, and this PR did not restructure that — it inserts the memory fetch into the existing ordering rather than rewriting Codex's hook sequence, which would be a larger and riskier change than this issue's scope.
Closes AI-1459.