Repository navigation
[AI-1463] Finalise the Gemini SessionStart memory certification - #416
Merged
Merged
Conversation
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>
PR Summary by QodoFinalize Gemini SessionStart memory live cert and harden test harness
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Overly verbose test comments
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+101
to
+122
| /// <summary> | ||
| /// The third case §3.3 owes: a SessionStart whose lifecycle POST genuinely fails, so the hook exits | ||
| /// non-zero while still emitting <c>additionalContext</c> — does the model still receive it? | ||
| /// | ||
| /// <para>§3.3 chose "no commit gate" on a reading of Gemini's hook runner (<c>stdout.trim() || | ||
| /// stderr.trim()</c>, parsed with no exit-code gate). That is a source reading, not a behavioural | ||
| /// one, and the spec's definition of done requires settling it live. This test is that settlement: | ||
| /// if Gemini ever did discard a failing hook's stdout, the lease would be spent on an injection the | ||
| /// model never saw, and the decision would have to flip to a commit gate.</para> | ||
| /// | ||
| /// <para>The failure is REAL, not simulated: a WireMock instance proxies the whole API to the | ||
| /// configured server and fails exactly <c>POST /hooks/session-start/gemini</c>. The index fetch on | ||
| /// the very same base URL is proxied through untouched, which is the only arrangement that produces | ||
| /// the shape under test — a hook that fetched a real index and then failed its POST.</para> | ||
| /// | ||
| /// <para><b>Two links, asserted separately, because the live turn alone cannot prove either.</b> | ||
| /// Gemini's exit code says nothing about its hook's exit code, so (1) the proxy is asserted to have | ||
| /// actually failed a session-start POST during the run, and (2) the same binary is driven directly | ||
| /// against the same proxy to prove that a failed POST does exit non-zero AND still writes parseable | ||
| /// <c>additionalContext</c>. Without (2) a green run here would be equally consistent with the POST | ||
| /// having quietly succeeded.</para> | ||
| /// </summary> |
There was a problem hiding this comment.
1. Overly verbose test comments 📘 Rule violation ⚙ Maintainability
New/modified test code introduces very long XML doc blocks and multi-paragraph inline comments that reduce readability and maintainability. This conflicts with the requirement to keep comments minimal and prefer self-explanatory code.
Agent Prompt
## Issue description
Several newly added/modified test files include extensive XML documentation and long inline comment blocks that restate behavior already expressed by test names, helper names, and assertions.
## Issue Context
Compliance requires keeping comments minimal and preferring self-explanatory code. The PR adds multiple multi-paragraph explanations in test code and harness utilities.
## Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/SessionStartMemory/GeminiMemoryIndexLiveCertTests.cs[82-87]
- test/Capacitor.Cli.Tests.Unit/SessionStartMemory/GeminiMemoryIndexLiveCertTests.cs[101-122]
- test/Capacitor.Cli.Tests.Unit/SessionStartMemory/GeminiMemoryIndexLiveCertTests.cs[180-188]
- test/Capacitor.Cli.Tests.Unit/SessionStartMemory/GeminiMemoryIndexLiveCertTests.cs[224-228]
- test/Capacitor.Cli.Tests.Integration/GeminiSessionStartHandshakeOnPostFailureTests.cs[12-25]
- test/Capacitor.Cli.Tests.Unit/SessionStartMemory/MemoryIndexLiveCertHarness.cs[397-414]
- test/Capacitor.Cli.Tests.Unit/SessionStartMemory/MemoryIndexLiveCertHarnessTests.cs[10-17]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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>
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>
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>
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>
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>
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>
Collaborator
Author
Qodo review — both items addressed🐞 "PATH fallback runs cwd" — FIXED in
|
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>
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.
Follow-up to #414, which merged with all four of its definition-of-done items still open. This runs the measurement they were gating on, writes the result back into the spec, and fixes the harness defects that running it exposed.
Test and docs only — no production code changes.
§3.3 is settled — live, row 1, option (a)
Against
gemini 0.53.0,kcap 0.11.10-alpha.0.10+998ec34(the tree merged ase2b2821), macOS 26.5.2 / osx-arm64.The failure is real rather than simulated: a WireMock instance proxies the whole API to the configured server and fails only
POST /hooks/session-start/gemini. The index fetch on the same base URL is proxied untouched — the one arrangement that produces the shape under test.400from the forced mapping, matched on its response-body markerGET /api/memories/index?machine=… → 200, same base URL, same invocationhookSpecificOutput.additionalContextcarrying the nonceThe turn continues and the context is consumed, so option (a) — no commit gate — is the design and option (b) is withdrawn.
Getting the measurement honest took four attempts
Each rejected version looked sufficient, and each was holed in review. Worth recording, because the same shapes will recur in the next adapter's cert:
kcap hook --geminirun — Gemini's own hook could have returned 0 through a session- or source-dependent branch.startupandresumeshare one).kcapon the PATH Gemini inherits captures each invocation's exit code, stdout and stderr. The invocation whoseadditionalContextcarried the nonce must itself have exited non-zero and written the failed-POST diagnostic to its own stderr.Harness defects the run exposed — all fixed
None is a product defect. The first three made the cert lie, which is worse than a cert that fails.
exit 55, before any model call). The positive case failed honestly; the negative control passed vacuously — a turn that never ran trivially contains no nonce. Fixed with--skip-trust; the negative control now asserts turn completion too.kcapversion described the wrong binary.Process.Startresolves a separator-free filename against the working directory before PATH, and this assembly's output folder holds akcapcopied there by the project reference.RecordCertEnvironmentAsync— which exists to stop a stale-binary misdiagnosis — recorded the test build while its own siblingwhich kcapline reported the PATH build. Resolution is now explicit, asksaccess(X_OK)rather thanFile.Exists, and throws rather than falling back to a bare name the working directory would satisfy.int.MinValuesentinel satisfied the property under test. An unreadable exit record is!= 0. Now nullable: a parse failure fails the assertion.kcapon PATH, and so are the four long-livedkcap mcpstdio servers Gemini launches. Buffering a stdio JSON-RPC server's stdout traps its handshake until it exits, which it never does — every run hung at exactly 120s with four wrapped processes alive and zero HTTP traffic. The shim nowexecs anything that is notkcap hook.ArchiveMemoryAsyncreports whether the archive was confirmed; an ambiguous save (no id or a throw from the call) sweeps for the slug and marks the run dirty unconditionally; a prospective gate refuses to start further cases and an[After(Assembly)]teardown fails the run — because a cleanup failure in the last case is otherwise observed by nobody.New coverage
GeminiMemoryIndexLiveCertTests— the non-zero-exit case (gated), plus turn-completion in the negative control.GeminiSessionStartHandshakeOnPostFailureTests— the integration half the test plan called for, carrying the branch-specific lease assertion (a) requires: the failed-POST invocation completes the lease, and a laterresumeemits no context.MemoryIndexLiveCertHarnessTests— seven hermetic cases pinning PATH resolution.LiveCertIndexCleanlinessGuard— the assembly teardown.Verification
GeminiMemoryIndexLiveCertTests(live, gated)MemoryIndexLiveCertHarnessTestsGeminiSessionStartMemoryTestsSessionStartMemoryFoundationTestsGeminiSessionStartHandshakeOnPostFailureTestsMutation-proven, not asserted: reverting the execute-bit check to
File.Existsfails exactly the two resolution tests; resuming a different session id fails the lease assertion; dropping the dash normalisation failed the (since-replaced) id correlation; pointing the stderr diagnostic at a route that does not fail here fails exactly that assertion.Review: six rounds with a hosted codex reviewer, clean at round 6; Qodo's
PATH fallback runs cwdfixed incbeaf65— it caught the working-directory fallback that the review flow did not reach.Also pinned, because it cost a wrong conclusion mid-run:
session_idis validated withGuid.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.🤖 Generated with Claude Code