3 Commits

Author	SHA1	Message	Date
Tom Boucher	9e4f0e99ad	fix(#3631): exclude only __pycache__-resident bytecode from the consent digest (#3650) ... * test(3631): failing-first coverage for bytecode-cache in the consent hash bundleContentHash digests a walk with no exclusion, so a routine 'python3 -m unittest' inside a Python-backed capability bundle writes __pycache__ under the bundle, the recomputed hash stops matching the consent record, and the capability silently goes inactive — no error, no warning, and loop render-hooks then omits its step and gate. Two distinct triggers, and the second is the sharper one: collectBundleEntries pushes a {kind:'dir'} entry for EVERY directory and the digest emits a TAG_DIR marker for it, so an EMPTY __pycache__/ flips the hash before a single .pyc is written. A fix filtering only *.pyc would leave that live. Verified by execution against the built lib: 5 of 7 probe rows diverge from intent today, including the empty-directory row. The anti-regression rows are the point of the shape: editing a real scripts/m.py and adding node_modules/pkg/index.js must BOTH still change the hash. node_modules is deliberately not excludable — its contents are required at runtime, so dropping it from the digest would stop consent binding executable content. The symlink row pins ordering: exclusion must apply after the lstat fail-closed rejection, never before. Refs #3631 * fix(3631): exclude derived bytecode caches from the consent digest RED proven at e5ba8f1fe on the remote runner: 8 failures, exactly the rows predicted to fail, with the four anti-regression rows already green. collectBundleEntries now skips a hardcoded, gitignore-independent set from the DIGEST: basenames __pycache__, .pytest_cache, .DS_Store, and any .pyc/.pyo file. Matching is byte-exact on the raw Buffer name (the walk never utf8-decodes) and case-sensitive, so the digest does not vary with how a name happens to be spelled on a case-insensitive volume. Three properties were preserved deliberately, each pinned by a test: - The filter runs AFTER the lstat symlink/non-regular fail-closed rejection. Filtering first would have turned the exclusion into a way to smuggle a symlink past the check; a symlink named x.pyc still throws. - Excluded entries still count toward BUNDLE_MAX_FILES and BUNDLE_MAX_TOTAL_BYTES. The caps guard the WALK; the digest answers a different question, and exclusion must not become an unbounded-bytes hole. - An excluded DIRECTORY is neither emitted as a TAG_DIR marker nor recursed into. The directory marker was the sharper half of this bug: an empty __pycache__ flipped the hash before any .pyc existed, so a *.pyc-only filter would have left it live. The issue proposed either a gitignore-aware walk or a list including node_modules. Both are rejected. A consent binding must not delegate its scope to a .gitignore the bundle author does not control — one line there would drop arbitrary executable content out of the hash. And node_modules holds code that is required at runtime; excluding it would stop consent binding executable content, turning a usability bug into a supply-chain hole. What makes __pycache__ different is that CPython validates each .pyc against its sibling source, which remains hashed, so a real code change still invalidates consent. Docs: CONTEXT.md's 'EVERY regular file AND directory' claim is corrected in place. ADR-2363's residual-gap section said the walk had 'no exclusions' — per docs/adr/README.md ('ADRs are append-only') that is corrected by a dated amendment rather than an in-place edit. Its D4 argument is unaffected: skill bodies are .md and stay bound. Fixes #3631 * fix(3631): narrow the digest exclusion after two isolated security reviews The first cut of this fix passed the full suite and was still wrong. Both orthogonal reviews rejected it, and the second one found a hole that has nothing to do with Python. HIGH — an excluded DIRECTORY was 'continue'd before recursion, so its whole subtree was permanently outside the digest. Declared hook script paths allow '_', '.' and '/' with no directory or extension rule, so hooks:[{script:'__pycache__/run.js'}] installed, executed via node, and its bytes could be rewritten forever without moving the hash. Ship benign v1, collect consent, then own the machine. No Python involved. FALSE RATIONALE — the justification I wrote into the code, CONTEXT.md, the ADR amendment and the changeset claimed CPython validates a cached .pyc against its sibling source, so the source staying hashed kept consent honest. That is not true, and I proved it by execution rather than argument: default timestamp invalidation compares only the source's mtime and size, both settable by anyone who can write the bundle. A forged pyc ran while the .py was byte-identical. Also wrong: '*.pyc' matched anywhere, but a legacy sourceless scripts/x.pyc IS importable, so excluding it was a live vector. Narrowed to what is actually defensible: - a DIRECTORY named __pycache__/.pytest_cache has only its TAG_DIR marker suppressed; the walk still recurses and hashes every non-excluded child. - .pyc/.pyo are excluded ONLY when the parent basename is exactly __pycache__. - a regular FILE named __pycache__, and a DIRECTORY named x.pyc, stay bound. - declared hook paths containing a __pycache__/.pytest_cache segment or a .pyc/.pyo basename are now rejected in both validator copies — a file named .pyc can contain perfectly valid JavaScript, so the exclusion must not be reachable from a declared surface. Accepted residual risk, stated plainly in ADR-2363 and CONTEXT.md instead of explained away: a forged __pycache__/mod.pyc matching an unmodified, still-hashed mod.py executes without moving the digest. Before this change that write was detected. It is accepted to stop routine bytecode caching from silently deactivating capabilities, and it is bounded — the attacker needs post-consent write access, everything outside __pycache__/*.pyc stays hashed, and no declared surface can point into the excluded space. Known limitation, not papered over: .pytest_cache CONTENTS still move the digest. Only the directory marker is suppressed. Excluding that subtree would reopen the HIGH finding. Refs #3631 * fix(3631): drop the .DS_Store exclusion and pin what the caps actually bind Second round of isolated review findings. The hardening closed the two original holes — both re-reviews confirmed that by execution — but it introduced a new one of the same shape, and left three claims unbacked. HIGH, self-inflicted: .DS_Store was excluded from the digest at any depth, but the hook path validator was hardened only for __pycache__/.pytest_cache/.pyc/.pyo. So script:'hooks/.DS_Store' was ACCEPTED, runnableHookCommand emits the bare quoted path for a non-.js name (the branch .sh hooks already use), and capability-source copies it with its mode bit intact. Ship it +x with a benign shebang, take consent, then rewrite it forever — the digest never moves. Fixed by DELETING the .DS_Store exclusion rather than teaching the validator about it: .DS_Store has nothing to do with this issue's Python bytecode symptom, and an excluded filename is a permanently unhashed name. The narrower the exclusion, the smaller the hole. The residual-risk bound in ADR-2363 and CONTEXT.md claimed declared surfaces cannot reach excluded space. That is false and is now stated correctly: node resolves an unregistered extension through the default .js handler, so a hashed, consent-covered hooks/run.js that requires '../__pycache__/mod.pyc' reaches it in one hop. The validator guard raises the bar for DECLARED surfaces; it does not contain the risk. The two bounds that are real — post-consent write access required, everything outside __pycache__/*.pyc still hashed — are kept. The BUNDLE_MAX_FILES boundary test had gone vacuous: it padded with root-level *.pyc, which the hardening made non-excluded, so it no longer proved anything about excluded entries while the ADR claimed the caps were test-pinned. It now pads __pycache__/f{i}.pyc, with the arithmetic re-derived by execution (capability.json + the still-counted __pycache__ dir + N). BUNDLE_MAX_TOTAL_BYTES had zero coverage at all and is now pinned by a sparse 32 MiB __pycache__/big.pyc that must still trip the size cap — the test that proves exclusion did not become an unbounded-bytes hole. Added the parity assertion CLAUDE.md's Generative Fix Divergence rule requires for the two isSafeHookScriptPath copies, and proved it can fail: mutating one BUILT copy to drop .pyo made the parity check report the divergence. Also pinned semantics that were correct but untested and would have survived mutation — __pycache__/sub/x.pyc stays hashed (the parent resets to sub, which is the recursion threading itself), .pytest_cache/y.pyc stays hashed, and .pyo in both directions, which was a free surviving mutant. Changeset rewritten: it still described the rejected wholesale-exclusion semantics. Refs #3631 * chore(3631): backfill changeset PR number (#3650) --------- Co-authored-by: sim <sim@local>	2026-08-18 22:35:54 -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
Tom Boucher	e7855bc217	fix(#1459): user-owned consent store gates third-party capability activation; env/cwd in disclosure; loader validator parity (#1473)	2026-06-20 01:59:03 -04:00