8f674281fd87bbe79346f26a306fd3d8767a56d7
1007 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
8f674281fd |
chore(#3875): sweep the spent ack fragments and automate the sweep (#3877)
* chore(#3875): sweep the spent ack fragments and automate the sweep
next has been red on every push since
|
||
|
Tom Boucher
|
1863f5569c |
enhance(#3871): the state transaction — mandatory snapshot, open()/rebuild() (#3874)
* test(#3871): failing-first regressions for the dropped curated progress block Pins ADR-3473 §8.6 / #3756 at the consumer's output: state record-session and state add-decision on an archived-milestone project drop the curated progress frontmatter entirely, exit 0, and report nothing. Reproduced against the real CLI before writing the tests, not inferred from the issue text. Also adds the unit-level probe that applyStatePreservation's preserve-always row is inert on a resyncing write, and an over-preservation guard that an empty project is never inflated. Refs #3871 * feat(#3871): make the STATE.md pre-write snapshot mandatory via open()/rebuild() ADR-3473 §8.6. StatePreservationInput's nullable preFm and the always-present preFmSnapshot were the same extractFrontmatter call, one of them nulled on resync — a policy flag baked into a snapshot. Both collapse into a single StateTransaction whose snapshot cannot be absent: openStateTransaction() applies preservation, rebuildStateTransaction() does not, and both carry the snapshot because the reporting phase needs it either way. An absent snapshot is now a construction failure; an empty one stays legal, because that is what a document with no parseable frontmatter honestly has. writeStateMd requires a rebuild transaction, which types ADR-3408 §8.3's closed exception list at both call sites (state sync, health --repair) instead of matching them as strings in a ratcheted baseline. Fixes the dropped curated progress block: an all-zero or absent derived total set is an unmeasured scan, not a measurement, so the curated block stands. Also fixes two defects surfaced while building — preserve-always reported a mutation even when it restored an identical value, and it re-entered the curated object by reference, which would alias the snapshot the next phase diffs against. Refs #3871 * fix(#3871): close the three remaining subsumed defects and restore the arm the type does not replace Review of the first two commits found four things. The guard shrink deleted the seam-bypass axis whole, but only its writeStateMd( arm became redundant. Its other arm catches a call site re-assembling syncStateFrontmatter + applyPostSyncPreservation instead of the owned composition, which the transaction type does not make unrepresentable and which #3469 found live. Restored as findCompositionBypasses, terminal rather than ratcheted. Three of the four issues this phase claims were untouched. All three are the epic's own shape and are fixed at the seam: current_phase_name is reasserted from the curated value when the caller names none, and cmdStateJson stops carrying a hand-maintained list parallel to FIELD_CLASSIFICATION and projects it instead. The construction failure that is the point of this phase had no test. Every enumerated matrix row now has one, including the measured-versus-unmeasured coercion boundary and a seeded property that no curated key is ever dropped. ADR-3473 §8.6 said the guard 'keeps only its raw-write check'. Verified against next: there was no raw-write check, and four other checks it does not name. Amended in place with the evidence. ARCHITECTURE.md separately advertised a preservation policy the code had deleted. Refs #3871 * fix(#3871): do not let the unmeasured-scan rule block an explicitly-requested resync The remote matrix caught over-preservation, the failure this phase's own negative space says must not happen. state update Progress re-derives the block from the body the caller just rewrote; on a project with no phase dirs the derivation yields zero totals, the unmeasured rule read that as 'the scan measured nothing', and the stale curated percent was restored over the resync the user asked for. preserve-always already said what the missing condition was: never overwrite unless the caller explicitly names this field. explicitProgressField carries it and is derived from shouldResyncStateProgress, not set by hand at a call site, so it cannot drift from what the caller asked for. Two defects found in the same mechanism and fixed with it. readModifyWriteStateMd enumerates its option keys, so a new option was silently dropped rather than rejected. And the raw-write axis captured its first argument up to the first comma, which lands inside a nested path.join, so a write to a STATE.md literal was invisible to it — the prove-it-can-fail test caught that one immediately. No test assertion was weakened; all three frontmatter rows encode #3242, #1969 B3 and #1972 and stand unchanged. Refs #3871 * docs(#3871): record why the raw-write check is kept, not why it was named The amendment justified findRawStateWrites as 'written because §8.6 requires it to exist', which is cargo-culting the contract and would have been the wrong reason to keep anything. The real reason is that writeStateMd acquires the STATE.md lockfile and a raw fs.writeFileSync acquires nothing, so this is a lock bypass and lost-update is the #500/#905/#1230 family — and after this phase it is the one reachable path into the file that nothing else covers. Also records why ADR-3408 §8.6's deletion of the 'clear' policy is not the precedent it looks like: 'clear' was dead vocabulary in a closed enum, this is coverage of a reachable path. Refs #3871 * chore(#3871): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
382bf7c423 |
fix(#3706): deliver the resolved reasoning effort to OpenCode subagents (#3867)
* test(#3706): failing-first coverage for OpenCode variant emission and frontmatter escaping * fix(#3706): emit the resolved reasoning effort as OpenCode's variant key `query resolve-execution` resolved an effort level for every agent, but the OpenCode bake wrote only `model:` — the effort never reached the generated agent, so subagents ran at whatever the runtime defaulted the model to. This is the effort-side twin of the model-side defect fixed in #3705. The key is written only when an `effort` block is actually configured. `resolveInstallTimeEffort` always returns a level (the catalog default is `high`), so gating on its return value would stamp `variant: high` into every existing OpenCode install — and OpenCode resolves a variant name against a `variants` map in the user's `opencode.jsonc`, so a value nobody declared is not a safe default. Gating on `readGsdEffectiveEffortConfig` keeps installs that never asked for effort routing byte-identical. Kilo does not receive the key: `EFFORT_ARGV` declares surfaces for claude, opencode and codex and has no kilo entry. This is deliberately asymmetric with the model side, where #2794 J8 requires the two runtimes to resolve alike. Both frontmatter sinks now route through `frontmatterScalar`, which quotes and escapes any value that is not a plain scalar. The raw interpolation predates this change, but it was already shown by execution during the #3705 security review to let a config value containing a newline inject additional top-level keys (`tools:`, `permission:`) into a generated agent file. This change adds a second write to that sink, so it is closed here rather than doubled. * fix(#3706): quote frontmatter values YAML would not read back verbatim Self-review of the predicate added in the previous commit. Treating /^[A-Za-z0-9._:/@+-]+$/ as 'safe to emit bare' answers the wrong question: a value can match it and still not round-trip. - A leading '@' is a YAML *reserved* indicator and may not open a plain scalar at all, so a scoped ID like '@org/model' emitted bare is a parse error, not an ambiguity — the whole agent file becomes unreadable. - 'no' / 'y' / 'off' / 'null' resolve to booleans and null, so a variant with one of those names would match no entry in the user's variants map. - '12:30' resolves to 750 under YAML 1.1 sexagesimal, and ':' is legal mid-identifier here, so the form is reachable rather than contrived. Real model IDs pass every clause and stay bare, so already-generated files remain byte-identical. * fix(#3706): route variant through the declared effort seam and cover the live path Addresses six findings from the isolated review, all confirmed by execution. The tests were the serious one: they required `../bin/install.js` while the fix landed in src/, which compiles to gsd-core/bin/lib/. They exercised a different copy of the converter than the one the bake actually uses, so the whole suite was green-by-construction against unchanged code and the remote run failed all 13. Every case now runs against BOTH copies from one table, which doubles as the parity assertion the generative-fix note in runtime-artifact-conversion.cts asks for, and bin/install.js carries the mirrored change. Emission no longer hand-rolls the value. It goes through `renderEffortArgv`, the declared OpenCode effort seam (EFFORT_ARGV.opencode: its own supported set and clamp). That is what rejects a level that is not a wire value — above all `inherit`, which per #3533 (10d) means "omit the key and follow the host default" and was previously written literally, naming a variant that cannot resolve. Reachable two ways, both now pinned: an agent_overrides entry and a routing_tier_defaults entry. A bare effort.default does NOT reach a tiered agent (the #3531 tier ladder answers first), so a test written against `default` alone asserts nothing — that is pinned too. The plain-scalar decision moved into frontmatter.cts beside `scalarNeedsDoubleQuoting` rather than sitting next to it as a second, weaker predicate. `agentScalarNeedsDoubleQuoting` is a documented superset: it adds a trailing `:` (read as a nested mapping key, which fails the whole frontmatter), boolean/null words, and numeric-looking values including YAML 1.1 sexagesimal. Docs now state the cascade plainly: the gate is on effort being configured at all, not on the individual agent being named, so every generated OpenCode agent gets a variant line once any effort block exists. * test(#3706): assert the two frontmatterScalar copies cannot diverge A hand-picked adversarial corpus plus a fast-check property over YAML-significant strings, both run against bin/install.js and the live src copy. Verified the property can actually fail: mutating one copy's quoting rule is killed well inside the run budget. * fix(#3706): close the review findings — predicate, seam, and dead mirror Third review round; every item below was confirmed by execution. The scalar predicate was wrong in two families, both found by a round-trip property test rather than by reading. Basing it on scalarNeedsDoubleQuoting dropped the "first character must be alphanumeric" clause, so `~`, `.inf`, `.nan`, `+1`, `-0` and `.5` went out bare and came back as null/floats/ints; and that base predicate only inspects the FIRST character, so an embedded `: ` (a nested mapping, i.e. a parse error) or ` #` (a comment, i.e. silent truncation) also passed. Dates round out the set: `2026-08-25` opens alphanumeric, survives every other clause, and YAML resolves it to a Date. The property now asserts the contract directly over generated values instead of trusting an enumerated character list. The bin/install.js mirror is gone. Its premise was false — install.js already requires bin/lib at :65 — and it was unreachable besides: install.js's convertClaudeToOpencodeFrontmatter has no `isAgent: true` call site, because its agents path resolves converters from the compiled module. It was a third copy of the YAML rules serving a test rather than a caller, so the file is back to origin/next and the tests target the live copy only. Effort clamping moved to `clampEffortForHost`, which renderEffortArgv now delegates to. The layout was calling renderEffortArgv with a hardcoded 'argv' to borrow its clamp, which read as if the frontmatter key were gated on the invocation-time axis. It is not: claude declares effortSurface "argv" and independently bakes an effort: key. One capability table, one clamp, two channels that no longer pretend to be each other. Also corrects an earlier claim of mine: adding EFFORT_RENDERING.opencode would NOT have made `effort sync` write the wrong key, because it guards on the runtime name before it ever renders. The seam choice stands on other grounds. `effort sync` still skips OpenCode, but its stated reason claimed OpenCode "does not use effort: frontmatter", which this change makes false — so the message now says what is actually true. * docs(#3706): restate the changeset around the round-trip contract * fix(#3706): restore the changeset fragment belonging to #3809 An earlier commit in this branch picked the first file in .changeset/ by glob order instead of the fragment created for this issue, and overwrote agile-geese-squeak.md (PR 3815 / #3809) with this change's body. Restored verbatim from origin/next; this change's text now lives in its own patient-cranes-parade.md, where it was created. * feat(#3706): maintain the OpenCode variant key from effort sync Install bakes the resolved effort into OpenCode agent frontmatter as `variant:`, so `effort sync` has to maintain it or a config change only takes effect on reinstall — and its skip message claimed OpenCode does not use frontmatter effort at all, which this issue made false. cmdEffortSyncOpencode mirrors the codex branch: resolve per agent, clamp through the declared OpenCode capability, then write, strip, or skip. A null target means the key must not exist, which covers both "no effort configured" and "resolved to inherit or to an unsupported level" — the same states under which install writes nothing, so sync and install agree by construction. The frontmatter line-editors are key-parameterised rather than copied: setEffortFrontmatter / removeEffortFrontmatter are now thin wrappers over the same internals the variant path uses, and a test pins that the claude `effort:` behavior did not move. The child-process test harness fixes both HOME and USERPROFILE, so the hermetic-config assertions cannot pass vacuously on Windows. * fix(#3706): scope the frontmatter line editors to the matched block Found by the security review of the sync path, reported as correctness rather than vulnerability, and reproduced against pre-fix code before being fixed. Both editors matched the frontmatter with a regex that can match a block after a preamble, then derived the EOL and the opening-fence length from the START OF THE FILE. On a CRLF document with a preamble those disagree, the offsets shift by one byte, and the reassembled document comes back with a mangled fence (`---\rname: x`). Both now take the EOL from the matched block. `setFrontmatterKeyLine` additionally did a whole-file `/m` replace when the key already existed, gated only on the key being present in the frontmatter body — so a preamble line starting with the same key was rewritten instead of the frontmatter one. It now replaces inside the frontmatter span only, which is the hazard `removeFrontmatterKeyLine` already documented and guarded against. Neither is reachable from an install-written `gsd-*.md` (those begin at byte 0 with `---`), and both predate this change — but the editors are in this diff because #3706 key-parameterised them, so they are fixed here rather than left for the next caller to trip over. Three regression tests, each confirmed to fail against the pre-fix build. * fix(#3706): treat a present-but-empty key as present, and pin the real seam Fourth review round. The MAJOR one: both sync branches read the current value with `(.+?)`, which needs at least one character, so a key present with an EMPTY value read as "key absent". When the target was also null the code concluded "already correct" and skipped — leaving the key in the file, where it reads back as YAML `null`: exactly the unresolvable-variant state this change exists to prevent. Whitespace decided whether it fired, since `variant: ` matched and `variant:` did not. Presence and value are now separate questions at both the opencode and the claude branch. The OpenCode writer now follows the codex branch rather than the claude one: tmp file plus retryRenameSync with orphan cleanup, and a write failure skips that agent and is reported instead of aborting the sweep. Same granularity, same transient-Windows-lock exposure, so the hardened sibling was the right precedent. Also: the generic line-editors escape their interpolated key, the JSDoc stranded by the clampEffortForHost extraction is back on renderEffortArgv, and a cast that declared a nullable function as non-nullable is corrected. Tests close the gaps the review listed — empty value (both spellings), CRLF round-trip through write and strip, the symlink guard, a body line starting `variant:`, a file with no frontmatter, and the YAML classes that actually broke the predicate. The new layout-seam test drives the real stage() path and was verified to FAIL when `variant` is removed from the converter call; a seam test that survives cutting the seam is worse than none. * fix(#3706): clear the round-five review findings No blockers or majors this round; the repo's review gate is zero-tolerance, so the minors are cleared too. A duplicated key was only half-stripped: the strip regex had no `g` flag, so a frontmatter carrying the key twice lost one occurrence, reported success, and left the "a null target means the key must not exist" invariant false on disk — converging only on a second run. Such a document is already invalid YAML, so this is robustness rather than a live corruption path, but a successful sync has to leave the invariant true. A run in which every write failed still summarised as `ok`, so a caller could not tell "nothing to do" from "everything failed". The OpenCode branch now reports `failed` when any write failed. The write-failure path was also the newest code in the change with no coverage at all; it now has a test that injects the failure by monkeypatching the write, per CLAUDE.md §4, rather than by chmod — mode bits do not bite under root in CI. `CodexEffortSyncWriteFailure` is renamed `EffortSyncWriteFailure` now that two branches share it. Removed a guard on the claude concrete path that was provably unreachable — no member of EFFORT_SET renders null there, so it read as protection that did not exist. The claude inherit path's presence check is load-bearing and untouched. Three stale statements corrected: the OpenCode result shape matches codex's, not claude's, now that it emits write_failures; the `thread()` test helper now calls `clampEffortForHost` so it genuinely mirrors the layout instead of merely claiming to; and a test helper restored `USERPROFILE` by assignment, writing the literal string "undefined" into the environment on POSIX — it deletes now. * fix(#3706): converge the set path, degrade on unreadable files, preserve mode Rounds five and six of review. No blockers or majors; the review gate is zero-tolerance, so the minors are cleared too. `setFrontmatterKeyLine` was the mirror of a defect already fixed in its sibling: `remove` was made global, `set` was not, so on a frontmatter carrying the key twice it rewrote the first and left a stale second. Last-wins YAML readers honour the stale value while the sync's own first-occurrence read reports "in sync" — permanently non-converging. It now collapses to exactly one occurrence, in the position of the first, so ordinary single-occurrence documents stay byte-identical (verified across seven shapes before and after). An unreadable agent file used to throw and abort the entire sweep, while a failed WRITE in the same loop degraded into a report. The OpenCode branch now reports read failures alongside write failures; the claude branch degrades to a skip without a new result field, because its shape is long-standing and widely consumed and one bad file aborting the sweep is the actual defect. The tmp+rename publish dropped the original file's mode — a plain writeFileSync preserves it, a rename does not — so a 0600 agent came back 0644. Both the OpenCode and the codex branch now carry the original's permission bits across the publish, masked with 0o7777: the raw stat mode includes the file-type bits, and POSIX leaves those unspecified for chmod. Linux is the only OS the remote matrix runs, so relying on Darwin's tolerance would have been untestable here. Also documents the `from` contract on EffortSyncChange (null means the key was absent, '' means present with an empty value — a distinction earlier rounds introduced and then collapsed in the output), adds OpenCode to the docs paragraph enumerating where the key is omitted under inherit, and records in a comment that the 'failed' summary reaches only raw mode and does not change the exit code, which is a CLI-contract change affecting all three branches and is deliberately not made here. * fix(#3706): guard the codex read, close the tmp permission window, rename the failure type Round seven, plus one thing I found myself. `cmdEffortSyncCodex` still had an unguarded `fs.readFileSync` — a read fault on one agent exited 1 and aborted the whole sweep. The claude and opencode branches were both guarded earlier this round and codex was missed, with the unguarded read sitting ten lines above the chmod block the previous commit did edit. It now reports read failures the way the OpenCode branch does, and a read failure flips its summary to `failed` — which write failures did not do there either, so both are corrected for consistency. The tmp file was created at the default mode and only tightened afterwards, so a 0600 agent's contents sat in a 0644 file for the length of the publish. I measured the window rather than assuming it, then closed it by passing the mode at creation. The chmod after the write is deliberately RETAINED and commented: the `mode` option only applies when the file is actually created, so a leftover tmp from an earlier crashed run would be truncated and reused at its old mode, and the chmod is what corrects that. `EffortSyncWriteFailure` is renamed `EffortSyncFileFailure` — it was typing a `read_failures` array, the same naming-lie the `Codex…` prefix had last round. Also pins the codex mode preservation with a test. It only writes on a path that genuinely rewrites the file, so the fixture is an Anthropic-flavoured model pin the sync strips, and the test asserts the content changed before checking the mode — otherwise it would pass on a sync that did nothing. * fix(#3706): guard the claude writes and share one escaping rule The security sign-off caught a comment of mine that was factually wrong: the new claude read guard said the failure is folded in "like the write path in this same loop does", and there was no write guard in that loop. Rather than correct the sentence, both claude write sites are now guarded the way the read is — a failed file is skipped, the sweep continues, and the raw summary token flips to `failed`. The JSON shape stays frozen deliberately, because it is long-standing and widely consumed; the token is the channel that can carry the signal without a compatibility risk, which is the reviewer's own suggestion. That makes all three branches consistent: reads and writes guarded everywhere, per-file failures degrade instead of aborting, and every branch reports `failed` rather than `ok` when something did not sync. `setFrontmatterKeyLine` interpolated its value raw while the install-side writer quoted through the shared helpers — two writers of the same frontmatter key disagreeing on escaping, the divergence class this repo requires closed. They now share one rule. Verified no churn: all six effort levels are plain scalars and emit byte-identically, with claude's documented minimal-to-low clamp the only difference in the table, exactly as before. * fix(#3706): publish claude agent writes atomically too Both reviewers found this independently, and it is data loss rather than a reporting gap. The claude branch wrote in place, so `fs.writeFileSync`'s O_TRUNC meant a post-open fault left the agent file truncated or half-written: an injected ENOSPC produced an empty file, and under `ulimit -f` a 60000-byte agent came back as 512 bytes of wrong content. The guard added earlier this round then counted that destroyed file as `skipped`, which in JSON mode is indistinguishable from "already in sync" — so a caller would have read the sweep as clean while an agent on disk was corrupt. It now publishes the way the codex and opencode branches already do: write to a tmp file created at the original's masked mode, chmod, then retryRenameSync, with the tmp unlinked and the agent skipped on any failure. The corrupting case is gone rather than merely reported, which matters because this branch deliberately takes no new result key. I had claimed all three branches were consistent after the previous commit. That was true for degradation and reporting and not for atomicity; the reviewer caught the overclaim. It is true now. Also sorts the claude file list, which the other two branches already did — readdir order is platform-dependent, so leaving it unsorted made the reported `changes` ordering differ across machines for identical inputs. * chore(#3706): backfill the changeset PR number pr:0 placeholder replaced with the real PR now that gh api returned it. * test(#3706): kill the frontmatter mutants this change introduced CI's Stryker frontmatter shard scored 60.58 against a break floor of 62. The cause is documented in the lane's own config, from #1882: this PR added a multi-clause predicate to frontmatter.cts and exported the escaper, but the tests constraining them live in tests/runtime-converters.test.cjs, which that shard does not run — so every mutant in the new code was uncovered there even though the behaviour is tested elsewhere. The fix is assertions that kill real mutants, per the repo's own instruction, not a lowered floor and not a Stryker disable: scripts/mutation-matrix.cjs is untouched. Each clause of agentScalarNeedsDoubleQuoting now has a true case AND a near-miss that must answer the opposite way, so flipping the clause fails a specific named test — alnum-first against `a-b`, trailing `:` against `foo:bar`, embedded `: ` against `a:b`, embedded ` #` against `a#b`, the word list against `yes1`/`nullish`, the numeric forms against `1a`/`0xzz`, the timestamp against `2026-08-25x`, plus the case-insensitive spellings that pin the `i` flag. escapeDoubleQuoted is pinned on exact output, including a case constructed so that escaping in the wrong ORDER yields a different string. Two of my expectations were wrong and are asserted as the code actually behaves: `12:99` is NOT quoted, because the sexagesimal alternative never range-checks minutes and so does not match — which is right, since YAML would not read it as sexagesimal either; and `20260825` is quoted by the numeric clause rather than the timestamp one, being a bare integer. * chore(#3706): ratchet the frontmatter mutation floor to 65 The lane measured 66.67 on PR 3867 after the mutant-killing unit tests landed — above its pre-change 63.35 baseline, not merely recovered. Step 3 of this file's own HOW TO UPDATE procedure says to set minScore = floor(measured) - 1 in the same diff, so 62 becomes 65 and the improvement is locked in rather than left free to slide back. The ledger of measured scores now records the new measurement, why the shard broke in the first place (logic added to frontmatter.cts whose only tests lived in a file this lane does not run — the same trap the #1882 note describes), and one discrepancy: step 3 also says to update "the matching RATCHET_BASELINE entry", but no such declaration exists in this file. The name appears only in that comment, so minScore and the ledger are all there is to update. * fix(#3706): update RATCHET_BASELINE alongside the raised floor The ratchet test caught the previous commit: it raised COVERED['frontmatter'] .minScore to 65 without updating the baseline that mirrors it, which is exactly the mismatch that guard exists to make visible in review. I had claimed RATCHET_BASELINE did not exist. It does — in tests/mutation-matrix-ratchet.test.cjs, not in scripts/mutation-matrix.cjs, which is the only file I searched before concluding it was a stale reference. The ledger comment is corrected to say where it lives and to record that the guard caught the error rather than leaving my wrong claim on the record. * docs(#3706): put the mutation ledger entries back under their own dates The 2026-08-25 measurement was spliced into the middle of the 2026-06-14 list, so adr-parser, config-schema, active-workstream-store and core-utils ended up sitting under the wrong heading and misattributing their measurement dates. That ledger is what a future change reads to calibrate a floor, so a wrong date there is not cosmetic. Each measurement is now under the date it was taken. Also drops the first-person account of my own mistake from the entry — the factual half (where RATCHET_BASELINE lives, and that it is updated in the same diff) is what a reader needs; the confession is not. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
86fa2917d7 |
enh(#3866): dispatch step and contribution hooks at verify:pre (#3869)
* test(#3866): pin that verify:pre must dispatch every hook kind verify-work.md's verify_pre_hooks step dispatches only `kind == "gate"`, so getWiredKinds reports verify:pre -> {gate} and gen-capability-registry rejects any capability declaring a step or contribution there. The verify lane is therefore closed to capabilities that want to contribute to what UAT covers rather than refuse to let it start. Failing-first: the step, contribution, and exact-kind-set rows are RED; the pre-existing gate row is a green regression pin so the new arms cannot orphan the arm verify:pre already had. Refs #3866 * feat(#3866): dispatch step and contribution hooks at verify:pre verify_pre_hooks dispatched `kind == "gate"` only, so getWiredKinds reported verify:pre -> {gate} and gen-capability-registry's validateHooksWired rejected any capability declaring a step or contribution there. A capability could refuse to let UAT start; it could not contribute to what UAT covers. Add contribution and step arms mirroring execute:wave:post, deferring to references/loop-hook-dispatch.md and carrying its ref.command in-context validation guard ahead of any shell-use prose. A verify:pre step is advisory: it never blocks the start of UAT and an erroring step is routed by its own onError. The gate arm and its check guard are untouched. Give extract_tests an additive consumption seam for the artefacts those steps declare via the existing steps[].produces field -- no new registry field, no new ordering, no invented filename. Manifest-supplied artefact names are validated in-context against an allowlist and resolved only inside PHASE_DIR. With no producing step the derivation is unchanged, pinned by test rather than asserted in prose. Review findings folded in: the artefact-name allowlist (isolated adversarial pass), the artefact-shape contract and the seam-inertness tests (spec axis), and the reference/how-to split so one constraint has one source of truth (standards axis). Closes #3866 * chore(#3866): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7bbbe495d7 |
docs(#3473): ADR-3473 — enforcement by construction, one owner per invariant (Phase 0) (#3870)
Phase 0 of epic #3473 ships this ADR alone. Every rule in §8 carries a status of Enforced or Required — Phase N. §8.1–§8.5 state the epic's B1–B5 criteria as contract rules with no phase assigned yet. §8.6–§8.8 assign Phases 1–3 to the STATE.md family that ADR-3180 and ADR-3408 left between them: the write seam's unvalidated input, its classification-excluded reporting, and the schema transcribed by hand into nine artifacts. Derivation authority is explicitly NOT in scope here — it lands as ADR-3180 Amendment 8, generalizing §7.5's already-locked sentence. Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
aa6c332b5a |
fix(#3701): resolve next_phase from the roadmap, selecting the numerically lowest successor (#3852)
* test(#3701): failing-first coverage for roadmap-order next_phase resolution * fix(#3701): resolve next_phase from roadmap order, keeping the disk scan for spelling and fallback * fix(#3701): select the numerically lowest successor in both scans, not the first row encountered * chore(#3701): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
fb2d122d7f |
feat(#3841): assert gsd-tools identity on every state-mutating verb (#3848)
* feat(#3841): assert gsd-tools identity before any state-mutating verb only this package publishes. The path-based branches — a project-local install, a runtime config directory — had no such guarantee; they trusted their configured location. This closes them. Mechanism: once resolution finishes, and before any verb runs, the preamble probes the tool it picked with `runtime-identity --raw` and matches the answer with a shell `case` pattern ANCHORED to the start of the compact payload (`{"packageName":"@opengsd/gsd-core"`). An unanchored substring match accepts the decoy `{"packageName":"get-shit-done-cc","note":"@opengsd/gsd-core"}`, which any colliding package could publish. The outcome is exported as the two-valued `GSD_IDENTITY_STATUS` (`ok`/`unverified`), so the gate is asserted on a VALUE rather than on warning prose. Rollout is warn-then-fail per the #3146 ruling: `unverified` prints one line naming BOTH causes and continues, because `no_identity_verb` cannot tell a foreign package from an `@opengsd/gsd-core` older than the verb, and at rollout the old-version case is the common one. The blocker was byte budget, not design. The preamble is inlined into 112 shipped files and several sat within single-digit bytes of frozen ceilings (`gsd-verifier.md` 16 bytes, `gsd-executor.md` 33, `execute-phase.md` 234); a first attempt broke five of them. What made room was collapsing the resolver's twenty near-identical `elif [ -f … ]` arms into one candidate-list helper (`_gsd_at`), which buys far more than the assertion costs. The preamble is now 2,624 bytes against 4,500 — a net 1,876 bytes SMALLER per inlined file, so every capped file moved away from its ceiling rather than toward it. No cap raised, no size-budget exception added, no override token emitted. Resolution order, every runtime-home probe, the `unset -f gsd_run` re-source fix, the fail-closed `exit 1`, and the `CLAUDE_ENV_FILE` persistence are all preserved byte-for-byte in substring terms; the snippet still begins with `_GSD_SHIM_NAME=` and still ends with `fi`, which the parity extractors anchor on. `gsd-core/references/gsd-run-resolver.md` is re-synced byte-equal. Also fixes two stale claims found in passing: CONTEXT.md and FEATURES.md both described an `[ -x ]` guard as the load-bearing re-source defense. That guard was tried and REMOVED in #3831 — it rejected the bare function name, fell through every branch, and hit `exit 1`, which kills a sourced caller's shell. `unset -f gsd_run` is the actual mechanism. Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3841): pair the anchor's brace by requiring a closed identity payload The matrix went red on `tests/new-project-mvp-prompt.test.cjs` — "new-project.md has unbalanced braces: net depth 2" — plus a knock-on report from its parent `bug #1516` describe, which is the same failure counted once at the child and once at the block. Root cause: that guard (:182-189, mirroring #3784 bd53925f) walks characters and increments on `{`, decrements on `}`, with no awareness of shell quoting. It scans `new-project.md` PLUS every `new-project/steps/*.md`, and both `new-project.md` and `steps/auto-mode-config.md` carry one inlined preamble copy — hence net 2 from a snippet that was off by exactly one. The unpaired brace was the `{` inside the single-quoted `case` pattern of the identity anchor, which is correct shell and invisible to a text scanner. Fix in the snippet, not the guard. The pattern now anchors at BOTH ends: `'{"packageName":"@opengsd/gsd-core"'*'}'`. That balances 51/51 with a brace that does real work rather than a cosmetic pair — a truncated payload whose prefix matches now fails too, where before it verified. Safe for any future additive field: a JSON object's own closing brace is always the last character, whatever type the last value has, which is pinned by two negative-space tests (a nested object and an array-valued last key must both still verify). Cost: +3 bytes, against the 1,873 the resolver fold already gave back. The alternative considered and rejected was dropping the literal `{` for a `?` glob. It balances too, but weakens the anchor from "must be an opening brace" to "must be any one character", and the anchor is the entire point. Two guards added so this cannot recur silently: - runtime-launcher-parity (F0) pins brace balance at the SNIPPET, so the next edit to that pattern fails on the file it broke instead of surfacing three files downstream in a test whose name mentions neither the launcher nor this issue. It also asserts depth never goes negative, since a `}` preceding its `{` nets to zero while being unbalanced at every prefix. - runtime-identity gains behavioral truncated-payload and trailing-garbage fixtures, so the added `}` is proven load-bearing rather than merely present. Verified: snippet 51/51 braces; new-project combined net depth 0; the seven other preamble-bearing files with nonzero depth are unchanged from merged next (their own prose, not the preamble, and not in any guard's scan set); all 112 inlined copies and the resolver reference re-synced byte-equal; sync:launcher idempotent on the second run. Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3841): backfill changeset PR number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
de95c03f72 |
fix(#3699): report why a derived frontmatter key was not written, and repair a missing body source (#3846)
* test(#3699): failing-first coverage for derived-key reporting and the case-D fallback * fix(#3699): report why a derived frontmatter key was not written, and repair a missing body source * fix(#3699): scope session-field writes to ## Session so an archived line cannot absorb the update * fix(#3699): resolve the session writer from body labels only, so a frontmatter key never writes the body * chore(#3699): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
36375513b9 |
feat(#3840): generate docs/FEATURES.md from per-feature fragments (#3845)
* feat(#3840): generate docs/FEATURES.md from per-feature fragments docs/FEATURES.md was hand-maintained, and every feature PR wrote into two shared mutable cells: the '### N.' heading whose integer was hand-allocated at authoring time, and the hand-maintained table of contents. Concurrent PRs all picked the same next integer, and two PRs adding differently numbered features still collided on the TOC. #3831 was renumbered 165 -> 166 -> 167 -> 168 across successive rebases, each collision also costing a full matrix verification run because the sha-keyed pass marker dies with the rebase. Mechanism: one fragment per feature at docs/features/<slug>.md carrying id/title/group (and an optional order) in frontmatter, consolidated by scripts/gen-features.cjs --write|--check into a marker-delimited region of docs/FEATURES.md that holds BOTH the TOC and every section body. Group headings and their order are derived too - a group sorts by its lowest-ordered member - so there is no shared registry to edit either; optional per-group prose lives in docs/features/_groups/<slug>.md. A contributor adds exactly one new file. Wired into regen:derived and lint:generated-sync alongside the eight existing generators, matching gen-adr-index.cjs's CLI shape and typed-REASON reporting. Migration froze all 168 existing numbers verbatim: identical section set, identical order, identical bodies. Two defects found in the tree are fixed inline rather than carried forward - the '## Related' block had been spliced into the middle of the document, orphaning §142's Reference line, and four inbound anchors were already broken on next (FEATURES.md#runtime-identity in two files, and #143-spec-phase-edge-completeness-probe off by one). Since the repo has no link checker, --check now validates every inbound FEATURES.md#anchor by resolved target, so that class cannot ship silently again; locale FEATURES.md files resolve elsewhere and stay out of scope. Refs #3840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3840): carry upstream §69 delta into its fragment and harden the generator Review found section 69 missing '[--strict]' and REQ-STATE-05/06 versus origin/next. Root cause was a stale base, not extraction loss: those lines landed in |
||
|
Tom Boucher
|
394bf384be |
fix(#3696): report the last_activity invariant and make the verdict gateable with --strict (#3844)
* test(#3696): failing-first coverage for the last_activity invariant and --strict exit status * fix(#3696): report the last_activity invariant and make the verdict gateable with --strict * fix(#3696): agree with the real reader on last_activity, and stop reporting structure as truncation * chore(#3696): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
6905726d9c |
chore(#3833): gate every PR compute lane behind a mergeability preflight (#3843)
* test(#3833): failing-first suite for the PR mergeability preflight * chore(#3833): gate every PR compute lane behind a mergeability preflight * fix(#3833): assert the preflight gate on parsed yaml and guard status-function if * fix(#3833): run stub-backed cli tests in-process to avoid a spawnsync deadlock * fix(#3833): fail the preflight open when its script is absent at the base sha --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
63abcface9 |
feat(#3146): resolve gsd_run so workflows cannot reach a foreign gsd-tools (#3831)
* feat(#3146): resolve gsd_run so workflows cannot reach a foreign gsd-tools The predecessor package get-shit-done-cc publishes a colliding gsd-tools bin whose phases.clear DELETES where this package's ARCHIVES, and both print success-shaped output against a gitignored .planning/ -- which is how #3129 cost a user 43 phase directories with no error and nothing recoverable from git. The launcher's PATH branch now resolves gsd_run, published only by this package and self-locating via its own symlink chain to the sibling shim, instead of the colliding gsd-tools. A foreign handler becomes unreachable from PATH, and when no gsd_run is reachable the resolver fails closed rather than falling back -- that fallback was the vulnerability. This is smaller than the branch it replaces, which matters: the preamble is inlined into 113 shipped files and agents/gsd-verifier.md sits 2 bytes under a red-line size cap. unset -f gsd_run leads the preamble so a re-source is idempotent. Without it, command -v finds the shell function, returns a bare name, and the resolver falls through to an exit 1 that kills a sourced caller's shell. Adds gsd-tools runtime-identity, a manual diagnostic reporting this runtime's package coordinates over the baked package-identity (#498) and readHostVersion, with a strict total classifier: only a JSON object with an exact packageName verifies, since JSON.parse admits 0/"str"/[]/null/true. An inlined identity assertion was built and reviewed first, then withdrawn -- it breaks five frozen size ceilings and no assertion fits in 2 bytes. Closes #3146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3146): stop sync:launcher relocating a deliberate preamble placement Pre-existing defect, surfaced by this PR because sync is a no-op unless the snippet content actually changes. transformFile inserts the preamble into the first block that CALLS gsd_run, but gsd-core/workflows/explore.md deliberately places it in a bootstrap-only block that DEFINES gsd_run without calling it -- its own comment explains why: declining the research offer must not leave Step 5's commit call unbootstrapped. Stripping empties that block of calls, so the preamble migrated forward and broke the define-before-use invariant tests/explore-command.test.cjs pins. Reproduced on a pristine origin/next checkout with the base snippet and base file, so this was not introduced here. The insertion target now honours a block that already carried the preamble, falling back to the first calling block for files that have none yet. Adds a behavioral regression test over a two-block fixture. Also updates three runtime-launcher-parity tests that pinned the removed PATH fallback to gsd-tools. Their intent is preserved -- the PATH stub is renamed gsd_run so it is reachable by the new resolver, and the RUNTIME_DIR-wins test still asserts the stub is never invoked. Fixture shebangs move to an absolute /bin/sh, because the fixture PATH is deliberately restricted and #!/usr/bin/env sh could not resolve. Refs #3146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3146): backfill changeset PR number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3146): document the FEATURES.md section-numbering practice The monotonically increasing section number in docs/FEATURES.md is the most frequent merge-conflict source in this repo, and it has TWO conflict cells, not one: the ### N. heading and the hand-maintained table of contents. Two PRs adding differently numbered features still collide on the TOC, so renumbering alone does not make a branch safe. This branch alone was renumbered 165 -> 166 -> 167 -> 168 across successive rebases. Adds a CONTRIBUTING section stating the practice: allocate the number last, never pre-emptively renumber, take max+1 after a rebase and update the TOC in the same commit, and never renumber someone else's section. Fork contributors are told explicitly they may leave the number to a maintainer at merge rather than chasing the counter. Agents are told to lease the allocation and to include the file in their published touched set. Records the durable fix as planned rather than pretending it exists: FEATURES.md should be generated from per-feature fragments the way CHANGELOG.md is generated from .changeset/, and the way tests/emitted-drift-acks/ works (#2914). Also renumbers this branch's own section to 168, leaving 167 to the PR already in flight. Refs #3146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
aaf47c5fc2 |
fix(#3691): let every reviewer lane take a prompt cap, and make the documented global resolve (#3832)
* test(#3691): failing-first coverage for the reviewer prompt budget No prompt cap can reach any CLI reviewer lane, by any configuration. Two independent defects compound: all nine `transport: spawn` lanes declare `promptBudgetKey: null`, so `budgetFor` returns on its first line; and the documented global `review.max_prompt_tokens` is advertised in the schema manifest but declared nowhere, so the resolver never materializes it and `budgetFor`'s fallback is dead code. Adds to tests/reviewer-config-federation.test.cjs, which already owns the per-reviewer budget config-set/config-get idiom: - a CLI lane inherits the global cap (RED: reports null) - an http lane with the -1 sentinel inherits the global cap (RED: reports null) - the resolved review surface carries max_prompt_tokens at all (RED: absent) - per-lane overrides the global on a CLI lane - the sentinel boundary: -1 inherits, 0 means do-not-trim and must NOT read as unset, 1 is the smallest real budget — the regression budgetFor's own comment warns about - anti-tightening pins that must stay green: an empty config leaves every lane null, the three existing budgeted lanes are unchanged, and config-set still rejects a per-reviewer key naming something that is not a declared lane - a fast-check property over the resolution contract itself, with -1, 0 and non-finite inputs generated explicitly rather than left to chance Every row was reproduced by hand against the real CLI before being written, so the RED/GREEN split is observed rather than predicted. Refs #3691 * fix(#3691): let every reviewer lane take a prompt cap, and make the global resolve No prompt cap could reach any CLI reviewer lane, by any configuration. Two independent defects compounded. The nine spawn-transport lanes — claude, coderabbit, antigravity, cursor, gemini, codex, kimi-code, opencode, qwen — declared `promptBudgetKey: null`, so `budgetFor` returned on its first line and `review-lane plan` reported `promptBudget: null` no matter what was configured. Each now declares `review.max_prompt_tokens_per_reviewer.<slug>` with the same `-1`-is-unset sentinel the three local-server lanes already use. Separately, the central `review.max_prompt_tokens` was listed in the schema manifest's validKeys and documented as a supported setting, but declared nowhere — the resolved surface is built from capability declarations plus the defaults manifest, and neither carried it. `configGet` returned undefined and `budgetFor`'s documented fallback was dead code. It is now declared with a `null` default, exactly as docs/CONFIGURATION.md already specified, so the default behavior is unchanged: nothing configured means nothing trims. Two things the diagnosis had not predicted, found and fixed while implementing: - `REVIEWER_LANES` in src/review-lane-descriptor.cts is a second, hardcoded registration site that `mergeReviewerLanes` prefers over the capability registry on a slug collision. Editing only the capability files left every CLI lane still null. Both sites now agree. - The generated `gsd-core/bin/lib/capability-registry.cjs` was stale and masked the capability edits; regenerated with `npm run gen:capability-registry` rather than hand-edited. docs/CONFIGURATION.md said "Only lanes that declare a budget key accept one — today ollama, lm_studio and llama_cpp". That is false as of this change and is corrected rather than left to rot. The trim-versus-refuse question the issue raises is deliberately not taken up here: the refusal path already exists for the case that matters — a reviewer whose minimum set exceeds its budget is skipped rather than sent a misleading prompt — and trimming above that floor is the documented, shipped design of the feature. Changing it would alter behavior for the three lanes that already work, which is not what the issue asks for. Fixes #3691 * fix(#3691): document the new global and narrow an invariant this change obsoleted The full suite surfaced two consequences of giving every CLI lane a budget key. `review.max_prompt_tokens` entered CONFIG_DEFAULTS without a matching entry in the planning-config reference, which config-field-docs guards. Documented, including the sentinel semantics a reader needs: a per-lane value overrides the global, `-1` means unset and inherits it, and `0` means "do not trim that lane" and is not unset. The #2797 federation guard asserted that "a lane with no model flag and no host owns no config keys". That held only because budget keys existed solely on the three local-server lanes, all of which have hosts. A lane can now legitimately own a config key for a third reason, so qwen tripped it. The assertion is narrowed rather than weakened: such a lane must still own no model key and no host key, and may own at most its own `review.max_prompt_tokens_per_reviewer.<slug>` — never another lane's. That is strictly more specific in the dimensions that still matter. Proven to still bite: hypothetically giving qwen a `review.models.qwen` key fails it with `model/host: review.models.qwen`. The name and comment cite #3691 for why the premise changed, so a reader sees a deliberate narrowing, not erosion. Checked the sibling assertions in that describe block; the other three do not rest on the obsolete premise and are untouched. Refs #3691 * fix(#3685): port the write-flag content-change contract to its three sibling sites #3685 fixed `phase complete`'s `roadmap_updated` / `state_updated`, which reported `fs.existsSync(path)` rather than whether the transaction wrote anything. Three sibling sites carried the identical defect and are ported here. - `cmdPhaseRemove` reported `roadmap_updated: true`, hardcoded. `updateRoadmapAfterPhaseRemoval` now returns whether the content changed and the flag reports it. #2640/#2974 already fixed `state_updated` at this same call site and left this one behind, so the correct shape was adjacent. - `cmdMilestoneComplete` reported `state_updated: fs.existsSync(statePath)` — byte-identical to #3685's bug in a different command. - `cmdMilestoneComplete` reported `milestones_updated: true`, hardcoded, never consulting the MILESTONES.md write. `gsd-core/workflows/remove-phase.md:100` extracts `roadmap_updated` for display and never branches on it, so the flip from always-true to content-based changes no workflow behavior. Verified by reading the step, not assumed. One trap found while implementing: the obvious in-memory `finalContent !== originalStateContent` comparison — copying `cmdPhaseComplete`'s shipped shape verbatim — gives a FALSE POSITIVE for milestone completion. `platformWriteSync` normalizes Markdown at write time, and the milestone-closure transform regenerates `## Current Position` fresh on every call, so its pre-normalize output always differs from the already-normalized file on disk even when the persisted bytes are identical. The comparison is therefore made against the post-write on-disk content. `cmdPhaseComplete`'s own comparisons are left untouched — their repeat-no-op tests pass, so they are not exposed to this artifact. `milestones_updated` has no reachable no-op: the MILESTONES.md write unconditionally appends an entry every call. Only the true direction is pinned, documented inline rather than faked with a passing test. Refs #3685 * fix(#3685): compare write-flag content through the writer's own normalizer An independent reviewer disproved a claim made while porting #3685's contract to its sibling sites: that `cmdPhaseComplete`'s comparisons were not exposed to the Markdown-normalization artifact already diagnosed in `cmdMilestoneComplete`. `platformWriteSync` normalizes on write — CRLF stripped, blank-line runs collapsed, a blank line inserted after a heading, a single trailing newline enforced. Every flag that compares the PRE-normalization in-memory string against the on-disk pre-image can therefore report a change when the persisted bytes are identical. `cmdMilestoneComplete` had been worked around by re-reading the file after the write; the other sites compared raw strings. All of them now go through one exported seam, `contentChangedAfterNormalize(filePath, before, after)`, which normalizes both sides exactly as the writer does. That removes the extra disk read the milestone workaround needed, and makes the sites agree by construction rather than by four independent implementations of one rule — the divergence the repo names as an anti-pattern. Reachability, stated precisely rather than uniformly: the seam is load-bearing at `cmdPhaseComplete`'s `roadmapUpdated`, `requirementsUpdated` and `stateUpdated`, where section-rewrite logic genuinely regenerates content into a different-but-normalization-equivalent shape. At `updateRoadmapAfterPhaseRemoval` it is defense-in-depth: the no-match branch never reassigns `content`, so the raw comparison was already correct there. The first analysis claimed the reverse; this is the corrected finding. Also fixes an unsound test premise the remote suite caught. The byte-identity precondition in `roadmap_updated is false when ROADMAP.md comes out byte-identical` asserted against a hand-authored, un-normalized fixture — so the very first write reformatted it and the file could not come back identical. The fixture is now written already-normalized, so the assertion compares a normalized pre-image against a normalized post-image and still fails if the flag regresses to a hardcoded `true`. Not platform-specific; it reproduces on macOS too, and the earlier local check simply never exercised it. The sibling true-direction and milestone tests were checked for the same premise and do not share it — they assert `notEqual`, or compare two post-write states produced through the same normalizing seam. Refs #3685 * chore(changeset): backfill PR number for #3691 fragment --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c933184b97 |
enhance(#3172): require a stated failing direction for every automated acceptance command (#3825)
* test(#3172): failing-first suite for the stated failing-direction probe Pins the <fails_when> pairing walk, placeholder denylist, MISSING sentinel exemption, degraded-read contract, CLI arm and the plan-authoring contract text. RED by construction: the module exports it requires do not exist yet. Executed on the remote runner. * feat(#3172): require a stated failing direction for every automated acceptance command Every runnable <automated> command now carries a <fails_when> sibling naming what output constitutes failure. A command with no expressible failure mode is not an acceptance test: it reads as rigour and is not falsifiable. - verify-command-grounding gains a failing-direction probe sharing the existing <automated> grammar, MISSING sentinel and walk guard rather than copying them - gsd-tools check verify-failure-directions <N> backs it; plan-phase dispatches it and hands the JSON to gsd-plan-checker check 8f - Dimension 8 detail extracted to references to stay under the agent size cap Verified on the remote runner. * fix(#3172): close four review findings in the failing-direction probe - MISSING_SENTINEL_RE matched an env-var assignment prefix (MISSING=1 cmd), so a real command was exempted from the new blocking gate. Tightened the SHARED constant rather than adding a second copy. - Both token regexes scanned to EOF on unclosed openers (O(n^2), 1562ms at 40k). Bodies are now non-crossing; 1ms, byte-identical on well-formed input. The pre-existing AUTOMATED_BLOCK_RE carried the same defect and is fixed here too. - probePhaseFailingDirections reported status 'ok' when one plan was unreadable, conflating 'could not look' with 'nothing to report'. - Extracted the phase-resolution block both check arms had copied verbatim. Also corrects a docs/AGENTS.md dimension list stale since #2401. Verified on the remote runner. * fix(#3172): project the planner rule onto the spawn contract, settle emitted bookkeeping The remote runner refuted the planner-side edit. agents/gsd-planner.md is frozen under a 49152-LF-char cap asserted by four suites and sat at 49,146 — six chars of headroom — so the +537 of authoring rule blew it. #3297/#3645 already settled where such a rule goes: the planner spawn contract in plan-phase.md, beside <tracked_source_paths>. The agent file is reverted to origin/next verbatim. - plan-phase.md gains <failing_direction_contract>; tests row 30 now asserts the contract there and row 30b guards the freeze in both directions - plan-phase.md growth acknowledged by APPENDING to the 3409 fragment, per the precedent that two ack sources may never name the same path - install-tree fixtures regenerated for the three new reference files Verified on the remote runner. * chore(#3172): backfill PR number into the changeset fragment pr:0 -> pr:3825 now that the PR exists. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
596540f864 |
feat(#3227): publish machine-readable state contract at step boundaries (#3824)
* feat(#3227): publish machine-readable state contract at step boundaries Adds src/state-contract.cts, a best-effort publisher that writes .planning/state.json (contract 1.0.0) at 11 step-boundary commands, so external tools read a versioned contract instead of parsing STATE.md and ROADMAP.md heuristically. Composes existing owners rather than re-deriving: phase rows come from a new locateProgressTable extracted from deriveProgressFromRoadmap (so the snapshot can never disagree with GSD's own progress counters), milestone identity from getMilestoneInfo, and next from classifyProject. Owners are required lazily to avoid the state -> state-contract -> smart-entry -> state require cycle. Also fixes a pre-existing defect in scripts/lint-test-file-count.cjs (maintainer-approved as a second concern): testEffectivePrefix never stripped the suite qualifier, so 65 dotted test files counted against no module and 9 mis-bucketed into a shorter one. Allowlist re-baselined for the 74 files the gate can now see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3227): backfill PR number into the changeset fragment pr:0 -> pr:3824 now that the PR exists. Doc-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3227): shape hostile-name fixtures away from the scan corpus The two hostile-input fixtures used a literal phrase from scripts/prompt-injection-scan.sh's corpus, so CI's Security Scan redded on this file. These tests assert that an arbitrary phase name round-trips into state.json as inert data -- the property holds for any string, so the injection flavor is illustrative, not load-bearing. Reshaped to a hyphenated fake instruction tag, which stays hostile-looking while matching none of the scanner's patterns. Allowlisting the file was rejected: that mechanism is for suites whose subject IS injection defense, and it would blind the scanner to this whole file permanently. See DEFECT.PROMPT-INJECTION-SCAN-COLLISION. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3227): ratchet the state-contract mutation floor to its measured score The module was registered at minScore 50, the ratchet's minimum permitted floor for a newly-registered module whose score had not been measured. This PR's own Stryker shard measured 66.25% (run 32769289750, job 97565813640), so the floor moves to floor(measured) - 1 = 65, per the rule the registry documents. 66.25 is below TARGET_MUTATION_SCORE (80), so this stays a ratchet candidate: raise as the tests improve, never lower. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
a2387a0545 |
feat(#3034): add opt-in parallel reviewer lanes (#3822)
* test(#3034): failing-first coverage for opt-in parallel reviewer lanes Executes the real invoke_reviewers dispatch block from review.md against a stubbed gsd_run seam rather than pattern-matching the workflow text, so the two properties that actually carry risk are observable: that every lane is joined before aggregation, and that concurrent lanes cannot tear a line in gsd-review-lane-results.jsonl. Concurrency is proven by a barrier fixture, not by elapsed time -- each stub lane blocks until all lanes have checked in, which can only complete if they overlap. Red against the current sequential dispatch, by design. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3034): add opt-in parallel reviewer lanes Reviewer lanes within one review pass inspect the same immutable plan snapshot and have no dependency on one another, but were dispatched strictly one at a time, so a multi-reviewer pass cost roughly the sum of its lanes. The serialization is a deliberate protection against provider rate limits, so it stays the default; review.parallel_lanes opts a project out of it. The loop body is hoisted into run_review_lane so the sequential and concurrent paths share one body -- two hand-synced dispatch bodies is the divergence class ADR-2782 spent a phase deleting. Each lane writes a slug-scoped result file, concatenated in selection order after the join: concurrent O_APPEND is atomic only below PIPE_BUF, and write_reviews parses that JSONL to render the models:/model_sources: frontmatter, so a torn line is a broken REVIEWS.md rather than a cosmetic log defect. Aggregating in selection order also keeps the artifact byte-identical between the two paths. The guard is strict equality on "true" and falls back to sequential when config-get fails -- the opposite polarity from the commit_docs guard, because failing open here fires the very requests the default prevents. Also corrects docs/COMMANDS.md and its four locale mirrors, which described --all as running every configured reviewer in parallel when dispatch was in fact sequential. Closes #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3034): de-duplicate dispatch slugs and scope lane locals Review finding (Standards axis): a slug repeated in SELECTED_REVIEWERS would put two concurrent background jobs on the same > -truncated per-lane result file. The shared-append form this replaced could not corrupt itself that way, so de-duplicating is what keeps the concurrent path no worse than the sequential one. Selection de-dupes today -- the roster is a Set and review.default_reviewers normalizes lowercase-unique -- but reachability analysis is not a contract, which is the same reason the roster derivation itself is guarded. Splitting once into DISPATCH_SLUGS also removes the duplicated tr-split the same review flagged: the dispatch and aggregation loops now share one list, which is what guarantees they walk the same slugs in the same order. A plain string accumulator rather than an array, because zsh and bash disagree on array indexing and this block runs under both. Also scopes run_review_lane's locals. Not a live fix -- each dispatched call already forks its own subshell -- but it makes the isolation a property of the function rather than of the dispatch mechanism happening to fork. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3034): acknowledge review.md growth, drop spent 2295 ack The differential attribution gate reported review.md growing 4173 bytes (30712 -> 34885) with no live acknowledgment. Adds the per-PR fragment it asks for, naming only the one path it reported. Deleting tests/emitted-drift-acks/2295-resolved-model.json is required, not opportunistic. That fragment declared review.md and nothing else, and its ripple is already absorbed into the base, so it is spent -- it can no longer clear anything, which is why the gate still reported review.md as unacknowledged. It could not simply be left alone either: two ack sources may never name the same path, so it blocked this PR's fragment outright. CONTRIBUTING is explicit that a fragment whose last entry is removed gets deleted with it, because an empty fragment signals nothing while its presence reads as a live alarm. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3034): backfill changeset PR number Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
a84f756303 |
fix(#3078): sweep all-spent ack fragments on next, name the collision remedy (#3823)
* fix(#3078): sweep all-spent ack fragments on next, name the collision remedy
`guard-no-ack-on-next` only ever watched the legacy tests/emitted-drift-ack.json.
#2914 exempted the fragment directory on the premise that a persisting fragment
"cannot conflict with any other PR". Fragments do not share a FILE, but they do
share a PATH KEY SPACE, and a path claimed by two sources is a hard failure in
the same script -- so a fully-spent fragment on next owns keys it can no longer
gate, and the next PR to grow one of those paths can declare it neither there
(spent) nor in its own fragment (duplicate). Measured at the sweep: 45 fragments
owning 403 paths, up from 13/272 at triage 19 days earlier.
- `assertNoAllSpentFragments` fails a fragment only when EVERY surviving entry is
spent against the copy at HEAD^, so a partially spent fragment -- and the
re-arm-by-appending route #2639/#2993 ship on -- keeps working.
- `ackProse` duplicates the gate's zero-width/whitespace stripping across the
scripts-ship/tests-do-not line, bounded by a prose-parity test.
- The guard job's checkout takes fetch-depth: 2; at depth 1 HEAD^ is absent and
every fragment reads as brand-new, i.e. the guard passes vacuously.
- The duplicate-ack error now names both resolutions, since the guard is
post-merge by design and cannot stop the colliding PR.
- All 45 spent fragments deleted, 0000-legacy-migration.json included, and the
three tests that pinned its permanence corrected.
Verification is the remote runner (gsd-test), not a local suite.
Closes #3078
* fix(#3078): make the prose-parity test two-sided, cover the git seam, base on the pre-push tip
Three review findings, all fixed:
- The parity test was a tautology: it checked ACK_INVISIBLE against a
hardcoded list matching its own definition, never against the gate. The
gate's INVISIBLE and its reason normalizer (hoisted out of diffEmitted as
normalizeAckReason) are now exported for that sole purpose, and the test
sweeps 0x00-0xFFFF against both surfaces. Mutation-checked: adding a
codepoint to one side and not the other now fails.
- resolveBaseRef, readFragmentAtRef and assertUsableBaseRef had zero direct
coverage -- the tests reimplemented the git reads in a local helper, so the
ls-tree-vs-show discrimination, the root-commit fallback and the
option-injection guard were never executed. All are exported and tested
against real temp repositories now, plus an end-to-end --base-ref subprocess.
- HEAD^ is not 'the state of next before this push'. The default branch allows
REBASE merges, so one push can carry N commits, and a 2-commit rebase-merge
whose first commit adds a fragment would be told to git rm it on the very
push that introduced it. CI now passes github.event.before via --base-ref and
fetches it explicitly; HEAD^ remains only the local fallback.
Also adds the safe.directory guard every other git call in this repo carries
(#2767), and stops naming the deleted migration fragment by filename in
CONTEXT.md, which tripped lint-removed-but-needed.
Refs #3078
* fix(#3078): keep the fragment directory alive after the sweep empties it
Sweeping every fragment leaves the directory untracked, and check-glossary-refs
then fails: CONTEXT.md references tests/emitted-drift-acks, which no longer
exists. The empty directory IS the intended steady state, so it has to survive
its own remedy.
Adds tests/emitted-drift-acks/README.md documenting the create/use/delete
lifecycle where a contributor actually meets it, matching the existing
tests/qa/smell-acks/README.md precedent. Every reader filters on .json, so the
README is invisible to the gate.
Also sweeps #3809's ack fragment, which the rebase onto origin/next brought in
and the new guard immediately reported as all-spent -- its own remedy applied.
Refs #3078
* fix(#3078): guard the added tests' git calls, drop a second fragment-existence pin
Both defects surfaced by the remote runner (linux-node24, 4/37445 failed).
- The new --base-ref E2E test ran `git rev-parse HEAD` against the checkout
without the #2767 safe.directory guard. The runner mounts the repo at a path
owned by another uid, so git refused every operation there with 'detected
dubious ownership'. Every git call the new tests make now names its own
specific directory as safe, via one local helper, mirroring safeDirArgs in
helpers/emitted-runtime.cjs.
- tests/agent-tracked-source-rule.test.cjs pinned the existence and contents of
the 3645 and 3409 ack fragments. That is a merged PR's paperwork, not live
behavior: once the growth is in next's baseline the acks are spent and this
PR's guard sweeps them. The third assertion pinned the hand-appended
workaround for the exact collision #3078 removes. Deleted; #3645's real
protection is the two behavioral tests above it, untouched.
Also restores #3809's ack fragment, which merged one commit before this branch.
Deleting an ack in the same window as its introducing PR races any consumer
whose baseline predates it -- the runner's container proved it, resolving
origin/next to
|
||
|
Tom Boucher
|
4b84be1da4 |
fix(#3683): wire gated learnings extraction into completion, align copy path (#3810)
* test(#3683): failing-first rows for learnings source resolution and wiring pins * fix(#3683): wire gated learnings extraction into completion, align copy path * test(#3683): register the learnings suite in the docs-guard lane, drop unverified markers * fix(#3683): close review findings — per-item parsing, readdir guards, docs paths * fix(#3683): route phase enumeration through the locator seam, fix assertion targets * fix(#3683): merge execute-phase ack into the 3003 fragment, fix fidelity targets * fix(#3663): replace the spent execute-phase ack entry with the 3683 re-arm * chore(#3683): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
4af59f8dd3 |
fix(#3662): resolve managed hook node runners at hook-fire time (#3790)
* test(#3662): failing-first suite for runtime-resolving hook runners * fix(#3662): resolve managed hook node runners at hook-fire time * fix(#3662): close review findings and document the resolver * fix(#3662): close adversarial and security review findings * chore(#3662): backfill changeset pr number * test(#3662): honor win32 skip return and platform-aware sh runner pin * test(#3662): pin the bare win32-claude sh-hook shape omitting the bash runner --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
cf15682d1c |
enhance(#3028): responsive Markdown separators instead of fixed-width rules (#3789)
* feat(#3028): responsive Markdown separators instead of fixed-width rules Stage banners, checkpoints, completion and error panels used fixed-width runs of box-drawing characters -- a 53-column heavy rule and a 62-column double-line box. Those runs are ordinary text to a Markdown-rendering host, so in a narrower pane they wrap and the border comes apart from the heading it framed. Shipped content now emits an ATX heading for a titled section and a blank-line-delimited --- for a break between sections, both of which adapt to the available width. The same convention is applied to the three code sites that built these strings at runtime: the UAT checkpoint renderer, the milestone-close audit report, and the TDD review checkpoint table. Removing the box also removes its only reason to exist -- the east-asian-width padding helpers that kept its right border aligned (checkpointBoxLine, displayWidth, isWideCodePoint, ZERO_WIDTH_MARK_RE, CHECKPOINT_BOX_WIDTH). RTL directional isolation is unchanged. The convention is specified in gsd-core/references/ui-brand.md and enforced across all shipped content by tests/responsive-separators.test.cjs. Refs #3028 * test(#3028): pin the heading form in checkpoint and audit-report assertions These suites asserted the exact box borders and the 62-column padded banner interior. With the box gone they assert the ### heading form, the --- break and the bolded instruction line, and each now carries a positive assertion that no box character remains -- which is what pins the fix rather than merely tolerating it. Language coverage is converted, not dropped: Japanese, Chinese, Korean, Hindi and Arabic all still assert their rendered banner, and the Arabic case still asserts the RTL directional isolates the box removal must not disturb. Adds a case for a banner longer than the old inner width, which previously produced a ragged border and now has none. Refs #3028 * chore(#3028): acknowledge execute-plan.md growth from the checkpoint display spec The checkpoint_protocol display spec described the drawn box; it now describes the heading, the --- break and the bolded action prompt, which costs 22 bytes (40111 -> 40133, 827 under the cap). Appended to the existing #3370 fragment rather than filed as a new one: a growth ack keys on the bare filename and #3370 already declares execute-plan.md, so a second source naming it would be a hard duplicate-key error. Same supersede-by-append route #3370 took for the spent #2652 fragment. Refs #3028 * docs(#3028): state the load-bearing half of the separator rule, and amend the zh-CN reference Review found three things. The rule as first written demanded a blank line above AND below every ---. Only the one above is load-bearing: it is what stops CommonMark reading the rule as a setext underline for the line above. The one below is cosmetic, because a thematic break is a leaf block. The rule now says that, with the reason, instead of asserting a stricter form the content does not keep. The zh-CN reference had received the mechanical box-to-heading swap but none of the prose behind it: it still claimed a 62-character checkpoint width and still listed --- among forbidden mixed banner styles, so it contradicted the convention it was translating. It now carries the separator section, the setext reasoning, the unconditional-vs-per-runtime rationale and a corrected anti-pattern list, in Chinese. The user guide asserted that a heading is not a degradation anywhere. That is an assertion, not a demonstration. It now says what was actually traded away in a plain terminal, points at the recorded rationale, and invites the report that would justify the capability flag instead. Refs #3028 * chore(#3028): backfill changeset PR number Refs #3028 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
107eb8c1d9 |
feat(#3753): run docs guards on the PR that changes the docs they read (#3787)
A PR whose diff is entirely under docs/ runs zero tests, so a guard whose INPUT
is shipped prose cannot protect the PR lane of the diffs it exists to check. Its
only firing opportunity is after merge, on the shared branch -- which is how next
went red on
|
||
|
Behruz Nassre Esfahani
|
622f43353c |
fix(#3299): tracer feedback gate honors workflow.human_verify_mode (#3390)
* fix(#3299): tracer feedback gate honors workflow.human_verify_mode
The tracer feedback gate (#2294) predates `workflow.human_verify_mode`
(#3309, whose scope was the planner and verifier only), and branched on
auto-mode alone. Under the documented `end-of-phase` default an
interactive run therefore halted after EVERY `type="tracer"` task,
synthesizing a `checkpoint:human-verify` no planner ever emitted and
asking the user to retype a verdict the executor had just computed —
at the cost of a full executor cold-start each time.
Planner-side suppression cannot reach this halt because the executor
synthesizes it at runtime, which is why #3309 did not close it.
The gate now branches on HUMAN_VERIFY_MODE in the interactive path:
under `end-of-phase` an automated-only tracer `<verify>` is re-run and,
on success, expansion continues with no checkpoint. HALT-on-failure is
unchanged. `mid-flight`, `gate="blocking-human"`, and tracers carrying
genuine `<human-check>` evidence all still stop; the autonomous branch
is untouched.
`--default end-of-phase` on the config read is load-bearing, not
decorative: `workflow.human_verify_mode` is absent from SCHEMA_DEFAULTS,
so a bare `config-get` exits non-zero with `Key not found` on any
project whose config.json predates #3309 — which is the reporter's
exact config and every pre-existing project.
Both copies of the rule (workflows/execute-plan.md and
agents/gsd-executor.md) are updated together; the reference doc records
the seam and the human-check-still-halts rationale so it cannot recur.
Fixes #3299
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(#3299): add changeset
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(#3299): reconcile the canonical schema table and the stale acceptance test
Review round 1 (trek-e) — three items, all in the drift class this PR is
about, two of them landed inside this PR's own diff.
1. docs/reference/plan-md.md:233 — CONTEXT.md names this file the canonical
schema reference for the tracer task-type contract, and its Task-types row
still claimed interactive runs unconditionally present a
checkpoint:human-verify. CONTEXT.md and docs/AGENTS.md were updated in the
first round; this one was missed, so the authoritative reference was the
wrong answer. The row now carries the human_verify_mode-conditional
behavior and points at the canonical precedence chain.
2. tests/tracer-bullet.test.cjs — the docs assertion only checked that a
tracer ROW EXISTS, never its content, which is why CI could not see the
drift. It now asserts the row's actual claims and rejects the pre-#3299
wording. Separately, the #1945 acceptance test named 'interactive run emits
checkpoint:human-verify after the tracer' kept passing only because its
substrings still occur in the fallback clause, while its name asserted the
opposite of shipped behavior. Renamed and narrowed to what #1945 still
guarantees, plus a new interactiveIsConditional pin so the unconditional
prose cannot be restored under a passing substring check.
3. plan-md.md's <verify> row now documents that the legacy bare-text form
(valid, and still shown at :179) does not reach the #3299 auto-continue —
only a <verify> carrying <automated> does — so the benefit is silently
unreachable for tracers using that format.
Mutation-verified: reverting the plan-md row fails 1 test; reverting the
executor's interactive branch fails 4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(#3299): make the tracer gate reachable from the planner template, and bind the assertions
Peer review round 3 found two Majors, both verified by reproducing the
mutation before fixing.
MAJOR 1 — the fix was largely inert on its own default path.
agents/gsd-planner.md's Nyquist Rule (:191) says every <verify> includes
<automated>, but the tracer-specific template twelve lines later emitted the
legacy bare-text form. The gate auto-continues only on a <verify> carrying
only <automated>, so every tracer produced from the canonical template fell
to the STOP fallback and #3299's benefit was unreachable for exactly the task
type it targets. Template now wraps in <automated>; a contract assertion pins
it so the two cannot drift apart again.
MAJOR 2 — the new assertions did not bind condition to action.
Appending 'Nevertheless, interactive runs always present a
checkpoint:human-verify' to the canonical row, and 'then immediately STOP and
return a checkpoint:human-verify' to the auto-continue clause in BOTH
operative copies, restored unconditional interactive checkpointing and left
the suite 35/35 green. Every required keyword still matched. Fixed by:
- clause 2 must now contain no STOP outcome and emit no checkpoint at all —
'never a checkpoint' has to be true OF the clause, not merely stated in it;
- interactiveIsConditional replaced with the ordered-clause parse plus the
same no-STOP property, instead of proving only that HUMAN_VERIFY_MODE
appears somewhere on the line;
- the plan-md.md Autonomy cell is now pinned EXACTLY rather than by keyword
presence. Deliberately brittle: CONTEXT.md names that table the canonical
schema reference, so a wording change must be a conscious edit in both
places.
Mutation-verified after the fix: the combined semantic regression now fails 3
tests; reverting the planner template fails 1.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): exact-pin the safety clauses instead of blacklisting outcome verbs
Peer review round 4. Blacklisting did not hold, twice over:
- Round 3 banned literal STOP and the 'return a'/'present a' checkpoint
forms in the auto-continue clause. Round 4 defeated that by appending
'then pause and invoke checkpoint_protocol with a checkpoint:human-verify
before expansion' — none of the banned tokens, same restored interruption
after every successful tracer. 36/36 passed.
- The planner guard looked for <automated> anywhere inside <verify>, so
'<verify>[...]<!--<automated>--></verify>' satisfied it while leaving the
legacy bare form operative. 107/107 passed across tracer, planner and the
three size-cap suites.
Synonyms are unbounded; the clauses are not. Both are now pinned exactly on
normalized whitespace, the same approach already proven on the plan-md.md
Autonomy cell, with defence-in-depth checks behind them: no checkpoint-emitting
or blocking outcome in any wording inside clause 2, and the planner's <verify>
body must be exactly one non-empty <automated> child with no commented markup.
These pins are deliberately brittle. Each is a safety contract, so changing the
behavior must be a conscious edit in both the prose and the expectation.
Mutation-verified: the synonym-checkpoint mutation fails 1; the commented-out
wrapper fails 1; the round-3 literal-STOP + contradictory-doc-row regression
fails 3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): strip comments, require uniqueness, pin whole regions
Peer review round 5. Exact-pinning one clause was still bypassable two ways,
both reproduced before fixing (each left the suite fully green):
- COMMENTED DECOYS. Put the correct text in an HTML comment followed by a live
wrong copy: every extractor selected the commented decoy. Worked against the
planner template, the canonical plan-md.md row, and both executor branches.
- SURROUNDING OVERRIDE. Insert 'after every tracer, pause and invoke
checkpoint_protocol before expansion, regardless of the mode-specific rules
below' immediately ABOVE the pinned clause, or 'ignore row 3; always wait for
approval' below the canonical table. The pinned text was untouched, so
equality held while the shipped meaning inverted.
The shape that holds, applied to every operative surface:
1. strip HTML comments BEFORE selecting, so a decoy cannot be chosen;
2. require the structural anchor to occur EXACTLY ONCE, so a live second copy
cannot hide behind a correct first one;
3. pin the ENTIRE decision region, not one clause, so no unparsed prefix or
suffix can override what the pin proves.
Applied to: the executor's whole tracer branch, execute-plan.md's whole
dispatch line, checkpoints.md's whole precedence section, and plan-md.md's
Autonomy cell.
Also addresses the round-5 Minor: the planner template is now asserted
STRUCTURALLY (exactly one <verify> in the fenced block, body exactly one
non-empty <automated> child) rather than pinning the descriptive placeholder
verbatim, so behavior-preserving wording changes no longer false-fail. The
clause and section pins keep their exact form — those have a safety rationale
the placeholder copy does not.
Mutation-verified, all six rounds: override-above-clause 1; commented decoy row
1; commented decoy branch 1; ignore-row-3 override 1; synonym checkpoint 1;
commented-out wrapper 2.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): drop the superseded exact-placeholder planner assertion
Peer review round 6, Minor. The round-5 brittleness fix ADDED a structural
planner assertion but left the old exact-placeholder one in place, so the
over-brittleness it was meant to remove was still live: rewording the
descriptive placeholder while preserving exactly one non-empty direct
<automated> child failed the old test and passed the new one.
Removed the old test. The structural assertion is the real contract — the gate
auto-continues on the SHAPE of the verify, not on the wording of a placeholder.
Verified both directions: a behavior-preserving reword now passes; reverting the
template to bare <verify> still fails.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): select operative prose via parsePredicates, not a hand-rolled scanner
Peer review round 7. I had judged the round-6 selector bypass adversarial-only
and out of scope, intending to disclose it. Both premises were wrong, and the
review said so:
- 'Needs new src API' — false. parsePredicates is ALREADY a public export and
internally uses the repo's interleaved fence/comment scanner. Instrumenting
candidate lines as throwaway predicate declarations borrows that scanner with
no src change at all.
- 'Adversarial-only' — false, and this is the part that mattered. Two ORDINARY
edits silently turned the guards into decoy checks:
* a forgotten '-->' comments the live rule through to EOF, and the
balanced-only stripper still saw and accepted the commented rule;
* a normal fenced documentation example of the rule, plus a whitespace-only
reformat of the live list item, made the selector choose the example.
Neither needs intent. A dangling comment is a typo; a fenced example is good
documentation. Together they reproduce exactly the accidental drift #3299 came
from — with CI green.
The selection layer now defers to parsePredicates for operativeness, uses
whitespace-tolerant anchors so a reformat cannot decouple the live line from its
pin, extracts regions by operative line index rather than string search, and
carries a self-guard test proving fenced / balanced-commented /
after-unclosed-comment copies are all excluded. The helper also ignores indexes
it did not inject, so a pre-existing GSDTEST.CANDIDATE line cannot pollute it.
Verified both ordinary-edit scenarios now fail the suite (each was green before).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): close the operative-selection gaps the maintainer blocked on
trek-e's Blocker: the operative-line selection layer had three gaps, all
reachable by ordinary future doc edits rather than sabotage. He independently
found a fourth I had not disclosed. All are fixed.
1. INDENTATION PROMOTION (his find, not in my disclosure). The instrumentation
replaced a matched candidate with an UNINDENTED marker regardless of the
original line's indentation. A 4-space-indented CommonMark code block is not
skipped by parsePredicates (it accepts indented declarations by design), so
stripping the indent PROMOTED an indented decoy to operative — the exact
inversion of the guard's purpose. The marker now preserves the original
indent, and a candidate that is itself indented 4+ spaces is never injected.
2. NO SET MEMBERSHIP. The filter accepted any in-range integer, so a
pre-existing literal GSDTEST.CANDIDATE=<valid index> in source text could
pollute the count. Now filters on a Set of the indexes actually injected on
this call.
3. RAW FENCE SELECTION (planner). The template test matched the first raw
```xml fence after the marker with no fence/comment awareness — the one
selection in the suite that was not operative-aware — so a commented-out
decoy template between the marker and the real one would be selected while
the live template regressed. The opener must now be operative AND the first
non-blank line after the marker.
4. RAW END ANCHOR (regionFrom). The end anchor was tested against raw lines, so
a fenced example containing a ### / <type line truncated the pinned region
early — a false FAILURE on a legitimate doc edit. End anchors now go through
the same operative filter as start anchors.
Mutation-verified: the indented-decoy + whitespace-varied-anchor combination
and the commented-out fence decoy each now fail the suite (both passed clean
before). Truncation is confirmed fixed by extraction — the region spans the
full section and retains the content following a fenced example, where it
previously stopped at it.
Note on the remaining brittleness: adding a fenced example INSIDE a pinned
region still fails the whole-region exact pin. That is the intended tradeoff
for a safety contract, not the truncation defect, and is called out as such.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): allow-list operative indentation; pin marker provenance
Review round 9.
BLOCKER — the round-8 indentation guard was written as a DENY-list,
/^(?: {4,}|\t)/, and CommonMark has more indented-code forms than that
enumerates: " \t", " \t" and " \t" all open an indented code block and all
slipped through, so an indented decoy was still promoted to operative while the
live rule regressed (34/34 green). Inverted to an allow-list — only 0-3 literal
spaces is ordinary block indentation; anything else is code. Enumerating the
bad shapes was the error, not the specific regex.
MINOR — the injected-index Set validated the marker's VALUE but not its SOURCE.
A pre-existing literal `GSDTEST.CANDIDATE=<n>` could name an index that some
other (skipped) candidate had contributed to the set, and be accepted. Now also
requires p.line - 1 === Number(p.value): the predicate must have been parsed
from the line it names.
MINOR (false negative) — ```xml title=x is a valid CommonMark info string, and
requiring exactly ```xml failed the suite (33/34) on a behavior-preserving edit.
Both the opener assertion and the extraction now accept an info string.
Mutation-verified: the mixed " \t" decoy and the forged-provenance marker each
now fail; the info-string fence no longer false-fails.
KNOWN LIMITATION, disclosed on the PR rather than papered over: parsePredicates
is a predicate parser, not a general CommonMark operativeness oracle. Two
standards-valid constructs still read as operative — a lazy blockquote
continuation line (state opens only on a line that literally starts with ">"),
and a comment opened mid-line ("prose <!--", where state opens only when the
trimmed line STARTS with "<!--"). Closing those means either teaching the shared
src/context-predicates.cts about container/lazy-continuation state — a change to
a module every health rule consumes, well outside a tracer-gate fix — or
hand-rolling a CommonMark parser inside a test, which is how this suite got into
trouble in the first place. Left for the maintainer to scope.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(#3299): re-arm the execute-plan.md emitted-drift ack after the base merge
The #3299 ack rode on tests/emitted-drift-acks/2652-quick-diagnose-dispatch-isolation.json,
which upstream retired in
|
||
|
Behruz Nassre Esfahani
|
a44d513566 |
fix(#3712): confine in-process installs to a sandboxed HOME (#3725)
* fix(#3712): confine in-process installs to a sandboxed HOME
A runtime kind may declare a global `home` override resolved from os.homedir()
rather than from the caller's configDir — codex's skills kind (`home: ".agents"`,
ADR-1239 / #2088) is the only live case. Sandboxing configDir/targetDir does not
contain it, and assertDestWithinConfigHome cannot see the class: that gate
confines a destSubpath to whatever root it is handed, and here the root IS the
escaped home. So an in-process caller that forgot to sandbox HOME wrote to, and
pruned gsd-* entries from, the developer's REAL ~/.agents/skills.
tests/agent-descriptor-parity.install.test.cjs's K1 loop did exactly that: it
iterates every agents-kind runtime (codex included) with a sandboxed targetDir
and an un-sandboxed HOME. Reproduced against a canary home on next @
|
||
|
Tom Boucher
|
004e9dd741 |
fix(#3007): resolve Codex reasoning effort per model and make every clamp visible (#3765)
* test(#3007): failing-first suite for per-model Codex effort capability RED by construction. Binds to behavior renderEffortForRuntime does not yet have: an optional third `model` argument, a per-model advertised-level table, `max` passing through instead of clamping to `xhigh`, `minimal` clamping to `low`, `ultra` rejected outright, and clamp visibility (`requested`/`clamped`/ `reason`) so a downgrade is legible from resolver output rather than silent. Two of these pin defects that exist on next today: - `max` is discarded. Both Codex models whose catalog entries are retrievable (sol, luna) advertise `max`; GSD clamps it to `xhigh` and reports nothing. - `minimal` is emitted to a model that refuses it. providerPresets.openai. haiku.low pairs gpt-5.6-luna with reasoning_effort "minimal", and luna's advertised floor is `low`. GSD is sending a value into a document Codex itself validates. The parity test is what pins that fixed, and it names the offending path/model/effort when it trips. Also corrects tests/model-resolver.test.cjs:351, which asserted renderEffortForRuntime('codex','max').value === 'xhigh' -- the defect pinned as though it were a contract. ADR-443 recorded "Codex has no max" as fact and it was true when written; Codex has since added both `max` and `ultra`. That is a stale premise, so the assertion is corrected here rather than worked around. The property test asserts the invariant the whole change exists for: a rendered effort is always a level the target model actually advertises, or an explicit rejection. There is no third outcome. * fix(#3007): resolve Codex effort per model, and make every clamp visible Codex declares supported_reasoning_levels per MODEL and validates against it, so a single per-runtime capability set cannot be right for all of them. GSD's was wrong in both directions at once. `max` reaches Codex now. ADR-443 recorded "Codex has no max" as fact and clamped max -> xhigh on that basis; it was accurate when written, and Codex has since added both `max` and `ultra`. Every Codex model whose catalog entry is retrievable advertises `max`, so the clamp was discarding a level the provider supports, silently, on the most-used path. `minimal` stops reaching Codex. No Codex model advertises it -- both retrievable entries floor at `low` -- yet providerPresets.openai.haiku.low paired gpt-5.6-luna with reasoning_effort "minimal". GSD was writing a value the receiver validates and refuses into a file the receiver reads. Being unconservative in what you send is the half of Postel's rule with no defensible reading, so that preset is corrected and a parity test pins it. `ultra` is refused rather than laddered. Codex's own catalog calls it "Maximum reasoning with automatic task delegation": at ultra, effective_multi_agent_mode returns Proactive and Codex spawns sub-agents on its own initiative, underneath GSD's orchestration rather than inside it (#2167). It is a mode switch, not a reasoning depth, so it is not added to the universal ladder -- which stays provider-agnostic by ADR-443's design -- and it is rejected even for gpt-5.6-sol, which does advertise it. Clamping it down to `max` was considered and rejected: that silently discards what the user actually asked for. Clamping is now visible. RenderedEffort carries requested/clamped/reason and resolve-execution surfaces them. The previous table clamped correctly but invisibly, so a user asking for `max` on Codex had no way to find out they were getting `xhigh` -- exactly the failure mode the robustness principle's modern critique warns about, and why "be liberal" has to mean "liberal and loud". Also closes a latent trap found while reviewing the implementation: the clamp-up loop walks the ladder upward, and for a future model advertising `ultra` but not `max` it would have selected `ultra` as the clamp target -- re-entering by the back door the mode the rejection above exists to keep out. A clamp may never produce a value that a direct request for that value would refuse. Unreachable with today's catalog, which is why no test caught it; a test now asserts the invariant directly. Signature stability is preserved: the third `model` argument is optional and the two-argument form still resolves, against the family baseline. That form's BEHAVIOR does change for `max` and `minimal`, and it must -- keeping the old answer would have fixed the defect only where a model happened to be threaded through and left it live everywhere else. tests/model-resolver.test.cjs:351 asserted the defect as if it were a contract and is corrected here rather than worked around. * fix(#3007): close every review finding on the Codex effort alignment Two isolated reviewers, correctness and security. Both found the same two blockers, and the per-model work was inert on every surface that matters until this commit. BLOCKER — resolve-execution never passed the model and discarded the clamp. cmdResolveExecution called the two-argument form and emitted only effort_rendered/effort_param/effort_propagation, so the per-model table was unreachable from production code (tests were its only caller) and requested/ clamped/reason were computed and thrown away. Requested outcome 3 names "the effective rendered effort in resolver output" specifically, so the feature was unmet on the exact surface the issue asks for. Now passes the resolved model and emits effort_requested / effort_clamped / effort_clamp_reason, flat, matching the existing key convention rather than introducing a nested object. BLOCKER — the docs described output that did not exist. CONFIGURATION.md showed a nested {"effort": ...} sample; the real result is flat and those keys were absent entirely. A reference doc asserting a JSON path a reader can copy is worse than no doc. Corrected against the actual emitted key set. MAJOR — the argv channel still shipped both original defects. EFFORT_ARGV.codex kept minimal in its supported set and still clamped max down to xhigh, so the invocation-time and install-time channels disagreed about the same runtime's capability: --host codex with max emitted xhigh while the generated TOML said max. This is the repo's documented generative-fix-divergence class, so both tables now cross-reference each other and a parity test fails if they ever diverge again. MAJOR — malformed catalog data failed OPEN and could crash the CLI. A null _baseline became an EMPTY Set that is nonetheless truthy, so the nullish fallback never fired and every effort rendered as null. And a non-array value made the Set constructor throw at module load — model-catalog.cjs is required across the whole CLI, so one bad JSON value killed every command, not just codex effort. Guarded on size and filtered to array values; both degrade to the hardcoded baseline. MAJOR — value widened to a nullable string with two consumers left behind. runtime-artifact-conversion passed it straight into injectEffortFrontmatter (a null effort key in generated frontmatter); install-effort-resolver still declared a non-nullable return, a structural lie that silently defeated null checking. Both corrected, both omitting the key on null — the same posture as 'inherit', where omission means "follow the host default". MAJOR — the per-model table is inert today, and the docs now say so. All three shipped models advertise the same usable range and ultra (sol's only differentiator) is rejected for every model, so no observable output differs by model. The table stays because Codex declares capability per model and the sets are free to diverge — a single per-runtime assumption is precisely what went stale and produced this issue — but overselling it as a visible per-model feature would have been the same class of error as the doc blocker above. Tests: three passed under a full revert and are strengthened rather than deleted, since each guards a real contract (#3533's inherit rule, the undeclared-host rule, off-ladder handling) — they now also assert the clamp-visibility fields, which only exist after this change. The fast-check property is kept for its shrinking, and a deterministic nested loop over the full cross-product now sits beside it so coverage is exhaustive rather than sampled. Also folded in earlier: bin/install.js generated the Codex TOML with the two-arg form and would have written a literal null reasoning effort on the ultra path; CONTEXT.md's Model Catalog Module glossary entry now records CODEX_MODEL_EFFORT. The installer defect was found by the co-change gate, not by a reviewer — install.js is a historical co-change partner of model-catalog.cts that this diff had not touched. * test(#3007): correct assertions that pinned Codex's stale effort premise Thirteen pre-existing tests encoded "Codex has no max" as fact and failed on the shipped commit. Every one is a stale pin, not a defect: each was probed against the built module before its expectation was changed, and none failed for a reason other than this premise correction. Kept as its own commit per CONTRIBUTING — a test-fixture correction made stale by a production change must not ride inside another commit, because the release-sdk hotfix cherry-pick filter routes by subject prefix and a correction buried under the wrong prefix ships a half-state (v1.42.3, #3621). The most valuable one was tests/model-resolver.test.cjs's cross-provider validity invariant, which hardcoded the Codex enum as `minimal|low|medium|high|xhigh` and failed with "real API would 400". That message is now false in both directions: Codex accepts `max`, and rejects `minimal`, which no model advertises. The enum is corrected to `low|medium|high|xhigh|max` and the guard is kept intact — it is exactly the "would the real API refuse this" check worth having, and it was right to fail here. It simply carried the stale fact in its own fixture. Test NAMES were corrected alongside their assertions wherever the name asserted the old behavior — "max is Anthropic-only", "max clamps to xhigh", "minimal passthrough". A renamed test that still claims the old thing is worse than a failing one, and a green test whose name states a falsehood is how the next reader inherits the wrong premise. Both channels are covered: install-time (renderEffortForRuntime, and the generated .toml in install-runtime-artifacts) and invocation-time argv (effort-surface-axis). They were deliberately brought into agreement in this change, so their assertions had to move together. Each site carries a #3007 comment recording that Codex gained max/ultra and that capability is declared per model, so a future reader can tell this was a deliberate premise correction rather than a test bent to fit an implementation. * test(#3007): separate the effort-precedence case from the clamp case The previous stale-assertion pass over-corrected one test. It saw `effort: { default: 'max' }` on codex expecting `effort_rendered: 'xhigh'`, assumed the xhigh came from the max→xhigh clamp #3007 removes, renamed it to "max passes through" and changed the expectation to `max`. The remote runner disagreed. Reproduced against the real CLI: with that config and `gsd-planner`, the resolver emits `effort: "xhigh"`, `effort_requested: "xhigh"`, `effort_clamped: false`. The xhigh is produced by effort-resolution PRECEDENCE — gsd-planner is heavy/opus tier and its routing-tier default outranks `effort.default` — so `max` never reaches the renderer at all. The test says nothing about clamping and never did; it only looked like a clamp pin because both mechanisms happened to yield the same string. Restored to `xhigh` and renamed to say what it actually tests. It now also asserts `effort_clamped === false` and `effort_requested === 'xhigh'`, which is what makes it impossible to mistake for a clamp pin again: those two fields prove the value is what the resolver produced rather than something the renderer downgraded. Before #3007 there was no way to tell the two apart from the output — which is precisely why the previous pass could not tell them apart either. Added the test that was actually missing: `effort.agent_overrides`, which outranks the tier default, so the requested level genuinely reaches the renderer and `max` survives to `effort_rendered` end-to-end through the real CLI. Verified by probe before asserting. One test now pins the precedence rule and the other pins the #3007 behavior, and neither can be read as the other. That the clamp-visibility fields are what resolved this is a small argument for having added them. * chore(#3007): backfill changeset pr number to 3765 * test(#3007): put model-catalog under the mutation gate The Stryker shard showed as `skipping` on this PR despite the diff rewriting model-catalog's effort logic. That was legitimate, not a detection bug: `model-catalog` was never in scripts/mutation-matrix.cjs's COVERED map, so the whole module — including everything #3007 touches — sat entirely outside mutation scoring with has_work "false". Registered, with a dedicated spawn-free surface. tests/model-catalog.unit.test.cjs is new: 44 in-process tests, no runGsdTools, no child process, no filesystem, no temp dirs. That shape is not stylistic — it is the #2790 precedent this file already documents. Stryker's command runner treats a whole `node --test <file>` invocation as ONE test costing whatever its slowest case costs, and re-runs it per mutant, so pointing a shard at tests/model-resolver.test.cjs (which uses runGsdTools throughout) would reproduce exactly the 15-minute shard-cap cancellation #2790 hit. The integration file is unaffected and keeps running in full in the normal test job. Coverage spans the module rather than only the diff, because the score is measured over the whole file: effort rendering across every model and ladder level in both channels, the prototype-chain host guard, the exported enums and maps, isAnthropicFlavoredModel's provider namespacings, the profile projections, nextTier, and mergeEffortTierDefaults. The last two were nearly left out and are worth naming — every uncovered exported function is score given away, and mergeEffortTierDefaults turned out to have a genuinely interesting contract (#3531: a partial override merges over the built-ins rather than replacing them, and isValid gates the VALUE, not the tier name, so an unknown tier key is still merged in). Every expectation was probed against the built module before being asserted. minScore is 1 and that is a PLACEHOLDER, flagged as such in the registry comment. Floors in this repo are measured, not chosen — the existing entries sit at 94, 75 and 56 — and they can only be measured in CI, because mutation shards run `node --test`, which is hard-blocked locally. The first CI run on this branch reports the real number and the floor gets ratcheted to it before merge. A placeholder of 1 reaching `next` would make the gate decorative: it would pass whether or not a single mutant is ever killed. Note the target is "never regress from measured", not a fixed 80 — planning-inspect sits at 56 and is documented as an accepted ratchet candidate. * test(#3007): bootstrap model-catalog's mutation floor legally The placeholder floor was structurally illegal and the remote run said so. tests/mutation-matrix-ratchet.test.cjs guards the guard: every COVERED module must carry a matching RATCHET_BASELINE entry in the same diff, minScore must EQUAL that baseline, and it must be at least 50. `minScore: 1` failed all three. That is the ratchet working exactly as intended — a floor nobody can satisfy accidentally is the point of it. Bootstrapped at 50 in both places. Fifty is not a measured score and the comment says so plainly: it is the minimum the guard permits, and it coincides with Stryker's own configured `break` threshold, so it is the lowest legal starting point for a module that has never been measured. It still must be ratcheted to floor(measured) - 1 before this PR merges. Also corrected a real defect in the file's own instructions. "HOW TO UPDATE" step 1 read "Run the per-module Stryker shard locally" — which cannot be done here, and which the same file contradicts eighty lines further down, where the #2790 scores are recorded as "not a local run; mutation shards run `node --test`, hard-blocked in this repo's local environment". stryker.config.mjs confirms the command runner invokes `node --test` once per mutant, and .claude/hooks/block-local-node-test.sh denies exactly that. So the documented first step sends the next contributor at a wall. Rewritten to describe the path that works — push, read the measured score off the CI shard, then set the floor and its baseline together in one diff — and to say why local measurement is not available, so nobody rediscovers it the slow way. GOODHART SAFETY is untouched. The two-step is inherent to the environment rather than a shortcut: a floor cannot be measured before the first CI run exists, and the guard rightly refuses to accept an unmeasured one below its minimum. * test(#3007): ratchet model-catalog's mutation floor to its measured score The shard ran in CI and reported 59.62% — 248 mutants killed, 168 survived, no timeouts, no errors (run 32605073352, job 97108869486). Floor set to 58 per this file's own rule, minScore = floor(measured) - 1, which is the same arithmetic every sibling entry used: 57.03 to 56, 76.58 to 75, 95.65 to 94. Both halves moved together, because the ratchet guard asserts minScore equals its RATCHET_BASELINE entry and would reject them drifting apart. The spawn-free unit surface is vindicated by the clock: 57 seconds, against a 15-minute shard cap and a 9m46s frontmatter shard in the same run. That was the whole reason for creating tests/model-catalog.unit.test.cjs rather than pointing the shard at tests/model-resolver.test.cjs — #2790 recorded shards being CANCELLED at that cap when they targeted a runGsdTools-heavy integration file. The registry comment is rewritten rather than deleted. It previously warned that the floor was provisional and must not ship that way; leaving that text next to a measured floor would make the file lie in the other direction. It now records the measurement the way the sibling entries do, including that 59.62 sits below TARGET (80) and is therefore a ratchet candidate like planning-inspect at 56 — comfortably clear of its own floor with real room to grow. Raise it as the tests improve; never lower it. Worth stating plainly: 168 surviving mutants is not a clean bill of health. It is an honest floor for a module that had NO mutation coverage at all an hour ago, and it is now pinned so it cannot silently regress. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
b6977d9d11 |
fix(#3762): enforce docs/INVENTORY.md roster rows, backfill 32 gaps (#3766)
* test(#3762): failing-first roster gate for docs/INVENTORY.md rows
Anchors the human half of the inventory-drift rule: every entry in
docs/INVENTORY-MANIFEST.json must have a hand-written row in
docs/INVENTORY.md. Expected RED on this commit -- next carries 32
unrostered surfaces, which is the defect the gate exists to catch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(#3762): enforce docs/INVENTORY.md roster rows, backfill 32 gaps
docs/INVENTORY.md calls itself the authoritative roster of every shipped
GSD surface, and CLAUDE.md's inventory-drift rule requires both a roster
row and a manifest regen. Only the manifest half was anchored, so a PR
could ship a surface, regenerate the manifest, omit the row, and stay
green -- as PR #3758 did with gsd-core/references/planner-coupling.md.
Adds the roster half to tests/inventory-manifest-sync.test.cjs, backed by
a pure matcher in tests/helpers/inventory-roster.cjs, and backfills the 32
surfaces already missing rows on next.
Fixes #3762
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(#3762): harden roster matcher against fenced blocks and trim exports
Skips fenced code regions when splitting level-2 sections so a documented
'## ' example inside a fence cannot truncate a family section (false red)
or contribute a phantom row (false pass); makes the heading pattern linear
rather than a backtracking lazy match; narrows the module surface to the
three names the gate consumes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(#3762): close two false-RED gaps found in orthogonal review
Indented headings: CommonMark permits an ATX heading to carry 1-3 leading
spaces, and a ^##-anchored pattern read such a document as having no family
sections at all -- reporting all six missing, a structural red for zero real
drift. Reproduced, then fixed and pinned.
Link-wrapped cells: a row written as [`x.md`](../x.md) was not recognized,
though docs/INVENTORY.md already uses that form elsewhere. Unwrapping now
peels a whole-cell link and a whole-cell code span, and only layers that
wrap the cell entirely -- a file mentioned mid-prose is still not a row, so
the false red is not traded for a false pass.
Adds a second fast-check property over the commands source-link rule, the
one family whose matching rule differs from the other five.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(#3762): boundary trio for the fence-marker length limit
RULESET.TESTS.boundary-coverage — the matcher's only numeric limit is the
fence marker's {3,}. Exercises 2 (inline markup, swallows nothing), 3, and
4 characters, for both backtick and tilde delimiters.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* chore: remove stray pwned_cmdsub injection-test canary from the repo root
A zero-byte file committed by
|
||
|
Tom Boucher
|
2f86278b5e |
fix(#3003): opt-in mechanism for intentional deletions in worktree.cleanup-wave (#3757)
* test(#3003): failing-first suite for declared deletions in cleanup-wave Binds the guard's opt-in before it exists, so the suite is RED against next. The rows that carry the weight are the over-authorization set: a directory declaration must not authorize its children, a glob declaration must authorize nothing, and a declaration must not act as a string prefix of another path. Each of those BLOCKS, and each would PASS under a prefix, glob, or startsWith matcher — which is how a path list quietly degrades into the boolean opt-in #3003 explicitly rejected. The glob row matters most: declaredScopePrefix already returns null ("matches everything") for a glob-leading pattern, correct for the advisory it serves and catastrophic for a gate. Also pinned: a failed deletion check blocks on its own reason rather than being filtered into a pass; the block detail names only the undeclared residue so the operator is not misdirected by paths that were fine; an entry with no declaration blocks exactly as before; junk and non-array declarations do not authorize; and a blocked entry still isolates rather than aborting the wave (#2852, which must stay fixed). Two advisory rows cover an interaction found while designing: git diff --name-only includes deleted paths, so without unioning the declaration into the #2596 scope check, authorizing a deletion would raise SCOPE_OUT_OF_DECLARED against the very path just authorized. A seeded property states the whole invariant the three over-authorization rows sample: a deletion merges iff its normalized path is in the declared set. * feat(#3003): declared deletions opt-in for the cleanup-wave guard The deletions guard blocked the merge-back of any executor branch whose diff removed a file, with no way to say a removal was intended. A plan that folded one test file into a sibling could not be merged by the tool meant to merge it, forcing a manual --no-ff outside the tool -- strictly less safe than what the guard protects against. A plan now declares removals in its own frontmatter (files_deleted), and that list rides the same path files_modified already travels: plan-document parse -> phase plan JSON -> the per-plan worktree gate -> record-agent/create --deletions -> declared_deletions on the manifest entry -> the guard. The guard blocks only the deletions NOT in that list. A path list rather than a boolean, per the pinned decision: a boolean disarms the guard for the whole entry, so an unexpected deletion riding along with a declared one would pass unnoticed. Matching is exact after the module's shared normalizer -- never a prefix, never a glob. Both would let one declaration authorize a whole set, which is the mass-deletion accident the guard exists to catch. That also means declaredScopePrefix is deliberately NOT reused here: it returns null ("matches everything") for a glob-leading pattern, which is right for the advisory it serves and would silently disarm a gate. The block detail now carries only the undeclared residue, so an operator is not sent looking at paths that were fine. A failed deletion check still blocks on its own reason and is never filtered into a pass. A blocked entry still isolates rather than aborting the wave (#2852). The #2596 scope advisory unions the declaration into its declared set -- git diff --name-only includes deleted paths, so without that, authorizing a deletion would immediately warn that the same path was out of declared scope. Optional and additive throughout: files_deleted is absent from PLAN_REQUIRED_FIELDS, a manifest entry without declared_deletions keeps the original unconditional block, and omitting --deletions leaves the on-disk entry shape untouched. Supersedes the spent #2856 emitted-drift ack entry for execute-phase.md, the same supersede that entry performed on #3370 and #3370 on #3324. * fix(#3003): wire --deletions on every dispatch surface, not just one Review found the feature inert on two of three dispatch paths. execute-phase.md (harness inline) passed --deletions, but the orchestrator-worktree path (executor-isolation-dispatch.md, worktree.create) and the Fleet-parallel batch path (capabilities/claude-orchestration/fragments/execute-wave-pre.md, worktree.record-agent) still passed only --files. A plan declaring files_deleted would have merged on one path and been blocked on the other two -- the exact bug #3003 exists to fix, left unfixed where most of the isolation actually runs. Worse, per-plan-worktree-gate.md already claimed --deletions was passed 'on the same worktree.record-agent / worktree.create calls', which was false for both untouched sites. A doc asserting coverage that does not exist is how a gap survives review. All four surfaces now pass the flag, verified by sweeping every .md under gsd-core/, capabilities/, commands/, skills/ and agents/ that invokes worktree.record-agent or worktree.create: each one that passes --files now also passes --deletions. The isolation-dispatch note explains why this flag, unlike --files, is not advisory -- omitting it does not skip a check, it blocks a merge the plan declared. Regenerates capability-registry.cjs, which the fragment edit made stale. Neither newly-grown file needs an emitted-drift ack: executor-isolation-dispatch.md sits under workflows/execute-phase/steps/ and execute-wave-pre.md under capabilities/, both outside currentSizes()'s non-recursive scan of gsd-core/workflows/ and agents/. * docs(#3003): document files_deleted where a plan author will actually find it The feature's entire user surface is one plan-frontmatter field, and the canonical reference for that frontmatter -- docs/reference/plan-md.md, the table that documents every other key -- never mentioned it. A field nobody can discover ships as a field nobody uses. Adds the files_deleted row and an example entry in all five locales (en, ja-JP, zh-CN, ko-KR, pt-BR), stating the property that makes the opt-in safe: matching is exact per path after separator normalization, with no globs and no directory prefixes, so a declaration can never authorize more than it literally lists, and omitting the field keeps the guard's original unconditional block. Also corrects two claims in the scope-conformance how-to that this change made false. Its opening paragraph described the recorded declared scope as files_modified alone; declared_deletions is now unioned into that comparison. Its "Renames are not detected specially" bullet asserted the deletions guard blocks any entry whose diff contains a deletion, full stop -- which was the whole point of #3003 and is no longer true. Reworked to say what now decides a rename's fate: declare the old path in files_deleted and both halves become ordinary paths for the advisory check, which is also why the old path needs no separate files_modified entry. Documentation that describes the pre-change behavior of the thing being changed is worse than no documentation, because a reader trusts it. * fix(#3003): close every review finding on the declared-deletions opt-in Two independent isolated reviewers, correctness and security. Neither found a blocker; both found real defects, and the directive treats a finding at any severity as blocking. All of them are fixed here. MAJOR -- the submodule worktree gate could not see a deletion-only plan. per-plan-worktree-gate.md intersected $SUBMODULE_PATHS against $PLAN_FILES alone, while $PLAN_DELETIONS was extracted and then never used. Before files_deleted existed, a path had to appear in files_modified to be planned at all, so the gate saw it; the new field plus the new docs telling authors a deleted path needs no files_modified entry opened a hole where a plan whose only submodule touch is a removal kept worktree isolation on -- the exact case #2772 disabled it for. Both channels now feed the intersection. Note the posture is deliberately the OPPOSITE of the cleanup-wave guard: there the channels stay apart because a deletion AUTHORIZATION must never be inferred; here they merge because a safety fallback must never MISS a touch. MAJOR -- same-wave conflict detection could not see a deletion. The planner's implicit-dependency rule compared files_modified only, so plan A editing src/x.ts and plan B declaring files_deleted: [src/x.ts] scored as conflict-free and ran in parallel: one branch removing what the other is writing, which is the sharpest conflict there is. Overlap is now computed across both channels. MINOR (both reviewers, one root cause) -- the advisory union gave one field two matching rules. declared_deletions was unioned into the scope list handed to planWaveScopeConformance, which reads it with prefix-and-glob semantics. So a field that is exact-match-only at the gate silently became wider at the advisory: ["*.md"], inert at the gate, yielded a null prefix meaning "matches everything" and muted the advisory completely, and ["src"] muted all of src/. The union also activated the advisory on plans that declared no modification scope at all, warning on every modified path. Replaced with subtraction from the findings, gated on files_modified alone. One field, one rule, everywhere. MINOR -- core.quotepath made the feature silently inert for non-ASCII paths. git emits "tests/\303\251.ts" C-escaped and quoted, which never equals the declared plain path, so a correctly declared deletion of tests/é.ts would block forever with nothing pointing at the encoding. Both diffs now pass -c core.quotepath=false. NIT -- flag() consumed a following flag as a value, so --deletions --files x swallowed --files and dropped both. Now treated as a missing declaration, which fails closed. Fixed at both call sites; the helper is duplicated verbatim in cmdWorktreeRecordAgent and cmdWorktreeCreate and leaving one would reintroduce it. TEST -- one test passed for the wrong reason. "a declared deletion is in scope for the advisory" asserted only that warnings omit the deleted path; under a full revert the entry blocks first, warnings come back empty, and the negative assertion passes anyway. It now asserts the entry actually merged, which is the load-bearing half. Four regressions added, one per fix above. Docs corrected rather than extended. The rename bullet in the scope-conformance how-to claimed a rename whose delete side is undeclared never reaches the advisory. Verified false: git's rename detection is on by default, so a pure rename is a single R entry that appears in no --diff-filter=D output and was never gated, before or after #3003. Only a rename that edits enough to fall below the similarity threshold decomposes into add+delete. The pre-existing sentence made the same wrong claim; this restates it correctly instead of sharpening the error. The localized plan-md.md reference edits are reverted: the PR template requires docs content added here to be English, and the translations already lag by three fields, so English-only is the repo's standing posture, not an oversight. Agent-file size caps respected: gsd-planner.md is XL-tier by bytes but carries a separate 49152-LF-CHAR cap asserted by four suites, so its edit is deliberately terse and lands at 49141 with 11 chars of headroom, with the rationale moved to docs/reference/plan-md.md, which has no cap. gsd-plan-checker.md lands at 49107 bytes, 45 under the LARGE cap. Both acks merged into the existing fragments that already name those paths, since two ack sources may never name the same path. * fix(#3003): decode git's path quoting instead of changing the git argv The previous commit's non-ASCII fix turned the remote suite red: 44 failures, 42 of them "unexpected git call: -c core.quotepath=false diff --diff-filter=D --name-only ...". The suite's git mocks match on exact argv, so adding two flags to the deletions diff and the advisory diff invalidated every existing fixture in tests/worktree-safety.test.cjs. Rewriting dozens of fixtures to accommodate one flag would be paying a large Hyrum's-law bill to fix a small defect. Both execGit calls are reverted to their original argv. The C-quoting is now decoded in normalizeScopePath instead, via a new decodeGitQuotedPath helper. That is the better fix on its own merits, not merely the cheaper one: the git argv is untouched so no fixture moves, the decode lands on the ONE normalizer already applied to both sides of the comparison so the declared and reported paths cannot disagree, and it holds regardless of the user's own core.quotepath setting rather than only when we remember to override it. A value not wrapped in a leading AND trailing quote is returned completely untouched, so the plain-ASCII path -- the overwhelmingly common case -- is byte-identical to before. Escapes decode to BYTES collected into a Buffer and UTF-8 decoded only at the end, because \303\251 is two bytes forming one character and decoding them separately yields mojibake. Malformed input never throws: a trailing lone backslash or a short octal escape degrades to the literal character, since one bad path must not take down a cleanup wave. Caught while reviewing the helper: the non-escape branch pushed a UTF-16 code unit rather than UTF-8 bytes. Git always escapes non-ASCII so its own output was fine, but this normalizer runs on the DECLARED side too, and an author may write a quoted path holding a literal é -- pushing 0xE9 alone is invalid UTF-8, so the declaration would decode to a replacement character and silently stop matching. That is precisely the failure this change removes, reintroduced on the other side of the comparison. Now converts whole code points, surrogate pairs intact. The other 2 failures: tests/parallel-dependent-plans.test.cjs pins the exact unbackticked substring "files_modified overlap" in gsd-planner.md, and rewording that comment to "declared-scope overlap" deleted it. The comment is restored verbatim and the files_deleted change rides in the pseudocode and the Rule sentence instead. Recorded in the ack fragment so the next contributor does not rediscover it the same way. Four regression tests cover the decode through the public cleanup-wave seam (the helper is module-private): a declared non-ASCII deletion merges against a C-quoted git report, the symmetric case where the DECLARATION is the quoted form, an undeclared non-ASCII deletion still blocks with the residue naming the decoded path an operator can act on, and a path merely containing a quote is left alone. Plain ASCII was already covered and is not duplicated. * fix(#3003): revert the leading-dash flag guard, the review nit was wrong The remote suite came back with 2 failures, down from 44, and both point at the same thing: tests/worktree-safety.test.cjs:7045 already pins the opposite contract, deliberately. test('a flag-shaped --files value is not re-parsed as a flag', ...) recordAgent(['--files', '--branch']) -> files_modified === ['--branch'] -> branch === 'worktree-agent-a1' ("the real --branch value must be untouched") So consuming the next argv element positionally, whatever its shape, is the tested intent of this parser, not an oversight. The security reviewer's nit claimed --deletions --files x would "swallow --files and drop both". It does not: each flag runs its own indexOf, so --deletions records the literal '--files' while --files independently still resolves to x. And that literal is a path git never reports as deleted, so it authorizes nothing -- already fail-closed with no guard at all. The guard bought no safety and silently changed --files behavior along the way, outside this issue's scope. Reverted at both call sites, which are byte-identical again, along with the test asserting the reverted behavior and the docs sentence describing it. The nit is recorded as REJECTED in the review artifact with the reasoning above, rather than as fixed -- a finding that turns out to be wrong should leave a trace of why, or the next reviewer files it again. docs/CLI-TOOLS.md now states the positional-read behavior plainly instead, so the next person meets it as documented intent rather than rediscovering it through a red suite. * chore(#3003): backfill changeset pr number to 3757 * test(#3003): cover parsePlanDocument's filesDeleted branch to clear the mutation gate CI's Stryker shard for plan-document failed at 73.28 against a break threshold of 75: 170 killed, 62 survived, 232 total. Eight of those survivors are the filesDeleted block this issue added to parsePlanDocument, which shipped with no direct coverage at all -- the field was exercised end to end through the cleanup-wave tests, but the parser itself was never called with a plan that declares it, so every mutant in the block lived. Four tests, each pinned to specific mutants rather than written for coverage percentage: - absent key yields exactly [] -- kills the array-literal seed (["Stryker was here"]) and the `fmDeleted = true` conditional, which would otherwise produce ["true"] - a scalar underscore `files_deleted:` wraps into a one-element array -- kills `fmDeleted = false`, the `&&` logical-operator swap, the `fm[""]` string mutation on the first operand, the emptied if-block, and the ternary's non-array branch - an array-valued hyphenated `files-deleted:` maps element-wise -- kills the `fm[""]` mutation on the SECOND operand (only reachable when the legacy hyphen alias is the one carrying the value) and the ternary's array branch - an empty list yields [] -- boundary case, and a genuinely distinct one from the absent key: [] is truthy in JS so it ENTERS the if, and only Array.isArray's true branch mapping over nothing produces the same [] Threshold arithmetic: 174 of 232 are needed for 75%, and these take it to about 178, so the shard clears with margin rather than landing on the line. Every expected value was confirmed by executing the built parser before being asserted, not inferred from reading the source. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
738f42f4fd |
feat(#2398): consensus gate for CYCLE_SUMMARY on multi-reviewer runs (#3755)
* test(#2398): failing-first suite for the CYCLE_SUMMARY consensus gate Binds the gate before it exists, so the suite is RED against next. The load-bearing rows are the two the closed PR #2417 did not have. The B2 regression row asserts a judgment-class lone HIGH counts WITHOUT corroboration when its raiser is unmarked — if anyone re-couples that class to corroboration, more reviewers again produce a weaker gate than one, which is what closed #2417. The parity row asserts every marker literal the gate names is one review-lane-runner actually emits, so the gate cannot key on a signal nothing produces; a mutation row and a seeded fast-check property prove that guard runs its failure branch rather than only reading a correct tree. Also pinned: gate position before Counting rules, the untouched CYCLE_SUMMARY line shape the orchestrator greps, fence balance, the single-reviewer no-op, classification by what a claim asserts rather than by citation presence, the all-marked fail-open, current_actionable staying out of scope, and the leading-marker requirement that stops a review which merely quotes a marker from suppressing its own findings. * feat(#2398): consensus gate for CYCLE_SUMMARY on multi-reviewer runs With review.reviewer_instances running several reviewer identities off one adapter, any single instance's fabricated HIGH could force a full replan cycle on its own. Across ~9 real cycles on two projects each of four instances fabricated at least once, and each was also the most accurate reviewer in some other cycle, so dropping to fewer reviewers trades away real signal. The gate engages only when 2+ reviewers actually ran, and weighs a lone HIGH by what the claim asserts rather than by whether anyone agreed with it. An existence claim -- a symbol, file, flag, commit or ID exists, is absent, or says something specific -- counts only if source-grounding confirms it or another reviewer raised the same concern. A judgment claim -- a design or correctness property -- counts unless that reviewer's own section opens with an evidence-quality discount marker the review lane already stamps ([reviewed-without-source-citations] #3194, [reviewed-without-repo-access] #2176, or a diff-only lane). That split is what resolves B2, the finding that closed PR #2417. B2 showed the approved wording made more reviewers produce a WEAKER gate than one: condition (a) pointed at the source-grounding pass, which verifies every symbol THE PLAN cites and never takes reviewer claims as input, so a genuine architectural HIGH that one reviewer caught and another missed was neither groundable nor corroborated and stopped gating. Judgment-class findings are therefore exempt from corroboration entirely -- reviewers catch materially different classes of issue, and demanding two of them independently raise the same architectural concern suppresses exactly what a multi-reviewer setup exists to surface. Guards on the gate itself: an all-marked cycle disengages it, so a cycle in which nothing was verified can never be counted as converged; the marker must OPEN a reviewer's section, so a review that merely quotes a marker does not suppress its own findings; a suppressed HIGH stays listed and tagged rather than dropped; current_actionable is untouched; and a single-reviewer run is unchanged. No new command, config key, or dependency -- the gate reads signals that already exist. The CYCLE_SUMMARY line shape the orchestrator greps is unchanged; only the integer it computes moves, and only for 2+ reviewers. Known limit, inherited rather than introduced: SOURCE_CITATION_RE checks citation presence, not resolution, which src/review-lane-runner.cts records as a deliberate #3194 scope boundary. A fabricated but plausible file:line still gates. Scope revised and re-approved on the issue before any code was written. * test(#2398): make marker parity behavioral, and stop overclaiming the gate Review found the parity tests were vacuous: they asserted a marker STRING appeared in review-lane-runner.cjs's source text, never requiring the module or calling the stampers, so they would pass even if stampUngroundedReview were broken or never invoked. They now invoke the real exported functions and assert what those functions PRODUCE — that an uncited review gains a leading marker blockquote, that a review carrying a file:line does not, that a self-reported blind review is stamped, and that stamping is idempotent. Removing the source read also removes an incidental no-source-grep evasion via a parameterized path. Review also found the changeset headline false for the class it matters most in. The discount markers detect 'cited nothing' and 'had no repo access'; they cannot detect 'drew a wrong conclusion from a real citation', so a judgment-class finding invented by an evidence-bearing reviewer still counts alone. That is the deliberate side of the tradeoff jags-faith named when closing #2417 — the alternative is requiring corroboration for design findings, which is B2 — but the changeset claimed lone hallucinations no longer force a cycle, full stop. Corrected there, and stated plainly in docs/COMMANDS.md and the design record. Also dropped the reviewer-instances.md entry from the emitted-drift ack: the growth ratchet's currentSizes() scans only gsd-core/workflows/ and agents/ (tests/helpers/emitted-runtime.cjs:916-929), so references/ is outside it and that entry acknowledged a delta the gate cannot see. * chore(#2398): backfill changeset pr number to 3755 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
dacae92730 |
docs(#2845): record the inventory-provenance limits where readers meet them (#3746)
The limits shipped with #2845 were disclosed only in the PR body, which is read once at merge and then buried. They are properties of what the feature does, so they belong in the documentation. Three surfaces, each at the point a reader forms an expectation: docs/how-to/design-a-ui-phase.md gains a 'What this check is and is not' subsection under the provenance how-to; docs/explanation/security-model.md gains a residual-risk pair matching the section's existing shape; and docs/AGENTS.md notes them where gsd-ui-checker's behavior is described. The substance: a provenance line makes an inventory's origin falsifiable rather than verified, since nothing re-runs the command or compares the count; the rule is agent-applied like the other six dimensions, not a schema check; and 'the checker never runs the recorded command' is an instruction rather than a capability boundary, because the checker holds a Bash grant it genuinely needs for the agent-skills bootstrap and tool grants here are not command-scoped. Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
4918c62d76 |
feat(#2845): require provenance for UI-SPEC component inventories (#3745)
* test(#2845): failing-first suite for UI-SPEC inventory provenance Binds two shared formats before either exists, so the suite is RED against next: the gsd-ui-checker dimension roster (asserted independently on twelve surfaces, eight English and four translated) and the provenance-line grammar the UI-SPEC template emits and Dimension 7 consumes. Every parity assertion is paired with a synthetic mutation case, so the guard's failure branch executes rather than only reading a correct tree: limit-1 (a surface still declaring 6), limit (7), limit+1 (8), a dropped dimension, a label that drifts on one surface only, a non-contiguous roster, a duplicated number, and a surface that stops declaring a count at all. A seeded fast-check property renders the roster under formatting noise (CRLF, padding, interleaved sections) and asserts the parse round-trips and is strictly sensitive to a dropped heading. Assertions are on parsed typed records, never raw substrings. * docs: normalize design-a-ui-phase how-to to American English House style for docs/ is American English (CLAUDE.md). This file carried colour/initialisation/initialise/artefact throughout. Spelling only — no content change; kept separate from the #2845 feature commit so the release-notes classifier and the hotfix cherry-pick filter see it for what it is. * feat(#2845): require provenance for UI-SPEC component inventories A UI-SPEC's component inventory was treated downstream as a closed allowlist while the document recorded nothing about whether the list had been enumerated from the installed design system or recalled from memory. A recalled inventory is indistinguishable from an enumerated one, so an executor complying with the spec builds against a fraction of what the package offers, and every gate stays green because they assert semantics rather than composition. The UI-SPEC template gains a Component Inventory slot carrying one of two provenance lines: the command that enumerated the list, the count it returned, the resolved package@version and the date; or a Could not enumerate record with a real reason. gsd-ui-researcher gains an enumeration ladder and must record the line rather than write the list from recall. gsd-ui-checker gains Dimension 7. An inventory with no provenance line, a count with no command, an empty could-not-enumerate reason, or a line still carrying the template's unfilled placeholders BLOCKs; a partial line, a line placed below its table, or an honest negative record FLAGs; a complete line passes, and so does a spec carrying no inventory at all, which keeps every UI-SPEC predating the dimension validating unchanged. Whatever the verdict, an unsourced inventory is reported as a non-exhaustive list of known-good components rather than a closed allowlist, so the executor is never blocked from a component the spec merely failed to mention. The checker never runs the recorded command. The dimension count moved on all thirteen surfaces that assert it, across five languages. Also corrects the claim in the English, Korean and Portuguese how-tos that this checker applies a scored six-pillar rubric — that rubric belongs to /gsd-ui-review's retroactive audit. * chore(#2845): backfill changeset pr number to 3745 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2b42b28687 |
fix(#3659): make the worktree base-check trust evidence, not baseRef (#3736)
* test(#3659): baseref-head suppress must be mode-aware regression rows * fix(#3659): make baseref-head suppress mode-aware and thread isolation mode * fix(#3659): review fixes - stale advice purge, message pins, mode alias * fix(#3659): pick-interceptable emit seam, ack merge, writeSync pin * test(#3659): rewrite set-baseref pin, fix writeSync row stub * chore(#3659): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
72819a4616 |
fix(#3031): opt-in reclaim of GSD hooks orphaned in ~/.kimi (#3731)
* test(#3031): failing-first coverage for opt-in ~/.kimi legacy reclaim Drives the user-reachable installer surface against a sandbox HOME seeded with the pre-#2755 wreckage: a GSD [[hooks]] block, hooks bundle and CommonJS marker orphaned in ~/.kimi by a --kimi-code install. Covers the reclaim itself plus the four guards the diagnosis identified as negative space: opt-in only (no flag, no deletion), user-authored TOML and hook files preserved, a --kimi install never reclaiming its own root, and the KIMI_SHARE_DIR/KIMI_CODE_HOME collision where both roots resolve to one directory. Adds a fast-check property that stripping the block never destroys user content. Red until --reclaim-kimi-legacy exists. Refs #3031 * fix(#3031): opt-in reclaim of GSD hooks orphaned in ~/.kimi A --kimi-code install older than 1.10.0 wrote its GSD [[hooks]] block, hook bundle and CommonJS marker into Kimi CLI's ~/.kimi. #2755 fixed the destination but could not reclaim what the old bug already wrote: the stale block is byte-identical to a legitimate Kimi CLI one — both runtimes render the same bytes for the same root, since the command paths derive from the hooks root, not the runtime — so no inspection can tell litter from a working install. Cleanup is therefore opt-in. `--reclaim-kimi-legacy` on a --kimi-code install removes GSD's own artifacts from the legacy root; without it nothing is touched, so a dual-product machine keeps Kimi CLI's hooks and #2755's acceptance criterion holds. Extracts the uninstall path's removal sequence into reclaimKimiHooksRoot() and drives both callers through it, so the reclaim removes precisely what a real uninstall removes rather than a hand-copied second implementation. Guards the wrong-runtime case (a --kimi install would delete its own hooks) and the KIMI_SHARE_DIR/KIMI_CODE_HOME collision where both roots resolve to one directory. Also corrects two pre-#2755 leftovers in the same surface that told users to run `--kimi --config-dir ~/.kimi-code` — the form that produces this very defect, since --config-dir moves only the skills root — and adds the missing --kimi-code entry to the installer's own help. Regression coverage folded into tests/kimi-upgrades.test.cjs beside the #2755 cases, per the regression-test-naming lint. Fixes #3031 * fix(#3031): never reclaim ~/.kimi when this run also installs kimi Found by the isolated adversarial review pass and independently while tracing --all ordering, then reproduced. selectRuntimesFromArgs orders 'kimi' before 'kimi-code' in both --all and an explicit --kimi --kimi-code, and installAllRuntimes installs in that order. So --all --reclaim-kimi-legacy installed a fresh, legitimate Kimi CLI hooks block into ~/.kimi and then deleted it moments later from the kimi-code leg — exiting 0 and reporting success while leaving the user with no Kimi CLI hooks at all. The collision guard could not catch it: kimi-code's own root is ~/.kimi-code, a genuinely different directory. The flag asserts "I only use Kimi Code"; installing kimi in the same invocation falsifies that, so the reclaim is skipped with a notice. Also hardens the collision guard itself. It compared path.resolve strings, which returns false for two spellings of ONE directory — measured, not assumed: a symlinked alias and a case variant on a case-insensitive filesystem both compared unequal, so the guard would not have fired and the install would have deleted its own freshly-written hooks. isSameDirectory now compares directories via resolve, then dev+ino identity, then realpath. Regression tests for all three cases; the two alias tests probe the real filesystem and t.skip() where the alias cannot exist. Refs #3031 * docs(#3031): reattach reclaimKimiHooksRoot's JSDoc to its own function Inserting isSameDirectory anchored on the function name, which placed the helper between reclaimKimiHooksRoot's doc block and the function it documents. isSameDirectory ended up with two stacked doc blocks above it and reclaimKimiHooksRoot with none. Refs #3031 * fix(#3031): warn when --reclaim-kimi-legacy cannot apply The flag only acts inside the kimi-code GLOBAL install branch. Passed with any other runtime, or with --local, it was consumed in silence: exit 0, no cleanup, no message. For a cleanup the user explicitly asked for, silence is indistinguishable from "it ran and found nothing". The scope warning is raised at argument-resolution time rather than inside install(). kimi-code declares hostBehaviors.localInstallDeferred, so install() returns early at the deferral check long before the kimi-hooks-toml branch — a guard placed there is unreachable, which is both dead code and a linted drift shape in this repo. Verified reachable by spawning the real installer. Neither case is a hard error: the flag stays composable with --all, where it is legitimately inert for the other seventeen runtimes. Refs #3031 * docs(#3031): document every case where --reclaim-kimi-legacy skips Refs #3031 * fix(#3031): resolve local config dirs from RUNTIME_META alone in the install harness The remote runner surfaced this: the #3031 warning test drives a local kimi-code install and died with "The path argument must be of type string. Received undefined". runMinimalInstall carried a SECOND, hand-maintained local-dir map beside RUNTIME_META, and it had drifted — four runtimes present in RUNTIME_META (hermes, kimi, kimi-code, zcode) were missing from it, so scope:'local' for any of them resolved path.join(root, undefined) and threw a bare TypeError naming neither the runtime nor the map at fault. #3023 had already hit exactly this for pi and fixed it by adding one more entry, which left the divergence itself in place for the next runtime to rediscover. Local scope now reads RUNTIME_META.localDir, the same table the global branch already reads, with the same loud named error the global branch raises. Parity verified for all 14 previously-supported runtimes: every one resolves to a byte-identical configDir. cline keeps its ternary — its local artifacts land at the project root itself, which is a real exception, not a directory name. Guarded in golden-parity-single-source.test.cjs beside the buildParityManifest anti-divergence test, and both arms of that guard were proven able to fail. Refs #3031 * chore(#3031): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
9a69a86f42 |
enhance(#2971): strict planning filter mode for /gsd-pr-branch (#3720)
* test(#2971): failing-first suite for the pr-branch planning-path filter Binds the not-yet-built planning.pr_strict mode and the corrected filter recipe for /gsd-pr-branch across six layers: pure classification and forbidden-path predicates, real-git fixtures that run the cherry-pick filter loop end to end, config-key registration through the real CLI and both manifests, the executed worktree-materialization claim the issue's triage asked to establish, fast-check properties over arbitrary path sets, and a drift guard over the shipped workflow. Two live defects in today's shipped recipe are pinned as regressions, both reproduced empirically first: `git rm -r --cached` stages a deletion of any .planning/ path the target branch already tracks, so the generated PR removes the base branch's planning files; and the same command leaves the cherry-picked file untracked on disk, so a second commit touching that path aborts the pick with "untracked working tree files would be overwritten" and every remaining commit is silently dropped. The test helper parses the canonical path lists out of gsd-core/workflows/pr-branch.md rather than restating them, so the workflow stays the single source of truth and the suite cannot drift from what ships. Refs #2971 * feat(#2971): strict planning filter mode for /gsd-pr-branch Adds planning.pr_strict — a boolean, default false, that selects what /gsd-pr-branch means by "filtered". Default mode is unchanged: structural planning state survives into the PR branch and the nine transient subdirectories do not. Strict mode drops every .planning/ path, structural files included, and carries a commit over only when it touches at least one file outside .planning/. Strict mode is what makes planning.commit_docs: true safe for a project that versions its planning tree locally but publishes none of it. The alternative posture, commit_docs: false, silently costs parallel executor isolation — a worktree is checked out from a commit, so an untracked or ignored .planning/ is simply absent inside it and the executor has no PLAN.md to read. That claim is now established by an executed fixture rather than inherited. The two path lists are declared once and both projections derived from them, so create_pr_branch and verify can no longer disagree about what the filter promised. verify previously counted every .planning/ path against a documented success criterion of zero while create_pr_branch was specified to preserve five structural files, so a correct run reported itself as failed on every phase that touched STATE.md — which is every phase. It now asserts against the active mode, and names the .planning/ paths default mode deliberately keeps rather than trading a wrong signal for silence. Two verified defects in the same recipe are fixed alongside, because strict mode would have amplified both. `git rm -r --cached` staged a deletion for any .planning/ path the target branch already tracked, so the generated PR removed the base branch's planning files — under strict mode that would have been the entire tree. The same command left the picked file untracked on disk, so a second commit touching that path aborted the cherry-pick with "untracked working tree files would be overwritten" and every remaining commit was silently dropped. Both were reproduced against real git before being fixed. The filter now forces excluded paths back to what the PR branch's HEAD carries, in the index and the working tree; a conflict outside the filter halts instead of being improvised past; a commit left empty by filtering is skipped rather than failing. A clean-working-tree precondition makes the worktree half safe. Closes #2971 * fix(#2971): unwind the checkout on a conflict halt, and test the real recipe Two review findings, both fixed in place. The isolated adversarial pass found that the conflict-outside-the-filter branch exited while leaving the user checked out on the half-built PR branch with cherry-pick state still live — this loop runs in the user's own working directory, so stranding them there is a real cost even though it is not a vulnerability. The branch now aborts the pick, returns to the original branch, removes the partial PR branch, and says so before exiting. The standards pass found the L2 fixtures executed a hand-written mirror of the cherry-pick filter recipe rather than the recipe itself, so a reordering in the workflow would not have been caught — and the order is load-bearing, since restoring a path from HEAD before removing it inverts the filter. The helper now extracts the canonical loop from the shipped workflow and the fixtures execute that verbatim, which also gives the conflict-halt unwind above real coverage. The drift guard additionally pins the two commands' relative order and asserts the workflow carries exactly one canonical loop. Also records the publication gate in the CONTEXT.md glossary next to the commit gate it is distinct from. Refs #2971 * fix(#2971): make the conflict-halt unwind actually unwind, and use the colon slash form The remote matrix caught two defects in the previous commit. The halt path claimed to restore the original branch but did not. `git cherry-pick --abort` does not apply to a single `--no-commit` pick with no sequencer file, and the fallback left the unmerged index in place, which makes `git checkout` refuse — a failure the `2>/dev/null || true` then swallowed, so the user was told they had been restored while still sitting on the half-built PR branch. The unwind now drops sequencer state, hard-resets the disposable PR branch to clear the unmerged index, and only claims a restore when the checkout actually succeeded; when it does not, it says where the user is and gives them the two commands to finish it by hand. Verified against real git: exit 1, the conflict named, HEAD back on the original branch, the partial branch gone, a clean tree and no CHERRY_PICK_HEAD. Two runtime-loaded source artifacts used the retired `/gsd-<cmd>` hyphen form, which names a command no runtime registers. The canonical authoring token for workflows and references is `/gsd:<cmd>`; docs keep the hyphen form, so the documentation added in this branch is unaffected. The comment in src/config.cts moves to the colon form too, since it propagates into the generated lib. Refs #2971 * docs(#2971): backfill PR number into the changeset fragments (#3720) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
14679b866b |
enhance(#2856): add default-off live-DOM UAT capability (#3716)
* test(#2856): add failing-first suite for the live-dom-uat capability Binds the approved triage shape before any of it exists: - containment — the execute:wave:post hook must not render unless workflow.live_dom_uat is true AND the capability resolves active (fail-closed on a missing state entry, and on a non-boolean value) - criterion 4 — agents/gsd-executor.md carries no browser MCP family; asserted as an absence, which is the only way it is observable - Hyrum guard — the pre-existing mcp__playwright__* branch must stay outside the key-gated block, or upgrading silently removes working automated UI verification for every current Playwright-MCP user - parity — the browser glob list now lives in two surfaces (agent frontmatter + workflow detection block); the assertion fails if either gains or loses a family without the other Red by construction: the capability, agent and workflow block do not exist yet. Verified on the remote runner. Refs #2856 * enhance(#2856): add default-off live-DOM UAT capability A phase whose acceptance criteria needed a live DOM could not be finished by the agent that executed it: gsd-executor carries no browser tools, so it correctly returned checkpoint:human-action even though the work was not human-only, just tool-less. Every such phase degraded to "executed, then finished by hand in the orchestrator", and autonomous: false could not distinguish "a human must judge this" from "the executor lacks the tool". Implements the shape approved at triage, not the one reported. The executor's tools: line is NOT widened, in any configuration: for a first-party agent the static list is the only control that exists (ADR-1244 D2, ADR-857 D4, no per-dispatch override). Instead one default-off capability owns the key, the agent, and the step: - capabilities/live-dom-uat/ — activationKey workflow.live_dom_uat (boolean, default false), one additive step at execute:wave:post (onError: skip, gates: []), so it can never halt a wave - agents/gsd-dom-verifier.md — the only GSD agent carrying browser MCP globs, in its own tools: line, with no Bash - verify-work automated_ui_verification — a gsd:live-dom-families block naming both new families AND the key; presence alone never activates Two independent fail-closed gates: isCapabilityActive renders a hook only on state.active === true, plus the step's own `when`. The pre-existing mcp__playwright__* branch keeps the gating it already had and stays outside the new block. Pulling it behind a default-off key would have silently removed working automated UI verification from every current Playwright-MCP user on upgrade. Also closes a host gap this surfaced: execute:wave:post dispatched only contribution + gate, so ANY registered step was declared and silently never run — exactly the single-kind hand-roll loop-hook-dispatch.md names. Step 5.75 now dispatches every kind == "step". The browser-profile lock is tolerated, not coordinated: --isolated is a flag on the operator's own MCP-server registration that GSD neither launches nor parameterizes, so the verifier reports could_not_look / profile_locked, names the flag, and stops. DOM-VERIFY.md keeps could_not_look and nothing_to_report distinct behind a closed reason enum — collapsing them is the ambiguous-run-notes defect reported. Verified on the remote runner. Closes #2856 * fix(#2856): apply review findings from the orthogonal passes Correctness pass (blocker): - delete detectionBlockIsCrlfSafe. It was pass-always: it read the file, replaced LF with CRLF, then indexOf'd marker strings that contain no newline, so the replacement could not change the result and the assertion could never fail for the reason it stated. There is no real CRLF risk on this surface either — the gsd:live-dom-families block has no parser, only human and agent readers. Deleted rather than replaced, per the repo's pass-always-test rule. Isolated security pass (two minors, both real): - execute-phase.md step 5.75: this change is what first activates kind == "step" dispatch at execute:wave:post, which newly opens the ref.command shell path at that loop point. Our own step uses ref.agent and never touches it, but the door is now open, so the step-dispatch line carries the same in-context validate-before-shell warning the sibling gate-dispatch line directly below it already carries. - gsd-dom-verifier: quoted page text in DOM-VERIFY.md is attacker influenced. Require it wrapped in inline code or a fence, kept short, and never left reading as a directive to the next reader. Verified on the remote runner. Refs #2856 * fix(#2856): settle the new-agent roster ripple Checkpoint 2 returned 28 failures, none in the new suite — all of them the guards that exist to make adding an agent a deliberate act. Each is a real boundary that had to move: - docs/AGENTS.md: Tools row must copy the frontmatter verbatim (#2526), so the browser globs lose their backticks; primary-agent counts 21->22, roster 33/34->34/35, Verifiers category 1->2 - docs/INVENTORY.md: roster completeness requires every agents/gsd-*.md to be classified exactly once - gsd-dom-verifier: add the anti-heredoc instruction and the commented hooks: frontmatter pattern both agent gates require - gsd-core/bin/shared/model-catalog.json: every shipped agent needs a profile entry (#3229) - copilot-install / kilo-upgrades / qwen-upgrades: expected agent list and the 34->35 roster boundary - execute-wave-post-gate-pipeline-e2e: execute:wave:post legitimately carries one step now. Asserted as an exact shape — one step, capId live-dom-uat, ref.agent gsd-dom-verifier, onError skip — so it stays a real guard against accidental change rather than being relaxed Two findings worth naming: mcp-tool-inheritance (#2526) rejected the agent for documenting mcp__playwright__* while its tools: line withholds it — a dead instruction that invites the agent to claim a path it cannot take. The prose now names the Playwright MCP family without the dispatchable token, in both the agent and the capability fragment. runtime-launcher-parity rejected the new gsd_run call: each fenced block is its own shell, so a workflow step file invoking gsd_run needs its own canonical preamble. Propagated with scripts/sync-runtime-launcher.cjs. That script also normalizes explore.md, which is unrelated pre-existing drift the parity check tolerates, so it is reverted to keep this diff scoped. The emitted-drift ack supersedes the spent #3370 entry for execute-phase.md — it is merged into next, so its ripple is absorbed at the base and it can no longer clear anything. That is the same supersede the #3370 entry itself performed on the spent #3324 fragment. Its unrelated execute-plan.md entry is untouched. Verified on the remote runner. Refs #2856 * fix(#2856): drop the stale emitted-drift ack entry The automated-ui-verification.md entry was written speculatively rather than from a reported growth, and the check names that precisely: an ack "written or reworded in THIS diff, but nothing here needed it, so it explains nothing". The growth tier keys on the bare filename as it appears under gsd-core/workflows/ or agents/. automated-ui-verification.md is nested under verify-work/steps/, so it was never in the tracked set — only execute-phase.md was ever reported, both before and after the launcher preamble landed. Only ack what the check actually reports. Verified on the remote runner. Refs #2856 * chore(#2856): backfill changeset pr number pr:0 -> 3716. The placeholder fails both changeset-lint (fail_invalid_fragment) and docs-lint (fail_malformed_fragment) by design and can only be resolved once the PR number exists. Both now report ok against GITHUB_BASE_REF=next. Refs #2856 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8df5cb36c2 |
enhance(#2951): refuse [VERIFIED] for a compatibility claim resting on absent metadata (#3718)
* test(#2951): pin the absent-evidence provenance contract (failing first) 17 tests / 22 anchors on the deployed agent text. Measured against the parent commit: 20 anchors fail, 2 pass. The two that pass are the sibling-integrity guards on the package-name and in-repo-value rules -- green before and after is their intended signature. Refs #2951 * enhance(#2951): refuse [VERIFIED] for a compatibility claim resting on absent metadata A claim of the form "X does not support Y" drawn from MISSING metadata -- no python_requires, no engines field, no per-version classifier, no changelog entry, no matching support-matrix row -- no longer earns [VERIFIED] however authoritative the source consulted. An absence is silence about every value, so the same evidence would "prove" both the version being ruled out and the version being standardized on. The only route from an absence to [VERIFIED] is a positive falsification attempt with its failing output pasted; everything short of that is [ASSUMED], which the file already routes to "needs user confirmation before becoming a locked decision". Third member of the family beside the package-name and in-repo-value provenance rules, mirroring PR #2768's shape. A present declared constraint and an affirmatively documented incompatibility are untouched. Closes #2951 * fix(#2951): close the allow-list ambiguity and the mutation gap review found Findings from the isolated adversarial pass and the two-axis review, all fixed: MAJOR (x2, one root cause) -- the absence clause and the present-constraint carve-out gave opposite verdicts on the same evidence for the commonest real case: a classifier list enumerating :: 3.9 through :: 3.13 with no :: 3.14. A researcher could read the enumerated list as a "declared" positive constraint and re-earn [VERIFIED], which is also the evasion vector. The rule now states the decision procedure -- does the declaration bound EVERY value or only the ones it names -- and closes the positive-reframing restatement explicitly. New contract test pins all four clauses. MAJOR -- 'licenses a positive falsification attempt as the route to [VERIFIED]' asserted two independent substrings and never that the route lands on [VERIFIED]. A mutant swapping the tag for [CITED] or [ASSUMED] inverted the rule and survived all 17 tests. Now pinned as one joined sentence. MINOR -- the attributable-failure test regex-matched illustrative examples ("a missing certificate, a wrong host"), so a copy-edit would break it for no reason; relaxed to the substantive clause. The no-paraphrase guard counted only the heading, missing the drift mode in its own name; it now also pins the core proposition to one occurrence, and the test name matches what it checks. An off-by-one in the new allow-list regex bound (141 actual vs 140) is fixed. MINOR -- docs/AGENTS.md listed four of the five governed absence forms while the agent prose, docs/COMMANDS.md and the changeset listed five; three copies disagreeing on list membership is the drift this repo treats as a defect. SCOPE -- removed docs/how-to/verify-a-dependency-compatibility-claim.md and its docs/README.md index line. Both reviewers flagged them as a seventh and eighth surface beyond the six the requester capped, and CONTRIBUTING's "Agent or skill change" row requires only docs/AGENTS.md. The actionable four-case guidance is retained in docs/COMMANDS.md, which is inside the approved scope. Ack byte figures corrected for the final size: 44250 -> 46602 (+2352), 2550 bytes headroom under the LARGE cap of 49152. Refs #2951 * docs(#2951): restore the how-to the phase gate requires Reverses the removal in 6404b43d3. Both /code-review axes had flagged docs/how-to/verify-a-dependency-compatibility-claim.md and its docs/README.md index line as a seventh and eighth surface beyond the six the requester capped, and CONTRIBUTING.md's required-docs row for an "Agent or skill change" names only docs/AGENTS.md, so they were dropped. gsd-phase-gate.cjs then denied gh pr create: it refuses when the recorded enablement sequence has more than one step and the how-to quadrant is empty. The sequence here is genuinely four steps -- run plan-phase, read the [ASSUMED] claim, probe or cite or accept it unlocked, then answer discuss-phase's checkpoint -- and the last step lands on a different capability's surface, so a reference table cannot carry it. Compressing the sequence to one step to unlock howToSkipReason would be gaming the gate, which is the same Goodhart failure this whole change exists to close. A machine-enforced repo gate outranks two reviewers' scope preference and my own reading, so the page is restored and the PR body discloses the two extra surfaces instead of hiding them. Reverting is a one-file change if a maintainer prefers the tighter scope. Refs #2951 * chore(#2951): backfill the changeset PR number pr: 0 -> 3718 now that the real PR exists. The placeholder fails scripts/changeset/lint.cjs with fail_invalid_fragment, which also blocks lint-docs-required from consuming the fragment. Refs #2951 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8da2dd3ad2 |
feat(#2790): add read-only planning.inspect schema-v1 snapshot query (#3708)
* feat(#2790): add read-only planning.inspect schema-v1 snapshot query Adds a read-only query emitting a schema-versioned JSON projection of .planning/ so downstream harness UIs can consume planning state without parsing GSD's Markdown a second time. Composed strictly from the ADR-3180 section 7 owners plus parsePlanDocument, parseRequirements and parseUatItems; markdown structure is read through the Markdown Sectionizer and Markdown Table Model seams. It declares its own flat external schema rather than serializing PlanningSnapshot, which is the diagnostic-rule subject and still growing. Extracts plan-document parsing out of cmdPhasePlanIndex into a shared leaf module so phase.plan-index and planning.inspect cannot drift, including the plan-id derivation both surfaces report. Also fixes parseRequirements dropping the separator delimiter used by the shipped requirements template, surfaced while wiring the requirement rows. * fix(#2790): close spec gaps and a raw-text test assertion found in review Review findings from the standards, spec and security passes: - phases[] rows carry goal and dependencies, the two per-phase elements the issue Summary names that had no corresponding field. Goal is bounded to the section's leading prose so the Depends-on line, the Plans checklist and the wave annotations are not duplicated into it. - requirement rows carry their own diagnostic codes, so a consumer no longer has to string-parse the global diagnostics subject to correlate. - roadmap_acceptance.checkbox is looked up through the phase-id key owners. It was compared raw against the on-disk directory name, so it read null for every real-world slugged phase directory and the evidence channel was inert. - the hostile-input test asserts the structured payload instead of matching the raw stdout string. The absence proof over raw stdout is kept deliberately. * fix(#2790): register planning in the runtime usage list and repair fixtures Remote runner reported 9 failures on 9b3f9aa. Two root causes, both fixed: - gsd-tools.cjs registered the planning family in HOST_COMMAND_ROUTERS but never added it to TOP_LEVEL_USAGE's Commands list. Those are two surfaces a parity test guards, and the top-of-file block comment is not the runtime help string. A real wiring gap that every local gate and three review passes missed. - the new suite's fixtures could not produce a resolvable phase set. STATE.md frontmatter omitted the milestone field, which ADR-3180 7.2 rule 1 makes the primary milestone selector, so the phase set scoped unscoped and every percentage was correctly withheld. Separately declarePhase returned a path without creating the directory, so a phase declared but never written to left phases empty. Both reproduced against the built module before fixing. No assertion was weakened. The withholding path is still exercised and still returns null when the roadmap is absent. * chore(#2790): backfill changeset pr number * test(#2790): cover every enumerated matrix row and contain a symlink escape Reverses a silent deferral. An earlier revision left 23 of the 78 enumerated matrix rows unimplemented and 7 more as one-off manual checks, with a paragraph in the artifact and the PR body describing the gap. CLAUDE.md is explicit that such a note is not a fix and is not surfacing. The rows are implemented instead and the manual-evidence bucket is gone: 49 test cases become 88, covering all 78. Writing the symlink row proved a real leak: a *-PLAN.md symlinked outside .planning/ had its content emitted into the payload, confirmed via a direct call and the spawned CLI. readDocument now resolves target and planning root with realpathSync and rejects an escape, returning the ordinary unreadable-document shape. Tested both ways, because a containment check that over-rejects is its own defect: an escaping symlink leaks nothing and degrades that plan alone, while a legitimately relocated .planning/ symlink stays fully readable. The three new modules are registered in the mutation COVERED registry, which had been reporting has_work false and skipping the Stryker gate entirely. Provisional non-binding floors so the shards run and report; raised to the measured value before merge, since the registry forbids calibrating from a local run. * fix(#2790): satisfy the mutation ratchet contract and scope the 1MB test Remote runner reported 16 failures on 8c451ed. Two causes. The COVERED registry has a paired contract the earlier commit violated: every module needs a matching RATCHET_BASELINE entry, and minScore must be between 50 and 100 with minScore === baseline. The provisional floor of 1 was illegal on both counts. All three modules now sit at 50 — the registry's own enforced minimum — with matching baselines. The score cannot be measured locally: the shard runs node --test, which this repo hard-blocks, so CI is the only source. Floors are raised to the measured value once this PR's shards report; a shard below 50 means the tests need strengthening, since the floor cannot go lower. The 1MB test was measuring the test harness rather than the product. The command handles the oversized payload correctly by spilling to a tmpfile and resolving it back, but the resolved stdout then exceeds runGsdTools' maxBuffer and the helper reports ENOBUFS. It now uses --pick so stdout stays one byte while the full 1MB document is still read and parsed end to end. * fix(#2790): wire containment across every document read this command drives An isolated security review of the containment control found the boundary logic sound but not comprehensively wired: two content reads reached the filesystem without it. An escaped phase DIRECTORY could enumerate external filenames into the file fields and diagnostic subjects. Both enumeration sites now containment-check the directory before reading. Worth recording that the leak was already prevented one layer earlier than the review claimed: Dirent#isDirectory() reports false for a directory symlink, so such a directory never becomes a phase row at all. The guard is defense-in-depth for a direct caller and for platforms where a reparse point reports as a directory. A *-VERIFICATION.md symlinked outside the root leaked one frontmatter value verbatim, because readVerificationStatus does its own read and copies an unrecognized status into the payload's next_action. Closed from the consumer side through that function's existing fs injection seam, so src/verification.cts keeps its signature and its other callers are untouched. The reviewer additionally rated a forged status: passed as an integrity bypass. It is not: anyone able to plant the symlink can plant a real VERIFICATION.md saying the same thing. The incremental risk is confidentiality, which is what these fixes close. src/plan-scan.cts is deliberately unchanged: isPlanSuperseded reads symlink-followed content but yields only a derived boolean, no document text. * test(#2790): give the mutation shards an in-process surface Two Stryker shards were CANCELLED at the 15-minute cap, not failed on score. CI log: 640 mutants instrumented, and the dry run reported 'Ran 1 tests in 20 seconds' because the shards pointed at the integration suite, where nearly every case spawns a gsd-tools subprocess and Stryker's command runner treats the whole test-runner invocation as a single test. 640 x 20s cannot finish in 15 minutes; at the kill it was 27/640 with an ETA over an hour. Every other COVERED module points at a property or unit file, and the workflow's own paths filter lists exactly those two patterns. In-process is the intended mutation surface; the shards were pointed at the wrong shape of test. Adds tests/planning-inspect.unit.test.cjs — 39 cases in 10 describes that spawn nothing and call the built modules directly. plan-document and the router need no filesystem at all, one being a pure content-to-object parser and the other taking an injected mock. The three shards now point here. The 91-case integration suite is untouched and still runs in the normal test job. * chore(#2790): ratchet mutation floors to the measured CI scores CI run 32392791843 measured all three shards, which is the only source the registry accepts — local runs count timeouts as kills and inflate badly. planning-command-router 95.65 -> floor 94 plan-document 76.58 -> floor 75 planning-inspect 57.03 -> floor 56 Applied the registry's own rule, floor(score) - 1, and updated RATCHET_BASELINE to match, since the ratchet test enforces equality. planning-inspect sits well below the file's target of 80 and is the obvious ratchet candidate as its tests improve. planning-command-router already exceeds the target. The placeholder comment about floors pending measurement is removed rather than left standing as a false statement. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
77fa08f1e8 |
fix(#2773): feed the spec-phase edge probe English-translated requirement text (#3713)
* test(#2773): failing-first contract and premise tests for translated edge-probe input Locks the Step 5.5 contract that a response_language project must feed the edge probe an English translation of each requirement's text, and binds that advice to measured engine behavior: the same requirement classifies to zero shapes in Portuguese and to collection/adjacency/empty/ordering in English. Also pins the honest limit — the issue's own repro sentence classifies to [] in English too, so translation is necessary but not sufficient and the authored shapes override is the documented fallback. Red before the doc change; the assertions are all false today. Refs #2773 * fix(#2773): feed the spec-phase edge probe English-translated requirement text The shape cues in src/edge-probe.cts are English word-boundary regexes, so a project running with response_language set wrote its SPEC requirements into the Step 5.5 $REQS_JSON heredoc in that language, matched no cue, classified to zero shapes, and landed every row in the unclassified sentinel (#1110). The taxonomy contributed nothing and --auto left it all unresolved — the probe was a silent no-op for exactly the spec type it exists to harden. Step 5.5 now states that the $REQS_JSON payload is engine input rather than user-facing output, so the response_language rule does not govern it: each requirement's text carries a faithful English translation, the SPEC keeps its original language, and requirement ids are never translated or renumbered. The instruction sits before the heredoc on purpose — the downstream APPLICABLE=0 warning fires only when every requirement is unclassified, so a partly-classified non-English spec would otherwise slip through with no signal at all. Measured against the compiled engine: the same requirement returns [] in Portuguese and collection -> adjacency/empty/ordering in English. Also measured: the issue's own repro sentence returns [] in English too, so translation is necessary but not sufficient — the instruction therefore points at the authored shapes override for prose carrying no cue in any language rather than promising that translation restores classification. Doc scope only, per the triage disposition on the issue. The compiled engine is untouched; the lang-hint / per-language cue-set fix is a separate follow-up. Closes #2773 * fix(#2773): clean up the edge-probe temp file on the placeholder-guard exit path Surfaced by the isolated security review of this branch. Between the mktemp and the unconditional cleanup, Step 5.5 has two sibling guards that disagreed about their own invariant: the engine-failure guard runs rm -f "$REQS_JSON" before exiting, while the empty/placeholder guard directly above it exited without one. A spec run that tripped the placeholder check therefore stranded a temp file holding the SPEC's requirement text in TMPDIR, once per failed run. The added contract test walks the region between the mktemp and the unconditional cleanup and asserts no exit path leaves the file behind, so the two guards can no longer drift apart. Proven to bind: run against the pre-fix file the walker reports the leaking exit; against the fixed file it reports none. Refs #2773 * docs(#2773): record the edge probe's English-cue input constraint in the predicate store The co-change gate flagged CONTEXT.md (13 co-changes with spec-phase.md) and docs/CONFIGURATION.md (11) as candidate-missing-updates, and both were real gaps rather than incidental coupling. CONTEXT.md's EdgeCompletenessProbeModule entry documents the input contract for classifyShape but did not record that SHAPE_CUES are English word-boundary patterns — so the predicate store implied text was language-agnostic, which is what a future agent reads before touching this seam. docs/CONFIGURATION.md's response_language row is what a non-English project reads when it turns the setting on; it now names the one deliberate exception and links to the FEATURES.md explanation, so the interaction is discoverable from the config key rather than only from the workflow. CONTEXT-INDEX.json regenerated via gen-context-index.cjs --write. The drift-ack fragment is updated for the final byte range and now also records the placeholder-guard cleanup fix folded into the same block. Refs #2773 * fix(#2773): append the growth rationale to the existing spec-phase.md ack entry The remote runner caught this: emitted-attribution.test.cjs pins the 0000-legacy-migration.json spec-phase.md entry permanently (the #2914 migration regression test asserts the exact '31987 -> 31997' delta text survives), so removing it to avoid a duplicate-key collision with a new fragment broke that test instead of satisfying the ratchet. The entry is an accreting log, not a single-use slot — #2733, #3132 and #3102 were each appended to the same reason string by later PRs, which is how a shared growth key coexists with the rule that two ack sources may never name the same path. This appends the #2773 rationale the same way and drops the separate fragment, whose spec-phase.md key was the collision. Verified locally by reproducing both affected tests against the real fragment before re-dispatching: the pinned delta survives, grown[0].acked is true, staleAcks is empty, and all 35 entries still read as spent. Refs #2773 * docs(#2773): add a how-to for probing edges in a non-English project The phase gate's enablementSequence check caught a wrong call of mine. I had recorded that no how-to was owed because the user takes zero extra steps — the workflow translates the probe input itself. Written out, though, the sequence from off to value is two steps and step 1 depends on response_language, a setting owned by a different capability than the edge probe, which is exactly the condition the how-to test names. There is also real task content a reference table cannot carry: the three-way split between a few unclassified rows (the classifier's recall gap), every row unclassified (the probe could not read the spec at all), and the silent partly-classified case where the APPLICABLE=0 warning never fires. That last one is what a user would otherwise misread as a clean bill of health. Shaped after the resolve-edge-coverage-findings / resolve-unreachable-guard siblings and indexed from docs/README.md next to its closest relative. Refs #2773 * chore(#2773): backfill the changeset PR number pr:0 placeholder replaced with the real PR number now that #3713 exists. Refs #2773 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
adb46cdd85 |
feat(#2734): surface STATE.md commit-age on the statusline (#3700)
* test(#2734): failing-first suite for the statusline STATE.md freshness marker Binds the contract before any hook change exists: a `state ~N commits back` segment gated on the state_head stamp landed by #2622, firing at the same advisory threshold /gsd-health's W024 uses rather than at > 0. Covers all five acceptance criteria — threshold parity (19/20/21 boundaries), both renderers including formatGsdStateCompact, an exact spawn-count assertion, repo-pinning and sub_repos degradation, and behavioral parity against readStateHeadFreshness rather than a source-grep of the two fence copies. 52 example-based tests plus 5 seeded fast-check properties. Red now by design. * feat(#2734): surface STATE.md commit-age on the statusline Adds an opt-in `state ~N commits back` marker to the GSD-state segment, consuming the `state_head` stamp and freshness contract landed by #2622. A solo developer returning to a project reads "Phase 4, executing" in STATE.md and acts on it, without noticing the codebase moved 40 commits since that line was written. /gsd-health reports it as W024, but only if you think to run it; the statusline is the surface you see without asking. Fires at STATE_HEAD_ADVISORY_COMMITS (20), the same threshold W024 uses, not at > 0: with commit_docs:true the commit carrying a STATE.md sync advances HEAD by one, so > 0 would alarm permanently on a fresh project. Costs exactly one bounded git subprocess per render and none when disabled. `rev-list --left-right --count` answers ancestry and distance together, and repo pinning is a filesystem check mirroring projectOwnsItsRepo rather than a --show-toplevel compare, which is unreliable on macOS /private/var and Windows 8.3 paths. Every unresolvable input degrades to the tri-state unknown -- the marker is absent, never a "fresh" claim the project cannot substantiate: a malformed stamp, a root that does not own its .git, a sub_repos workspace, history rewound past the stamp, or git being unavailable. Also collapses statusline config resolution onto one resolveStatuslineOptions() seam. runStatusline() and renderStatusline() duplicated it byte-for-byte; one copy is what keeps a newly-added key from reaching only one of them. * test(#2734): route the e2e spawn through the process seam and fix fixture leaks Review findings from the two orthogonal passes: - `bothEntryPointsResolveOptionsIdentically` spawned a child and substring-matched its stdout to test a pure function. It now calls resolveStatuslineOptions() directly — no subprocess, no text matching. - `skipsFreshnessWorkWhenTodoTaskActive` genuinely needs a child (the !task gate lives in runStatusline, which reads stdin), so it now spawns through tests/helpers/process-seam.cjs and proves the negative with a filesystem fact: the git shim appends to a marker file on every invocation, and the assertion is that the marker never appears. Stronger than asserting text is missing, and it drops the last stdout substring match in the block. - Every fixture-creating test now registers `t.after(() => cleanup(dir))` instead of a trailing cleanup(dir), which leaked the temp repo on assertion failure. derivationAgreesWithStateModule reassigns `dir` across five fixtures, so it binds each directory at scheduling time rather than cleaning only the last. Also corrects markerCoexistsWithMilestoneComplete, which asserted the wrong expectation rather than finding a code defect: `percent` drives the progress bar too, so the milestone segment reads "v1.9 [##########] 100%". The marker appends after it, which is what the test exists to prove. CONTEXT.md's opt-in statusline key list was missing statusline.show_git as well as the new key; both are now enumerated. * docs(#2734): backfill changeset PR number (#3700) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2fca0e17e4 |
enhance(#2554): resolve code review depth from path-scoped override rules (#3695)
* test(#2554): failing-first suite for path-scoped code review depth overrides Binds the not-yet-built code-review-depth module: segment-aware path-prefix matching of a changed-file set against ordered {paths,depth} rules, resolution order flag > strongest matching rule > global > standard, typed validation errors, and the large-scope downgrade boundary. Also proves behaviorally that workflow.code_review_depth_overrides is not yet a registered config key. Refs #2554 * feat(#2554): resolve code review depth from path-scoped override rules Adds workflow.code_review_depth_overrides — an ordered array of {paths, depth} rules matched against a review's changed-file set by segment-aware path-prefix comparison. Resolution order is --depth= flag, then the strongest matching rule, then workflow.code_review_depth, then standard; a matching rule replaces the global rather than being max'd with it, so quick and standard rules stay meaningful. Glob metacharacters are a hard configuration error rather than sugar for a prefix, and malformed rules halt the review instead of degrading to standard. The resolver is pure and reports its own provenance, so the workflow can print the resolved depth and the rule that matched. The pre-existing >50-file deep-to-standard downgrade moves into the module and now names the rule it overrode. The key is registered centrally rather than as a capability config slice: the federated slice channel admits only boolean/string/number/enum, so an array slice would be dropped as malformed. Closes #2554 * test(#2554): correct depth-provenance assertions and pin out-of-repo paths Two corrections to the failing-first suite. The source assertion for a non-matching rule with no global configured expected 'config'; with no global set the depth comes from the default, and a companion assertion tolerated either value, so both passed against an implementation that derived provenance from whether any rules existed rather than from where the depth came from. The out-of-repo absolute-path case used a home-directory path that matched neither implementation, so it never exercised the defect it named. It now pins the discriminating cases: an absolute path outside the repo root must not match a repo-relative rule, and one under the root must. * docs(#2554): document path-scoped code review depth overrides Reference rows for workflow.code_review_depth_overrides in the configuration, features and commands references plus the locale copies that carry those tables, and in the planning-config reference. Explanation of why escalation is whole-review rather than per-file and why v1 is prefix-only. New how-to for scoping review depth by path, carrying the configuration-error reason table and the distinction between nothing to report and could not look. CONTEXT.md glossary entry and the INVENTORY row for the new CLI module. ja-JP and ko-KR CONFIGURATION.md carry no code_review keys at all, and ko-KR and pt-BR FEATURES.md carry no code-review config table, so those files are deliberately untouched. * fix(#2554): make the depth-misconfiguration halt executable and reject control chars Three review findings, all in this change. The misconfiguration halt was prose rather than shell: the error-printing fence was followed by an unconditional extraction fence, so an ok:false result threw and left the depth empty instead of stopping the review. Prose is not a guard — the two fences are now one block with a real conditional, and anything that is not the literal string true fails closed. An interior control character in a rule path survived validation and reached the provenance string and the summary box; rule paths now reject control characters via a new PATH_CONTROL_CHAR reason, after the glob check so precedence is unchanged. That in turn makes the field record safe to delimit, so the seven node invocations that each re-parsed the same result to read one field collapse to one. Also corrects the glossary entry's illustrative paths, which the glossary-ref check read as real repository references. * fix(#2554): use the fast-check v4 string API and acknowledge workflow growth Two failures from the remote matrix on d3111f45, both this branch's. The property block built its segment arbitrary with fc.stringOf, removed in fast-check v4. Because the arbitrary is constructed in the describe body, the throw took out all four property tests rather than one — they had never executed. Rewritten to fc.string({unit, ...}), the form this repo already uses in emitted-attribution.test.cjs. Every other fast-check helper in the file was audited against the installed module. The emitted-attribution growth arm needed an acknowledgment for code-review.md, which grew 5376 bytes. The pre-existing 3503 fragment keying the same file is spent — its ripple was absorbed when #3503 merged, and the base file is exactly the 34435-byte baseline this growth is measured against — so it cannot clear anything, while the ack lint hard-fails on a duplicate key across two sources. Removed it in favor of the new fragment, which is exactly how #3503 itself replaced the spent 3191 fragment. * docs(#2554): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8526bd46f8 |
enhance(#2475): scope ADR-443 item 1 to the operator surface and ratify the ADR (#3688)
* test(#2475): widen the item-1 effort-caller guard to both CLI argument shapes The guard matched only `resolve-execution ... --effort\s`, but the CLI also accepts `--effort=<level>` (gsd-core/bin/gsd-tools.cjs). A workflow written with the equals form was a live invocation-override caller the guard passed silently, along with `--effort` at end-of-input. Lift the matcher to a shared predicate and assert it directly against every shape the CLI accepts, plus the decoys it must not fire on (--effortless, a bare --effort with no resolve-execution, item 6's --attempt caller, a call and flag split across lines). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2475): scope ADR-443 item 1 to the operator surface and ratify the ADR ADR-443 sat at Proposed on one condition: its Decision item 1 orchestrator invocation override needed a caller in shipped orchestration. Per the maintainer's ruling, take unblock path (b) for item 1 only -- record that the override is an operator-facing CLI surface, deliberately not driven by shipped orchestration, and ratify. The ADR's own path (b) wording is not adopted verbatim: it says the scope is limited to static install-time propagation, which is false on both counts -- item 6 has a live caller (#2296) and #2481 delivered a live invocation-time argv channel. Only one precedence step is narrowed. No consumer was invented to clear the gate: nobody has asked for a per-run effort override, and #2475's actual complaint is already closed by the cascade-to-argv path. The amendment states explicitly that --effort remains supported and is not deprecated, so the scoping is not read as dead code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2475): correct two bare-`gsd` invocation examples to `gsd_run` There is no `gsd` binary -- package.json exposes gsd-core, gsd-tools, gsd_run and gsd-mcp-server. Both sites presented a command that cannot run as written. One is in this branch's own new ADR-443 amendment; the other is a pre-existing error in the docs/CONFIGURATION.md assumption_delta row, fixed here rather than deferred. No translated copy carries either line, so no i18n drift is created. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2475): record the guard/CLI divergence risk on the item-1 matcher The predicate independently models gsd-tools.cjs's argument parser rather than sharing a constant with it, so a third --effort spelling would leave the guard reporting green while ADR-443's ratifying invariant silently stopped holding. Name that risk where the next editor will meet it. Also restores the bounded-prose rationale that was attached to the eslint directive removed in 39793079c -- the directive went unused once the regex moved to a const, but the reasoning it carried is still worth having. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
ea594300d9 |
fix(#3606): validate hook-kind coverage at call sites and dispatch generically (#3687)
* test(#3606): pin hook-kind coverage in the wired guard * fix(#3606): validate hook-kind coverage at call sites and dispatch generically * fix(#3606): address review - segment-granular narrowing, zero-coverage diagnosis, quick.md, fragment extraction * fix(#3606): drop stale shrink-ack, export HOOK_GROUP_KINDS, dedupe scanner regex * chore(#3606): regenerate install-tree fixtures for new wave-post fragment * chore(#3606): sync canonical launcher preamble into new fragment * fix(#3606): keep fragment preamble ahead of first gsd_run mention * fix(#3606): revert sync script's preamble move in explore.md * chore(#3606): regenerate derived manifests post-rebase * chore(#3606): allowlist peer test files - base was red on the count lane * chore(#3606): regenerate inventory for peer's verify-command-grounding doc * chore(#3606): grounding test maps to its own module by longest prefix * chore(#3606): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
79781e68eb |
enhance(#2401): ground verify-command paths and inherit prior-phase commands (#3678)
* feat(#2401): ground <automated> verify-command paths and inherit prior-phase commands Adds a deterministic resolvability probe over each PLAN.md <automated> verify command and surfaces the nearest prior phase's proven commands to the planner at every context window. - src/verify-command-grounding.cts: recognizer (not a shell interpreter) that grounds a leading cd <literal> chain and npm --prefix <literal>, and reports unresolvable rather than guessing. Never executes command text. - gsd-tools check verify-command-paths <N>: per-phase probe, wired into plan-phase.md before the plan-check pass. - init.plan-phase gains prior_verify_commands, ungated by context_window. - gsd-plan-checker: new Verify Command Path Resolvability dimension that reports the failing target and never prescribes a replacement. Also fixes first-match-wins prefix bucketing in scripts/lint-test-file-count.cjs (readdir order is not stable across platforms, so a module whose name extends another's with a hyphen bucketed differently on Linux than on macOS). Closes #2401 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2401): ground the canonical --prefix form, quoted paths, and absolute cd resets Independent review found three defects in the recognizer: - npm --prefix DIR run SCRIPT never reached the script-existence check, because the pattern required npm and run to be adjacent. That is the form the docs tell planners to prefer, so script_missing never fired for it. The prefix flag and its value are now stripped before matching. - --prefix captured with \S+, so a quoted path containing a space was truncated to a stray opening quote and reported as a missing directory - a false blocker, worse than the bug this feature fixes. The capture is now quote-aware. - A chained cd whose later segment was absolute concatenated instead of resetting, producing a nonsense path and another false blocker. The fold now resets on an absolute segment. Also replaces the bespoke phase-directory regex with the canonical phase-id helpers. Real phase directories are NN-slug, not phase-N-slug, so the prior-command harvest matched nothing outside its own fixtures and the planner-inheritance half of this feature was dead code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(#2401): source task blocks from the canonical sectionizer The module carried its own copy of the <task>-block grammar - a fourth hand-rolled mirror of the one markdown-sectionizer owns. verify.cts keeps its copy only because it needs the type= attribute the canonical helper discards; this module never reads that attribute, so it can share the owner outright instead of adding a test around a copy. extractAutomatedCommands now takes task bodies from extractTaggedBlocks and the out-of-task remainder from stripTaggedBlocks. A task-grammar parity test pins the attributed task-name set against the canonical helper across six awkward task shapes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2401): extract agent-file overflow to references and repair the property arbitrary The remote matrix run came back red with 19 failures, four root causes: - agents/gsd-plan-checker.md and agents/gsd-planner.md both blew the 49152 agent cap. Their bodies move to gsd-core/references/, leaving @-reference stubs, per the documented overflow pattern. - The new checker dimension invoked gsd_run before the canonical preamble that defines it. The call is deleted outright: plan-phase.md already runs the probe and hands the result in as {VERIFY_PATHS}, so the dimension consumes that rather than re-running anything. - fc.fullUnicodeString does not exist in fast-check 4.8.0. Replaced with fc.string({ unit: 'binary' }), which covers the same 0000-10FFFF range. - Three runtime-loaded files grew; acknowledged in the existing ack fragments that already own those bare filenames, since two ack sources may never name the same path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2401): regenerate golden install-tree fixtures for the new references Adding two files under gsd-core/references/ changes what the installer emits into every runtime's tree, so all 19 golden install-parity fixtures went stale. Regenerated with npm run gen:install-tree; the delta is exactly the two new reference paths per runtime, no removals. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2401): backfill changeset pr number to 3678 * fix(#2401): treat ~ as a home expansion only at the start of a path Windows CI caught this on both shards; the Linux-only remote matrix cannot see it. The dynamic-path refusal rejected ~ anywhere, and a GitHub Windows runner's tmpdir is an 8.3 short name - C:\Users\RUNNER~1\AppData\Local\Temp - so a valid absolute Windows path came back unresolvable/dynamic_path. This was a production bug, not a test artifact: any Windows user whose project path carries an 8.3 short name, or any literal ~, silently lost the probe entirely - every command degrading to unresolvable with no explanation. ~ is a home expansion only at the start of a path; elsewhere it is an ordinary literal. The check is now split: $, backtick, *, ? and newline stay refused anywhere (substitution and globs, and the glob characters are illegal in Windows path components regardless), while ~ is refused only leading, tolerating one leading quote since the check runs before quote stripping. The prior tests only caught this on Windows because only Windows puts a ~ in tmpdir. Four new tests pin it on every platform via a fixture directory literally named RUNNER~1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
4e60dba717 |
fix(#3604): make glossary ref visibility independent of backtick parity (#3680)
* test(#3604): pin parity-dependent ref visibility in the glossary gate * fix(#3604): make glossary ref visibility independent of backtick parity * chore(#3604): regenerate CONTEXT-INDEX for corrected predicates * chore(#3604): regenerate examples CONTEXT-INDEX for corrected predicates * fix(#3604): complete retired-family exemptions and pin the guard rails * chore(#3604): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
1bf73d957b |
enhance(#2295): record the resolved model per reviewer in REVIEWS.md frontmatter (#3649)
* test(#2295): failing-first coverage for per-lane resolved-model recording * feat(#2295): record the resolved model per reviewer lane * docs(#2295): document the recorded reviewer model and its provenance * fix(#2295): refuse control characters in a recorded model value * test(#2295): correct watermark assertions for the widened mark shape * fix(#2295): anchor the role-manipulation injection pattern at a word boundary * feat(#2295): record the applied reasoning effort in the model value * chore(#2295): backfill changeset pr number * chore(#2295): restore em-dash in changeset body --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
1adf6d2245 |
fix(#3620): point the docs at files that actually exist (#3658)
* fix(3620): point the docs at files that actually exist docproof found 34 stale references; the reporter hand-read all 34 and reported the 8 that are real, explaining why the other 26 are deliberate (files the documents themselves label legacy or "superseded by", and one pre-Diataxis link label whose target still resolves). Those 26 are left alone — re-touching them would contradict the issue's own analysis. Every claim was re-verified against git ls-files at HEAD before editing. docs/INVENTORY.md said its roster is anchored by six drift-control tests. Five are gone (commands-doc-parity, agents-doc-parity, cli-modules-doc-parity, hooks-doc-parity in 5d8a8c4d; command-count-sync in |
||
|
Tom Boucher
|
9e4f0e99ad |
fix(#3631): exclude only __pycache__-resident bytecode from the consent digest (#3650)
* test(3631): failing-first coverage for bytecode-cache in the consent hash
bundleContentHash digests a walk with no exclusion, so a routine 'python3 -m unittest'
inside a Python-backed capability bundle writes __pycache__ under the bundle, the
recomputed hash stops matching the consent record, and the capability silently goes
inactive — no error, no warning, and loop render-hooks then omits its step and gate.
Two distinct triggers, and the second is the sharper one: collectBundleEntries pushes a
{kind:'dir'} entry for EVERY directory and the digest emits a TAG_DIR marker for it, so an
EMPTY __pycache__/ flips the hash before a single .pyc is written. A fix filtering only
*.pyc would leave that live. Verified by execution against the built lib: 5 of 7 probe
rows diverge from intent today, including the empty-directory row.
The anti-regression rows are the point of the shape: editing a real scripts/m.py and
adding node_modules/pkg/index.js must BOTH still change the hash. node_modules is
deliberately not excludable — its contents are required at runtime, so dropping it from
the digest would stop consent binding executable content. The symlink row pins ordering:
exclusion must apply after the lstat fail-closed rejection, never before.
Refs #3631
* fix(3631): exclude derived bytecode caches from the consent digest
RED proven at e5ba8f1fe on the remote runner: 8 failures, exactly the rows predicted to
fail, with the four anti-regression rows already green.
collectBundleEntries now skips a hardcoded, gitignore-independent set from the DIGEST:
basenames __pycache__, .pytest_cache, .DS_Store, and any .pyc/.pyo file. Matching is
byte-exact on the raw Buffer name (the walk never utf8-decodes) and case-sensitive, so the
digest does not vary with how a name happens to be spelled on a case-insensitive volume.
Three properties were preserved deliberately, each pinned by a test:
- The filter runs AFTER the lstat symlink/non-regular fail-closed rejection. Filtering
first would have turned the exclusion into a way to smuggle a symlink past the check;
a symlink named x.pyc still throws.
- Excluded entries still count toward BUNDLE_MAX_FILES and BUNDLE_MAX_TOTAL_BYTES. The
caps guard the WALK; the digest answers a different question, and exclusion must not
become an unbounded-bytes hole.
- An excluded DIRECTORY is neither emitted as a TAG_DIR marker nor recursed into. The
directory marker was the sharper half of this bug: an empty __pycache__ flipped the
hash before any .pyc existed, so a *.pyc-only filter would have left it live.
The issue proposed either a gitignore-aware walk or a list including node_modules. Both
are rejected. A consent binding must not delegate its scope to a .gitignore the bundle
author does not control — one line there would drop arbitrary executable content out of
the hash. And node_modules holds code that is required at runtime; excluding it would stop
consent binding executable content, turning a usability bug into a supply-chain hole. What
makes __pycache__ different is that CPython validates each .pyc against its sibling
source, which remains hashed, so a real code change still invalidates consent.
Docs: CONTEXT.md's 'EVERY regular file AND directory' claim is corrected in place.
ADR-2363's residual-gap section said the walk had 'no exclusions' — per
docs/adr/README.md ('ADRs are append-only') that is corrected by a dated amendment rather
than an in-place edit. Its D4 argument is unaffected: skill bodies are .md and stay bound.
Fixes #3631
* fix(3631): narrow the digest exclusion after two isolated security reviews
The first cut of this fix passed the full suite and was still wrong. Both orthogonal
reviews rejected it, and the second one found a hole that has nothing to do with Python.
HIGH — an excluded DIRECTORY was 'continue'd before recursion, so its whole subtree was
permanently outside the digest. Declared hook script paths allow '_', '.' and '/' with no
directory or extension rule, so hooks:[{script:'__pycache__/run.js'}] installed, executed
via node, and its bytes could be rewritten forever without moving the hash. Ship benign
v1, collect consent, then own the machine. No Python involved.
FALSE RATIONALE — the justification I wrote into the code, CONTEXT.md, the ADR amendment
and the changeset claimed CPython validates a cached .pyc against its sibling source, so
the source staying hashed kept consent honest. That is not true, and I proved it by
execution rather than argument: default timestamp invalidation compares only the source's
mtime and size, both settable by anyone who can write the bundle. A forged pyc ran while
the .py was byte-identical.
Also wrong: '*.pyc' matched anywhere, but a legacy sourceless scripts/x.pyc IS importable,
so excluding it was a live vector.
Narrowed to what is actually defensible:
- a DIRECTORY named __pycache__/.pytest_cache has only its TAG_DIR marker suppressed;
the walk still recurses and hashes every non-excluded child.
- .pyc/.pyo are excluded ONLY when the parent basename is exactly __pycache__.
- a regular FILE named __pycache__, and a DIRECTORY named x.pyc, stay bound.
- declared hook paths containing a __pycache__/.pytest_cache segment or a .pyc/.pyo
basename are now rejected in both validator copies — a file named .pyc can contain
perfectly valid JavaScript, so the exclusion must not be reachable from a declared
surface.
Accepted residual risk, stated plainly in ADR-2363 and CONTEXT.md instead of explained
away: a forged __pycache__/mod.pyc matching an unmodified, still-hashed mod.py executes
without moving the digest. Before this change that write was detected. It is accepted to
stop routine bytecode caching from silently deactivating capabilities, and it is bounded —
the attacker needs post-consent write access, everything outside __pycache__/*.pyc stays
hashed, and no declared surface can point into the excluded space.
Known limitation, not papered over: .pytest_cache CONTENTS still move the digest. Only the
directory marker is suppressed. Excluding that subtree would reopen the HIGH finding.
Refs #3631
* fix(3631): drop the .DS_Store exclusion and pin what the caps actually bind
Second round of isolated review findings. The hardening closed the two original holes —
both re-reviews confirmed that by execution — but it introduced a new one of the same
shape, and left three claims unbacked.
HIGH, self-inflicted: .DS_Store was excluded from the digest at any depth, but the hook
path validator was hardened only for __pycache__/.pytest_cache/.pyc/.pyo. So
script:'hooks/.DS_Store' was ACCEPTED, runnableHookCommand emits the bare quoted path for
a non-.js name (the branch .sh hooks already use), and capability-source copies it with
its mode bit intact. Ship it +x with a benign shebang, take consent, then rewrite it
forever — the digest never moves. Fixed by DELETING the .DS_Store exclusion rather than
teaching the validator about it: .DS_Store has nothing to do with this issue's Python
bytecode symptom, and an excluded filename is a permanently unhashed name. The narrower
the exclusion, the smaller the hole.
The residual-risk bound in ADR-2363 and CONTEXT.md claimed declared surfaces cannot reach
excluded space. That is false and is now stated correctly: node resolves an unregistered
extension through the default .js handler, so a hashed, consent-covered hooks/run.js that
requires '../__pycache__/mod.pyc' reaches it in one hop. The validator guard raises the
bar for DECLARED surfaces; it does not contain the risk. The two bounds that are real —
post-consent write access required, everything outside __pycache__/*.pyc still hashed —
are kept.
The BUNDLE_MAX_FILES boundary test had gone vacuous: it padded with root-level *.pyc,
which the hardening made non-excluded, so it no longer proved anything about excluded
entries while the ADR claimed the caps were test-pinned. It now pads __pycache__/f{i}.pyc,
with the arithmetic re-derived by execution (capability.json + the still-counted
__pycache__ dir + N). BUNDLE_MAX_TOTAL_BYTES had zero coverage at all and is now pinned by
a sparse 32 MiB __pycache__/big.pyc that must still trip the size cap — the test that
proves exclusion did not become an unbounded-bytes hole.
Added the parity assertion CLAUDE.md's Generative Fix Divergence rule requires for the two
isSafeHookScriptPath copies, and proved it can fail: mutating one BUILT copy to drop .pyo
made the parity check report the divergence. Also pinned semantics that were correct but
untested and would have survived mutation — __pycache__/sub/x.pyc stays hashed (the parent
resets to sub, which is the recursion threading itself), .pytest_cache/y.pyc stays hashed,
and .pyo in both directions, which was a free surviving mutant.
Changeset rewritten: it still described the rejected wholesale-exclusion semantics.
Refs #3631
* chore(3631): backfill changeset PR number (#3650)
---------
Co-authored-by: sim <sim@local>
|
||
|
Tom Boucher
|
2972da4c9d |
enhance(#3619): ratchet the platform seam with local/no-private-binary-resolution (epic #3411 Phase 3) (#3636)
* chore(#3619): ratchet the platform seam with local/no-private-binary-resolution
Epic #3411 Phase 3, the ratchet. Scope revised with maintainer approval and
recorded on the issue: the epic's literal ask was a rule rejecting a bare-name
spawn outside the seam. Surveyed at
|
||
|
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> |
||
|
Tom Boucher
|
924f649f87 |
docs(#3625): record the spawn-library evaluation as ADR-3625 (#3632)
Spike outcome for #3625: evaluate cross-spawn / nano-spawn / execa against the hand-rolled Windows binary resolution and cmd.exe mediation that epic #3411 Phase 1 (PR #3621) is landing in the platform seam. Verdict: stay hand-rolled, with revisit-if conditions recorded so the call is not re-litigated in a future PR. Evidence, per the issue's "Done when" list: - Sync/async verdict per candidate. nano-spawn is async-only — settled by its own README, which lists "synchronous execution" among the features execa has and it does not. cross-spawn exposes `.sync`. execa exposes execaSync, which its own docs discourage. - CVE-2024-27980 escaping verdict per candidate. None uses shell:true. cross-spawn independently arrives at the SAME mechanism the seam uses: cmd.exe /d /s /c with a pre-escaped line and windowsVerbatimArguments. That validates the seam's approach rather than superseding it. - Maturity axes scored with measured data (registry metadata 2026-08-18, transitive footprint measured by install). Two premises in the issue did not survive measurement, both recorded: - The CRITICAL 167-symbol/53-file blast radius is a `direction:both` measurement, inflated by downstream callees. A call-shape change ripples to CALLERS: upstream at depth 15 is 14 symbols / 7 files, MEDIUM, and it terminates at depth 4. The decision does not rest on the CRITICAL figure. - The cited vendoring precedent path does not exist; the real one is gsd-core/bin/lib/vendor/re2js.cjs. Decisive against the only structurally-eligible candidate (cross-spawn): it resolves process.cwd() first on Windows even when an explicit PATH is supplied, calls process.chdir() during resolution, and keys escape depth on a node_modules/.bin/*.cmd path regex of the same shape #3411 was filed to delete. Adoption would also break execTool's observable not-found contract across 53 dependent files. Doc-only: docs/adr/** plus a root-level CONTEXT.md pointer. The CONTEXT.md addition is a new line rather than an edit to the seam's glossary paragraph, which PR #3621 rewrites wholesale — same-line edits would conflict on merge. ADR index regenerated via scripts/gen-adr-index.cjs --write. lint:ci exit 0; lint-docs-required and changeset/lint both run with GITHUB_BASE_REF=next and report ok_no_user_facing_changes. Closes #3625 Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
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> |
||
|
Tom Boucher
|
cc3fd4548d |
docs(#2866): reconcile ADR-3574 against the shipped environment (#3622)
The ADR was written mid-epic and describes a tree that no longer exists. There are now two choreographies, not three: phase 6 deleted bin/install.js's agent-staging loop and its _DESCRIPTOR_AGENTS_RUNTIMES gate outright, so the refusal in decision 1 governs a two-way divergence. The ADR's own revisit condition has been met. It said to reopen the unification question when the applySurface descriptor-agents migration completed. It has. Revisited on evidence: the shapes did not converge on the axis that mattered, because phases 6 and 7 touched agents and exports, not the prune. applySurface still prunes by allow-list so it cannot delete a user file by construction; installRuntimeArtifacts still wipes and restores a snapshot. Decision 1 stands, for a narrower and better reason than when it was written. Records delivery status per decision, including that decision 3 was already satisfied when measured and decision 4 was the hardest part rather than the independent one the ADR predicted. Notes that #2875's AC1 is now doubly stale - deliberately unmet, and naming three call sites where two remain - so anyone reconciling the tracker treats this ADR as governing rather than the criterion as outstanding. Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |