6 Commits

Author	SHA1	Message	Date
Behruz Nassre Esfahani	5ad9a36f35	fix(#4255): resolve reviewer-lane effort from the lane, not from gsd-plan-checker (#4275) ... `review-lane plan` resolved every cross-AI reviewer lane's reasoning effort by spawning `query resolve-execution gsd-plan-checker --host <slug>`. The agent id was a hardcoded literal, so `--host` chose only the argv RENDERING while the LEVEL always came from the installed plan-checker's frontmatter — `low` under every shipped model profile. Every prompt-fed lane therefore ran at a fast structural verifier's effort, and because the rendered argument is a CLI config override it silently beat the effort the operator had configured for that CLI. At `low` a large source-grounded prompt makes a model end its turn with no final message, so the lane came back empty and its stub read as a crash. Effort is a property of the review, so the lane declares it. Two new fields on ReviewerLane — `effortConfigKey` (`review.effort.<slug>`) and `defaultEffort` — carried through each capability manifest and the generated registry, set on the three lanes with an argv effort channel and null on the other nine. A new pure `resolveLaneEffort()` resolves config key -> lane default -> nothing, where "nothing" emits no effort argument at all and the reviewer CLI's own configuration decides; `inherit` selects that path explicitly and an unrecognized level falls back to the lane default rather than being forwarded to a CLI that would reject it. The host's negotiated effortSurface still gates the rendering, so ADR-1239/#2481's trust boundary holds on this path too. Resolving in-process also removes up to twelve subprocess spawns per review. The empty-output stub now names the effort the lane ran at and distinguishes a clean exit from a timeout kill, a non-zero exit, and a process that never ran — `status` is null for both a timeout and a signal, so those were indistinguishable before. The hint is hedged: a clean empty exit is most often a model stopping short, but it is also consistent with a CLI writing its output elsewhere. Also: the capability validator now knows both fields, rejects a malformed key or an out-of-vocabulary default, and rejects a default declared without a config key (a level the operator could never override). An existing end-to-end row in tests/effort-surface-axis.test.cjs asserted the old coupling; it now configures the lane's own key and pins the decoupling in the same real spawn, with the agent execution tier set to a level that must not appear. Emitted-Drift-Ack-Growth: review.md — the effort/model resolution-order table this fix adds. The workflow is where an operator looks to find out which knob set a lane's model and effort; leaving the new key undocumented there is the same invisibility that made the plan-checker coupling survive this long. Emitted-Drift-Ack-Growth: review.md — the effort/model resolution-order table this fix adds. The workflow is where an operator looks to find out which knob set a lane's model and effort, so leaving the new key undocumented there is the same invisibility that let the plan-checker coupling survive. Claude-Session: https://claude.ai/code/session_01CRMEuzNMWn3gs5uUW2ghcF Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-05 05:25:44 -04:00
Tom Boucher	62b0d939b6	feat(#3274): make reviewer-lane timeout configurable via timeoutConfigKey (#4083) ... * feat(#3274): make reviewer-lane timeout configurable via timeoutConfigKey Add an optional `timeoutConfigKey` field to the reviewer lane descriptor, resolved in `resolveLanePlan` at invocation time and falling back to the frozen `timeoutFloorMs` when unset or invalid, in the same spirit as the existing `promptBudgetKey`/`modelConfigKey` fields. All 12 shipped lanes declare `review.timeouts.<slug>` on both surfaces (the descriptor and their capability.json manifest), validated by capability-validator.cjs. For the antigravity lane, the native `agy --print-timeout` flag — previously a second hardcoded literal (`540s`) independent of the outer cap — is now derived from the same resolved outer timeout in `antigravityArgv`, preserving the existing 60-second buffer relationship (ADR-2782 D6: the outer bound is declared data, the inner one is handler-owned). The antigravity default timeoutFloorMs stays at 600s per the maintainer's disposition; users raise it through the new config key instead. * docs(#3274): document review.timeouts.* and extract resolveTimeoutMs helper Address code-review findings on the timeoutConfigKey change: extract the inline timeout-resolution logic into a named, exported, directly-tested resolveTimeoutMs helper (matching the file's existing configString/ normalizeHost convention); document the new review.timeouts.* federated config keys in docs/CONFIGURATION.md, docs/reference/capability-manifest.md, and docs/how-to/ship-a-reviewer-lane.md; add the changeset fragment. * fix(#3274): resolve native antigravity timeout in resolveLanePlan, not the runner gsd-test caught two design mistakes in the prior commits: 1. SpawnPlan.argv is documented and tested as fully resolved by resolveLanePlan (model/effort/output/prompt already folded in) — leaving the antigravity '{{nativeTimeout}}' marker unresolved until the runner's antigravityArgv violated that contract and broke tests that read plan.argv directly (tests/antigravity-reviewer.test.cjs, tests/review-default-reviewers-workflow.test.cjs). Fix: '{{nativeTimeout}}' is now a fifth ARGV_PLACEHOLDER member, resolved by resolveLanePlan itself via the new nativeTimeoutToken() helper, exactly like the other four. antigravityArgv reverts to its pre-#3274 four-argument form. Also missed updating capabilities/antigravity/capability.json's invoke.args to match the descriptor, which broke the manifest/descriptor parity test. 2. tests/reviewer-config-federation.test.cjs enforces a deliberate, narrow invariant (#3691 narrows #2797): qwen, cursor, and coderabbit — the three lanes with neither a model flag nor a host — may own no config key beyond their own prompt-budget key. Adding review.timeouts.<slug> to all 12 lanes violated it. Fix: those three keep timeoutConfigKey: null and own no review.timeouts.* key, matching their existing modelConfigKey: null. The other 9 lanes are unaffected. * chore(#3274): backfill changeset PR number (pr:0 -> 4083) --------- Co-authored-by: sim <sim@local>	2026-08-30 14:26:02 -04:00
Tom Boucher	1bf73d957b	enhance(#2295): record the resolved model per reviewer in REVIEWS.md frontmatter (#3649) ... * test(#2295): failing-first coverage for per-lane resolved-model recording * feat(#2295): record the resolved model per reviewer lane * docs(#2295): document the recorded reviewer model and its provenance * fix(#2295): refuse control characters in a recorded model value * test(#2295): correct watermark assertions for the widened mark shape * fix(#2295): anchor the role-manipulation injection pattern at a word boundary * feat(#2295): record the applied reasoning effort in the model value * chore(#2295): backfill changeset pr number * chore(#2295): restore em-dash in changeset body --------- Co-authored-by: sim <sim@local>	2026-08-19 08:59:37 -04:00
Tom Boucher	483083aea6	fix(#3194): verify source-grounded lane evidence from review output (#3436) ... * fix(#3194): verify source-grounded lane evidence from review output * chore(#3194): fill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-14 00:29:42 -04:00
0xdhx	0396d9cab1	enhance(#2483): stop the claude reviewer lane from inheriting CLAUDE.md + auto-memory (#2493) ... * enhance(#2483): env-guard the claude reviewer leg against CLAUDE.md injection The claude reviewer in workflows/review.md was a bare headless `claude -p` spawn run from the project cwd, so it inherited the invoking user's global CLAUDE.md, the project CLAUDE.md, and Claude Code auto-memory. That made it the only reviewer leg seeing anything beyond the prompt file. gather_context assembles PROJECT.md, the roadmap section, every PLAN file, CONTEXT.md, RESEARCH.md and REQUIREMENTS.md into the prompt before any reviewer runs; the gemini leg receives only that prompt and the codex leg runs --ephemeral. Beyond the measured ~4k tokens/spawn, the asymmetry cuts at the workflow's own premise: "independent review" meant something different for the claude leg than for the other two. Guard both dispatch lines with a per-invocation `env CLAUDE_CODE_DISABLE_CLAUDE_MDS=1`. `env`, never `export` — the flag must not leak into the orchestrating session (which may itself be Claude Code on the SELF_CLI="auto" path) or into any later spawn. review.md is the only claude -p call site in the installed tree, so this is two lines on one surface. The self-skip logic is untouched. * enhance(#2483): fix CRLF-fragile split and regenerate workflow baselines Two CI failures from the first push, both mine: 1. lint-tests: the new regression test split readFileSync content on a literal "\n". On a Windows git-autocrlf checkout that leaves a trailing "\r" on every line (local/no-crlf-fragile-split). Use .split(/\r?\n/). 2. golden-install-parity / workflow-size-budget / workflow-compat: editing gsd-core/workflows/review.md changes its content hash and byte size, and both are pinned in committed baselines. Regenerated via the repo's own generators (npm run size:baseline, npm run gen:golden). The regenerated diffs are review.md-only: exactly one hash line per golden-install-parity fixture and one size entry in workflow-size-baseline — no unrelated drift swept in. Full suite now green locally: 2113 pass, 0 fail, 3 skipped (run with HOME and CLAUDE_CONFIG_DIR overridden to throwaway dirs; live profile verified untouched afterward). * enhance(#2483): adapt guard-test matcher to the effort-args dispatch reshape The effortSurface wiring (#2481) reshaped the bare-model dispatch to `claude $CLAUDE_EFFORT_ARGS -p -`; the invocation matcher's dash-first form could no longer see it, and the count assertion failed exactly as designed. The matcher now tolerates variable expansions between `claude` and its first literal flag. Negative-controlled both ways: a stripped guard and a deleted dispatch line each still fail. * enhance(#2483): also guard the claude leg against auto-memory injection CLAUDE_CODE_DISABLE_CLAUDE_MDS suppresses CLAUDE.md file loading; auto-memory is an independently-toggled mechanism with its own flag. Add CLAUDE_CODE_DISABLE_AUTO_MEMORY=1 to both dispatch lines, correct the docs/COMMANDS.md and changeset claims that credited the first flag with covering auto-memory, and extend the regression test to require both flags on every claude invocation (negative-controlled: 2/4 assertions fail with the new flag removed). * enhance(#2483): match the claude binary in command position, not argument position The line-oriented invocation matcher counted any line where the token `claude` was followed by a flag. #2589 (landed on next as 920a5f3f) reshaped the effort-args lookup from --host claude 2>/dev/null | jq -r '.effort_argv_string // ""' to --host claude --pick effort_argv_string which put a flag immediately after `claude` and made the config query read as a third claude dispatch, failing the count assertion. The defect class is a binary name in *argument* position being read as a command. Fixed at the class rather than the instance: tokenise the line and skip any `claude` whose preceding token is a flag. That also covers the latent sibling one line away in review.md (`command -v claude`), which escaped today only because its next token is a redirect. Negative-controlled four ways: stripping CLAUDE_CODE_DISABLE_AUTO_MEMORY=1 fails, stripping the whole env guard fails, adding a genuine third unguarded dispatch (`timeout 900 claude --output-format text -p -`) still fails — so the narrowing did not blind the matcher to reshapes, which is the property the count assertion exists for — and the pre-#2589 jq form of the lookup still passes, so the matcher is not pinned to today's base. * enhance(#2483): carry the claude reviewer's memory guard as declared lane data ADR-2782 Phase 5b replaced the hand-authored per-CLI dispatch legs in review.md with the declared lane table, so the two `env`-prefixed shell lines this PR previously added no longer have a surface to live on. The guard is reimplemented where the lane contract now lives. `SpawnInvoke` gains an optional `env`, the claude lane declares the pair, the resolver folds own string-valued entries into `SpawnPlan.env` (absent or empty resolves to null, so the runner has one shape to test), and the runner passes it to spawn. Production merges it OVER `process.env` into a fresh object for that one child, so nothing reaches the orchestrating session or any other lane in the run. Declared data rather than a handler (D6): the pairs are static per lane, which is precisely what the manifest vocabulary is for. The capability manifest carries the same field, because the lane-fidelity test compares manifest and descriptor over the union of `invoke`'s keys. The regression test is rewritten against the resolver and runner rather than review.md's text. It gains the property the source-text assertions could only approximate: that `process.env` is never mutated. Scope boundary, asserted rather than left in prose: `env` is not part of the trust-disclosure surface, which is safe only while no manifest body reaches the resolver — the registry's reviewer bodies contribute slugs to the parity check and execution resolves from `REVIEWER_LANES`. The new test fails first if that ever changes. * enhance(#2483): restate the guard's mechanism in the docs and changeset Both described the fix as two `env`-prefixed dispatch lines, which is the surface ADR-2782 Phase 5b removed. The user-visible behaviour is unchanged; the carrier is not, and a changeset that ships a description of a mechanism the tree does not have is a CHANGELOG entry nobody can verify against the code. * enhance(#2483): cover the production spawn wiring end to end The unit tests stop at the runner's `deps.spawn` seam — every one injects a spy. Production supplies that seam in `gsd-core/bin/gsd-tools.cjs` as a hand-written object no test constructs, so the chain could be correct all the way to `SpawnPlan.env` and the merge could still be wrong or absent with the suite green. Deleting those four lines was the one mutation that left every other control silent. This runs the real `spawnSync` through `gsd-tools review-lane invoke`, with a `claude` shim on PATH that records the environment it was handed. It asserts both halves in one test: the pair arrives, and an unrelated inherited variable survives — a wiring that REPLACED the environment rather than merging over it would satisfy the first and break every lane's PATH and HOME. POSIX-only; mediating a Windows `.cmd` shim is a separate concern the repo already tests on its own. Noted rather than fixed: `timeout`, `killSignal`, `maxBuffer` and `shell: false` on that same object are equally uncovered. That is the epic's gap, not this change's, and closing it is not in scope here. * enhance(#2483): validate the invoke.env shape and register it as spawn-only `env` was the one spawn-invoke field with no shape enforcement: every sibling in `validateSpawnInvoke` is checked, and a manifest declaring `env` as an array, a string, a number, or an object with non-string values passed validation in silence. That matters more than an ordinary schema gap here, because `resolveLanePlan` DROPS a non-string value rather than coercing it — so an unvalidated manifest declares a pair that never reaches the spawn, which is the failure a memory guard can least afford. Two registrations, not one. `env` was also absent from `SPAWN_ONLY_INVOKE_FIELDS`, which is the list the openai-http arm rejects against — so `invoke.env` was accepted on a transport that issues an HTTP POST and has no child environment at all. It was the only spawn-shaped field accepted there; the other six each produce two errors. Self-found while sweeping the class, not raised in review. Keys are held to the portable POSIX environment-name grammar. That is a policy, not a claim about what an environment can hold: measured, only NUL is actually rejected by `spawnSync`, while `=`, a leading digit, a dash and a space are all carried through to the child (an `A=B` key arrives as the raw entry `A=B=value`). They are refused because a name outside the grammar is not portably addressable by the program meant to read it. `__proto__` is refused for a different and concrete reason. It passes that grammar and is a real own key once a manifest is JSON-parsed, but assigning it onto a plain accumulator goes through the inherited `__proto__` setter rather than creating an own property — and for the string values this field permits the setter is a no-op that does not even change the prototype. The pair would validate and then simply vanish before the spawn. (An environment CAN carry a literal `__proto__` entry; this is about the resolver's accumulator, and the error message says so.) Deliberately narrower than the sibling reserved-name guards in this file, which also reject `constructor`/`prototype`: those guard bracket lookups that resolve prototype members, whereas this reads via `Object.keys` plus an own-value read, where `constructor` assigns as an ordinary key the spawn could carry. `effortChannel` is deliberately left in neither field list: ADR-2782 D2 defines it for both transports, so it is shared rather than spawn-only. Reversion-controlled, three mutations, all three fire a named test: dropping `env` from the discriminator fails `httpTransportRejectsEnv`; removing the `__proto__` arm fails `envRejectsProtoKeyThatWouldSilentlyVanish`; disabling the block fails four. (#2483) * enhance(#2483): amend ADR-2782 D2 for the invoke.env vocabulary widening D2 records the spawn `invoke` shape as a closed vocabulary, and its Amendments section carries a dated entry for every prior widening (Phase 1 #2794, Phase 2 corrections #2795, Phase 5b #2799). This change extended that vocabulary in code without touching the ADR governing it, so the ADR contradicted the implementation — and the repo's own convention, recorded in CONTEXT.md, is that the ADR is amended in the same PR precisely because the prior widenings did it correctly. Adds the `invoke.env` row to the D2 table and a dated Amendments entry. The entry also corrects the authority this change cited. The source comment pointed at D6, which governs the closed `handler` enum — imperative behavior admitted first-party — and says nothing about the `invoke` field vocabulary. That is D2's territory, so the citation never covered the gap. Two claims are corrected rather than restated, both about the trust boundary that justifies leaving `env` out of the D5 disclosure signature: - The regression test does not enforce that boundary. On one forged lane it shows the resolver folds whatever it is handed, so a future path feeding it manifest lanes would not make any assertion in that test fail. Its comment claimed it "will fail first"; that was wrong, and both the comment and the ADR now say the boundary is a property of the production call chain instead. - The ADR is internally inconsistent on whether third-party manifest lanes execute at all: Consequences says adding a reviewer needs "no core patch", while `gsd-tools.cjs` rejects every slug absent from the first-party REVIEWER_LANES map. CONTEXT.md, `workflows/review.md` and the resolver's own header take the first view. #2483 did not create that inconsistency and does not resolve it; the entry records it rather than settling it in its own favour. (#2483) * enhance(#2483): document invoke.env in the capability-manifest reference ADR-2782 points capability and plugin authors at `docs/reference/capability-manifest.md` as where the lane vocabulary must be visible, and its `invoke` row enumerates the spawn sub-shape field by field. `env` was absent from that table while being part of the real shape, so the one document a third-party capability author would actually consult to learn the field exists did not mention it. Squarely Diataxis reference material — a field-by-field schema description — so it goes here rather than in the user-facing prose, which was already updated. States the constraints a manifest author can actually trip, and is explicit that the name grammar is a portability policy rather than an OS limit, so a reader does not take it for a claim about what an environment can hold. (#2483) * enhance(#2483): disclose and sign the reviewer lane's env and residual invoke fields `invoke.env` was undisclosed at install time. That was defensible while manifest lanes could not execute — the premise this PR's own ADR amendment recorded — and #2927/#3062 retired it: `routeReviewLane` now merges installed overlay `reviewer` bodies into its lane map via `mergeReviewerLanes`, which is a field-identical merge by ADR-2782 D1 and deliberately does not deep-validate. An overlay's whole `invoke` therefore reaches `resolveLanePlan`, and `env` reaches the spawned child. A consented third-party capability could set `NODE_OPTIONS=--require ./evil.js` on a reviewer lane with no install-time disclosure and no re-consent. The same file already decided what `env` means in a manifest: MCP servers fold it into the disclosure signature and render each key and value in the consent prompt, with an inline rationale naming this exact shape. Reviewer lanes get the identical treatment. `env` was the ninth unsigned invoke field, not the first. `defaultHost` (the manifest's OWN fallback egress host, used whenever the config key resolves to nothing), `path`, `outputChannel`/`outputArg`, `modelArg`, `effortChannel` and `modelDiscovery` all reach `resolveLanePlan` and none was bound. Enumerating a ninth name leaves the tenth open, so the lane signature carries a RESIDUAL of every other declared `invoke` key — the completeness backstop `rawConfig` already gives the MCP line (#1459 finding 5), and the "sign the whole object" remedy the recorded decision on this class prefers. `defaultHost` is also rendered: `resolvedHost` comes from user config, so a lane whose key is unset displayed "(unresolved …)" — which reads as "no destination" — while the runtime egresses the plan and review text to the address the manifest picked. D4.5 is preserved one level down: the extra element is appended ONLY when the lane declares something beyond the eight already-bound fields, so an env-free lane's signature stays byte-identical and no already-consented capability is re-prompted for a field it does not use. A lane that does declare one re-consents, which is the point. Execution-primitive env names are FLAGGED in the prompt, not refused in the validator. A denylist cannot be the boundary here: `PATH` alone is a complete execution primitive for a spawn lane and can never be refused, the child is an arbitrary third-party binary so the true set spans every interpreter's injection vars, and the MCP `env` this mirrors refuses nothing and discloses everything. Missing a name costs a quieter line, never a boundary. Refs #2483. * enhance(#2483): exercise the real overlay merge path in the guard test The test named for the manifest/first-party boundary did not test it. It built a forged lane locally, handed it straight to `resolveLanePlan`, and asserted that `REVIEWER_LANES` did not contain it — so no assertion in it depended on the claim its name made, and a code path that fed manifest lanes to the resolver would not have made it fail. Its own comment said as much, and named the production chain as the real carrier of the guarantee: "gsd-tools.cjs builds its lane map solely from REVIEWER_LANES". That sentence is now false. #3062 merged overlay reviewer bodies into that map, so the test's premise and its subject both moved. The replacement routes through `mergeReviewerLanes` — the real helper the production path calls — and asserts the overlay lane is admitted, resolves, and carries its `env` into `SpawnPlan.env`. That makes the security property falsifiable instead of narrated. It then asserts what now backs it: the env is disclosed on the surface, rendered key and value in the consent prompt, flagged when the name is an execution primitive, and bound to the signature so a value change, an addition, or a removal each force re-consent. Three further cases, because the finding's generative half is what stops it recurring: the residual backstop is asserted against five fields including one that does not exist (`aFieldThatDoesNotExistYet`), so a future vocabulary widening cannot silently re-open this; a fully-enumerated lane is pinned to its original 8-tuple, which is what keeps the fix from re-prompting every consented capability; and an http lane's manifest-declared `defaultHost` is asserted to reach both the prompt and the signature. Reversion-controlled, seven mutations, all seven fail a named test: env dropped from the surface, the prompt's env line removed, the execution-primitive warning removed, the signature's extra element never appended, the residual emptied, the defaultHost line removed, and the declares-something test un-widened. The last of those was SILENT on its first run and its test was written in response, then the control re-run. Refs #2483. * enhance(#2483): correct the ADR amendment's manifest-lane premise The amendment argued `env` needed no D5 disclosure because a manifest's `invoke` fields never reach `resolveLanePlan`. That was true when written and #3062 retired it 22 hours after this branch's last commit: `routeReviewLane` now builds its lane map from `mergeReviewerLanes(REVIEWER_LANES, loadRegistry({includeInstalled: true}))`, and D1's no-translation-layer rule makes that a field-identical merge, so an overlay's whole `invoke` reaches the resolver and executes. The entry had named this exact trigger — "were manifest lanes ever made executable, `env` must join the disclosed surface in that change, and nothing here will trip if it does not." Nothing tripped. The premise is rewritten to current truth rather than annotated, because an ADR is read in fragments and a superseded paragraph left standing reads as live reasoning to the next author; a one-line dated tombstone points at git for the withdrawn text. The rewritten entry records four things the first draft could not: that the enumeration itself was the defect (`env` was the ninth unbound `invoke` field, and `defaultHost` and `path` are egress-relevant on their own), that the residual is what closes the class, that D4.5's byte-identical-signature property is preserved by appending the residual only when a lane declares something beyond the eight bound fields, and that consent — not shape validation — is the boundary, since no honest env denylist can exclude `PATH`. It also closes the internal inconsistency the previous entry could only record. This ADR, `CONTEXT.md`, `gsd-core/workflows/review.md` and `resolveLanePlan`'s own header all said overlay lanes reach the resolver while the runtime said otherwise; #3062 resolved that in the documents' favour, which is what makes the disclosure mandatory rather than defensive. Refs #2483. * enhance(#2483): record in the manifest reference that invoke fields are consent-bound `docs/reference/capability-manifest.md` is the field table ADR-2782 points capability authors at, and it described `invoke` purely as a schema. A third-party author reading it could not learn that everything they declare there is shown to the user at install and bound to the consent signature — which is exactly what they need to know now that an overlay reviewer lane executes (#2927/#3062). States the two things the schema alone cannot: that `env` and `defaultHost` are named in the consent prompt and the rest is covered by a residual, so any change to a declared `invoke` field forces re-consent; and that `env`'s validation is a portability policy rather than a safety boundary, since `PATH` is a complete execution primitive and cannot be refused. Names that are execution primitives are highlighted in the prompt instead. Refs #2483. * enhance(#2483): add a Security changeset for the reviewer-lane disclosure The existing fragment describes the enhancement this PR was opened for and stays as it is. The disclosure fix is a separate user-visible change of a different type: a capability declaring `invoke.env` or `invoke.defaultHost` will ask for consent once more, and users are entitled to read why in the changelog rather than discover it as an unexplained prompt. Type is `Security` rather than `Changed` because the entry describes a closed code-execution disclosure gap, not a behaviour adjustment. Refs #2483. * enhance(#2483): correct this round's own claim about who gets re-prompted Self-found while auditing the round's claims before publishing them. The changeset and the ADR entry both stated that a capability declaring `invoke.env` or `defaultHost` "will ask for consent once more". That is wrong, and it overstated the cost of the fix in the one direction a maintainer would have had to take on trust. A code change to `disclosureSignature` re-prompts nobody. `hasProjectConsent` matches on the recomputed bundle `contentHash` — the signature has not been the security binding since #1459 CB-1/CB-2 — and the upgrade path's `executableSetChanged(old, new)` compares two disclosures both computed by the CURRENT code, so widening the signature moves both sides of that comparison equally. First-party capabilities never reach the path at all: the install flow blocks a first-party id before trust evaluation. What the widening actually buys is forward-looking, and is the real argument for it: an upgrade whose manifest edits a declared `invoke` field now registers as an executable-surface change and re-consents, where before it could change what the lane runs in silence. Also measured and recorded, because the D4.5 property was stated more strongly than it deserved: of the twelve first-party reviewer capabilities, ZERO are in the byte-identical-signature class — every real lane declares at least `effortChannel`. The property is a guarantee about minimal lanes, not a description of the fleet, and the ADR now says so. Refs #2483. * enhance(#2483): sign and disclose the probe binary and the lane's outer fields Found by this round's own adversarial review, and it is the same defect one level out: the `invoke` residual cannot reach the lane body's OUTER fields, and `probeLane` SPAWNS `probe.binary` with `--help` before dispatch (`review-lane-runner.cts`, the `command-exists`/`command-capability` arms). An overlay naming an arbitrary probe binary therefore executes it — unsigned and undisclosed, exactly as `invoke.env` was, and reachable on the same #3062 path. The lane element now carries a second residual over the outer fields, and the probe binary is shown in the consent prompt when it differs from the dispatch binary — it is a program that runs, and the user is entitled to see it. TWO fields stay excluded, and that is a decision rather than an omission: `reviewsSection` and `timeoutFloorMs` are ADR-2782's cosmetic carve-outs (matrix A10/A13), where re-consenting would present a prompt carrying no security information. A test pins that they remain excluded, so a later widening cannot quietly reverse D4.5 while claiming to complete this fix. Also corrects a miscount introduced by the previous commit: the source comment said the enumeration had fallen behind by "seven fields" and omitted `fallbackModel`, while asserting `env` was the ninth. `resolveLanePlan` reads twelve `inv.*` fields and four were bound, so the number is eight. The comment now states the derivation rather than just the total. Reversion-controlled: emptying the outer residual fails "repointing the probe binary must force re-consent"; removing the render line fails its own named assertion. Refs #2483. * enhance(#2483): refuse execution-primitive env names as defence in depth Adopts the review's B5 after this round's own adversarial pass refuted my reason for declining it. I had argued a denylist was worthless because `PATH` can never be refused. That was wrong on the facts: no shipped reviewer manifest declares `PATH`, so it can be refused, and it is the most complete primitive in the set — repoint it at a directory holding a fake binary and the declared `invoke.binary` is irrelevant. A list that cannot be exhaustive can still close the highest-confidence, lowest-legitimacy routes. So the validator now rejects `PATH`, `NODE_OPTIONS`, `LD_PRELOAD`, `DYLD_INSERT_LIBRARIES`, `BASH_ENV`, `PYTHONPATH`, `PERL5OPT`, `RUBYOPT`, `GIT_SSH_COMMAND`, `JAVA_TOOL_OPTIONS` and their siblings on a reviewer lane. A lane needing a specific executable declares an absolute `invoke.binary` instead of reshaping the child's environment. The comment states plainly that this is defence in depth and NOT the boundary — the boundary is install-time consent, which discloses every declared pair and binds it to the signature, so an unlisted name is still SEEN before it runs. That framing is load-bearing: a future reader who mistakes the denylist for the control will under-invest in the one that is, which is the failure mode I was trying to avoid by declining it outright. Two tests: the rejection itself across ten names, and a guard asserting no shipped reviewer capability declares a denied key — so if the list ever outgrows its evidence, that surfaces as a decision rather than a silent removal. Refs #2483. * enhance(#2483): fix two stale D5 enumerations elsewhere in the ADR The previous commit rewrote the amendment's premise but swept only the amendment. Two normative passages earlier in the same ADR still enumerated the old closed field list and now contradicted it: the `executableSetChanged` trigger list, and the split-binding note asserting the seven manifest-derived fields were "everything that is SHA-pinned". That is the failure the rewrite-don't-annotate rule exists to prevent, one section over — an ADR is read in fragments, and a fragment carries no supersession marker, so a reader landing on either passage would have taken the superseded enumeration as current. Both now name the residual as the mechanism rather than restating a list, which is also what stops them going stale the next time the vocabulary widens. Found by this round's adversarial review, which grepped the whole document rather than the section under edit. Refs #2483. * enhance(#2483): stop the probe disclosure claiming a spawn that does not happen The probe line added one commit ago rendered "probes by running: <binary> --help" for every lane. That is false for `kind: "command-exists"`, which only calls `hasBinary` — a PATH/filesystem scan that starts no process. Only `command-capability` spawns. A false statement in a consent prompt is worse than a missing one: the prompt is the surface a user is asked to trust, and this one overstated what a lane does. Worse, the test I wrote to prove the fix used `command-exists` — the kind that does NOT spawn — so it pinned the wrong claim and would have kept the error green forever. The surface now carries `probeKind` and the two kinds render differently: a spawn is described as a spawn, a presence check as a presence check. The test exercises both, and asserts the `command-exists` path never emits the spawn wording. Also corrects the field-count parenthetical to state its derivation unambiguously — `resolveLanePlan` reads thirteen `inv.*` fields including `env` (twelve before this PR), four were bound, so eight were unbound before `env` and nine including it. The bare "twelve" was true only of the pre-PR tree and read as a claim about the current one. And retires two comments that argued AGAINST the validator denylist this round then shipped. Leaving them would have handed the next reader the reasoning for removing it. Reversion-controlled: conflating the two probe kinds fails a named test. Refs #2483. * enhance(#2483): match the reviewer-lane env denylist case-insensitively The denylist added one commit ago compared exact case, so `Path`, `path`, `node_options` and `Node_Options` all passed it. Windows environment lookup is case-insensitive, so those reach the child as `PATH` and `NODE_OPTIONS` — the exact inputs the list names. An exactly-cased denylist is worse than none: it reads as a control while admitting the input it was written to refuse, and the next reader has no reason to doubt it. Members are stored uppercase and the key is folded before lookup; the name grammar already constrains keys to ASCII, so a plain fold is sufficient. Reversion-controlled: restoring the exact-case compare fails `envDenylistIsCaseInsensitive` on `Path`. Refs #2483. * enhance(#2483): correct the docs that still described the denylist as absent Both the ADR and the manifest reference still said `env` carries no denylist and that `PATH` "can never be refused" — written when that was this round's position, and left standing after the round reversed it. A reader landing on either passage would have taken the superseded argument as current, which is precisely the failure the rewrite-don't- annotate rule exists to prevent. Both now describe the denylist, name `PATH`'s inclusion and the case-insensitive match, and keep the limit explicit: the list cannot be complete against an arbitrary child and disclosure runs before validation, so consent remains the boundary. The ADR's byte-identical-signature claim is also corrected rather than softened. With the outer residual in place, a lane producing no residual is one the validator rejects — it declares no `flags`, `probe`, `emptyOutput`, `evidenceClass`, `requiresBinaries` or `promptBudgetKey`. So the property is about the ENCODING, not a claim that any real signature is unchanged, and it is not the argument for the change being safe. That argument is that consent binds to the bundle contentHash and no existing consent is invalidated at all. Refs #2483. * test(#2483): cover the three new lane disclosure fields in the injection-safety parity guard The PARITY test in section N exists to catch a renderer field that skips `renderValueForPrompt` (#3248). Its payload manifest is hand-maintained, so it covers the fields that existed when it was written — slug, binary, args, hostConfigKey, handler — and none of the fields this PR adds. This PR renders three further manifest-supplied values into consent-prompt lines: `invoke.env` (keys and values), `invoke.defaultHost` and `probe.binary`. The gap was silent rather than theoretical: with the lane env line reverted to the pre-#3248 raw form, the whole 948-test lane/capability/trust-disclosure suite stayed green. Two manifests, because the shapes render disjoint lines — `defaultHost` only on the openai-http branch, `env`/`probe` only where declared, and the probe line only when the probe binary differs from the dispatch binary. Non-vacuity is asserted on the typed disclosure object and on structural line counts, not by substring-matching rendered prose: CONTRIBUTING.md forbids raw text matching on test output, and this section's own header promises structural assertions only, so a prose match here would have made that promise false. Negative-controlled three ways against the merged tree, each producing exactly one named failure: env rendered raw, defaultHost rendered raw, probe binary rendered raw. * docs(#2483): extend the #3248 render-site comment to the fields this PR adds The comment enumerates every manifest-supplied value that must pass through `renderValueForPrompt`, and it stopped at `handler` — the reviewer-lane fields that existed when #3248 landed. This PR renders three more (`defaultHost`, the probe binary, and the env keys and values), so the list understated its own contract in the one place a future author would check before adding a fourth. A comment enumerating a closed set is a set that can silently fall behind the code it describes; the parity test added alongside is what makes the omission fail loudly rather than read as deliberate. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-11 17:42:41 -04:00
Tom Boucher	3f6b063fbb	chore(#2799): invoke_reviewers and write_reviews iterate declared lanes (#2861) ... * chore(#2799): resolve reviewer lanes into executable invocation plans Phase 5b of ADR-2782. Adds the resolver and runner that let invoke_reviewers iterate declared lanes instead of hand-authored per-CLI bash. Five additive descriptor amendments, each forced by a lane that ships today: - LaneHandler gains 'opencode' — the lane rebuilds its review from assistant text parts of a --format json stream; a plain stdout copy re-breaks #1936. - modelConfigKey — antigravity's key is review.models.agy, not .antigravity, so resolving by slug silently dropped a configured model. - defaultHost/fallbackModel — Phase 4 federated every *_host with a default of empty string; the real fallback only existed in the bash. - args becomes an argv template with a closed four-placeholder vocabulary. Positional splicing produced 'codex --model M -o F exec --ephemeral', which is not a valid invocation: codex injects in the middle, twice. - kimi-code lane, with the bounded command-capability probe (needle --output-format) that tells Kimi Code from the legacy python kimi-cli. Parity gate re-pointed: the workflow-text families it scanned are the text this phase deletes, so they are replaced by descriptor-to-registry parity plus an anti-parity check that no bespoke leg returns. jq, curl and external timeout/gtimeout all drop out of the review path. Refs #2782 * chore(#2799): add review-lane query surface and widen the manifest vocabulary Adds the gsd-tools 'review-lane' route (plan/invoke/sections) the workflow loops over, projects all twelve lanes into their capability manifests, and widens capability-validator for the amendments. opencode admitted to VALID_LANE_HANDLERS under the second arm of the enum's own admission rule: one lane, justified by a documented upstream defect data cannot express (#1936 — the agent can end its turn with zero output tokens and --format default then drops the assistant text entirely). Two bugs caught by an end-to-end stub run and fixed here: - loadConfigResolved returns a provenance wrapper, not the config; using it directly resolved every key to undefined, which reads as 'nothing configured' and silently dropped every model override. - hasBinary used shell:true with an args array (Node 26 DEP0190). Replaced with a PATH scan that spawns nothing at all. Refs #2782 * chore(#2799): iterate declared lanes in invoke_reviewers and write_reviews Replaces the eleven hand-authored per-CLI bash legs with a loop over resolved lanes, and renders REVIEWS.md sections from each lane's declared reviewsSection instead of thirteen hardcoded headings. review.md drops from 1104 lines to 507 (61KB to 28.7KB). Parity gate re-pointed, as agreed: the leg-marker and section-heading families scanned exactly the text this phase deletes, so they are replaced by descriptor-to-registry parity in both directions, plus an anti-parity check that fires if a bespoke leg is ever re-added. Enum, emitting sites and the Object.keys lock moved together. The budget-trim helper is hoisted out of the Ollama leg: it was always lane-agnostic, and any lane may now declare a promptBudgetKey. Refs #2782 * feat(#2799): bind the consented egress host and re-verify it at invocation Completes ADR-2782 D5. Rule 1 was recorded in the ADR as delivered by Phase 3 but was not implemented: ConsentRecord had no host field and nothing in the tree bound one, so this phase's rule-4 comparison had no baseline. ConsentRecord gains an OPTIONAL reviewerHost. Optional is the whole design: isValidConsentRecord does not require it, so every record already on disk stays valid and no re-consent storm fires (D4 rule 5). It is deliberately excluded from disclosureSignature — the loader has no config resolver, so folding a config-derived value in would make loader and lifecycle compute different signatures for the same manifest and re-prompt forever. Install resolves hostConfigKey (falling back to the lane's declared defaultHost, which is what the invocation path uses) and records it. Invocation re-resolves and blocks on mismatch rather than silently redirecting. Absence allows: no record, or a record predating the field, means nothing to compare — denying there would break every existing local-model user on upgrade. Refs #2782 * test(#2799): cover the resolver, runner and handlers; retarget the parity suites Adds the golden invocation-plan table (one row per shipped lane, derived from the bash legs rather than the descriptor types) plus runner coverage for the probe, empty-output policy, the three handlers and the egress check. Retargets the existing suites onto the new contract: descriptor-to-registry parity, the anti-parity check, the opencode handler, and the twelfth lane. Two corrections found by running them: - modelConfigKey was required; that breaks D4 rule 2, since a reviewer manifest authored before this phase would fail validation on upgrade. It is optional, read as null when absent. - the antigravity non-zero-exit test pre-seeded the transcript, which asserted that a STALE entry leaks through — the exact bug the watermark prevents. The spawn now appends, as the real tool does. Refs #2782 * fix(#2799): restore agy --add-dir and the self-report prompt in the handler Retargeting the three legacy reviewer suites off the deleted bash surfaced two real regressions in the port, both #2176: - --add-dir was dropped. Without it agy's permission context never receives the cwd repo, so the agent anchors on its own scratch dir and reviews the plan text in isolation — the exact failure the Review Instructions forbid. It is capability-probed, because an older agy rejects the unknown flag outright and a lane that fails to start is worse than one running on the prompt anchor. - the prompt lost the clause mandating a REVIEWED-WITHOUT-REPO-ACCESS self-report, which is what makes a blind review distinguishable from a grounded one. antigravity now builds its own prompt variant. Also ports the #2073 mode-2 cli.log diagnostic, which was dropped: a pinned model that 404s exits 0 with empty stdout AND an empty transcript, so agy's own log is the only evidence that anything failed. The three suites now assert against the plan and the handler instead of matching fence text, so they no longer need allow-test-rule exemptions. Refs #2782 * docs(#2799): document the declared lanes, the new flag, and dropped prerequisites COMMANDS.md gains --kimi-code and replaces the jq-prerequisite paragraph, which is now false: no lane requires jq, curl or an external timeout. Adds the changed-egress-destination behavior, since a blocked lane is something a user can hit. CONFIGURATION.md records that the model config key is declared per lane rather than derived from the flag — antigravity's is review.models.agy — and adds review.models.kimi-code. reviewer-instances.md now routes an instance through its lane's single invocation seam instead of a copied per-adapter bash block, which is what lets a cross-cutting fix reach instances for free. That required implementing the --model/--agent/--as flags it documents; --model re-resolves through the lane's argv template rather than splicing, so the flag lands where the lane declares it rather than ahead of a subcommand. CONTEXT.md glossary gains both new modules. Refs #2782 * chore(#2799): drop the stale emitted-drift acknowledgment The only entry was #2797's, acknowledging COMMENT-ONLY GROWTH in review.md. That file now shrinks by ~32KB and every emitted hash that moved is attributable to this diff, so the ack no longer explains anything. Removing the last entry means removing the file: its presence is the alarm, and an empty one signals nothing. Verified by deleting it and re-running the attribution and provenance gates plus lint:ci — all green without it. Refs #2782 * docs(#2799): record the Phase 5b vocabulary widenings in ADR-2782 Five additive amendments, each forced by a lane that ships today, plus two corrections the phase had to make rather than work around: - D5 rule 1 was recorded as delivered by Phase 3 and was not implemented, so this phase's rule-4 comparison had no baseline. Recorded because an ADR asserting a rule was delivered is exactly what stops a later phase checking. - The DEFECT.GENERATIVE-FIX gate is re-pointed: its workflow-text families scanned the text this phase deletes. Also records that D7's 'skip the probe where no bounding mechanism exists' carve-out is obsolete — in practice it meant the Antigravity lane ran unbounded on every stock macOS host, which ships neither timeout nor gtimeout. Refs #2782 * fix(#2799): close four defects found by adversarial review Two confirmed bugs, both reproduced before fixing: - resolveLanePlan was not total. An openai-http lane with a missing or non-object invoke dereferenced inv.hostConfigKey and threw, contradicting the module's own documented contract; the spawn branch guarded correctly and the http branch did not. The CLI seam resolves every selected lane in one map, so one malformed overlay manifest would have aborted the whole review rather than dropping its own lane. Guarded, plus a per-lane try/catch at the seam so a throw can never take down siblings. - A reviewer-instance model was silently dropped for any lane declaring modelConfigKey null (cursor, qwen, coderabbit). reviewer_instances validates that cli is a known slug but never that the slug accepts a model, so a user could configure one, get a clean run, and never learn a different model reviewed their plan. Now warns explicitly. Two hardening fixes: - The slug is concatenated into artifact paths, so LANE_SLUG_RE is enforced in the resolver rather than inherited from a validator that does not run on this path — the module documents itself as the overlay-manifest trust boundary, so it should not depend on someone else having checked. - normalizeHost mangled a scheme-less value: new URL('localhost:11434') parses with an empty hostname, so it became 'localhost://11434' and was compared and requested as if real. An empty hostname now means not-a-URL. Also documents the one gap that cannot be closed here: the antigravity watermark is keyed by workspace, so two concurrent reviews of the same repo share a transcript. agy exposes no per-invocation id to filter on, so the handler now states which half of its never-stale guarantee actually holds. Refs #2782 * test(#2799): retarget the remaining eight review.md-asserting suites The remote runner found 37 failures the local sweep missed (it hit the shell's two-minute cap before reaching these). All eight extract per-CLI bash from review.md that this phase deletes; each protects a real invariant, so each is retargeted onto the plan, the runner or the handler rather than removed. Three real defects surfaced by doing so: - effort args never reached ANY lane. model-resolver.cjs exports no resolveExecution, so effortFor silently returned [] every time. Restored by calling the same bounded resolve-execution query the bash legs used — and NOT with --raw, which prints the resolved effort rather than the picked field, so claude got 'low' instead of '--effort low'. - the timeout guidance lost 'a silent empty output is a timeout kill, not a crash' — the operator note that exists because of the Codex 0xc0000142 misdiagnosis. Restored. - the opencode handler dropped EMPTY assistant text parts. The shipped jq was , and only substitutes for false/null — an empty string is truthy in jq and contributed a blank line. Found by a property test shrinking to ['', '']. The opencode property suite no longer spawns jq at all, which deletes the #2099 hang mechanism it was architected around rather than mitigating it. Refs #2782 * fix(#2799): register the two new generated modules, and untrack them The remote runner caught build output committed to git. Both new modules compile from src/*.cts into gsd-core/bin/lib/*.cjs, and every sibling generated that way is gitignored and eslint-ignored (ADR-457) - including Phase 1's own review-lane-descriptor.cjs. Mine were neither, so repo-invariants' "each bin/lib/*.cjs is linted xor ignored according to migration state" failed. Registered both in .gitignore and eslint.config.mjs alongside the Phase 1 module, and dropped them from the index. Nothing about the shipped behaviour changes; the artifacts are rebuilt by build:lib. This is the new-.cts-module registration ripple, and it is the one part of it I had not completed - the CONTEXT.md glossary and the inventory manifest were already done. Refs #2782 * chore(#2799): backfill changeset pr number to 2861 * chore(#2799): backfill changeset pr number to 2861 --------- Co-authored-by: Test <test@example.com>	2026-07-30 12:48:06 -04:00