Repository navigation
[AI-1463] SessionStart memory index: Gemini CLI adapter - #414
Merged
Merged
Conversation
Sixth harness into the existing foundation. Most of it already exists — the GeminiMemoryEnvelope, its AOT JSON context and the identity mapping were built during the Claude baseline; only the GeminiHookCommand call site is missing. Verified rather than assumed: the speculatively-written envelope shape is correct. A static scan of the installed @google/gemini-cli shows its own code references hookSpecificOutput (150 hits), additionalContext (96) and hookEventName (15), so no new wire format is being invented. The one genuinely new risk is that Gemini treats hook stdout as a JSON decision channel (continue / decision / stopReason / systemMessage / suppressOutput) and this dispatcher currently emits NOTHING. Turning that into a JSON emitter could, if the contract is wrong, block a user's session at startup — so the live cert is mandatory, not optional, and fail-open must be byte-identical to today (zero bytes, not an empty object). Also records two known traps: hookProcessStart is not currently threaded to Gemini (same omission fixed for Codex/Copilot/Kiro), and the Codex early-return bug where a failed POST skipped the stdout handshake. Two items deliberately left open for the reviewer: whether 'clear' should re-inject, and whether to add a Copilot-style commit gate given that Gemini's resume offers a natural retry.
…ement
The reviewer refused to accept the decision-channel contract on field-name hit
counts, and was right to. Read Gemini's actual hook runner out of the installed
@google/gemini-cli bundle instead:
- Blocking requires an explicit decision: isBlockingDecision() returns
decision === "block" || "deny". A hookSpecificOutput-only payload cannot block.
- additionalContext is consumed via a string-typed accessor.
- Malformed/truncated stdout is FAIL-OPEN: a JSON.parse failure degrades to
convertPlainTextToHookOutput, never a synthesised block.
- stdout is parsed UNCONDITIONALLY: `textToParse = stdout.trim() || stderr.trim()`
inside child.on("close") with no exit-code gate; the exit code only sets
`success`.
That last fact settles finding 1 (blocking): the commit-gate premise was that
stdout might be discarded on a non-zero exit. It is not, so option (a) — no gate.
The reviewer's own objection also kills (b) independently: it relied on a later
resume, and a startup failure may prevent the session ever becoming resumable.
Finding 2 (clear): adopted the reviewer's position over mine. Suppressing `clear`
is a silent feature loss, because clear destroys the context holding the
injection. Lease key gains a durable context generation — startup injects, resume
suppresses, each clear injects again — with acceptance over the SEQUENCES
(startup->resume, startup->clear, clear->clear), since a state-only test passes
with an always-inject counter.
Finding 3 (partial write): the "zero bytes on any failure" claim was too strong
and the shown catch didn't cover the write at all. Split into render-then-single-
write with a separate non-fatal catch that cannot alter the exit code, plus tests
for a writer throwing before and after a partial write. Residual risk is now
bounded and verified rather than asserted, via the fail-open finding above.
NEW HAZARD found while verifying, recorded as a follow-up: because empty stdout
falls back to stderr, Gemini already parses kcap's stderr diagnostics as hook
output on every failed-POST Gemini session. Benign today, but it makes writing
stdout on the failed-POST path protective rather than merely harmless.
Finding 4 (cert): explicit snapshot-and-restore including the unset state,
positive control asserts opt-out is disabled so a leaked true can't pass it
vacuously, unconditional nonce archive.
Finding 5 (version): minimum 0.53.0, cert asserts the exact binary version, and
on older/unknown versions still emit — the measured fail-open makes a wrong guess
benign, whereas a version gate would trade that for silent feature loss.
…, stderr in scope Finding 1 (blocking): the reviewer refuted my reasoning twice over and was right both times. Parsing is not consuming — a downstream success gate would be semantically identical to discarding stdout for this feature, since the model gets no index while option (a) has permanently completed the lease. And my "resume may never occur" argument was backwards: if no resume occurs releasing is harmless, if it does occur releasing is what permits recovery, so the asymmetry favours (b). The design is now CONDITIONAL on the real-Gemini non-zero-exit test result, with a three-way table, and the implementation must leave the release path behind a seam so switching is wiring rather than redesign. The issue ships on what the test returns, not on the test existing. Finding 2: replaced the generation table with an atomic contract — keyed by (harness, normalizedSessionId), ReadOrInitGeneration on startup/resume, and a single atomic IncrementAndReserve on clear, because splitting increment from lease acquisition admits the exact race the reviewer described (a resume landing between them acquires the new generation and injects while the causing clear stays silent). Fault behaviour specified for process death mid-transition, provider failure, duplicate clear delivery and SessionEnd cleanup, with concurrency/fault tests required rather than the three sequential happy paths. Also corrected the file scope, which was self-contradictory once the generation rule was adopted: this is not a two-file change and it DOES touch the foundation (lease store + orchestrator). The generation is a lease-store concept, not an envelope one, so touching the foundation says nothing about the envelope — and a default generation of 0 keeping all five merged adapters byte-identical is now a required regression assertion. Finding 3: brought the stderr hazard IN SCOPE for SessionStart. The adapter claims its failure paths are inert; measurement proves empty stdout is not, because those are precisely the paths that write stderr. Worst case is the reviewer's: with memory injection opted OUT, a failed POST can still inject kcap text — including server/auth detail — into model-visible context. So SessionStart now always emits a valid allow/no-context JSON result rather than zero bytes, deliberately diverging from the other five adapters for a Gemini-specific runner reason. SessionEnd/Notification stay a follow-up. Finding 4: dropped the "benign degradation" rationale as over-extrapolated from a single 0.53.0 observation. Below 0.53.0 is stated as unsupported rather than safe; newer versions are an accepted, documented compatibility risk mitigated by the cert asserting its version.
Round-3 findings 1 and 2 pushed on duplicate-clear identity and lease-key upgrade
compatibility, and following them landed somewhere better than answering them:
the clear re-injection work does not belong in this issue at all.
Verified: ClaudeHookCommand lets `clear` fall through to SessionLifecycleReason.New
and there is no Clear reason in the contracts, so Claude has the identical silent
loss. The mechanism to fix it — a durable atomic generation in the SHARED lease
store, a stable delivery id, and a persisted-key migration — is foundation-wide,
and the migration risks re-injection across all five merged adapters. Split to
AI-1617 (Todo, AI-1456). AI-1463 now matches the other five: the lease suppresses
clear, documented as a known gap rather than claimed correct.
Consequence: the file scope is genuinely small again — no lease-store or
orchestrator change, so no migration risk rides along with an adapter PR. Added an
explicit tripwire: if a lease-store change becomes necessary, AI-1617 scope has
leaked back in.
Round-3 finding 3 (in scope, now specified exactly):
- The no-context payload is `{"continue":true}`. Chosen over `{}` (parses but
asserts nothing, relying on defaults staying benign — the exact assumption §3.1
had to go verify) and over an additionalContext-less hookSpecificOutput (a shape
needing separate verification for no benefit). Carrying no hookSpecificOutput key
makes getAdditionalContext() short-circuit on its own `in` guard.
- Path scope has no exceptions: every RECOGNISED SessionStart writes exactly one
JSON object, including the disabled-session and excluded-path early returns,
because Program.cs drains the spool before dispatch and can write stderr there.
Unrecognised input (bad JSON, missing event name, non-GUID session id) keeps the
silent return — Gemini has no hook result to attribute.
Round-3 finding 4: added a Definition of Done requiring the live result and
version written back, the unselected branch DELETED, branch-specific lease
assertions, and escalation rather than a passing cert if the turn aborts.
Also split AI-1618 for the stderr shadowing on SessionEnd/Notification.
Sixth harness. The envelope, its AOT context and the identity mapping already
existed from the Claude baseline; this adds the call site, the budget origin, and
the Gemini-specific output invariant.
The divergence worth knowing about: every other adapter writes zero bytes on the
no-memory path, and Gemini must not. Its hook runner selects the text to parse as
stdout.trim() || stderr.trim(), so silent stdout makes it read kcap's STDERR --
where failed-POST and auth-lapse diagnostics go -- as the hook's output. Worst
case, with memory injection opted OUT a failed POST could still put kcap text into
the model's context. So a recognised SessionStart now writes exactly one JSON
object on every returning path: the memory envelope when there is a fragment, an
explicit {"continue":true} otherwise. Unrecognised input (bad JSON, no event
name, non-GUID session id) stays silent -- Gemini has no hook result to attribute
to it.
Also threads hookProcessStart, which was missing for Gemini alone (the same
omission fixed earlier for Codex, Copilot and Kiro), and writes the hook result
BEFORE the failed-POST return so the Codex early-return defect is not
reintroduced.
clear maps to New exactly as it does for Claude, so the lease suppresses
re-injection after a context reset. That is a known foundation-wide gap owned by
its own issue, pinned in a test so it stays deliberate.
Load-bearing rather than optional: Gemini's hook stdout is a decision channel, so a green unit suite proves the bytes we emit, not that Gemini accepts them. The positive case asserts the turn COMPLETES (exit 0) as well as reproducing the nonce — a harness masking a failed invocation would otherwise look like a pass. Cleanup is explicit, not a [NotInParallel] side effect: snapshot before creating anything, restore in finally including the unset state, and the positive control asserts opt-out is disabled so a leaked true from an earlier failed run cannot make it pass vacuously. The archive is nested inside its own finally so a throwing restore cannot skip it. Records the exact gemini --version: a cert passing against an unknown build is how the earlier memory-cert failures were misdiagnosed as a code defect when the real cause was a stale installed binary.
PR Summary by QodoGemini CLI SessionStart memory index adapter (stdout JSON handshake)
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Both P1s were real, and the first showed the SPEC's exclusion list was wrong. 1. A SessionStart whose session_id was missing or not a GUID returned silently: isSessionStart was computed after the session-id validation. The event is recognisable from hook_event_name alone, and Gemini reads our stdout whatever we make of the rest of the payload, so that path re-exposed the stdout||stderr fallback. Recognition moved up to immediately after the event name. Only truly unrecognisable input stays silent now: unparseable stdin and a missing/blank hook_event_name. 2. WriteSessionStartOutput had two ordinary zero-byte returns — a render throw and an empty render — both reinstating the exact problem the helper exists to prevent. Restructured to start from the allow payload and only upgrade to the envelope when rendering genuinely succeeds and is non-empty. The allow payload is now a const literal rather than a serialized record, and the GeminiAllowEnvelope type is removed. Serializing it added a throw path to the one value every failure path degrades to — the value that must be impossible to fail to produce. Adds the Handle-level tests the reviewer asked for: the writer-level tests call WriteSessionStartOutput directly and structurally could not catch an early return in Handle. Both directions covered — unusable session id still emits exactly one object; unrecognisable input stays silent.
The important one is a real behavioural bug that the codex round could not see, because the shared helper was in omitted unchanged code: - I hand-rolled LifecycleReasonFor defaulting unrecognised sources to New, while SessionStartMemoryHookSupport.ReasonFor already exists, is used by Copilot, and maps unknown -> Unknown precisely so the lifecycle policy suppresses injection. Its own doc says why: 'inventing a reason would invent an injection decision'. My version would have injected on an unverified source AND spent the once-per-session lease on it. Local mapper deleted; the shared one is used. Note this also moves from New to Unknown — same observable outcome (no re-injection after a context reset) but suppressed by the POLICY rather than incidentally by the lease. My own spec said 'reuse Claude's, do not invent one' and I then invented one. - The file-level remarks still claimed the dispatcher emits nothing on stdout, which this change makes false. Rewritten to state the actual contract including the stdout||stderr reason and the SessionEnd/Notification exposure. - ThrowingWriter never wrote a partial payload: it threw before writing on the first call, so the advertised mid-payload case was untested. It now writes N characters and then throws, and the test asserts the bytes actually written so neither case can pass by never throwing. - Replaced raw JsonElement.ValueKind checks with the project's Obj/Str helpers. That also made the envelope assertions structural rather than substring-based, which is strictly stronger: substring matching would pass on a malformed document that merely contained the right characters.
…ision Review round 2 (P2), and it confirmed my own lean. The mapper test called ReasonFor directly, so it would stay green if this call site reintroduced a local mapper — the exact regression that had just occurred. LifecycleFor extracts the SessionMemoryLifecycle the adapter actually hands the orchestrator, and the new test runs it through the real SessionStartMemoryLifecyclePolicy.Decide. Asserting the DECISION rather than mocking I/O is deliberate and also answers what the reviewer could not assess: GetFragmentAsync calls Decide FIRST, before TryBeginAsync and before the provider fetch, so RetryLaterNoCommit proves no lease is acquired and none is spent. That gets the real property — suppressed without burning the session's one injection — deterministically, with no filesystem or HTTP. startup/resume are the positive control: without them asserting EligibleOneShot, 'clear is suppressed' would prove nothing, since a broken LifecycleFor that suppressed everything would pass. Also pins the rest of the record (top-level, authoritative, non-repeating) so a future edit cannot silently give Gemini Kiro's per-turn callback shape, and fixes an orphaned doc comment plus a stale cref to the deleted GeminiAllowEnvelope.
…ini-memory # Conflicts: # src/Capacitor.Cli/Commands/GeminiHookCommand.cs
Collaborator
Author
Review outcome + one blocked DoD itemGates: codex spec-review 4 rounds → clean · codex code-review 2 flows → clean (the second because a post-clean behavioural fix invalidated the first verdict — a clean verdict is pinned to a SHA, not a PR) · Qodo 4 findings, all addressed. Notable fixes after the initial push
Mutation ledger — every guard proven to fail when removed: zero-bytes-on-null → 1 · remove write catch → 2 · silent on unusable session id → 3 · Merged
|
realtonyyoung
added a commit
that referenced
this pull request
Jul 31, 2026
* test(gemini): finalise the SessionStart memory certification The adapter merged (#414) with all four of its definition-of-done items still open. This runs the measurement they were gating on and writes the result back into the spec. §3.3 is settled live against gemini 0.53.0. A WireMock instance proxied the whole API to the real server and failed only POST /hooks/session-start/gemini, so the index fetch on the same base URL still succeeded: the hook exited 1 and wrote 558 bytes of additionalContext, the turn completed (exit 0), and the model reproduced the nonce. That is row 1 of the decision table — option (a), no commit gate. Option (b) is withdrawn rather than left standing as an alternative. Two harness defects the first live run exposed. Neither is a product defect; both made the cert lie, which is worse than a cert that fails. Every cert runs in a freshly created throwaway worktree, which 0.53.0 refuses as untrusted — exit 55, before any model call. The positive case failed honestly on it. The negative control PASSED VACUOUSLY: it asserted only that the nonce was absent, and a turn that never ran trivially contains no nonce. Now passes --skip-trust, and the negative control asserts turn completion too. Process.Start resolves a separator-free filename against the working directory before PATH, and this assembly's working directory holds a kcap copied there by the Capacitor.Cli project reference. So RecordCertEnvironmentAsync — the method that exists to stop a stale-binary misdiagnosis — recorded the test build while its own sibling `which kcap` line reported the PATH build the hook would actually run. Resolution is now explicit and pure (PATH and platform passed, not probed), used by both process call sites, with hermetic mutation-proven coverage. New coverage: GeminiMemoryIndexLiveCertTests gains the non-zero-exit case (gated). It asserts the two links a live turn cannot prove on its own — that the same binary against the same proxy exits non-zero with parseable additionalContext, and that the proxy really failed a session-start POST during the turn. GeminiSessionStartHandshakeOnPostFailureTests is the integration half the test plan called for, and carries the branch-specific lease assertion option (a) requires: the failed-POST invocation completes the lease, and a later resume emits the bare allow object with no context. Mutation-proven — resuming a different session id re-emits the index and fails the test. Also pinned, because it cost a wrong conclusion: session_id is validated with Guid.TryParse, and a failure takes the same emit-and-return-0 path as a suppressed session. A decorated id produces a plausible exit 0 with the allow object and no HTTP traffic at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(gemini cert): close the review's five findings Codex round 1 on the finalisation PR. Every one was real. The exit code is now read from the hook GEMINI ran, not from a stand-in. The first draft proved non-zero exit with a separate direct invocation and inferred the rest — a substitution, not a proof: every assertion could have held while Gemini's own hook returned 0 through some session- or source-dependent branch. A recording shim named kcap is prepended to the PATH Gemini inherits, and the test asserts that the invocation whose additionalContext carried the nonce is the one that exited non-zero. Same process, both facts. That shim promptly found its own trap, which is now pinned in the spec: the same PATH entry is used by the four long-lived `kcap mcp <server>` stdio servers Gemini launches, and buffering a stdio JSON-RPC server's stdout traps its handshake until it exits, which it never does. Every run hung at exactly the 120s harness timeout with four wrapped mcp processes alive and zero HTTP traffic. The shim now execs anything that is not `kcap hook`. The 400-count assertion matched "a 400 at that path", which an upstream 400 relayed by the catch-all proxy would also satisfy. It now matches the forced mapping's own response-body marker. ResolveOnPath selected on File.Exists, which is not the test a shell applies: `which` and execvp skip a non-executable match and keep walking. It could therefore have recreated the very disagreement it was added to remove. Now checks the execute bits, with two mutation-proven tests (an earlier non-executable match and an executable-only-match case) and PathProbe gained an `executable` switch. Dispose in the integration test called _server.Stop() before restoring the developer's real config, so a throwing shutdown would leave the active profile pointed at a dead WireMock URL. Restoration is now first and unconditional, with the rest in a finally. Field declaration order also moved the config snapshot above the server start, so a throwing read cannot leak a server the never-completed constructor would never dispose. Nonce-memory setup: the worktree is deleted if the save throws, and the save-failure message now names the slug — that is the one leak the caller cannot clean up, since archiving needs an id the failed call never returned. The AdditionalContextOf(...).IsNotNull() weak assertion is gone with the direct-invocation code it belonged to; the shim path asserts the nonce itself. Re-measured after the change: three live cert cases green against gemini 0.53.0, and the cert now records the PATH binary (+998ec34) rather than the test build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(gemini cert): close the review's round-2 findings The nonce-delivering invocation is now correlated with the forced POST failure by SESSION ID, not by a turn-global "some 400 happened". Round 2 was right that with two hook invocations, one could carry the nonce and exit non-zero for an unrelated reason while the other hit the forced mapping, and every assertion would still pass. The shim now also records each invocation's stdin, and the test asserts that each delivering invocation's session id is among the ids the forced mapping rejected. Mutation-proven: dropping the dash-normalisation makes the ids stop matching and fails exactly that assertion. The recorded exit code is nullable rather than sentinel-defaulted. int.MinValue is != 0, so an unreadable or half-written exit record satisfied the very property under test — a false-pass path in the certification link. A parse failure now fails the assertion. PATH resolution asks the kernel: access(path, X_OK) via libc, not "any execute bit is set". The old rationale — over-accepting can only fall back to the platform's error — was simply wrong, because the resolver returns an ABSOLUTE path and there is no fallback to a later PATH entry. A file that is owner-executable but owned by someone else, or on a noexec mount, would have been selected and would have failed to launch where a shell would have kept walking. File.Exists stays as the first gate: access(X_OK) succeeds on a DIRECTORY, so without it a directory named kcap could shadow the binary. The partial-save memory leak is recovered rather than merely reported. If save_memory creates a memory but its response yields no id, the harness now looks the memory up by its unique nonce-derived slug and archives it, and says in the exception whether the index was left clean. Naming the slug made the leak diagnosable; this makes it not happen. Re-measured end to end after the changes: three live cert cases green against gemini 0.53.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(gemini cert): close the review's round-3 findings Per-invocation evidence, at last. Correlating on session id was still not enough and round 3 was right about why: a session id is REUSED across invocations — `startup` and `resume` share one, which the integration test in this very PR exercises — so the two-invocation counterexample survives with equal ids. The shim now records each invocation's stderr, and the assertion is that the invocation which delivered the nonce wrote the failed-POST diagnostic (`… session-start/gemini: HTTP 400`) to its OWN stderr. The proxy's marker assertion stays, doing the one job stderr cannot: proving that 400 came from this test's mapping rather than from upstream. Neither implies the other. Mutation-proven: pointing the diagnostic at a route that does not fail here fails exactly that assertion. Stdin recording is dropped again — it only ever existed to support the session-id correlation it has now replaced. Recovery-by-slug is polled, not asked once. A memory that WAS created may not be searchable the instant after, and a transient search failure looks identical to absence; a single lookup would report "nothing there" about a memory that surfaces a second later and pollutes every later cert. Bounded retries now, and the result distinguishes confirmed-absent from could-not-tell — only the first means the index is clean, and the exception says which. Unconfirmed cleanup is a gate rather than a log line. An unconfirmed archive, a throwing archive, or an unrecoverable save now marks the run, and SkipUnlessLiveGateReady refuses to START any further live case in that process. A stale nonce makes a later positive case pass on the previous run's evidence, which is the one failure a cert must never produce, and a stderr line the operator may not read is not a sufficient response to it. The check sits after the skips, so CI still skips rather than throws. Re-measured after the changes: three live cert cases green against gemini 0.53.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(gemini cert): close the review's round-4 findings A cleanup failure now fails the RUN, not just the next case. The dirty mark was only ever read prospectively, so a failure in the last case — or the only one — was observed by nobody and the run reported green while leaving its nonce in the real index. An [After(Assembly)] teardown now fails on the same mark. Both gates are kept: one stops a later case starting against a dirty index, the other stops the run claiming success after leaving it dirty. ArchiveMemoryAsync returns whether the archive was CONFIRMED. The recovery path previously said "has been archived, so the index is clean" even when the archive had merely been logged and marked — asserting the very thing that had just failed. An empty search result is no longer treated as absence. The code said in one breath that the read model may lag and in the next that three empty polls prove nothing was created; no maximum lag is published, so that cannot follow. The classification was weaker still than it looked: one successful empty attempt followed by two failures counted as "confirmed absent". There is now no confirmed-absent branch at all — anything short of "found and archived, confirmed" is dirty. Recovery archives EVERY exact-slug match, accumulated across polls, rather than the first. Slug uniqueness is not something this harness can verify, and the property required is that no memory carrying this run's nonce remains. Re-measured: three live cert cases green against gemini 0.53.0, harness 30/30, integration 1/1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(gemini cert): close the review's round-5 findings A throw from the save CALL is now swept and marked, not just an unusable response. Round 5 was right: the wrapper only ever inspected a returned frame, so a timed-out child or a lost response exited before the sweep and before the dirty mark — the current case failed, but the assembly guard saw null and the run could finish clean while a nonce sat in the real index. A throw does not mean the server declined the create, so it now takes the same path as an unparseable one. An ambiguous save marks the run dirty UNCONDITIONALLY, even when every observed match was archived. Archiving what a search returned proves only that the matches visible during a finite polling window are gone; slugs are unique per pool but service-enforced rather than database-constrained, so a duplicate is not excluded by construction, and with no published projection lag a second memory can surface after the last poll. "Everything I could see is archived" is weaker than "nothing carrying this nonce remains", and only the second would justify calling the index clean. The sweep mitigates; the mark tells the truth. Re-measured: three live cert cases green against gemini 0.53.0, harness 30/30. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(gemini cert): an unresolvable command throws instead of falling back Qodo's finding, and one the code-review flow did not reach: returning the bare name on an unresolved lookup reintroduced the exact bug the resolver exists to prevent. The comment claimed it let "Process.Start produce its own, clearer error" — but Process.Start consults the WORKING DIRECTORY before PATH, and this assembly's output folder holds a `kcap` copied there by the project reference. So on the one command that matters, the fallback produced no error at all: it silently ran the test build, on precisely the path where resolution had already failed. Now throws FileNotFoundException naming the PATH it searched. The two affected tests assert the throw rather than the passthrough. Live cert 3/3, harness 30/30. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(gemini cert): don't break Windows CI with Unix file modes Self-inflicted, caught by CI: `File.SetUnixFileMode` throws PlatformNotSupportedException on Windows, and PathProbe called it unconditionally — 8 failures on windows-latest. The Unix-semantics cases are now skipped there rather than adapted. The branch under test is the one guarded by `isWindows: false`; it asks the kernel access(X_OK) and its probes need real execute bits, none of which Windows can provide, so adapting them would test a different resolver. The isWindows: true passthrough still runs everywhere and touches no file mode. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes AI-1463. Sixth harness into the SessionStart memory-index foundation.
Spec (in this PR) went through four codex spec-review rounds to clean —
docs/superpowers/specs/2026-07-30-ai1463-gemini-memory-adapter-design.md.Most of this already existed
The
GeminiMemoryEnvelope, its AOT JSON context and the identity mapping were built during the Claude baseline. Only the call site was missing: of the six harnesses, only Gemini's hook never invoked the orchestrator.The envelope shape was verified rather than trusted, because it had been written speculatively before any adapter consumed it. Gemini's own bundle references
hookSpecificOutput(150),additionalContext(96) andhookEventName(15) — it genuinely parses the Claude-shaped envelope, so no new wire format is invented here.The one thing that makes Gemini different
Gemini's hook stdout is a JSON decision channel, and this dispatcher previously emitted nothing at all. Review rightly refused to accept the contract on field-name counts, so it was read out of Gemini's hook runner:
isBlockingDecision() { return this.decision === "block" || this.decision === "deny" }— our envelope leavesdecisionundefined, so it cannot blockadditionalContextis string-typedgetAdditionalContext()guards ontypeof context !== "string"JSON.parsefailure degrades toconvertPlainTextToHookOutput, never a synthesised blockchild.on("close", …)parses with no exit-code gate; the exit code only setssuccessThe hazard that changed the design
That same line reads:
When stdout is empty, Gemini parses the hook's STDERR as its output — and kcap writes diagnostics there (failed lifecycle POSTs, the auth-lapse notice). This already happens today on every failed-POST Gemini session.
The worst case is specific and bad: with memory injection opted OUT, a failed POST could still put kcap text — including server/auth detail — into the model's context.
So this PR inverts the usual fail-open rule for Gemini: a recognised
SessionStartnow writes exactly one JSON object on every returning path — the memory envelope when there is a fragment, an explicit{"continue":true}otherwise — so stdout wins the||and shadows stderr. That is a deliberate divergence from the other five adapters, documented at the call site so it is not "harmonised" away.{"continue":true}was chosen over{}(parses but asserts nothing, relying on defaults staying benign — the exact assumption that had to be verified) and over anadditionalContext-lesshookSpecificOutput(a shape needing separate verification for no benefit). Carrying nohookSpecificOutputkey makesgetAdditionalContext()short-circuit on its owninguard.Path scope has no exceptions. The disabled-session and excluded-path fast paths emit too — they are not stderr-free, since
Program.csdrains the spool before dispatch. Unrecognised input (bad JSON, missing event name, non-GUID session id) stays silent: Gemini has no hook result to attribute to it.Two known traps, both avoided
hookProcessStartwas missing for Gemini alone — the identical omission fixed earlier for Codex, Copilot and Kiro. Threaded.HookBudget.Remainingalready subtracts its own safety margin, so it is not subtracted again (double-subtraction was a real Copilot defect).return 1on a failed POST skipped the stdout handshake — is explicitly not reintroduced: the hook result is written before that return.Scope deliberately split out
Review found that suppressing
source: clearis a silent feature loss (cleardestroys the context holding the injection). Chasing it established it is foundation-wide, not a Gemini bug:ClaudeHookCommandletsclearfall toSessionLifecycleReason.Newand there is noClearreason in the contracts at all — Claude has the identical loss.Fixing it needs a durable atomic generation in the shared lease store, a stable delivery id, and a persisted-key migration that risks re-injection across all five merged adapters. That does not belong in an adapter PR, so:
SessionEnd/Notification(theSessionStarthalf is fixed here).Consequence: no lease-store or orchestrator change in this PR, so no migration risk rides along.
clearmaps toNewexactly as Claude does — pinned in a test so the gap stays deliberate and visible.Testing
GeminiSessionStartMemoryTests— 17 passing, and the three load-bearing guards are mutation-proven:clear→Resumeinstead ofNewAlso covers: neither payload can carry
decision/stopReason/continue:false; fragments are JSON-escaped and round-trip exactly (inverse of Kiro's raw-text contract); every payload is exactly one parseable JSON object; a writer throwing before or mid-payload does not propagate.GeminiMemoryIndexLiveCertTests(env-gated,KCAP_GEMINI_MEMORY_LIVE=1) asserts turn completion as well as nonce reproduction, snapshots/restores the opt-out including the unset state, asserts the positive control runs with opt-out disabled, and records the exactgemini --version.Known-not-done
Per the spec's Definition of Done, the §3.3 commit-gate choice is conditional on the live non-zero-exit result and must be finalised — the unselected branch deleted and a branch-specific lease assertion added — before merge. Current code implements option (a) (no gate), which the measured unconditional-parse behaviour supports; the cert run is what confirms it.