Repository navigation
Caller-selected sandbox and approval posture for hosted Codex agents - #417
Merged
Merged
Conversation
…ned, reconnect-safe) + bridge-defeating warning
PR Summary by QodoAllow caller-selected sandbox/approval posture for interactive hosted Codex launches
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…rrowed echo arm, handoff + argv pinning tests, README accuracy)
…tion in the snapshot-borrow echo test
… comment Addresses both Qodo review-recommended findings on #417.
realtonyyoung
added a commit
that referenced
this pull request
Aug 1, 2026
main has been failing "Build and test (ubuntu-latest)" since #393, and every open PR inherits the red on rebase. Two independently-green PRs conflicted semantically. #417 added a pinned argv vector, written as a literal transcription precisely so that any added, removed, renamed or reordered argument fails. #393 then added an unconditional --dangerously-bypass-hook-trust to CodexLauncher.BuildArgs, so an unattended launch is not parked on codex 0.146's interactive "Hooks need review" prompt. #393 branched before the pin existed, so its own CI never ran against it. Neither change is wrong. The pin fired exactly as designed and the new flag is intentional; what was missing was the re-pin. Both expected vectors now carry the flag in its actual position — after the approval pair, before -m — so the pin still asserts ordering rather than mere membership. Also corrects the test's doc comment. It described the vectors as "literal transcriptions of the pre-change output", which stopped being true the moment a post-change flag was pinned into them. It now says what a failure here means: the argv changed, and re-pinning is correct only after confirming that change is intended and correctly ordered. Test-only; no production change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
realtonyyoung
added a commit
that referenced
this pull request
Aug 1, 2026
* docs(spec): Gemini CLI as an unattended review-flow reviewer Rev 1, implementation-ready. Four live probes against gemini 0.53.0 on the real ACP argv, each with its control: - Gemini DOES honour session/new.mcpServers, so the flow-result channel is viable and SupportsMcpServers:false is stale. This was the go/no-go. - The --allowed-mcp-server-names clamp gates INJECTED servers too, not only repo-authored ones: with the deny-all placeholder the channel is never spawned and the model reports TOOL_NOT_VISIBLE. So the allowlist/capability coupling is a measured requirement, not a guess. - A FIXED injected name re-opens repo-controlled execution. A repo-authored .gemini/settings.json server named kcap-flow-result loads under INHERITED trust with no --skip-trust, matches an allowlist holding that literal, and is called outright when nothing is injected; with both present ours wins tool dispatch but the repo's impostor still runs as the daemon user. Hence a per-launch unguessable channel alias -- the AI-899 3.1d fix shape at a new sink. - --approval-mode yolo is REQUIRED, not a convenience: without it Gemini gates its own injected result channel behind session/request_permission, which no human is there to answer. With it, no interaction frame at all -- which is also what makes UnattendedInteractionPolicy.Fail the honest contract. Also records that the descriptor constructor forces the capability flags, trust argv and interaction policy to change atomically, so the "flip SupportsUnattended last" ordering an earlier draft assumed is unconstructable; verification moves before the single descriptor commit instead. Scope held deliberately narrow: no borrowed review (that is Kiro AI-1410's credential blocker and an unattended reviewer does not need it), no model override (AI-899 3.3 measured the write half fails silently), no vendor-neutral rename of the reserved channel id. * docs(spec): rev 2 — answer the spec review's six findings Three were High and all three were real design defects, not wording. 2.6 (new, measured): rev 1 claimed "a repository cannot match a per-launch GUID" on evidence that only showed one exact name matching and one different literal being blocked. Review called it a generalisation -- correctly, and it is the error class I have repeated on this epic. Measured it: four hostile servers in one workspace's own settings.json against an allowlist holding only the injected name. Prefix, glob and different-suffix are ALL refused; only a byte-identical name is spawned. So admission is name equality and nothing else, the GUID is sufficient, and the whole defence now provably rests on unpredictability -- which the spec then protects deliberately (v4 GUID, never derived from launch inputs, never written anywhere the repo can read). Duplicate-option semantics are deliberately NOT measured because 3.3a asserts exactly one option instead of depending on them. 3.2a (new): rev 1 said "derived once per launch" and then split derivation and argv substitution across two files without saying what passed between them -- so two aliases, or a renamed server with a stale allowlist, were both implementable and the tests would still pass at helper level. Now an explicit launch-scoped ReviewChannelIdentity(CanonicalId, WireName), created once, threaded to both sinks, with acceptance moved to the launch artifact: the serialized session/new name and the parsed argv value must be the same string. 3.3a (new): rev 1 leaned on "the trust argv is only appended under IsReviewFlow" as if that enforced a must-never property. It does not -- it says nothing about --approval-mode arriving from Argv or a caller, nor about legacy --yolo from any source. Now a final-argv boundary check after every contributor has run, plus the exactly-one-allowlist-option assertion 2.6 relies on. Also records that IsReviewFlow and unattended are already the same condition in code. 6 (rewritten): rev 1's test #7 contradicted the code it tested -- the placeholder cannot survive into an interactive final argv because substitution runs before the review branch. Gone, split into three artifact-scoped rows. The plan is now a table naming, per row, the artifact it reads and the mutant it must die to; #5 compares serialized name to parsed argv rather than an alias to itself, #6 proves traversal rather than constancy, and two new rows cover concurrent-launch alias sharing and derivation-from-inputs. The live cert gains a bounded wait so "never reported" cannot look like "skipped". 8: borrowed-snapshot refusal becomes a required negative acceptance test rather than an inherited guarantee. And the Cursor barrier is now NAMED rather than deferred -- Cursor has no allowlist gate at all (--approve-mcps suppresses approval), so it rests on AI-1626's measurement that its ACP path does not load repo-authored servers; that measurement predates our understanding of the production worktree shape and AI-1626 exists because an earlier reading of the same area was wrong twice, so it is explicitly not a clean bill of health. AI-1632 re-verifies it, using this spec's method. * docs(spec): rev 3 — ground the matcher in source, measure secrecy lifetime, make non-derivation structural Round 2 of the spec review returned three more High findings plus one process failure of mine (I left a placeholder literal in the submission, so the reviewer never saw rev 2's sections and had to assess summaries). 2.7 (new, read from the installed bundle rather than sampled): the matcher is Array.prototype.includes on the raw name -- exact, case-sensitive, no glob, no regex, no Unicode normalization. So 2.6's four negatives were not four lucky samples; there is no non-equality path for them to have taken. Same read settles the config-precedence question the reviewer raised: the CLI value is null-coalesced (??) over settings.mcp.allowed, so when the flag is present a repository-level allowlist is NEVER consulted and the excluded list is dropped too. No union, so 'exactly one CLI option' does close it. Also surfaces the length>0 trap -- an EMPTY allowlist admits everything, which is why 3.3a asserts exactly one VALUE and not merely at most one option. 2.8 (new, measured): the reviewer's sharpest finding was that the alias is unguessable but not invisible -- it is in the reviewer's own argv, and a prompt-injected reviewer under yolo could read it, write a settings.json naming it exactly, and cash in 2.6's byte-identical collision. Measured: the repository MCP set is evaluated once at process start and is not re-read, not mid-session and not for a new session in the same process. So the alias only has to stay secret for the launch, and it does. Two boundaries now stated rather than implied: per-launch freshness is load-bearing (a later launch would read the file a previous reviewer wrote, and only a fresh GUID makes the stale name harmless), and peer same-user processes are explicitly outside the claimed boundary since they can already read the token store. 3.2a: ForLaunch now takes its entropy as a parameter and has no launch context in scope at all, so derivation from session id / path / clock is unrepresentable rather than untested, and the test gets a deterministic oracle. Empty GUID aborts the launch -- no predictable fallback, since a degraded name is worse than no reviewer. 6: rows 12-14 replace the black-box non-derivation test the reviewer called unfalsifiable (a UUID-shaped hash of the session id would have passed it). * docs(spec): rev 4 — the delegate leaked, the validator was value-blind, the boundary was dishonest Round 3 returned six more findings, three High, and two of them killed claims rev 3 made in its own defence. F1: rev 3's Func<Guid> did NOT make derivation unrepresentable -- a delegate closes over its caller's scope, so ForLaunch(vendor, () => Hash(ctx.SessionId)) was legal and the tests would still pass with their own stub. The signature moved the boundary outward. Now a context-free production overload ForLaunch(vendor) that reaches its own entropy, plus an internal FromGuid(vendor, Guid) test seam -- a VALUE cannot close over anything. The mutant moves to the factory call site, since a test that only exercises the type cannot catch a caller reaching for the internal overload with a derived value. F2: rev 3's validator counted option tokens and never looked at values, so --approval-mode default passed it while silently restoring prompting, and --approval-mode=yolo bypassed it entirely. Now parses values in both yargs spellings and requires exactly [yolo]. Read the option definitions while fixing it and found a hazard I would have shipped: allowed-mcp-server-names is coerce: coerceCommaSeparated and type: array, so one token with a comma yields SEVERAL names -- the assertion has to be on the coerced set. approval-mode is choices-constrained, which rejects an unrecognised value at Gemini's parser but not a recognised-and-wrong one. F3: the same-user boundary was drawn dishonestly. Under blanket yolo, a prompt-injected reviewer's tool use can create a daemon-user process, read the token store, inspect a concurrent reviewer's argv and exfiltrate -- so a repository-induced process is not outside the threat model, it is reachable through the in-scope path. New 2.9 states it as an ACCEPTED security property instead of defending it, notes it is a property of every shipped unattended reviewer rather than of Gemini, that sandboxing is the only thing that changes it (which is why Copilot's borrowed review exists), and that the alias defence is not weakened because it was never aimed at that actor: 2.3's attacker declares and cannot execute, 2.9's executes and needs no declaration. DoD now requires that acceptance be signed off knowingly rather than buried. F4: 'evaluated once at process start / never hot-reloaded' outran the probe for the third time -- it tested two transitions and the bundle has seven refresh call sites and a 300ms coalesce. Narrowed to what was measured, and the design no longer rests on it: an actor able to use a reload already has strictly more (2.9). F5: narrowed the matcher claim to exact equality of the values as delivered; 'no Unicode normalization, no vendor aliasing' would need both operands' producers traced and this read does not do that. F6: 2.6 and 2.8 stated incompatible secrecy requirements. Whole-launch secrecy was never the property; it is unpredictability until admission is frozen, non-reuse by a later launch, and kcap not persisting the alias into the worktree. * docs(spec): rev 5 — stop emulating the vendor parser; widen the accepted property honestly Round 4: five findings, three High, all real. F3 was the most useful and it simplified the design. My hand-written OptionValues was a parallel parser whose grammar I had not proven identical to the vendor's -- and it was not: the bundle shows camel-case-expansion and boolean-negation both true, so --approvalMode binds and my exact-token --yolo check misses --yolo=true. Rev 4's C# comma split had the same defect against a coerceCommaSeparated I never read. So 3.3a stops parsing and starts REFUSING: kcap owns the launch, emits one canonical "--key value" pair per guarded option, and rejects any token that could bind a guarded key and was not emitted that way -- camelCase, =-joined, negated, duplicated, or legacy. That needs no model of yargs' grammar and makes the coercer's exact behaviour irrelevant (WireName is asserted comma-free). F2 was a regression I had introduced: rev 4 guarded only the review arm, so a caller-supplied argument could append a second --allowed-mcp-server-names to an INTERACTIVE launch, and since yargs arrays accumulate that would admit a repository server -- breaking the deny-all property this spec claims is unchanged. Both launch kinds are now validated. F1: 'for the duration of the review' was false comfort and the paragraph above it already contradicted it. The accepted property is now arbitrary code execution with full daemon-user authority, with confidentiality (copied tokens outlive the review), integrity (writes reach other worktrees, daemon config, the installed CLI -- so one review can alter a later one) and persistence consequences enumerated, and no time bound claimed. Also names who accepts it and where that sign-off is recorded, since a spec section is not a risk register. F5: 2.9 now states plainly that the alias gives NO verdict integrity -- steered repository content can just ask the legitimate model to submit a fabricated clean, with no impersonation needed. 'The real channel won dispatch' means the bytes came from our server, not that the review was honest. F4: sidestepped rather than answered, per the reviewer's own suggestion -- a comma-free canonical value plus exact-emission refusal means the coercer's semantics never matter. * docs(spec): rev 6 — the guard defended an input surface that does not exist Five rounds of review kept telling me the argv guard could not be complete while "arbitrary raw caller argv" was accepted, and I kept making the guard smarter. Nobody checked whether that surface exists. It does not. RuntimeStartContext -- the only launch input -- has NO argv field; its members are typed (Model, Effort, Tools, McpAllowlist, ...). The complete set of contributors to a Gemini argv is descriptor.Argv, descriptor.UnattendedTrustArgv (both compile-time constants) and a Copilot-only branch. There is no config-supplied extra-args setting. So the argv is two constants plus two GUIDs this code generates, and nothing untrusted reaches it. So the runtime input filter is deleted rather than extended: it had nothing to reject, and rev 5's IsGuardedToken grammar list -- which review rightly said could never be provably complete -- was answering the wrong question. The invariants become test-time assertions over the built argv, which is where properties of two constants and one branch belong. One runtime assertion is kept with its purpose restated honestly: it guards against a FUTURE contributor to the list, not against an attacker, which is the AI-899 "the next value added inherits it" lesson. Several of the round-4 and round-5 findings are therefore answered by the input surface rather than by the guard, and the spec now says so instead of keeping a guard that implies otherwise. 3.3b: GeminiLaunchIdentity now carries CanonicalId, WireName AND DenyAllName, so the deny-all value the validator expects is the same object the substitution emitted. Review found that name referenced but never defined -- a real gap with exactly the two-generators shape 3.2a exists to prevent. 2.9: audit integrity added (daemon-user writes reach session captures, logs and the installed reviewer, so neither the verdict nor its local evidence survives a compromise -- any post-incident story must rest on server-side data). And the consent model is corrected: the assets at risk belong to the daemon OPERATOR, not the repository owner, so a sign-off from me covers only my own deployments. For anyone else the consent event is enabling it -- which makes Gemini opt-in, never a default, with the property stated in operator-facing docs. That is a real scope addition and it is now in 4 and 7 rather than assumed. Parity with Cursor/Claude/Codex is narrowed to what I can evidence: Cursor's flags are read from the descriptor, the other two were never examined, so the claim is now "may already exist, must be assessed separately per vendor" instead of an asserted equivalence. * docs(spec): rev 7 — operator-enforced opt-in, a whole-vector oracle, real identity construction Round 6: four findings, one High. All concrete. F1 (High): "opt-in per deployment" was documentation, not enforcement -- a caller can still request vendor=gemini explicitly and need not be the operator whose credentials are exposed. Adds a daemon-side capability gate Reviewers:Gemini:Unattended:Enabled, DEFAULT FALSE, in the DAEMON's config because the daemon operator is the affected principal (not the server's flow settings, which the requester controls). Checked twice: at availability advertisement so a disabled daemon is never selected, and again pre-spawn so an explicit request cannot bypass advertisement. Enabling it is the consent event, and the key's own documentation carries 2.9's property so the operator reads it while deciding. F2: replaced the guarded-key scan with a WHOLE-VECTOR template. A template over the complete final argv fails on any new token whatever its spelling, so it needs no model of the vendor's grammar -- which is why it succeeds where three revisions of a key parser failed. Also tests the closed input surface as a premise rather than asserting it: no argv-shaped member on RuntimeStartContext, no config key supplying extra args, and ImmutableArray on both descriptor collections (a type guarantee, not a convention). The retained runtime assertion IS the template comparison, which is the only thing that makes keeping it worthwhile. F3: GeminiLaunchIdentity gains a real construction path -- private ctor, one production factory generating two INDEPENDENT v4 GUIDs, a test-only concrete-value seam, and refusal of empty or equal values. The deny name is the pre-existing barrier, so a fixed/empty/reused/derived one reopens AI-899's original hole even while argv equality tests pass. Independence is asserted because a refactor sharing one GUID would make the deny name derivable from the alias the reviewer can already read. F4: stopped overselling server-side data. The accepted compromise includes theft of the daemon's server token, so unless the server's records are append-only and that token carries no update/delete/impersonation authority -- neither established here -- remote evidence can be forged or erased too. Honest version: local audit is untrustworthy AND reliable post-incident reconstruction may be unavailable, depending on token scope and server-side retention, neither of which this issue examines. * docs(spec): rev 8 — version-pin the certified matcher, place the gate, make the oracle independent Round 7: four findings, two High. F1 (High) was the sharpest and is the AI-1592 lesson in a new place: 2.7's exact-match and no-union conclusions were read from the 0.53.0 bundle, and nothing tied them to the binary the daemon actually launches -- GeminiPath resolves whatever is installed. An npm update could switch the matcher to prefix-matching or make settings union with the CLI value while a capability flag set months earlier keeps advertising the reviewer and keeps carrying consent to a mechanism that no longer holds. So the capability decision now includes the resolved binary's version against a declared supported range meaning "the 2.7 matcher behaviour has been CERTIFIED for these versions", checked at availability and pre-spawn, fail-closed: an unknown or out-of-range version makes Gemini unavailable as a reviewer rather than warning and proceeding. Deliberately stricter than AI-899's hosting path, which runs any installed Gemini, because hosting degrades to a broken agent while an unattended reviewer with a broken MCP gate degrades to repository-controlled process execution. F2 (High): "checked pre-spawn" was not a location. ValidateAndBuildReviewFlowMcp is the wrong home -- its name says result-channel, and a transport change or a path supplying prebuilt MCP state would route around it. The authoritative check moves to the single factory operation immediately before process creation, keyed on the RESOLVED descriptor's vendor identity rather than requester text so an alias or case variant cannot slip past. Advertisement stays, explicitly as an optimisation rather than a boundary. F3: the whole-vector assertion needed three conditions rev 7 did not state -- the expected template must be literal and independent (deriving it from descriptor.Argv would let a new token appear on both sides and pass, the same defect as an oracle derived from the thing under test); it must run after every contributor; and the checked vector must be frozen and be the one that populates ProcessStartInfo.ArgumentList, or the assertion is true of something the OS never sees -- which is worse than no assertion because it looks like coverage. One mutant per condition, including a token added AFTER the assertion. F4: the gate is fail-closed on absent/malformed/unreadable, daemon-local only (no server or requester override, since the requester is who it protects the operator from), and the pre-spawn read uses current state rather than a value captured at construction, so a stale advertisement cannot spend withdrawn consent. * feat(gemini): the per-launch identity and the operator capability gate Two pure types, the security core of the Gemini reviewer, landed before any wiring so their construction rules are proven independently of the launch path. GeminiLaunchIdentity: private ctor, one production factory generating two INDEPENDENT v4 GUIDs, and a test seam taking concrete Guid VALUES rather than a factory delegate (a delegate closes over its caller's scope, which is how an earlier design revision left derivation-from-launch-context legal while its tests still passed). Empty or reused GUIDs are refused: the vendor's MCP allowlist is an exact-name gate, so a predictable allowlisted name lets the repository under review name its own server to match and get a process spawned as the daemon user. The two names must be independent because the reviewer can read its own argv, so learning the wire name must not yield the deny-all name. GeminiReviewerCapability: the daemon OPERATOR's consent, fail-closed on two axes. The operator flag lives in daemon-local config because the risk lands on the daemon operator, who need not be the person requesting the review. And a CERTIFIED VERSION SET rather than a floor -- the containment rests on 0.53.0's allowlist semantics, read from its own matcher, and GeminiPath resolves whatever is installed, so an upgrade must take the reviewer offline rather than let a flag set months earlier carry consent across a mechanism change. Newer versions are refused in the same direction as older ones, which is the point. 21 tests: exact-name composition against chosen GUIDs (a derived generator would pass format and uniqueness checks but not equality), comma/whitespace-free names (which is why the vendor's comma coercion never has to be reproduced), freshness, independence, empty/reused refusal, and the gate refusing disabled, unresolved and uncertified versions with distinguishable reasons. * feat(acp): make the launch identity vendor-neutral (decision C) Tony chose the vendor-neutral shape over a Gemini-specific type or widening the connectionSource delegate, and it is better than I framed it: - it ABSORBS the per-launch deny-all name AI-899 already generated inline inside SubstituteUnmatchableNames. That generator had exactly the two-derivations shape this type exists to prevent -- the value never left the substitution, so nothing else could assert on it. So this simplifies existing code rather than only adding a mechanism. - no delegate churn: the factory will derive the identity onto the context once, so the same instance reaches both the MCP list and the argv builder through the existing Func<RuntimeStartContext, ...> seam, leaving the ~68 factory tests that inject a connectionSource untouched. - a caller cannot choose it: the factory always overwrites, so a value supplied on the way in never reaches a launch. Aliasing is opt-in per vendor, so Cursor, Copilot and Kiro keep ResultChannelWireName == ResultChannelCanonicalId and their wire behaviour is byte-identical -- a regression guard, since applying aliasing vendor-neutrally would change three shipped reviewers as a side effect of a Gemini change. 13 tests, both arms: exact composition against chosen GUIDs, the non-aliasing arm keeping the canonical id on the wire, a fresh unmatchable name for EVERY vendor (that behaviour predates aliasing and must not become conditional on it), comma/whitespace-free names, independence, and empty/reused refusal. * feat(gemini): wire the launch identity, the capability gate and the argv oracle The reviewer is now constructable end to end. Threading: StartAsync derives the identity ONCE onto ctx and deliberately overwrites anything supplied, so a requester cannot choose the names whose unguessability is the entire MCP containment. AcpReviewFlowMcp injects the channel under that instance's wire name; SubstituteUnmatchableNames consumes its unmatchable name instead of generating one inline (absorbing the AI-899 generator that nothing else could assert on); and the review arm REPLACES the deny-all allowlist value with the wire name rather than appending, because the option is array-typed and comma-coerced so a second entry would widen the gate rather than move it. Gate: GeminiUnattendedReviewerEnabled (daemon config, default false) plus the certified-version check, evaluated in BuildProcessStartInfo -- the pure builder StartRealProcess calls immediately before Process.Start -- and keyed on the resolved descriptor's vendor, so an explicit vendor request cannot reach a spawn by routing around advertisement. The factory's SupportsUnattended also consults it so an opted-out daemon is never selected in the first place; that arm is an optimisation and says so. Argv oracle: the whole emitted vector is compared against a literal template with only the identity values substituted -- deliberately NOT derived from descriptor.Argv, or a newly added token would appear on both sides and pass. It runs last and reads psi.ArgumentList, which is the vector the process receives. Descriptor flip, atomic because the constructor requires it: SupportsUnattended and SupportsMcpServers true, UnattendedTrustArgv --approval-mode yolo, UnattendedInteractionPolicy Fail. Two AI-899 tests correctly caught the change and are updated on their own terms. The coupling test's SupportsMcpServers==true branch was a GUESS -- that flipping the flag would make the descriptor carry the injected names. Measured, the coupling lives one layer down: the template always holds the placeholder and the launch replaces it, because a per-launch name cannot be a constant. That assertion is now unconditional and the coupling is asserted where it happens. Note for review: my first pass at those updates was a global find/replace that also flipped Copilot's SupportsMcpServers and Kiro's SupportsUnattended -- both correctly false. Caught by reading the diff, reverted, and redone with vendor-scoped anchors; every vendor's four flags are now verified semantically rather than by diff, since a diff is what hid it. * test(gemini): launch-artifact tests for the reviewer invariants 17 tests reading the argv the process receives, not descriptor fields. The two silent failures they guard: an allowlist that does not admit its own result channel (the reviewer starts and can never report) and approval prompting restored (it stalls on a frame no human answers). Whole-vector comparisons are joined strings, NOT IsEquivalentTo -- that assertion ignores order, which is exactly what a vector guard must not do: the allowlist option and its value being adjacent IS the property. Joining also makes a failure read as the command line it is. * test(gemini): cover the argv oracle, and prove the identity is not a caller input Mutation testing found the whole-vector assertion's CALL SITE deletable with nothing going red. It is unfalsifiable through the real path today -- nothing untrusted reaches the argv -- but an uncovered guard proves nothing and its whole purpose is to catch a FUTURE contributor. So the future is simulated: a synthetic Gemini-vendor descriptor with an extra argv token is exactly what a fourth contributor produces, and the launch must fail. Re-run: the call-site mutant is now DETECTED 3/24. Note on oracle independence: ExpectedGeminiArgv takes (isReviewFlow, identity) and no descriptor, so deriving the expected side from the thing under test is structurally impossible rather than merely tested -- the mutant that would prove it does not compile, which is the stronger outcome. Also: a caller-supplied LaunchIdentity must not reach the spawn seam, asserted at the seam itself (the factory overwrites), plus advertisement respecting the gate and other vendors' advertisement being unaffected. Two traps hit while writing these, both worth knowing: the mutation loop's `git checkout --` reverted a real source change (making the assertion internal) as collateral, so it had to be re-applied; and a bare FakeAcpAgent never answers initialize, so a StartAsync test needs a bounded token or it hangs the suite -- the spawn seam records before any frame is exchanged, so bounding costs nothing. * docs(readme): the Gemini reviewer opt-in, and what enabling it accepts The operator-facing half of the consent model. The capability flag is daemon-local and defaults off because the assets at risk -- this daemon's credentials, other worktrees, host persistence -- belong to whoever runs the daemon, who is not necessarily the person requesting a review. So enabling it IS the consent event, and it has to be informed where the operator will actually read it rather than in a design spec. States the accepted property without softening it: unbounded daemon-user code execution, credentials that outlive the review, writes reaching other worktrees and the installed CLI, possible persistence, a local audit record the same authority can edit, and no verdict integrity (steered content can just ask the model to report clean). Also that this is a property of unattended review generally rather than of Gemini, that sandboxed borrowed review is the one path without it, that the certified-version check is not bypassed by the flag, and that Gemini never becomes a default reviewer. * test(gemini): gated live certification of the reviewer's MCP containment The three design probes promoted to tests, so a Gemini upgrade that invalidates one fails here instead of silently reopening a repository-impersonation hole. This file IS what CertifiedVersions means: adding a version asserts these passed against that build. Cert1 positive -- the injected channel loads and is CALLED under a per-launch unguessable name. Without it there is no feature. Cert2 negative control -- the deny-all name blocks even our OWN channel, which is what proves the allowlist is a gate at all rather than Cert1 passing vacuously. Cert3 hostile repository -- a prefix of the allowed name, a same-shaped different guid, and a wildcard attempt must ALL fail to spawn while the real channel still works. This is the case that catches the single change that would silently reopen the hole: the allowlist becoming prefix- or glob-matched instead of exact. Evidence is a marker file each MCP server writes, never the ACP transcript -- the client cannot see whether Gemini spawned a server, and a process having RUN is the boundary that matters, not who won tool dispatch. Bounded turn timeout so 'never reported' stays distinguishable from 'skipped'. Gated behind KCAP_GEMINI_REVIEWER_CERT=1; CI has no gemini binary and each case spends a real turn. * fix(gemini): gate before ANY connection source; de-hang the version probe Round 1 of code review, four findings, one High. All correct. F1 (High): the capability gate was bypassable. It lived only in BuildProcessStartInfo, which only the DEFAULT connection source calls -- so a supplied source was invoked for a disabled daemon or an uncertified vendor version and could spawn directly. A test seam is still a bypass of the claimed invariant. The gate now runs in StartAsync before _connectionSource, with the builder keeping its own check as defence in depth for a direct builder call. Two new tests: a disabled daemon never reaches a supplied source (reached == 0), plus the positive control that an enabled one does, so the first cannot pass because StartAsync failed for an unrelated reason. F2: the "10s bounded" version probe could block forever. It called ReadToEnd() BEFORE WaitForExit, so a vendor holding stdout open blocked ahead of the timeout, and stderr was redirected but never drained so filling its buffer wedged the child. Both streams are now drained concurrently with the wait. Worse, SupportsUnattended evaluated the probe as an argument even when the operator flag was false -- an installed-but-wedged gemini could hang daemon STARTUP on a path the operator had opted out of. The flag is now checked first and short-circuits, in both the advertisement and the gate. F3: the live-cert harness discarded responses and prompted with sessionId: null, so the positive certs could not establish invocation against a conforming implementation and the NEGATIVE cert was vacuous -- startup failure, a rejected session/new and an invalid prompt all produced "no marker appeared", read as "the deny-all name blocked the channel". It now awaits each response, prompts with the returned session id, and Cert2 requires the session and turn to have SUCCEEDED before an absent marker counts as evidence. F4: three tests did not prove their names. - the allowlist/channel agreement test built argv and MCP specs from two separate Ctx() calls whose fixture identities happened to hold equal GUIDs, so it stayed green even if production handed a different identity to each consumer -- exactly the silent failure the type exists to prevent. One context now. - the disabled-advertisement test had no version seam, so on a host with no gemini it was false because the version was unknown and would have kept passing if advertisement stopped honouring the flag. The resolver is pinned certified, with a positive control and a new uncertified-version case. - TheCanonicalVector_PassesTheAssertion fed ExpectedGeminiArgv back into an assertion deriving its expectation from the same helper. Replaced with independently written literals for both launch kinds. * chore: keep Linear issue keys out of C# sources scripts/check-linear-ids.sh rejects AI-<digits> in tracked src/**/*.cs and test/**/*.cs, and I had put them in ten doc comments. Replaced with descriptive references ("the Gemini reviewer design spec", "the hosting work") so the provenance survives without the key; the spec filename and the PR carry the id. Also records why the ubuntu failure is not from this branch: UninstallCommandTests.Cursor_hooks_write_failure_propagates_to_exit_code throws "Cannot be used to terminate a process tree containing the calling process" from DaemonCommands.StopByName's Process.Kill(entireProcessTree). Neither DaemonCommands.cs nor UninstallCommandTests.cs is in this diff, and main's last four CI runs are green -- a stale PID file resolving to the test runner's own tree. * test(gemini): pin the version seam, and prove threading across the real sinks Round 2, two findings, one High. Both correct. F1 (High) was a bug my own F1 fix introduced: moving the gate ahead of the connection source meant StartAsync resolves a version before the spawn seam is reached, so the caller-supplied-identity test began depending on whether a certified gemini happens to be installed -- green on this machine, red on CI where the gate refuses as version-unresolved and the captured identity stays null. Pinned, like the two source-boundary tests already were. Worth noting the shape: a fix that made production stricter silently made a test host-dependent, and only CI would have shown it. F2: the allowlist/channel test still only proved the two HELPERS agree when their caller hands them the same identity. The real invariant lives in StartAsync -- build the MCP list from the rewritten context, then hand that same context to the connection source -- so a regression regenerating the identity in between would have left every identity test green. Now asserted across the two ACTUAL sinks of one real StartAsync run: the serialized session/new.mcpServers payload the vendor received (read from FakeAcpAgent.ReceivedCalls, not re-derived) against the context the spawn seam was handed. Plus that the name IS an alias, so it cannot pass trivially the way it would for a non-aliasing vendor. Mutation-proven with the exact regression review described -- regenerating the identity immediately before _connectionSource -- which fails precisely this one test (1/75) and nothing else, which is why the other tests were not enough. Also: fake.RunAsync(ct) is what serves the handshake. Without it StartAsync waits on initialize and the token cancels before session/new is sent, which presents as "Sequence contains no matching element" from the payload helper rather than as a timeout. * fix(gemini): extract the version token; drain the cert harness's stderr Qodo review: two bugs, one rule violation. Both bugs real. Bug 1 -- brittle version gating. ResolveGeminiVersion returned the whole trimmed stdout and the capability did exact set membership, so any prefix, suffix, banner or multiline output would make the gate fail closed and SILENTLY disable the reviewer. I measured it before fixing: gemini 0.53.0 prints the version to stdout AND stderr, so today's code happens to work -- but the vendor already emits banner lines (skill-conflict warnings) on other paths, and an "update available" notice would have been enough to break it. Now extracts the first dotted-numeric token from either stream, deliberately narrow so anything unrecognisable reads as UNKNOWN -- which denies -- rather than as a near-miss string. 14 tests: noisy prefixes, a leading v, trailing build metadata, a banner line, and six non-version inputs that must extract to null. Bug 4 -- the live-cert harness redirected stderr and never drained it. The same deadlock class review caught in the production probe, which I fixed there and not here. gemini writes real volume to stderr (skill-conflict warnings on every launch), so the buffer fills and wedges the child mid-handshake -- presenting as a mysterious cert timeout. Now drained concurrently, and the tail is surfaced on timeout since stderr is where an auth/project misconfiguration is reported, which is the likeliest reason a gated cert fails on a fresh machine. The rule violation (doc-comment length on GeminiReviewerCapability) is declined for the same reason as its siblings: each paragraph records a measured fact or a decision the code cannot show -- why a certified SET rather than a version floor, why the flag is daemon-local, why an unknown version denies. Those are the reasoning a future reader needs to not "simplify" the gate into a floor. * Re-pin the hosted-Codex argv vectors after the hook-trust flag main has been failing "Build and test (ubuntu-latest)" since #393, and every open PR inherits the red on rebase. Two independently-green PRs conflicted semantically. #417 added a pinned argv vector, written as a literal transcription precisely so that any added, removed, renamed or reordered argument fails. #393 then added an unconditional --dangerously-bypass-hook-trust to CodexLauncher.BuildArgs, so an unattended launch is not parked on codex 0.146's interactive "Hooks need review" prompt. #393 branched before the pin existed, so its own CI never ran against it. Neither change is wrong. The pin fired exactly as designed and the new flag is intentional; what was missing was the re-pin. Both expected vectors now carry the flag in its actual position — after the approval pair, before -m — so the pin still asserts ordering rather than mere membership. Also corrects the test's doc comment. It described the vectors as "literal transcriptions of the pre-change output", which stopped being true the moment a post-change flag was pinned into them. It now says what a failure here means: the argv changed, and re-pinning is correct only after confirming that change is intended and correctly ordered. Test-only; no production change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- 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.
Lets a caller select the Codex
--sandbox/--ask-for-approvalposture for interactive hosted launches, while keeping the two containment invariants non-overridable and fail-closed.Linear: AI-633 (parent epic AI-1399). Design spec and implementation plan live in kcap-server at
docs/superpowers/specs/2026-07-30-ai633-codex-posture-selection-design.mdanddocs/superpowers/plans/2026-07-30-ai633-codex-posture-selection.md(spec is codex-review clean, rev 5).This is PR 1 of 2 — the daemon contract only. The server/UI half (selector dropdowns, capability gate, applied-posture chips) follows in kcap-server and bumps the submodule to this branch's merge SHA. Until that lands nothing sets a posture, so behaviour on this PR alone is byte-identical to today.
The problem
CodexLauncher.BuildArgsderived both knobs and no caller could influence either:Two of those derivations are containment guarantees, not preferences, and stay non-overridable:
read-only— a borrowed reviewer runs in the requester's real checkout;workspace-writewould let a hosted agent mutate the repo they are working in.never— reviewers run unattended; any prompting mode wedges the round waiting for an approval nobody will give.The gap was the one case outside both: an interactive launch in a daemon-owned worktree.
What changed
Models.cs):CodexLaunchPosture(Sandbox, Approval); trailingCodexPostureonLaunchAgentCommand; trailingSandboxPolicy/ApprovalPolicyonAgentRegistered; trailingSupportsLaunchPosture = falseonUnattendedVendorCapability. All appended-last optional fields on name-bound JSON DTOs, so an older server or daemon is unaffected.CodexPosturePolicy(new): the single source for eligibility + token validation and effective resolution, so the argv and the registration echo cannot diverge. Coded rejections:codex_posture_wrong_vendor:,codex_posture_not_overridable:,codex_posture_invalid:(covers unknown tokens, mixed case, a half-specified pair, and the upstream-deprecatedon-failure).BuildArgs: honours the selected pair on the interactive branch, with a belt-and-braces throw if a posture somehow reaches a borrowed or review-flow context.DisablesApprovalPromptsnow also reflects a selectednever, so the PTY submit strategy matches reality.AgentInstance(predicate: codex andKind == Defaultand owned worktree) so both the initial registration and every reconnect re-registration report the same pair — a server restart cannot lose it. Every other launch shape reports nulls, which is what lets a consumer render it with no launch-kind discriminator.neverordanger-full-accesslogs a Warning naming the agent — the permission bridge can never prompt for it.BuildReviewArgsstays fixed (spec D1), recorded in a comment on the method.Verification
CodexPosturePolicyTests(3×3 accepted matrix + every rejection with its exact coded prefix + derived resolution), 13 newCodexLauncherTests(argv per combination, absent-block byte-identical regression, belt-and-braces throws, approval-prompt gating), 12 new orchestrator tests (guard position, echo for selected/derived, the three null-echo shapes, reconnect resend, warning on/off), 1 capability test.origin/main(verified by diffing both runs; all in the known macOS config/MCP-registration baseline). Zero new failures.dotnet publish -c Releaseclean, no IL2026/IL3050.help-*.txtchange because no command-line surface changed.🤖 Generated with Claude Code