647 Commits

Author	SHA1	Message	Date
Tom Boucher	53ea8e0664	fix(#3057): make a guard's failure distinguishable from its benign result — Wave 1 (#3088) ... * fix(#3057): refuse the write when the duplicate scan cannot complete writeManifest documents itself as a fail-closed duplicate guard: if any existing manifest shares plan_id with a different, non-terminal job_id it must refuse, because dispatching again would duplicate the external job. It could not honour that. The scan reads every sibling manifest looking for the duplicate, and an unreadable or unparseable sibling was `continue`d past. If the corrupt file was the one holding the live duplicate, the scan found nothing and a duplicate external job dispatched. The asymmetry is what gives it away: a malformed TARGET refused with malformed_existing because clobbering is unacceptable, while a malformed SIBLING was skipped — yet siblings are the only thing the duplicate check reads. Adds a scan_incomplete verdict that refuses and names the offending file, so an operator can quarantine or repair it. Fail-closed alone would let one stale corrupt manifest wedge every dispatch for that planning dir permanently; naming the file is what makes refusing survivable. malformed_existing is untouched, so the target/sibling distinction stays visible. The docstring is updated — it previously stated a rule the function did not keep. memFs() gains an optional failReads map so these branches are reachable at all; they had zero coverage because the fake could not express a per-file read fault. The signature is additive and every existing caller is unchanged. The regression is proved by a pair, not a single test. A control writes a readable sibling holding a genuine non-terminal duplicate and asserts duplicate_plan_id, establishing the scenario is real; the regression then makes that same path unreadable and asserts scan_incomplete. A first draft of this test used a corrupt-JSON fixture containing no plan_id at all while its comment claimed otherwise — it duplicated the unparseable-sibling case and proved nothing, which is the defect class this phase exists to remove. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): make a guard's failure distinguishable from its benign result Wave 1 of the negative-space backfill: the branches where a guard that could not verify something reported the same value it reports when everything is fine. That indistinguishability is the defect; every fix here makes the two states tellable apart, and every test proves it with a pair — one for the failure, one for the benign case. A single test cannot establish that two states are distinguishable, which is the whole property being fixed. state.cts phaseInventoryProvider returned null for both a real disk-scan failure and a genuinely empty phases dir, so `state rebuild` could report success while phase-table reconciliation never ran. It now returns a discriminated result and the CLI surfaces phase_inventory_scan_failed plus a reason. The reason field turned out never to have been wired into the emitted JSON at all — it existed only as an internal variable — so a test could only assert on the operator-facing note. It is a real field now. state.cts treated an unreadable lock body the same as an empty one, applying the 1-second stealable floor. A lock we cannot read is not a lock we know is stale; an unreadable body is now held to the deadman ceiling like a live holder. verification.cts findStaleVerificationSummary returned null on any fs, scan or clock failure — meaning "not stale". It now returns a discriminated StaleCheckResult and the caller records that the check was indeterminate. git-base-branch resolveBaseBranch returned 'main' both when no candidate branch existed and when every git tier timed out. A diagnostics variant now reports whether the answer was verified, and the CLI writes an unverified-fallback note to stderr. The stdout contract five workflows parse is untouched. worktree-safety snapshotWorktreeInventory left exists:true when statSync threw, so a guard that could not check reported the worktree present; exists is now tri-state and a stat failure surfaces as an 'unverified' finding. planWorktreePrune reported 'no_worktrees' for a parse failure, which is not the same as an empty list — and it drives a prune. It now reports 'parse_failed'. Fixing the inventory change exposed a second fail-open in verify.cts: the validate-health consumer silently dropped findings whose kind it did not recognise, so the new kind would have vanished. That is closed too — worth noting that the survey enumerated producers of degraded verdicts, not consumers that discard them. worktree-base-ref and state-transition gain the distinguishing signal without changing what they do: headAbsenceVerified, and a phase-inventory scan meta. Whether those guards should ACT differently is a product question this change does not answer, and both are flagged rather than quietly settled. rescueSummaryArtifacts is left alone: rescuing on an uncertain cat-file is deliberate per #2556. It now has tests proving it, and a recorded negative finding — git cat-file -e returns 128 for both "absent from HEAD" and a fatal error, so "uncertain" and "certain-and-fine" are not separable at the git level. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): assert typed values, not rendered text Ten assertions in the rebuild CLI suite matched substrings of produced output — STATE.md body fields, a markdown table row, an audit-log heading, and JSON keys read as text. CONTRIBUTING prohibits that: if the code under test produces text, the test asserts on its structured surface instead. No production surface had to be built. Every one already existed and was already compiled into bin/lib: stateExtractField for body fields, parseMarkdownTable for the phase table, collectSection for the audit-log section, and result.data.log — already a typed RebuildLogEntry[]. The tests were matching rendered text sitting next to the structured data. One of those assertions was passing for the wrong reason. `stdout.includes ('rebuilt')` matched the JSON KEY name, not a value: the dry-run path emits `mutated` and the real path emits `rebuilt`, so it would have passed whether the value was true or false. It now asserts the value. external-job's refusal already had to name the offending file — that naming is why the fail-closed variant is survivable rather than a permanent wedge — but the tests proved it by substring of a prose message. The failure result now carries offendingPath as its own field and the tests assert it by value. The human message is unchanged; operators read it. Array membership is left alone. `phaseIds.includes('99')` and `result.updated.includes('Completed Phases')` are membership checks on real arrays, not text matching, and converting them would weaken nothing and clarify nothing. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): execute acquireStateLock instead of grepping its source The non-EEXIST lock test asserted on the TEXT of the built .cjs and never called acquireStateLock. It carried an allow-test-rule: architectural-invariant exemption to permit that. A source grep proves a literal is present in a file, not that the behaviour works — it is weaker than a liveness test, which at least runs the code, and it was the only coverage the fatal-errno path had. Replaced with tests that inject the errno through fs and assert what actually happens: a fatal EACCES propagates out of acquireStateLock with zero backoff sleeps, while EAGAIN/EINTR/EINVAL/EIO/ENOENT/ESTALE/EPERM/EBUSY retry once and succeed. The exemption is removed and its allowlist entry with it. One old assertion is deliberately not carried over: it checked the retryable errnos were expressed as a Set rather than an inline literal. That is a shape check with no runtime signature; the behavioural tests fail if the code reverts to the old inline check, which is the regression it was really guarding. The #3057 lock-body tests move into that same file rather than a new one, which is what lint-test-file-count asks for and puts every acquireStateLock test in one place. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): surface an indeterminate staleness check to its callers An isolated review caught an inconsistency inside this wave. Two of the three "add the distinguishing signal" fixes wire through to something a user sees: git base-branch writes an unverified-fallback diagnostic to stderr, and an unverifiable worktree surfaces as a W020 finding. The third set staleCheckIndeterminate on readVerificationStatus's result and nothing read it. A signal nobody consumes leaves the fail-open exactly as silent as before: the staleness check could fail and the operator saw precisely what they would see if the answer were genuinely "not stale". That is the defect this issue exists to remove, so it is not defensible as scaffolding when its two siblings in the same change already wire through. All five callers now surface it, each through the channel it already had rather than a mechanism imposed uniformly: phase complete adds it to its existing warnings array and, on the blocked path, as an additive note on the error text; init and roadmap carry it as a field on output they already emit; the UAT report carries it without ever gating passed/blockers; workstream inventory takes an injectable writeDiagnostic mirroring the git base-branch idiom, because its return shape had nowhere to hang a per-phase field without rippling the builder's types. The routing decision is unchanged everywhere. What changes is only that a caller and an operator can now tell a failed check from a completed one. That diagnostic carries structured meta rather than being asserted by regex — the default still writes only the human message to stderr, but tests assert phaseDir and reason by value. Two earlier assertions in this branch were converted the same way; this was the last raw-text assertion left. Also records a scope correction: the completePhaseCore guards now compare stateReplaceField's result to the body instead of testing truthiness, so a field whose substitution produced identical text no longer reports as updated. That is a real behaviour fix, not the signal-only change this file was described as carrying, and its tests cover both the changed and unchanged cases. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): bound two heavy subprocesses for a loaded bench, not an idle one The remote matrix surfaced three failures unrelated to this branch's changes. All were bad tests, and a re-run would have hidden every one of them. The reviewer-flags parse block bounded bash -> node -> a full gsd-tools cold start at 5 seconds. On a bench running thirty thousand tests in parallel that is not a hang, it is a busy machine. Raised to 30s, matching the convention sibling suites already use for script invocations, with a comment saying what the budget covers so nobody tightens it back. Two further copies of the same 5-second spawn in the same file had the identical defect and are raised too — they were not in the failure report, but they will be next time. The fragment-propagation test bounded npm run regen:derived — a full build plus eight generators, the heaviest subprocess in the suite — at five minutes, and node22 was killed near the end. The captured output proves it: every generator had written its files and gen:install-tree had emitted all fifteen runtimes before the kill. Raised to fifteen minutes. That failure read as `null !== 0`, which says nothing. status null means killed, not a non-zero exit, and the two want different responses: one is a timeout to size correctly, the other is a real build break. The assertion now distinguishes them and names the signal. Neither test's assertions were weakened and no retry was added. A retry here would suppress exactly the signal the timeout exists to produce. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture fd 1 through the mock tracker, not a raw reassignment The phase suite reported zero test results on both lanes while running for five and a half minutes and exiting 1. No assertion text, no stderr, four events for the whole file: enqueue, start, dequeue, complete. That shape is not a failing assertion — it is the runner being unable to read the child at all, because it parses its event stream from the child's stdout. The cause was the capture helper reassigning fs.writeSync directly. Proven rather than assumed: a standalone probe patched fs.writeSync and called process.stdout.write, and the interception fired only when fd 1 resolved to a FILE, not when it was a pipe. The remote runner captures the event stream to a file, so a helper that was invisible against a pipe swallowed the reporter's own output on the bench. That is also why the two sibling suites wired the same way in this change pass cleanly — they use the mock tracker, the seam io.test.cjs established for this exact function. The helper now uses t.mock.method with an explicit restore after each call, so teardown belongs to node:test rather than a second hand-rolled implementation, and the interception cannot outlive the one synchronous call it wraps even if that call throws. Ten call sites thread the test context through; three test callbacks gained the parameter they lacked. The three B3 tests are untouched — same assertions, same fault injection. Only how the context reaches the helper changed. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture phase-complete output from a subprocess, not fd 1 Two attempts to make in-process fd-1 interception safe both failed on the bench. The suite reported zero test results on either lane while exiting 1 — four events for the whole file — because the runner parses its event stream from the child's stdout, and process.stdout.write routes through fs.writeSync whenever fd 1 resolves to a file, which is how the runner captures. Patching that seam anywhere in a file can therefore destroy the file's own reporting, and tightening the window only moved the runtime from 326s to 125s without recovering a single event. So the interception is gone rather than tuned. The helper now spawns gsd-tools as a real subprocess and reads stdout the way the OS already gives it to us, which is what the rest of the suite does. It asserts the command succeeded before parsing, so a genuine failure can no longer present as a JSON parse error. The two fault-injecting tests could not survive that move as written: a subprocess cannot see a mock installed in the parent. Instead of reinstating the interception they now produce the fault on disk — the summary artifact is created as a dangling symlink, so the staleness check's real statSync throws inside the child. That is a more honest fixture than a mock in any case, since it is a condition a user's tree can actually be in. Skipped on Windows, matching the existing symlink precedent in the write-guard suite. Three further call sites turned out to depend on parent-process writeFileSync mocks the subprocess could not see. Those call the CJS function directly, which is what they always wanted — they never needed stdout at all. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): one name for one signal, one encoding for one distinction Standards review found four things this branch introduced, all of them inconsistencies with itself rather than with the repo. One upstream bit reached its consumers under three names — verification_stale_check_indeterminate in two modules, the same value with "stale" dropped in a third, and stderr only in the fourth. Standardised on the long name wherever it is a field. The workstream inventory keeps its stderr channel, since its return shape has nowhere to hang a per-phase field without rippling the builder's types, but it now says the same word for the same thing. worktree-safety encoded one three-way distinction two ways in a single file: a named union for a finding's kind, and boolean|null for an inventory entry's existence. The second is now a named union too. Two assertions matched human prose because the blocked and non-blocked completion paths carried no typed field for the signal. Both now assert typed values. The first round of this fix added the field but left the regex beside it, which is the banned pattern sitting next to its own replacement; the second removed it and added an assertion on the reason enum so nothing was lost. The remaining two were reasoned away before being fixed, and both reasons were bad. "No typed surface exists" is the condition CONTRIBUTING says to fix by adding one — it took three lines. "The file already does this dozens of times" is not licence to add instance number thirty-one; a convention that violates a documented rule is debt, not precedent. Vocabulary differing across DIFFERENT modules is left alone: CONTEXT.md rejects a single shared result envelope, so per-module shapes are precedented, and a baseline smell does not outrank a documented standard. A census of every line this branch adds to a test file now finds no regex or substring assertion on produced prose: 87 strictEqual, 25 ok (all non-empty or shape guards), 12 equal, 3 throws (all typed err.code predicates), 3 deepStrictEqual, 2 notStrictEqual. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3057): backfill changeset pr number to 3088 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-05 16:00:52 -04:00
Tom Boucher	7203011400	feat(#3072): ship the deferred MCP served catalog (resources + prompts) (#3083) ... * test(#3072): add failing-first coverage for the mcp served catalog 55 input-class rows from the phase test matrix, across four suites: the catalog module over injected readFile/readDir seams, the protocol surface through handleMessage, the install-vs-catalog parity gate, and fast-check properties for uri round-trip, traversal refusal, and pagination partition. src/mcp-catalog.cts lands as a skeleton whose functions throw, so the suites fail on BEHAVIOR rather than on a missing module. The REASON enum is real so tests assert typed codes instead of message prose. Hostile coverage for the one client-controlled path surface (resources/read): dot-dot and backslash traversal, percent- and double-encoded traversal, absolute posix and windows paths, file:// scheme, null byte, symlink escape, unindexed sibling, non-string and empty uri, wrong root segment. IO faults are injected by monkeypatching the seam, never chmod 0o000 - root bypasses mode bits, so a permission-based test silently passes with zero coverage in root CI. Refs #3072 * feat(#3072): serve the mcp catalog as resources and prompts gsd-mcp-server now serves GSD's own content alongside its three tools: the workflow, reference and command tree as MCP resources (resources/list, cursor paginated, and resources/read over gsd://<segment>/<relpath> uris) and the 71 commands/gsd/*.md as MCP prompts keyed by bare command name. initialize advertises resources and prompts, and deliberately does not advertise subscribe or listChanged - the catalog is fixed for a server process lifetime, so declaring a notification we never send would be a lie a host acts on. Composition scope is SHARED, not re-declared. shouldCompose lives in src/mcp-catalog.cts and bin/install.js now imports it instead of carrying its own regex, so the served catalog and the installed file floor cannot drift on what gets composed. Proven behavior-preserving across all 2871 tracked paths plus windows-backslash, absolute and near-miss-prefix cases: zero mismatches. tests/mcp-catalog-parity.test.cjs asserts served text equals the installer composition-stage text over the real tree, with anti-vacuity guards requiring both a marker-bearing workflow and a non-composed file in the comparison set. Two measurements corrected the literal issue text. Composition is scoped to gsd-core/workflows/ only, because a reference or command that documents marker syntax with an unfenced example would otherwise be parsed as carrying a real marker and have that line lossily dropped. And parity is asserted at the composition stage rather than against an emitted runtime tree, since install applies per-runtime path rewrites afterwards and the catalog is host-agnostic, so byte equality with any one runtime would be false by construction. resources/read is the one client-controlled path surface and is guarded in two independent layers: the uri must be an exact key in the prebuilt index, which defeats every traversal string by construction, and the mapped path is then re-checked with validatePath so a symlink planted inside a root after indexing is still refused. Also fixes a real drift defect found while here: SERVER_VERSION was hardcoded 1.7.0 while the package is at 1.9.1. It now resolves lazily from VERSION or package.json, reusing the precedent in runtime-artifact-conversion. Closes #3072 * test(#3072): make the catalog parity gate drive the real installer Review found the parity gate vacuous: it never imported or spawned bin/install.js, and recomputed the installer side with the SAME shouldCompose and composeWorkflow the catalog calls internally. It therefore proved only that src/mcp-catalog.cts is self-consistent. The old row 52 compared shouldCompose against a regex literal frozen in the test file rather than against the installer at all. An inline divergent regex re-added to bin/install.js - the exact regression ADR-1671 asks this gate to catch - would have left the suite green. The gate now spawns a real bin/install.js and compares the composition DECISION, observed as gsd:section marker survival, against what the catalog serves for the same files. Marker presence is the right observable because the installer applies per-runtime path rewrites after composing while the catalog applies none, so raw byte equality between the two surfaces is false by construction and must not be asserted. Sensitivity was proven, not assumed: overlaying the shouldCompose export that bin/install.js imports so it always returns false makes a real spawned install leave autonomous.md's markers in place while the catalog still strips them, and the row 48 assertion diverges. Anti-vacuity guards are kept and extended - the comparison set must be non-empty, must contain a workflow that actually carries markers, must contain a file the predicate declines to compose, and the install must have emitted a non-zero file count. The marker-documenting reference case has no instance in the real tree, so it uses an overlay fixture built with the same technique workflow-fragments-emission.install.test.cjs already uses. Renamed to .install.test.cjs so it lands in the install suite it now belongs to. Refs #3072 * test(#3072): retarget the unknown-method assertion off a now-implemented method tests/gsd-mcp-server.test.cjs used 'resources/read' as its example of an UNKNOWN JSON-RPC method. The served catalog implements that method, so it now returns -32602 (no uri supplied) rather than -32601. The remote runner caught it deterministically on both linux lanes: -32602 !== -32601. The test's intent is still correct and worth keeping, so it is corrected rather than deleted or weakened. It now uses 'resources/subscribe', which the server deliberately does not implement and deliberately does not advertise in initialize's capabilities, because it never sends the corresponding notification. That turns the assertion into a real contract - the advertised capability surface and the implemented method surface agree - instead of an arbitrary method name a future feature could invalidate the same way. Swept the rest of the suite for other assertions pinning the newly implemented methods; this was the only one. Refs #3072 * chore(#3072): backfill changeset PR number 3083 * test(#3072): make the catalog fake fs separator-agnostic for windows CI caught this on windows-latest (22 and 24): every catalog fixture indexed ZERO entries, surfaced by the anti-vacuity guards as 'fixture catalog must actually index resources for this property to mean anything'. Mechanism: makeFakeFs keyed its dirMap/fileMap on POSIX-joined paths (${root}/${rel}), while production buildCatalog looks paths up with path.join, which is backslash-separated on Windows. Every lookup missed, tryReadDir returned null, and the catalog came back empty. Production is NOT at fault and is unchanged. The same CI run proves it: on windows-latest the real-filesystem tests all passed, including 'installer composition decision matches the served catalog for every file in the real installed tree' and the row-51 non-vacuity proof against a real spawned installer. A real Windows fs accepts both separators; the FAKE did not, so the fake was the unfaithful one and is what changed. Lookup keys are now normalized unconditionally with .replace(/\\/g,'/') in readDir and readFile - never path.sep-conditional, never platform-gated. The row-42/43 injected-fault wrappers got the same treatment, since they compared raw production paths against POSIX-literal fixtures. No assertion was weakened, and the anti-vacuity guards that caught this are untouched - they are the reason this surfaced as a loud failure instead of a suite that silently asserted nothing on Windows. Refs #3072 --------- Co-authored-by: sim <sim@local>	2026-08-05 13:30:55 -04:00
Tom Boucher	2bc53baa03	fix(#2947): preserve preamble phase details when milestone section has none (#3084) ... * test(#2947): milestone anchor must prefer heading with Phase details Row 1 of the test matrix: the failing-first regression test. When the phase-listing heading (## Phases) is NOT version-bearing but a later version-bearing progress heading (### v9.0 phase progress) exists, extractCurrentMilestone latches onto the progress heading and silently drops the phases (phase_count: 0, exit 0). Five cases: the regression, the version-bearing control (must keep working), the no-phase-details fallback, the closed-vs-open preference, and an end-to-end roadmap.analyze check. Reproduced locally against built lib + confirmed by maintainer triage (trek-e). The one-word control (## Phases -> ## v9.0 Phases) restores phase_count: 2. * fix(#2947): preserve preamble phase details when the milestone section has none Root cause was one layer deeper than the issue title: the anchor selection (selected = first non-closed version-bearing heading) is fine — the real drop happens in the preamble strip. When the phase list lives under a non-version-bearing ## Phases heading (the shipped greenfield template's own shape) and the selected version-bearing heading is a LATER progress/notes sub-heading with no ### Phase N: details of its own, the preamble strip removed every phase-detail heading from the pre-milestone region (intended to avoid duplication with a Phase Details section that does not exist here) — silently dropping all phases (phase_count: 0, exit 0, empty stderr). Fix: only strip preamble ### Phase N: headings when the selected milestone section (currentSection) actually contains its own phase details. When it does not, the preamble phases ARE this milestone's phases and must be preserved. Falls back to today's behavior (strip) whenever the selected section has phase details, so multi-milestone roadmaps with a dedicated Phase Details section (#730) are unaffected. Surgical: one conditional on the existing strip, no signature change, no change to computeSectionEnd or the #730 Phase Details append. Blast radius CRITICAL (84 upstream symbols) — the change is gated on currentSection's content so every existing roadmap that currently resolves phases correctly keeps doing so byte-identically. * chore(#2947): add changeset fragment * chore(#2947): backfill changeset PR number 3084 --------- Co-authored-by: sim <sim@local>	2026-08-05 13:10:52 -04:00
Tom Boucher	5719efbc6b	fix(#2766): scan archived phases and read table-shaped Gaps/deferred entries (#3082) ... * fix(#2766): scan archived phases and read table-shaped Gaps/deferred entries Three silent false negatives in cmdAuditUat and its readers, all failing in the reassuring (false-negative) direction — a UAT audit whose entire job is to catch leftover work reporting zero over real work: 1. Archived phases invisible (src/uat.cts cmdAuditUat): on milestone completion milestone.cts MOVES phase dirs into .planning/milestones/<version>-phases/ (archive-by-default since #1871), leaving .planning/phases/ empty or absent. Partial archive → false all-clear; full archive → hard error indistinguishable from a broken install. Fix: enumerate archived dirs via the canonical getArchivedPhaseDirs seam (phase-locator.cts); archived dirs deliberately bypass getMilestonePhaseFilter (which scopes to the CURRENT milestone — applying it to past-milestone dirs would discard every one and reinstate the bug). 2. Table-shaped deferred-items.md yielded zero items (splitGapsEntries keyed on bullet openers only; a GFM table row starts with |). Fix: union of bullet + numbered + table-row splits. 3. Table-shaped ## Gaps yielded zero items (same bullet-only splitter). Fix: same union walker. The table walker is deliberately NOT routed through parseMarkdownTable (ADR-2143 §3 — that reads only the first table and treats ragged/headerless shapes as errors, the wrong contract for a hand-written backstop table that must surface its rows). New additive archived_milestone field labels provenance. Fix authored by issue reporter gavin-ray and verified against the published tarball; maintainer triage (trek-e) confirmed all three findings. Cherry- picked onto fresh next after prior PR #2832 closed for staleness; re-verified under gsd-test + reviews. 21 regression tests including the negative direction (bullet-only unchanged, status: resolved still suppressed, empty phases dir still succeeds). * chore(#2766): backfill changeset PR number 3082 --------- Co-authored-by: sim <sim@local>	2026-08-05 11:19:17 -04:00
Tom Boucher	481ac7c71b	fix(#2946): run milestone complete unstarted-phase guard independent of STATE (#3081) ... * test(#2946): milestone complete unstarted-phase guard fails open on STATE desync Row 1 of the test matrix: the regression test that fails first. Adds seven cases to tests/milestone.test.cjs covering the desync, absent, no-file, --force-override, mismatch-WARNING, fresh-project-noop, and sentinel-skip behaviors. RED on next: the guard's entire scan is nested inside `if (stateVersion && stateVersion === version)`, so any STATE.md milestone: value that does not exactly equal the version argument skips the scan with no warning — functionally an implicit --force on a one-way-door operation. * fix(#2946): run milestone complete unstarted-phase guard independent of STATE The entire ROADMAP phase-directory scan was nested inside `if (stateVersion && stateVersion === version)`, so any STATE.md milestone: value that did not exactly string-equal the version argument — a desynced value, or no milestone: field at all — skipped the scan with no warning, functionally an implicit --force. The operation the guard fronts is a one-way door: ROADMAP.md and REQUIREMENTS.md are archived and phase directories are MOVED into .planning/milestones/<version>-phases/. The scan was already driven by the version argument through getMilestonePhaseFilter / extractCurrentMilestone; the STATE match was a redundant second gate that shadowed and broke it. Decouple: the scan now runs whenever --force is absent, and a present-but-mismatched STATE milestone: field emits a WARNING naming both values so the suspicious condition is visible rather than silent. A fresh project with no Phase headings in the scoped slice still yields an empty scan (no false positives) — the intent the STATE-match short-circuit was reaching for, now achieved by the scan itself. * docs(#2946): document milestone complete --force, --dry-run, and the unstarted-phase guard The CLI-TOOLS reference signature omitted --force and --dry-run entirely, and neither the unstarted-phase guard nor its override was documented anywhere user-facing. Add a flags table and a factual guard description to the Reference page (CLI-TOOLS.md), and a practical guard note to the /gsd-complete-milestone How-to (COMMANDS.md) covering what to do when the guard fires and the new STATE-mismatch WARNING (#2946). American English per CONTRIBUTING.md language policy. * fix(#2946): emit STATE-mismatch WARNING as JSON in --json-errors mode Follow-up to the guard decoupling: a structured caller using --json-errors parses stderr line-by-line as JSON, so the plain-text WARNING would break such a parser. Honor getJsonErrorMode() and emit a structured JSON object ({ ok, level, message }) in that mode, plain text otherwise — mirroring io.cts error()'s JSON shape. Addresses the isolated-review observation (~45% but credible, since --json-errors is a documented CLI flag). * fix(#2946): address review — drop JSON-mode WARNING scope creep, tighten test assertions Standards + spec review findings (code-review two-axis + isolated adversarial): 1. The --json-errors JSON WARNING branch (commit dee719404) was scope creep the issue never asked for, AND emitted ok:true for a suspicious-condition warning (a category error — a stderr JSON parser keying on ok would treat the suspicious state as success), AND its comment falsely claimed to mirror io.cts error()'s {ok:false,reason,message} shape. Dropped: the WARNING is now plain-text stderr only, matching the existing [gsd-tools] WARNING convention (state.cts). The issue asked for 'at minimum warn', not a structured JSON surface. 2. The WARNING test asserted on /WARNING/ regex (raw-text matching on stderr prose). Tightened to assert on the stable operator-facing tokens — the WARNING: marker and both version literals the operator must see — not the surrounding formatter prose. Added a paired negative test confirming no WARNING is emitted for an absent milestone: field (a missing declaration is a normal fresh-project state, not suspicious drift). * fix(#2946): sanitize STATE milestone value before stderr WARNING interpolation Security review (minor): stateVersion is read from a user-controlled file (STATE.md) and is not validated like the CLI version arg. Sanitize before interpolating into the WARNING — strip ANSI/control chars (/[\x00-\x1f\x7f]/g -> '?') and truncate to 80 chars — so a corrupted or hostile STATE.md cannot echo terminal escapes or secret-looking strings verbatim into a CI log or terminal aggregator (CONTRIBUTING.md security: secret-looking values in stderr). version is already constrained to [A-Za-z0-9._-] by ARCHIVE_VERSION_LABEL_RE upstream, so it needs no sanitization. * test(#2946): correct stale fixtures that relied on the guard being silently disabled Four pre-existing tests broke under the #2946 fix because their fixtures only passed thanks to the bug — the unstarted-phase guard was skipping on STATE mismatch, so fixtures with missing or non-matching phase directories slipped through. The tests exercise version-forwarding / version-scoping, not the guard, so give them legitimate directories: - milestone.test.cjs #3043: dirs were '103.old'/'104.old'/'108.new' (dot), which phaseTokenMatches rejects — renamed to hyphen form. The v3.6 stats scoping still yields 1 phase (getMilestonePhaseFilter scopes correctly); the guard now sees all three phases as having directories. - milestone-archive.test.cjs 'returns version in response data': ROADMAP listed Phase 1 but no directory was created. Added 01-foundation so the scan is satisfied. Per CONTRIBUTING.md, test-fixture corrections land as their own test: commit, not bundled into fix: (release hotfix cherry-pick routes by prefix). * chore(#2946): backfill changeset PR number 3081 --------- Co-authored-by: sim <sim@local>	2026-08-05 10:42:42 -04:00
Tom Boucher	e6fcf14d02	fix(#2977): tolerate a leading UTF-8 BOM before the frontmatter fence (#3076) ... * test(#2977): prove extractFrontmatter returns {} on a leading BOM Failing-first regression for #2977. extractFrontmatter's startsWith('---') byte-0 check fails on any leading byte, so a UTF-8 BOM (Windows PowerShell/Out-File, several editors) makes every frontmatter field silently disappear. Rows 1-2 assert BOM-prefixed frontmatter parses identically to no-BOM (incl. BOM+CRLF); Row 3 guards the no-frontmatter negative space; Row 4 covers STATE/PLAN/SUMMARY/UAT artifact shapes; Row 5 is the control. * fix(#2977): tolerate a leading UTF-8 BOM before the frontmatter fence extractFrontmatter's byte-0 startsWith('---') fence check failed on any leading byte, so a UTF-8 BOM (\uFEFF) — written by default by Windows PowerShell `>`/Out-File (PS 5.1) and several editors — made every frontmatter field silently disappear. STATE.md/ROADMAP.md/PLAN.md/UAT.md/SUMMARY.md all lost phase/status/name with no error, no warning. Strip a single leading BOM before the fence check. The rest of the function is unchanged — the BOM is one codepoint, and removing it restores byte-0 alignment. Negative space preserved: BOM + no-frontmatter and BOM + thematic-break Markdown both still return {} (no false diagnostic). Scope: BOM only; the generalized 'arbitrary content before the fence' fork (tolerate vs diagnose) is a separate product-intent decision, surfaced in the PR. * chore(#2977): add changeset fragment * chore(#2977): backfill changeset PR number 3076 --------- Co-authored-by: sim <sim@local>	2026-08-05 02:18:25 -04:00
Tom Boucher	7e1004d89e	test(#3056): add the in-process fault-injection adapter, and normalize execGit's shape (#3077) ... * refactor(#3071): normalize execGit's call and result shape, unify ExecGitFn ExecGitFn was declared four times. Three were hand-copies of one signature and two of those were wrong: they typed exitCode as number|null when _spawnResult returns `result.status ?? 1` and can never yield null, weakened signal from NodeJS.Signals to string, and widened error from Error to unknown. Only verification.cts got it right, via `typeof execGit`. The root cause was a missing export: SpawnResultOutput was declared without `export`, so no other module could name the return type of execGit. Three authors independently hand-copied it instead. Exported now. Normalizing the type alone would have left the pressure that caused the divergence, so the function is normalized on both sides. It now ACCEPTS every call its consumers make — worktree-safety's declaration could not express an env-carrying call at all — and RETURNS every result code they need: timedOut moves into _spawnResult, so execGit, execNpm and execTool all carry it and the one extension that justified a separate type disappears. All four sites are now `typeof execGit` with nothing left to restate. timedOut reuses the existing isSpawnTimeout predicate introduced by #3050 rather than re-deriving it. That predicate checks error.code === 'ETIMEDOUT' only; the signal === 'SIGTERM' conjunct was deliberately dropped there because Windows does not reliably report SIGTERM and requiring it risks a false negative. There is no false-positive risk, and a test proves it: an externally-delivered SIGTERM leaves error null, so it is still not reported as a timeout. No dead null-checks surfaced. Every exitCode comparison in the two affected modules is === 0, !== 0 or === 128 — never a null guard — so the nullable declaration had never been written against. Closes #3071 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3056): add the in-process fault-injection adapter Adds tests/helpers/faulty-deps.cjs — makeFaultyGit() and withFaultyFs() — so a module's error branch can be driven deterministically and its degraded verdict asserted, instead of a counter-test that only proves the call did not throw. makeFaultyGit returns a value structurally assignable to `typeof execGit`, so one stub satisfies all four seams that the #3071 normalization collapsed into that single shape. A parity test drives the same stub through a real injectable entry point in each of worktree-safety, git-base-branch, worktree-base-ref and verification; it fails the moment any of them re-grows its own shape. Faults are scoped rather than global — by argv predicate and by call ordinal — because a fault adapter that faults everything looks like it works and proves nothing, and because verification.cts's two-call error handling needs to fault the second call only. Invocations are recorded so a test can assert an exact call count. The timeout fault carries error.code === 'ETIMEDOUT', and a test asserts the real isSpawnTimeout predicate matches it, so the fixture cannot drift from the production definition of a timeout. A companion test asserts an externally delivered SIGTERM with a null error is still NOT reported as a timeout — the false-positive guard for #3050's dropped conjunct. withFaultyFs restores in a finally so a throwing body still restores, patches only the named methods, and nests without clobbering an outer saved original. It never uses chmod: that no-ops under root, so the test would pass with zero coverage in root Docker/CI. The adapter is in-process via deps only. The Phase 1 process seam is documented as deliberately not a fault-injection surface — it cannot distinguish an injected timeout from a genuine bench OOM and would retry it. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3071): add changeset fragment for the execGit normalization Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3071): route the last two timeout checks through the shared predicate An isolated review found this branch had normalised the timeout verdict but left two callers still hand-rolling the fragile version of it. check-command-router's runBoundedShell computed `timedOut: r.signal === 'SIGTERM'` while the correctly-derived `r.timedOut` sat on the same result object. graphify's execGraphify branched on `result.signal === 'SIGTERM'`, with a comment asserting the very premise isSpawnTimeout exists to reject. Both fail in both directions. On Windows a genuine timeout is not reliably reported as SIGTERM, so the guard silently fails to fire — the false negative #3050 was raised for. And an externally-delivered SIGTERM is not a timeout at all, so the check also fires when it should not; isSpawnTimeout avoids that because `error` is null in that case and it keys on error.code. Both now read the derived `timedOut`, and graphify's comment states the actual rule instead of the fragile assumption. Also replaces a vacuous test: "execGitDefault now accepts env" never called execGitDefault (it is unexported), called execGit — whose signature already accepted env before this branch — and asserted only that exitCode was a number, which would pass whether or not the change under test existed. It now proves env reaches the child by asserting `git var GIT_EDITOR` returns the injected sentinel. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3071): make the graphify timeout fixture faithful to a real timeout The remote matrix failed on both Linux lanes: graphify's "returns exitCode 124 on timeout" got 1 instead of 124. The fixture was wrong, not the production change. It stubbed spawnSync as { status: null, signal: 'SIGTERM', error: undefined }. That is not a timeout. A real spawnSync timeout also sets error.code 'ETIMEDOUT'; a SIGTERM with no error is an externally delivered signal — a kill. The old `result.signal === 'SIGTERM'` check accepted it as a timeout, which is the false positive the shared predicate exists to reject, so this test was locking that bug in rather than guarding against it. The fixture now carries a real ETIMEDOUT error and all three original assertions pass unchanged. A counter-test is added alongside it: an externally delivered SIGTERM with no error must NOT be reported as a timeout. That is the assertion whose absence let the false positive live. Swept every SIGTERM/SIGKILL stub under tests/ for the same unfaithful shape. No other instance: the worktree-safety, worktree-base-ref and commit-staging fixtures already set ETIMEDOUT, and the remaining hits are either deliberate external-kill tests or feed code that never consults timedOut. Two sites keep their own signal check deliberately and are NOT changed: capability-source.cts:1301,1386 fail closed on ANY abnormal termination, which is correct — reading timedOut there would stop it failing closed on a kill and let it parse stdout from a killed process. Their reason strings, and check-latest-version.cjs:115, label any signal as "timed out", which is imprecise wording over a correct verdict, not a silent failure. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3071): backfill changeset pr number to 3077 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-05 02:14:41 -04:00
Tom Boucher	59b74c4e7d	fix(#2945): roll back the REQUIREMENTS checkbox when the traceability row rejects the write (#3073) ... * test(#2945): prove phase complete checkbox ignores traceability row rejection Failing-first regression for #2945. cmdPhaseComplete flips the REQUIREMENTS.md checkbox unconditionally and never rolls back when the traceability row exists but rejects the Status write (Deferred/Blocked), so a deferred requirement reads as shipped. Rows 1-2 assert the checkbox stays [ ] for Deferred/Blocked; Row 3 guards the forward-status flip; Row 4 covers the no-row boundary. * fix(#2945): roll back the REQUIREMENTS checkbox when the traceability row rejects the write cmdPhaseComplete's inline requirement-write loop flipped the REQUIREMENTS.md checkbox unconditionally and kept the flip when the traceability row existed but rejected the Status write (Out/Deferred/Blocked), so a deferred requirement read as shipped — the #2788 defect-2 fix was written into cmdRequirementsMarkComplete (milestone.cts) only, never the phase.cts inline copy. Port the rollback: capture beforeCheckbox, track tableHit in the updateTraceabilityCell callback, and when reqUpdate.ok && !tableHit (row existed but rejected the write), restore beforeCheckbox. The two surfaces can no longer silently diverge. Forward-status rows (Pending/In Progress/Gaps Found) still flip + advance; absent rows still flip (nothing to disagree with). * chore(#2945): add changeset fragment * chore(#2945): backfill changeset PR number 3073 --------- Co-authored-by: sim <sim@local>	2026-08-05 00:45:03 -04:00
Tom Boucher	c97f5debb9	fix(#2949): exclude sentinel phase ids from stage-3 next-phase candidacy (#3070) ... * test(#2949): prove phase complete stage-3 admits 0.x backlog sentinels Failing-first regression for #2949. cmdPhaseComplete's stage-3 lowest-outstanding loop has no sentinel filter, so completing the last real phase with an unchecked 0.x backlog row present selects the sentinel as next_phase, corrupting STATE.md. Row 1 asserts the 0.x sentinel is not selected; Row 2 guards the #2028 real-lower- phase out-of-order behavior; Rows 3-4 cover STATE desync and the checked-sentinel boundary. * fix(#2949): exclude sentinel phase ids from stage-3 next-phase candidacy cmdPhaseComplete's stage-3 lowest-outstanding-override loop (#2028) had no sentinel filter, so completing the last real phase with an unchecked 0.x backlog row present selected the sentinel as next_phase — comparePhaseNum("0.1","12") === -12 sorts it below every real phase — corrupting STATE.md and desyncing current_phase from current_phase_name. Add !isSentinelPhaseId(cbm[2]) to the stage-3 condition, reusing the existing zero-caller predicate (SENTINEL_RANGES = [0, 999]) so both sentinel ranges are excluded. A real lower-numbered outstanding phase is not a sentinel and is still selected, preserving #2028's out-of-order-completion behavior. Stage-3 only: PR #2815 (in-flight, #2786) covers stages 1-2; the two PRs touch disjoint code. * fix(#2949): compare next_phase numerically in Row 2 (handles padded/unpadded) Row 2's assertion /09/.test(next_phase) failed because the CLI returns the unpadded "9", not "09". Compare numerically (parseInt === 9) so the assertion holds for either form. * fix(#2949): mark Phase 11 complete in Row 1 so only the 0.x sentinel is unchecked Row 1's fixture left Phase 11 unchecked, so completing Phase 12 correctly selected Phase 11 as the real lower outstanding phase (is_last_phase=false) — the assertion is_last_phase===true was wrong for that fixture, not the code. Mark Phase 11 [x] so the ONLY unchecked lower row is the 0.x sentinel, which is the actual #2949 scenario. Confirmed locally: with Phase 11 checked + the fix, completing 12 yields is_last_phase=true, next_phase=null (sentinel excluded). * chore(#2949): add changeset fragment * chore(#2949): backfill changeset PR number 3070 --------- Co-authored-by: sim <sim@local>	2026-08-04 23:58:49 -04:00
Tom Boucher	8f75e27554	fix(#3045): fail closed when an executor dispatch drops its resolved isolation (#3069) ... * feat(#3045): deny an executor dispatch that drops its isolation flag Every isolation gate already resolved correctly. The resolved value then reached the executor through a prose instruction telling the model to substitute it into a call the model composes itself, and nothing verified the substitution. When it was dropped, the executor edited and committed in the user's primary checkout with no consent and no warning. A prose backstop would be the same class of artifact as the defect, so this is a shipped PreToolUse hook on the Agent tool. It fires at the instant of the call rather than being read once at the top of a workflow, which is the only placement the model cannot skip. The guard is inert unless it can positively establish that this is a GSD project, that the project resolves to harness isolation, and that the dispatch targets an executor. A non-GSD repo has no invariant to enforce. Where it cannot read the configuration at all, it denies rather than assuming, with its own reason -- a guard that cannot verify must not answer safe. A malformed payload allows rather than throwing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3045): extend the isolation guard to Cursor Cursor is the second of only two runtimes that resolve harness isolation, so shipping the guard for Claude alone left half the exposed surface unguarded while the changeset implied it was covered. The two runtimes fail differently. On Claude the harness flag is a per-dispatch kwarg the model must copy into a call it composes, and the defect is that it can be dropped. On Cursor the flag is --worktree, which applies to the whole session, and the subagent-start payload carries no isolation field at all. There is no flag to check, so the guard verifies the effective state instead: whether the workspace is genuinely running outside the user's primary checkout. That is a stronger check than the Claude one because it tests reality rather than intent, and it is commented so nobody later rewrites it into a flag check. Isolation is established two ways, either sufficient: the workspace resolves to a linked git worktree, or it sits under the worktree root Cursor manages. The second matters because a directory Cursor placed there is a legitimate isolated session even before it becomes a distinct git worktree, where linkage alone would report no repository. Detecting linkage required a new primitive rather than the existing context resolver. That resolver short-circuits on finding a local .planning directory before it ever compares the git directory to the common one -- and an isolation worktree normally has its own checked-out .planning. Reusing it would have read a correctly isolated session as unisolated and denied it, which is the failure direction that gets a guard switched off. The comparison is now its own shortcut-free function that the resolver delegates to after its own shortcut, so existing behavior is unchanged, and the case that would have broken is pinned. The subagent type is checked before any configuration is read, so an unreadable config cannot deny a dispatch this guard would never have enforced against. The input-schema comment on the Cursor hook documented only the fields common to every event and omitted the ones specific to this one. That omission cost a halt during this work; it now documents both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3045): enforce the resolved dispatch decision, not the host capability The guard keyed on the registry's dispatch.isolation, which says only that a runtime is CAPABLE of harness worktrees. The decision that actually governs a dispatch is the one the workflow resolves after gating, and that legitimately comes out as sequential in three documented cases: a project setting use_worktrees false, a per-plan submodule intersection, and the base-check auto-degrade. The workflow tells the model to omit the flag in exactly those cases, and the guard was denying every one of them. The third case matters most. The preceding fix made the base-check degrade on git timeouts and a missing git binary, where it had previously answered "safe". That correction is right, and it means a transient hang now degrades to sequential far more often than before -- so the two changes composed into a trap where the workflow behaved exactly as designed and the guard blocked it. The workflow already resolves isolation in shell, deterministically, which is what makes it a trustworthy source in a way the model-authored call is not. It now records that resolved value through a dedicated verb, and both guards read it first. A fresh record is authoritative, so sequential dispatches pass untouched. Absent or stale, the guards fall back to the capability check combined with the project's use_worktrees setting, which still covers the case that never reaches the workflow. Also widened the matcher to accept Task alongside Agent, since a host that names the tool Task would otherwise leave the guard silently inert while implying coverage; stopped assuming Claude when no runtime is declared, which is the shipped default and would have demanded a Claude-only argument elsewhere; and made a non-git project inert rather than denied, since advising a worktree session is not actionable without a repository. The original diagnosis never modeled sequential mode as legitimate. That omission is what let this through, and it is now recorded there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3045): record at resolution and bind the record to its dispatch Two independent reviews converged on the same failure: the guard was fail-open in a default install, so it did not catch the defect it exists to catch. A shipped project carries no runtime key, which made "runtime not confidently known" the common case rather than a corner one. A record asserting that isolation was required but carrying no flag then fell through to a capability lookup that answered "none", and the dispatch was allowed. The flag itself only arrived from a second shell block -- the same block a model dropping the argument would also skip. A test had pinned that behavior as intended. The record is now written by the resolver, as an unavoidable consequence of asking for the value, rather than by a step the model is told in prose to go and run. A guard against a prose-carried value cannot itself depend on prose. Mode, flag and identifiers are written together and atomically, so the flagless window is gone, and a record asserting isolation with no resolvable flag now denies instead of degrading. Runtime is also resolved from the installer's own recorded default, which makes confident resolution the normal case. The per-plan submodule gate degrades after the phase-level decision and never re-recorded, so a plan that legitimately ran sequentially was denied against a still-fresh phase record. It now records its own, scoped to the plan. A record also authorized any dispatch for four hours. One phase degrading to sequential could silently license an unisolated dispatch in the next. Records now carry phase and plan, the guards require them to match, and the window is minutes rather than hours -- the resolver rewrites it before every dispatch, so a long window bought nothing and only widened the hole. The flag validator rejected any value beginning with two dashes, which is exactly the form Cursor and Windsurf declare, so their real value could never have been stored. Writer and reader also derived the record path differently and diverged inside a linked worktree without local planning state. The predictable path remains a way to silence the control without leaving a trace in the diff. It grants no access an agent with shell does not already have, so it is documented as accepted rather than redesigned around. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3045): correct the staleness boundary and unmask a vacuous parity test The remote runner returned twenty failures. One was a real production defect the boundary case existed to catch: a record whose age exactly equalled the staleness window was treated as fresh, so it stayed authoritative for one tick past its own expiry. Freshness is now strictly inside the window. The parity test meant to stop the two guards' executor lists from drifting could never have failed. Its project fixture was a bare directory rather than a repository, so the non-git inert branch answered before the executor list was ever consulted. It asserted agreement it never actually measured. The fixture is now a real repository, like every sibling in the file. A test also asserted that Windsurf declares the worktree flag. It does not -- Windsurf resolves to no isolation by design, having no named concurrent dispatch to isolate. The test claimed a registry fact that was never true, and a comment in the resolver repeated it. Both corrected, and the test now proves what it should have all along: that the parser accepts any bare flag value, rather than one runtime's supposed value. The new guard was missing from the bundled-hook whitelist, which is the surface that decides what actually ships, and the per-plan gate had gained calls to the launcher without the preamble those calls require. The changeset carried parenthetical product descriptions the purity rule forbids. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3045): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3045): make the guard tests hold on Windows Two tests redirect HOME to control where the installer-persisted runtime default is read from. Node resolves the home directory from USERPROFILE on Windows and never consults HOME, so both silently read the real runner profile, found no recorded runtime, and asserted against a project the hook had not recognised. The production code was already correct in asking the platform rather than the variable; only the tests were wrong to assume one variable answers everywhere. The helpers now mirror the override onto both. The symlink spoofing test also created a directory symlink unconditionally, which needs elevated privileges on Windows. It survived on this runner, but it would fail on any host without them, so the creation is now attempted and the test skips explicitly when it cannot be done -- a bare return would have counted as a pass and hidden the gap. Skipping alone would have left the platform uncovered, so the behaviour it proves is now also driven in-process through an injected realpath, following the seam already used for the clock. That case no longer depends on privileges at all, and the end-to-end test keeps its original assertions wherever symlinks work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-04 23:42:16 -04:00
Tom Boucher	c899f5ada3	chore(#3065): build the deterministic load-bearing-fragment contract gate (#3068) ... * test(#3065): build the load-bearing contract gate ADR-1671 promised Epic #1671 Phase 7. A post-merge audit of every promise in ADR-1671 against the merged tree found one mitigation asserted-but-absent and two stale records. ADR-1671 names exactly one correctness risk — trimming a load-bearing fragment, with the recorded history of a paraphrased META.RULE causing agent violations — and #2931 amended its mitigation to a deterministic contract gate that proves no load-bearing fragment was omitted or shrunk, treats a floored fragment as a success, and asserts the isolate prefix survives byte-identical, with an explicit anti-vacuity rule. That gate did not exist. What existed was tests/context-composer.test.cjs: synthetic unit tests of the composeWithinBudget primitive over invented fragments, asserting nothing about real declared strategies. The ADR asserted a mitigation that was never built, which is the promised-but-not-built shape the epic's own coverage discipline exists to catch. The gate derives its load-bearing set from declared verbatim strategies rather than a hand-maintained list, so it cannot go stale as upstream changes. It sweeps budgets from 4x total down to a quarter of total and asserts at every step that no load-bearing id appears in omitted or shrunk, that isolatePrefix is byte-identical, and that hardFailed is surfaced rather than silently passed. Both anti-vacuity guards are EXECUTABLE, not comments. One proves the empty load-bearing set guard actually throws. The other proves a sweep that never applies pressure is rejected — because a gate that only ever runs unpressured is exactly how the original mitigation went missing without anyone noticing. Measured: underPressure true at 6 of 7 budgets, false only at 4x total. Three ADR records corrected in the same change, all doc-vs-reality drift: - Decision item 2 describes a composer that trims by priority to fit a measured per-runtime cap. composeWorkflow in fact passes MAX_SAFE_INTEGER with every fragment verbatim (both verified in source), so no trimming happens there; the emitted-byte cap is a separate measure-and-fail gate and Windsurf's limit a bespoke truncation. The wording described an option as shipped behavior. - flag:--converge never reached a terminal state. #2992 withheld six atoms; five were resolved explicitly. This one was resolved in code by reusing state:plan-strategy-converge but recorded nowhere — the same gap #2995 closed for flag:--verify-only, and I closed five of six. - The open-questions list enumerated three questions while two Resolved-by blocks resolved an unlisted Question 4. It is now listed. Refs #3065 * fix(#3065): make the gate assert over production, not a copy of it The isolated review found a blocker, and it was fatal to the gate's purpose: it hand-copied applyBudget's fragment array into the test, so flipping a strategy in src/prompt-budget.cts — say roadmap from verbatim to drop — would leave the gate computing from its own untouched copy and still passing. A guard built as an instance of the very divergence class it exists to prevent (DEFECT.GENERATIVE-FIX) is worse than no guard, because it reports green. Fixed by eliminating the duplicate rather than adding a parity assertion, the same resolution used for the FAMILIES table in #2996. applyBudget's inline construction is extracted to an exported buildBudgetFragments(), which both applyBudget and the gate now call; the 1024 plan floor is exported as PLAN_FLOOR_CHARS instead of being re-declared in the test. The extraction is pure — verified behavior-preserving at budget=2000: hardFailed false, omitted ['context'], projectMd shrunk, plan truncation ~27.8%, all headers present. There is no longer a second copy to diverge from. Also fixed a vacuous assertion the same review caught: isolatePrefix was pinned across the sweep, but no production fragment sets isolate:true, so the value is always '' and the check could never fail. The pinning assertion stays, with an honest comment that nothing in production sets it today, and a second test now constructs an isolate:true fragment set and proves the prefix is non-empty and byte-identical across a roomy and a severely tight budget — which is what makes the first assertion capable of detecting a real change. Refs #3065 * chore(#3065): backfill changeset pr number to 3068 --------- Co-authored-by: sim <sim@local>	2026-08-04 22:41:01 -04:00
Tom Boucher	83a26ed1dc	fix(#2939): honor the declared depth budget in shouldFlattenDispatch (#3063) ... * test(#2939): prove shouldFlattenDispatch ignores the depth budget Failing-first regression for #2939. shouldFlattenDispatch checks only background+backgroundDispatch, never nested/subagentToolkit/maxDepth, so a maxDepth:1 descriptor (no room for a bg orchestrator plus a leaf) is told it may background. Row 1 (codex-like, maxDepth:1) asserts true (flatten) and fails today; rows 2/3 guard the unchanged depth-sufficient cases. * fix(#2939): honor the declared depth budget in shouldFlattenDispatch shouldFlattenDispatch checked only background+backgroundDispatch, never nested/subagentToolkit/maxDepth, so a maxDepth:1 descriptor (no room for a backgrounded orchestrator plus a delegated leaf) was told it may background — producing a depth-2 tree (Codex MultiAgent V2) the declared contract cannot support. canBackground now ALSO requires nested:true + subagentToolkit:"full" + a depth budget > 1 (or unbounded -1), reusing the exact predicate shape from bin/install.js _normalizeDispatchCallSpan and matching degradationFor's treatment of maxDepth===1 as flat. Non-finite/missing maxDepth fails closed to flatten. Correct the two existing pins that asserted the buggy output (bare {bg,bgDispatch} now fail-closes on missing depth; the codex-like maxDepth:1 pin flips to flatten) and add a maxDepth:2 negative-space row. * fix(#2939): propagate depth-aware flatten to all pinned descriptors + tests The isolated adversarial review found the depth-aware predicate reclassifies codex/kimi/kimi-code (previously background-eligible under the two-field rule) to flatten — the correct behavior, since each lacks what a backgrounded nesting orchestrator needs: - codex: maxDepth:1 (no room for a depth-2 leaf) - kimi: nested:false (cannot host a nesting orchestrator) - kimi-code: subagentToolkit:'built-in-only' (cannot delegate to full subagents) Only cursor (maxDepth:2) remains background-eligible. Update the three test files that pinned the old contract (host-integration-descriptors EXPECTED_FLATTEN, kimi-upgrades UPGRADE 2, trae-imperative-reference), and align the unbounded convention to maxDepth < 0 (matching degradationFor/negotiateHostCapabilities) with an accurate docstring noting the deliberate nested-check addition over _normalizeDispatchCallSpan. * fix(#2939): update dispatch-should-flatten CLI query pins for codex The depth-aware rule (a0ad0f680) reclassifies codex (maxDepth:1) to flatten, but command-routing-hub.test.cjs exercises the contract through the CLI query route (runGsdTools query dispatch-should-flatten), not a direct shouldFlattenDispatch call — so neither the reviewer's caller-search nor a grep for the symbol found it; only the full gsd-test matrix did. Update the codex query assertions to shouldFlatten=true (maxDepth:1 insufficient), preserving cursor (maxDepth:2 → false) and the backgroundDispatch:true descriptor field. * chore(#2939): add changeset fragment pr:0 placeholder backfilled with the real PR number once the PR exists. * fix(#2939): rephrase changeset for product-name-purity + opencode flatten pin Two failures from the full gsd-test matrix on the prior sha: 1. product-name-purity: changeset fragments must not include parenthetical product descriptions (they render verbatim into CHANGELOG.md). 'Codex (and kimi/kimi-code)' tripped it — rephrase to lead with the behavior, naming runtimes inline without the parenthetical. lint:ci changeset-lint does not catch this; only the test does. 2. opencode-imperative-reference: the #2087-retraction pin flipped only the two background booleans and asserted shouldFlatten:false. Under #2939 that is no longer sufficient (opencode lacks nested + full toolkit + depth budget), so the retracted axes now correctly flatten — update the pin to true with rationale. * chore(#2939): backfill changeset PR number 3063 --------- Co-authored-by: sim <sim@local>	2026-08-04 20:33:50 -04:00
Tom Boucher	c547e73a71	fix(#2927): merge installed overlay reviewer lanes into review-lane invocation (#3062) ... * test(#2927): prove overlay reviewer lanes are invisible to review-lane Failing-first regression for #2927. routeReviewLane builds its lane map from the static REVIEWER_LANES array only, so an installed overlay reviewer lane (role:"reviewer" capability) is roster-visible and disclosed at install but never selectable, plannable, or invocable. The test exercises a pure mergeReviewerLanes(firstParty, registry) helper that does not exist yet, so every row fails at the require(). * fix(#2927): merge installed overlay reviewer lanes into review-lane invocation routeReviewLane built its lane map exclusively from the frozen first-party REVIEWER_LANES array, so an installed, consented third-party reviewer lane (role:"reviewer" capability) was roster-visible and disclosed at install but never selectable, plannable, or invocable — sections/flags/plan/invoke all shared the one static map. Add a pure, total mergeReviewerLanes(firstParty, registry) helper (src/review-lane-descriptor.cts) implementing ADR-2782 D8: first-party ∪ installed overlay reviewer bodies, first-party winning on slug collision. The overlay body is field-identical to ReviewerLane per ADR-2782 D1 ("no translation layer"), so the helper MERGES rather than PROJECTS. Malformed overlays (missing/non-object body, empty or grammar-invalid slug) are skipped, never thrown — one bad third-party manifest cannot take the first-party lanes down. routeReviewLane consults loadRegistry({includeInstalled:true}) and degrades to the static set on any load failure. * test(#2927): add CLI-seam coverage for the wiring defect + normalize slug Two findings from the isolated adversarial review: 1. The test matrix's rows 9-10 (acceptance criteria #1-#3: overlay appears in sections/flags and plan resolves ok) were documented as covered but had no backing tests. The eight pure-helper tests would stay green if the one-line routeReviewLane wiring were reverted — the actual defect this PR closes had no regression guard. Add real end-to-end CLI tests that install a global-scope role:"reviewer" overlay and assert review-lane sections/flags/plan see it through loadRegistry -> mergeReviewerLanes. 2. mergeReviewerLanes trimmed the slug for the map key but stored the body with its untrimmed slug, diverging from deriveReviewerSlugs (which trims before adding to the roster). Normalize the stored lane's slug to the trimmed value so the two surfaces agree on the canonical key. * test(#2927): correct CLI-seam fixtures for reviewer manifest shape Two corrections from local CLI smoke-testing before the verification run: 1. role:"reviewer" manifests must omit feature-only fields (skills/agents/ steps/contributions/gates/hooks/runtimeCompat) — the validator rejects them. Match the shipped capabilities/lm-studio shape. 2. The plan subcommand renders an ARRAY of {slug,ok,section,transport,...} (it strips the nested invocation plan object), so assert on the array element, not a top-level object. Also drop the malformed-flag-filter assertion: the capability validator enforces flag grammar at install time, so a lane with a malformed flag cannot be installed and never reaches the flags shape filter (which is defense-in-depth, not independently reachable). * fix(#2927): drop unnecessary type assertion flagged by lint:ci The `body as object` cast inside the spread is redundant — body is already narrowed to object by the preceding typeof check. eslint no-unnecessary-type- assertion flagged it; lint:ci is a merge gate. * chore(#2927): add changeset fragment pr:0 placeholder backfilled with the real PR number once the PR exists. * fix(#2927): access runGsdTools result via .output in CLI-seam tests runGsdTools returns {success, output, exitCode, error}, not a string. The CLI tests (rows 9-10) passed the result object directly to JSON.parse/.split, which string-coerced to "[object Object]" and threw under gsd-test (3 failures). My local smoke test ran the CLI directly (string stdout), so it missed this — the helper wraps execFileSync and returns a result object. Access .output and assert .success explicitly, matching the established capability-cli.test.cjs convention. * chore(#2927): backfill changeset PR number 3062 --------- Co-authored-by: sim <sim@local>	2026-08-04 18:59:19 -04:00
Tom Boucher	ed360cd99f	chore(#2995): extend fragment emission to agents/ and reclaim size-cap headroom (#3058) ... * feat(#2995): extend fragment emission to agents/ across every read point Epic #1671 Phase 6.4. `composeWorkflow` stripped `<!-- gsd:section -->` markers only for `gsd-core/workflows/`, so a marked agent shipped its markers verbatim into every runtime — and agent text is loaded into a subagent's context on every dispatch. The issue proposed widening the `copyWithPathReplacement` guard. That is a no-op for agents: agents never traverse that function. Agent content is read for emission at five independent points, and the obvious chokepoint `stageAgentsForProfile` short-circuits on the DEFAULT `full` profile (`skills === '*'` returns the real unstaged directory), so a hook placed there is dead code on most installs. Composition now happens at two call sites instead of five parallel surfaces: `stageAgentsForRuntimeWithConverter` (with `agentsKind` and `kimiAgentsKind` routed through it via an identity converter) and the inline agent loop in bin/install.js. Both compose BEFORE any path rewrite, so a `.claude/` -> `.windsurf/` regex can never reach inside a marker attribute — the ordering #2930 established for workflows. `installCodexConfig` was the fifth read point: Codex embeds each agent's prompt into a per-agent `.toml` via its own readFileSync. Call-graph analysis missed it; the exhaustive per-runtime emission sweep found it. That is why the new guard is behavioral rather than structural — a sixth read point fails the sweep without anyone remembering to extend a list. tests/agent-fragments-emission.install.test.cjs spawns a real installer for every runtime at every agent-bearing scope, derived from RUNTIME_META and the capability registry at run time so a new runtime cannot be silently under-covered. It asserts markers are absent AND the `when="always"` body is retained, so marker-absence cannot be satisfied by dropping content. An identity-composer negative control proves the assertion can fail. Verified: 0 install failures, 0 marker leaks, body retained on 27 runtime/scope paths; red before the wiring on claude(global+local), zcode(global+local), kimi, codex and opencode. Refs #2995 * chore(#2995): give the tightest agents headroom and correct the design lock Epic #1671 Phase 6.4, second half. `agents/gsd-verifier.md` had 12 bytes of headroom under its 49,152-byte LARGE cap and `agents/gsd-debugger.md` had 147 under its 57,344-byte XL cap. Both now extract reference material to `gsd-core/references/` behind an @-reference — the documented DEFECT.AGENT-FILE-SIZE-CAP-BREACH remedy: gsd-verifier 49,140 -> 46,371 B headroom 12 -> 2,781 gsd-debugger 57,197 -> 48,851 B headroom 147 -> 8,493 Byte accounting proves no content was lost: the combined agent+reference delta is exactly the new files' headers plus the agents' slim replacement blocks. Each agent keeps its routing table and a one-line summary per entry, so it degrades gracefully on a runtime that does not inline @-references. `agents/gsd-planner.md` is untouched and still passes both char guards (49,130 < 49,152); it needed no change, so it took none. The other nine LARGE/XL agents carry NO gsd:section markers, and that is deliberate, not deferred. `when=` selection is read from gsd-core/workflows/section-manifest.json, which gen-section-manifest.cjs derives from gsd-core/workflows/*.md only — shape `{workflows: ...}`, no per-agent key, no per-agent init entry point. An agent atom therefore fails admission gate (2) ("a fact the init seam demonstrably computes at a real entry point") and would evaluate false forever while looking like working gating. Marking agents would manufacture exactly the silent-inertness rot the frozen vocabulary exists to prevent. ADR-1671 gains three amendments, two of which close gaps /adr-phase-coverage found against what actually merged: - The 19 -> 29 vocabulary widening shipped in #2994 with no coordinated ADR amendment, which that bullet's own rule forbids. Recorded now. - `flag:--verify-only` was one of six atoms #2992 withheld and deferred to "the LARGE/XL rollout phase". Five shipped; this one is permanently rejected, and that disposition lived only in a merged PR body. - Phase 6.4's own finding: emission extends to agents/, gating does not. CONTEXT.md's glossary was stale on both seams — Workflow Fragments Module still listed the original 4-atom vocabulary and described when= as "not yet acted on", and Section Manifest Module still described InvocationFacts as {waveFlag, phaseNumber, hasPriorPhases}. Both now match the shipped contract. Inventory manifest regenerated AFTER build:lib per the documented ordering landmine; 19 install-tree fixtures pick up the two new references. Refs #2995 * chore(#2995): correct the compose-site count and mark the raw stager Self-review found two comment defects in the prior commit. The agentsKind comment claimed composition lands at TWO call sites; it is three, since installCodexConfig's per-agent .toml writer was added after that comment was written. And stageAgentsForProfile is now production-dead — both callers route through the composing stager — while staying exported and unit-tested, which makes it a trap: it does a raw copyFileSync and short-circuits to the unstaged source directory under the default profile, so a future caller would silently reintroduce the marker-shipping path. Its JSDoc now says so. * test(#2995): guard the marker-documenting-doc class for agents Widening the composer's scope to agents/ makes reachable the exact class #2930 narrowed scope to avoid: a file that DOCUMENTS the marker syntax with an unfenced example is indistinguishable from a real marker, so the composer drops that line from the emitted artifact. Three rows. A fenced example must compose byte-identically. No shipped agent may carry a marker outside a fence — asserted by parsing every real agent and requiring zero explicit sections, which is what makes the fence protection load-bearing rather than decorative. And a non-vacuity row asserts an UNFENCED marker IS parsed as a real marker, so if that ever stops being true the second row is guarding nothing. Also applies two review findings: stageAgentsForProfile's new JSDoc claimed it had no production caller, which is false — bin/install.js's _stageAgents still calls it, and its consumers compose before writing. Corrected to state the invariant instead. And a let/const nit in the emission sweep. * fix(#2995): keep verifier status vocabulary in the agent, fix a wrong fixture The first remote run came back red with three failures. Both root causes were mine. 1. tests/agent-frontmatter.test.cjs requires agents/gsd-verifier.md to literally contain HOLLOW and DISCONNECTED. The Step 4b extraction moved that status vocabulary into gsd-core/references/verifier-wiring-patterns.md, so the agent no longer had it. Byte accounting said no content was lost, and byte-wise that was true — but a contract required those tokens to live IN THE AGENT. That is ADR-1671:66's flexReserve floor stated concretely: a load-bearing fragment must not be trimmed out of its host, and "the bytes still exist somewhere" is not the test. The two status tables are restored to the agent and deliberately mirrored in the reference with a note saying so, so the procedure there still reads standalone. gsd-verifier lands at 47,069 B — headroom 12 -> 2,083, rather than the 2,781 the first attempt claimed. 2. Row 12b of the new marker-documentation guard asserted that an unfenced marker example parses as a real marker, and threw instead: "unmatched /gsd:section close marker". The grammar is WHOLE-LINE only. The fixture had put the OPEN marker inline mid-sentence, so it was correctly not recognised as an open while the close, on its own line, was. That is a real refinement of the hazard this guard exists for: only a marker on its OWN line is mis-parsed — which is exactly how a documentation example is normally written. Row 12b now uses a whole-line marker, and a new row 12c pins the inline case as explicitly NOT a marker. No test was weakened to accommodate the change; the change was corrected to satisfy the tests. Refs #2995 * chore(#2995): backfill changeset pr number to 3058 --------- Co-authored-by: sim <sim@local>	2026-08-04 18:10:31 -04:00
Tom Boucher	4eb8e3648c	fix(#3050): consolidate the spawn-timeout predicate and propagate the unresolved-root reason (#3060) ... * chore(#3050): changeset and review artifacts for the follow-up Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3050): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-04 17:21:24 -04:00
Tom Boucher	24066e536e	fix(#3050): fail closed when a worktree guard cannot verify safety (#3054) ... * fix(#3050): fail closed when a worktree guard cannot verify safety Three places answered "safe" when they had not actually checked. The base-divergence gate held the clearest evidence against itself: within one function, an unresolvable fork ref correctly degrades, while an unresolvable HEAD twenty-five lines earlier returned "proceed". Because a timeout collapsed into the same branch as "not a git repository", a locked index or a stalled mount produced a green gate that had never resolved the fork base -- and that value decides parallel versus sequential dispatch. Timeouts are now distinguished from a genuine absence of a repository. A timeout degrades with its own reason and message; not-a-git-repo keeps today's non-degrading behavior, because there is no worktree concern there. The same conflation in worktree-context resolution is surfaced rather than silently falling back to the current directory. Worktree creation's root confinement was opt-in: omitting the root skipped the check entirely, leaving only the leading-dash and parent-segment guards. The sole caller always passed it, so nothing was exploitable -- it is now mandatory so a future caller cannot inherit an unconfined path by forgetting. The timeout predicate was checked against what Node actually emits on a spawnSync timeout, not only against the fixtures, so it cannot be a guard that fires solely in tests. Coverage is deliberately behavioral. The existing worktree suites -- 134 tests across two files -- require no production module and call no production function; they assert against prose and would pass with the implementation deleted. That is how three fail-open guards survived in a heavily-tested module, so the new tests drive the real resolvers through an injected git seam, with five of them pinning the paths that must NOT change. One existing test asserted the opt-in confinement behavior and was rewritten rather than left green against the corrected code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3050): stop a CLI exit code leaking into the test process The runner reported the new file as failed while the file's own summary said nine tests passed and none failed. That signature is a non-zero process exit after a green run, not a failing assertion. Cause: the confinement test calls the worktree-create command function directly, and that function sets process.exitCode on its failure path as a CLI would. In process, that exit code became the test file's own exit status. The sibling suite already guards this with a save/restore wrapper and a comment naming the hazard; the new file simply did not follow the convention. It does now. Root cause is in the test, not the production code -- setting an exit code is correct behavior for a command entry point, and the existing convention exists precisely because tests call these functions in process. Verified by exit-code and active-handle probes rather than by re-running: exit was 1, is now 0, with zero lingering handles and all nine tests still passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3050): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-04 16:18:21 -04:00
Tom Boucher	8c1962200d	fix(#2911): resolve surface re-stage destinations the way the installer does (#3049) ... * fix(#2911): resolve surface re-stage destinations the way the installer does Two writers computed the same destination differently. The installer honors a skills-kind home override; the surface re-stage ignored it and always resolved against configDir. For a global Codex install that override points at $HOME/.agents, so every re-stage built a second GSD-managed skill tree under $CODEX_HOME alongside the correct one, with nothing indicating which was live. Honors the override as a fallback, never a replacement -- runtimes without one still resolve against configDir, which is most of them. The real deliverable is the parity test, not the one-expression fix: it walks every runtime in the registry across both scopes, computes the installer and surface destinations, and fails naming the runtime if they ever disagree. Today only Codex global carries an override, so it discriminates on exactly one runtime -- stating that plainly rather than implying broader coverage -- but it is derived from the registry, so a newly-added runtime is covered without anyone remembering to add it. Two further defects fixed rather than deferred: - The legacy dev-preferences migration carried the identical defect, which the issue flagged as a latent instance of the same shape. - Fixing it exposed a symlink-escape guard confined against the wrong root: it checked the span between configDir and the skill dir, but a home override moves the skill dir outside configDir entirely, so the span was meaningless and threw a false-positive escape. Now confined against the install root the destination actually resolves under. The guard is unchanged in strength and still honors its opt-in; only the root it measures from is corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2911): honor the home override in the fourth destination writer too Adversarial review found a writer the fix had missed: the opencode-family skills installer resolved its destination and its symlink guard against targetDir, never consulting the skills-kind home override, while its three siblings all already honored it. Pre-existing and currently dormant -- it is reachable only for the combined-family runtimes, and none of them declares an override today, so no user is affected right now. Fixed anyway rather than left as a latent instance of the same shape, which is exactly what this issue asked for in the case of the legacy migration. Mirrors the shape used for the other three: a single installRoot local that both the destination and the guard derive from, so the two cannot drift apart. The guard's message now names the root it actually confined against. Coverage extended to this writer and proven non-theatre: reverting the change in a scratch build makes it fail for both combined-family runtimes. Enumerated every remaining site that computes a destination from destSubpath or calls the confinement helper -- install and uninstall paths, the surface module, the read-side skills-root reporter. All honor the override or structurally cannot express one. No fifth defect. The one adjacent shape, the flat command directory, reads a different descriptor field that no kind declares an override for in the current schema; noted rather than papered over with a fallback for a field that cannot exist. Verified no behavior change for the affected runtimes today: normalized file-tree hashes before and after are identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2911): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-04 14:31:07 -04:00
Tom Boucher	ef823ca9d9	fix(#2830): propagate a halted plan to its transitive dependents (#3038) ... * test(#2830): add failing regression tests for halted-plan dependent blocking Add tests/fix-2830-halted-plan-dependents.test.cjs covering direct, transitive (2 and 3 hop), and diamond dependents of a halted plan across both independent "which plans are incomplete" readers (phase-plan-index's cmdPhasePlanIndex and findPhaseInternal/searchPhaseInDir), the negative case (an unrelated decoupled plan stays runnable), and a parity check that the two readers agree. Uses only modules that already exist at this commit (gsd-tools.cjs via subprocess, the pre-existing phase-locator.cjs) so the test file loads and runs cleanly on a fresh clone of this exact commit. These fail against current behavior: neither reader has any concept of a halted plan or a blocked_by/runnable view yet. * fix(#2830): a halted plan no longer leaves its dependents on the runnable work list A plan that reaches a designed stop still writes a SUMMARY, so both "which plans are incomplete" readers saw it as an ordinary completion and reported its dependents as ordinary runnable work — never checking whether an upstream plan had halted rather than finished. - New `status: halted` frontmatter value, documented in all four SUMMARY templates alongside the existing `status: complete`. - New shared src/plan-dependency-graph.cts: a single computeHaltPropagation pass that both phase.cts's cmdPhasePlanIndex (wave-grouping) and phase-locator.cts's searchPhaseInDir (the phase-location primitive, ~50 dependent symbols across 5 command routers) now call, so the two-implementation divergence that caused this bug cannot recur. It accepts an optional precomputedOrder so cmdPhasePlanIndex — which already runs Kahn's algorithm in computeDependencyLevels for wave assignment — passes that order straight through instead of a second traversal; searchPhaseInDir (no prior traversal) lets the module derive its own. The two small duplicated predicates each reader would otherwise carry (is this status "halted"?, which summary file matches which plan id?) are centralized in the same module as isHaltedStatus/buildSummaryFileIndex. - Additive fields only: `halted`/`blocked_by`/`runnable` on cmdPhasePlanIndex's plans[] and top level, `halted_plans`/`blocked_by`/ `runnable_plans` on searchPhaseInDir's result. The pre-existing `incomplete`/`incomplete_plans` fields are unchanged in meaning and membership. - execute-phase.md's discover_and_group_plans step now also skips any plan whose `blocked_by` is non-empty, reporting it by name with its blocking chain, in addition to (not instead of) the existing has_summary skip rule. Extends tests/fix-2830-halted-plan-dependents.test.cjs (introduced in the prior commit) with direct unit coverage of computeHaltPropagation (including the precomputedOrder call shape) and a fast-check property test — both only possible once this commit's new module exists. Closes #2830 * fix(#2830): surface the halt-aware view from init execute-phase The adopted work made phase-locator compute halted_plans / blocked_by / runnable_plans, but cmdInitExecutePhase builds its output by explicitly enumerating fields, so all three were computed and then silently dropped at the exact consumer the issue names as regressed. Forwards them additively -- incomplete_plans and incomplete_count keep their name, type and semantics byte-for-byte -- and adds the same three empty defaults to the roadmap-only fallback so the shape is consistent in both branches. Covered by a new test that drives the real CLI end to end rather than the locator function, since the locator already worked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2830): fail closed on dependency cycles and stop the templates inviting the defect Three review findings, all fixed: - BLOCKER (isolated adversarial). Cycle participants never reach indegree 0 in the Kahn pass, so they were excluded from the topological order, never visited by the forward pass, and vanished from blocked_by entirely -- i.e. reported as runnable. The wave-grouping reader hard-fails on a cycle so it never hit this, but the phase-location reader does not, so init execute-phase offered a plan depending directly on a halted plan. Reproduced, then fixed in the shared engine so every consumer is safe regardless of pre-checks: a node absent from the order is now blocked with a deterministic, non-empty named cause. A plan silently missing from both blocked_by and runnable is the exact disappearance this issue exists to prevent. - MAJOR (isolated adversarial). All four summary templates showed the field as an inline comment on the value line. Frontmatter parsing does not strip trailing comments, so an executor copying the templates' own presentation wrote a halt that parsed as a non-halted string, silently reproducing the original bug. Guidance moved off the value line, and the halt predicate now tolerates an unquoted trailing comment. - HARD standards violation. A test regex-matched child-process stderr prose for /cycle/i, which CONTRIBUTING bans. Replaced with the structured failure signal plus a differential assertion (same fixture without the cycle edge must succeed), so it stays cycle-specific without matching prose. Also folds the duplicated read-summary-and-check-halted wrapper out of both readers into the shared module -- centralizing only the predicate left the exact two-copies-that-drift pattern the module exists to prevent -- and commits the artifact-types documentation for the new status value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2830): stop the property generator hanging the whole suite The remote runner did not fail -- it hung. Two containers sat in this file for 31+ minutes, and an earlier attempt ran 9 hours before I killed it. The runner passes --test-timeout=0, so nothing ever reaps it: this would have hung CI indefinitely, not reported a failure. Root cause: the DAG generator built edges by rejection -- from: fc.integer({ min: 0, max: n - 1 }) to: fc.integer({ min: 0, max: n - 1 }) .filter(({ from, to }) => from < to) With n === 1 both integers are forced to 0, so the predicate is unsatisfiable and fast-check retries value generation forever. n is drawn from 1..12 and fast-check biases toward boundary values, so n === 1 is reached almost at once. This also explains why the failing-first run completed normally while the fixed run hung: before the fix the graph module did not exist, so the property test threw on import and never reached generation. It only starts hanging once the code under test works. Generates the DAG by construction instead -- `to` is drawn strictly above `from`, with the degenerate single-node case short-circuited to an empty edge list -- so no rejection sampling is involved. Switches the import to the shared fast-check setup so the seed and run count are pinned per CONTRIBUTING, and adds a bounded regression guard that samples the arbitrary directly, so a future reintroduction fails loudly instead of hanging. Verified: the file now completes in 2 seconds, 29 tests started and 29 finished, zero failures, against an indefinite hang before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2830): restore the depends_on display contract and acknowledge the workflow growth Full-suite run surfaced two things the focused harnesses could not. 1. Regression of a pinned pre-existing contract (#3785). A refactor routed the EMITTED depends_on field through the new dependency resolver, which also consults the canonical-prefix map. The original consulted the plan map only, so a short canonical prefix passed through verbatim -- '24-01' stayed '24-01' rather than becoming '24-01-auth-hardening'. The emitted field is a DISPLAY mapping, not the DAG resolution, and #3785 pins that. Reverted with a comment recording why it must not use the resolver; full resolution is still used for the wave DAG and halt propagation, which is what needs it. 2. The workflow file grew 518 bytes without an acknowledgment, from the halt-aware skip rule and the widened parse contract. Acknowledged. Note on where the acknowledgment landed: the guidance is to add a NEW fragment, but execute-phase.md is already named by an existing fragment and the linter hard-fails when two ack sources name the same path. Appending to the owning fragment, following its own established multi-PR pattern, was the only lint-clean option. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2830): 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>	2026-08-04 06:47:46 -04:00
Tom Boucher	ff4a57b78c	chore(#1671): migrate the remaining 13 LARGE/XL workflows to the fragment model — Phase 6.3 (#3030) ... * chore(#2994): fragmentize progress.md forensic audit onto the fragment model Extract the --forensic-gated forensic_audit step to workflows/progress/steps/forensic-audit.md behind a section marker, and repair progress.md's init line to forward --forensic so the atom is actually true in production rather than only under direct CLI tests. progress.md shrinks 32630 -> 27207 bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#2994): fragmentize the four manifest-wired workflows new-project, quick, new-milestone and progress each already had a dedicated cmdInit* entry point but zero marked sections. Extract nine gated bodies to workflows/<wf>/steps/ behind section markers and repair each init line to forward its flags. Fold --full into the discuss/research/validate facts inside cmdInitQuick so the when= grammar never sees an OR, per the chunked-mode precedent. Fixes found while working, per the no-defer rule: - cmdInitProgress passed no phase info to buildSectionManifestField, so state:phase-mvp-mode was permanently false — an atom in the vocabulary whose fact could never be computed. - the quick init router folded flag tokens into the free-text description, which the new forwarding would have corrupted. - a #2508 dispatch note was nested inside quick.md's Agent(prompt=) fence, leaking orchestrator guidance into the subagent prompt. - progress.md had a 3-vs-4 backtick outer-fence imbalance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#2994): fragmentize verify-work.md and admit state:ui-phase-active Wire cmdInitVerifyWork to buildSectionManifestField — it was a dedicated entry point that never emitted a manifest — and mark two sections. state:ui-phase-active folds (plan:pre hooks include an active ui step) OR (the phase dir holds a *-UI-SPEC.md) into one boolean in init.cts, so the grammar still sees a single operator-free atom. The inner Playwright-MCP check stays as prose inside the fragment: it is live session state and no init seam can precompute it. The MVP false-branch note is a real fallback, not redundant prose, so it sits outside the marker — gating it away would delete the text needed precisely when MVP mode is off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#2994): follow moved workflow content in drift guards Retarget every guard that asserted on content this branch moved into workflows/<wf>/steps/, mirroring 815b3d897. Each retargeted assertion was verified to still fail when its step file is blanked, so none was weakened into vacuity. Three assertions in verify-mvp-uat were genuinely red. Three more were worse than red — passing for the wrong reason: - quick-commit-boundary and worktree-cleanup anchored on indexOf('Step 5.6'), which matched a later cross-reference and sliced 16069 chars that coincidentally held the asserted substrings. Replaced with an expandWorkflowSections helper that splices step content back in place. - phase6-review-capabilities lost its end boundary and widened to EOF. - playwright-ui-verify matched 'UI' in an unrelated bullet and 'fall back' in a subagent-dispatch line after the real content moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#2994): fragmentize code-review and complete-milestone, admit three atoms Add dedicated cmdInitCodeReview and cmdInitCompleteMilestone entry points alongside the shared generic ones rather than modifying them — init.phase-op and init.manager carry a CRITICAL blast radius (179 dependents, 24 processes) and stay byte-identical for their other callers. Admit flag:--fix, state:fallow-enabled and state:git-create-tag, each with a consuming section and a fact its own entry point computes. Both sections had the resolver-in-body hazard: the fallow config-gate and the git.create_tag check each sat inside the very block being gated, so gating would have disabled the resolver that decides the gate. Both are hoisted into init and the bodies now consume the resolved fact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#2994): retarget code-review and milestone drift guards, fix two red tests Retarget guards that asserted on content moved into steps/, proving non-vacuity by blanking each step file and confirming failure. Also fixes two genuinely red tests found while working, per the no-defer rule: - workflow-fragments' frozen-vocabulary lock was missing state:ui-phase-active, so commit 7ef7f8336 shipped red. Lint and build both passed over it, which is why neither is sufficient verification. - code-review's quick.md capability-hook assertion carried a stale delimiter after the 18ff35d20 extraction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#2994): fragmentize autonomous.md and admit state:plan-strategy-converge Five sections share one atom, the pattern plan-phase already uses for flag:--research-phase. The atom folds --converge OR --cross-ai into a single boolean in cmdInitAutonomous so the grammar stays operator-free. cmdInitAutonomous is additive; init.milestone-op, init.manager and init.phase-op are untouched and still consumed. The $PLAN_STRATEGY bash resolver is deliberately retained — ungated local-planning bullets still read it, so the init-side fact supplements it rather than replacing it. converge-fail-fast required splitting one bash fence so the always-run CONVERGENCE_ARGS construction stays outside the marker. All three flag-absent fallbacks were left outside their markers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#2994): fragmentize review and discuss-phase-assumptions Admit state:reviewer-instances-configured (two peripheral notes share it; the core reviewer-lane dispatch stays unmarked — it is the workflow's primary always-evaluated logic, not an optional branch) and state:auto-advance-active, which folds --auto OR two config keys into one boolean so the grammar stays operator-free. discuss-phase-assumptions was the highest-risk edit in this PR. Its auto_advance step is a full if/elif/else; gating it whole would have deleted the flag-absent fallback needed exactly when --auto is off. Split verified exact: resolvers 636-651 and the 'End here' fallback 668-669 both stay outside the marker; only 653-667 is gated. Adds emitted-drift acks for the two files that grew — review.md (+55 B) and autonomous.md (+737 B from 80799211c, which had none and would have red-gated the push. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#2994): fragmentize docs-update, update, transition and new-milestone Part A Completes the 13-workflow rollout. Three of these had no init call at all and gained a dedicated entry point plus their first gsd_run query line. Admits state:is-monorepo and adds state:next-channel, state:workstream-active and state:flat-mode. Vocabulary 26 -> 30 atoms. Part A of new-milestone applies when NO workstream is active — the negation of state:workstream-active. Rather than teach the grammar negation, which is the Greenspun drift the frozen list exists to prevent, it gets a separate positively-phrased atom whose fact is the inverse. Part B, which always runs, stays outside the marker. flag:--verify-only is deliberately NOT admitted: docs-update has no contiguous purely-additive region for it, and an atom without a consuming section is dead vocabulary. Evidence recorded in the slice report. update.md reuses its existing resolved $GSD_TOOLS rather than prepending the canonical preamble, which would have clobbered it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2994): stop automated-ui-verification re-resolving its own gate, retire dead vocabulary Two defects the new tests caught. The automated-ui-verification step re-ran gsd_run loop render-hooks and recomputed UI_PHASE_ACTIVE inside a body that is only read when that fact is already true — the circular self-disabling pattern this design forbids, introduced by 3c654b168. cmdInitVerifyWork now exposes ui_phase_active and the step consumes it. Its launcher preamble goes too: no gsd_run remains. The Playwright-MCP check stays as prose — that is live session state. Dead vocabulary predating this PR: flag:--full and state:needs-codebase-map were admitted with a gate-1 claim that never materialized. flag:--full is removed, redundant once quick folds it into discuss/research/validate. state:needs-codebase-map gets the real consumer it always lacked, gating new-project's codebase-map offer. Vocabulary 30 -> 29, and no atom is now without a consuming section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#2994): add the atom-admission, inversion and resolver-hoist gates The two existing parity guards prove vocabulary/predicate symmetry but never that a fact is computed — an atom no cmdInit* assembles evaluates false forever. These close that hole: - per-atom satisfiability for all 29 atoms, plus an anti-vacuity assertion so the loop cannot silently cover zero atoms - dead-vocabulary check against the shipped manifest - inversion guard: the flag-absent fallbacks in discuss-phase-assumptions and verify-work must stay outside their markers - data-driven resolver-hoist guard over the shipped manifest, so a future extraction cannot reintroduce the circular class - compound-fold coverage (--full, --cross-ai, --rc, config-only --auto) - null-vs-[] degraded/computed distinction, and flag value shapes Also repairs the frozen-vocabulary lock, which was stale and red for the seven atoms earlier commits on this branch shipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(#2994): add changeset for the fragment-model rollout Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#2994): cite the issue on the two new allow-test-rule exemptions ADR-456 requires an issue ref on the same line as the annotation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(#2994): correct the atom-count claims after retiring flag:--full The vocabulary doc comments still said 30 entries; it is 29 since flag:--full was removed as dead vocabulary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2994): dedupe the phase-fallback block and harden --ws parsing Review findings. MAJOR: the three new init entry points each pasted a verbatim copy of the guardedFindPhase/guardedGetRoadmapPhase fallback, taking the repo from four copies to seven — DEFECT.GENERATIVE-FIX. Extracted applyRoadmapFallback and folded six of the seven; each call site keeps its own field-set via a closure. Duplication removed rather than papered over with a parity test. cmdInitPhaseOp stays out: its fallback omits has_reviews, so it is not a byte-identical copy, and it is CRITICAL-radius. LOW, pre-existing: GSD_WS captured [^[:space:]]+ and expands unquoted, so a workstream name holding glob metacharacters would expand against the filesystem. Narrowed to [A-Za-z0-9._-]+. The unquoted expansion is kept — it must word-split into two args and vanish when empty. Also restores the vocabulary ordering convention, and fixes a masked test bug the mandated run surfaced: the flag-forwarding guard checked only the first init line per workflow, but new-milestone has two, so a real failure was reporting exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2994): drop the stale new-milestone emitted-drift ack new-milestone.md was acked for a +406 B growth measured against an intermediate commit. Net against origin/next it SHRANK by 8 bytes, so nothing needed the ack and it explained nothing — which the differential attribution check reports as a stale acknowledgment, not a pass. update.md's entry stays: it genuinely grew +703 B. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2994): resolve the 15 failures from the full matrix run All 15 were real and identical on both lanes. REAL REGRESSION: autonomous.md hit 41479 chars against the #2196 guard's 40960 cap — a CHARS cap distinct from the LARGE tier byte cap, which the five section stubs pushed it over. Extracted the 3a.5 UI Design Contract body to references/; now 39968 chars, and the file nets -795 B vs base, so its growth ack is deleted rather than left stale. REAL DEFECT: docs referenced /gsd-transition, which is not a live registered command. Reworded. STALE FIXTURE: the emission byte-identity test hardcoded two marked workflows; this branch legitimately marks fifteen. Fixture corrected — the source was right. The rest were drift guards over the eight workflows the earlier sweep did not cover, retargeted at where the content now lives with non-vacuity proven by blanking each step file and confirming failure. The GSD_WS forwarding guard was checked as a possible real break and is not one: the charclass narrowing is intact and forwarding works end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2994): drop the ack for a newly-added reference file A new file's emitted ripple is attributable to the diff that adds it, so the acknowledgment explained nothing and the differential check reports it as stale. Removing the last entry removes the fragment — an empty one signals nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2994): retarget the UI-contract guards and clear two transitive advisories The §3a.5 extraction that brought autonomous.md under the #2196 char cap moved its body to references/autonomous-ui-design-contract.md, so ten guards in autonomous-ui-steps and check-ui-safety-gate were asserting it against the host. Retargeted via a combined read, each proven non-vacuous by blanking the reference file and confirming failure. This class had already bitten twice on this branch because each sweep was scoped to the workflows touched at that moment, so this one was exhaustive: ~70 test files across all 13 workflows, zero further broken or vacuous assertions found. Also clears two high transitive advisories the matrix flagged on one lane — fast-uri GHSA-7p8r-x3mc-p8w7 and three ip-address SSRF/trust-boundary issues. Both pre-date this branch: package-lock.json was untouched until now, so the production tree was byte-identical to the base. Lockfile-only, package.json unchanged, verified against a real npm ci install. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#2994): backfill changeset pr number to 3030 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-03 19:59:58 -04:00
Tom Boucher	c6ce4d1d9a	fix(#2755): resolve the kimi hooks-TOML root per runtime (#3032) ... * test(#2755): failing-first coverage for per-runtime kimi hooks root Install/uninstall filesystem-shape rows over a sandbox HOME (no permission tricks) plus resolver unit rows. Covers both uninstall directions, which is where a fix applied only to the install call site would drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2755): resolve the kimi hooks-TOML root per runtime resolveKimiHooksTomlDir took no runtime argument and hardcoded ~/.kimi, but both kimi and kimi-code route through the single hooksSurface=kimi-hooks-toml branch. A --kimi-code install therefore wrote its [[hooks]] block, hook bundle and CommonJS marker into Kimi CLI's config file, and a --kimi-code uninstall stripped Kimi CLI's block. Adds a runtime selector to the resolver -- kimi keeps ~/.kimi + KIMI_SHARE_DIR, kimi-code gets ~/.kimi-code + KIMI_CODE_HOME, per Kimi Code's own upstream data-locations and hooks docs -- and passes the runtime at both the install and uninstall call sites. An omitted or unrecognized runtime still resolves ~/.kimi, so the exported no-arg contract is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2755): use centralized helpers and add a divergence guard Review findings, all fixed in-PR: - The new test block reimplemented runMinimalInstall, createTempDir and toPosixPath. Extends runMinimalInstall with optional root/extraEnv instead (back-compat: every existing caller passes neither) and uses the centralized helpers, per CONTRIBUTING's Use Centralized Test Helpers rule. - Adds a parity assertion between the capability registry and the resolver: a third runtime declaring hooksSurface kimi-hooks-toml would silently inherit ~/.kimi, re-creating this very defect. The guard fires the moment those two surfaces drift. - Adds an installer-level test proving KIMI_SHARE_DIR and KIMI_CODE_HOME do not interfere when both are set, which only the resolver unit covered before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2755): track the kimi-code hooks root in the emitted-artifact gates The remote runner caught a real ripple: moving kimi-code hooks to ~/.kimi-code made 31 emitted paths unattributable and 58 emitted hashes unexplained, because three parallel surfaces keyed on the literal .kimi path. - HOOK_CONFIG_RELATIVE_PATHS excluded only .kimi/config.toml, so kimi-code's config.toml became manifest-visible; it embeds a platform-varying node-runner command and must stay out for both products. - HOOKS_ROOTS, the package.json-marker branch and the synthesized-install-metadata pattern each named .kimi only. - tests/fixtures/install-tree/kimi-code.json still recorded the old paths; regenerated via gen:install-tree. Adds the per-PR drift acknowledgment for the 58 paths whose bytes are unchanged but whose destination moved - a ripple no source diff can show, since no hook script was edited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2755): clear production-tree security advisories The remote runner's npm-integrity gate reported 2 high advisories in the production dependency tree. My diff touches neither package.json nor package-lock.json, so these come from the base -- but a red gate is not something to wave off as pre-existing, so it is fixed here rather than deferred. Lockfile-only, semver-in-range, via npm audit fix: fast-uri 3.1.4 -> 3.1.5 (host confusion via backslash authority introducer) ip-address 10.2.0 -> 10.4.0 (three SSRF / trust-boundary bypasses) hono 4.12.31 -> 4.13.0 (moderate; reverting it traded a high for a moderate, so the full remedy is taken) npm audit now reports 0 vulnerabilities at every severity, npm ci installs clean from the updated lockfile, and the build and the kimi behavior both re-verified afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2755): backfill changeset pr numbers Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-03 18:56:42 -04:00
Tom Boucher	fd64389616	fix(#2703): strip GSD-2 frontmatter with the canonical parser (#3027) ... * test(#2703): failing-first coverage for CRLF frontmatter strip in SUMMARY.md Drives the exported buildPlanningArtifacts seam. Rows for CRLF/LF parity, stacked blocks and a leading BOM fail against the current hand-rolled regex; the negative-space rows pin behavior that must not change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2703): strip GSD-2 frontmatter with the canonical parser buildSummaryMd matched the closing delimiter with a hardcoded bare \n, so a CRLF-authored task summary never matched and fell through to the raw-passthrough branch. The function then prepended its own block, emitting a SUMMARY.md with two stacked frontmatter blocks and no warning. Delegates to stripFrontmatter from frontmatter.cts -- the canonical, line-ending tolerant primitive this repo already deduplicated once (#2143) -- instead of adding another hand-rolled variant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2703): strip only the first frontmatter block in gsd2 import Adversarial review caught a regression in the first cut: stripFrontmatter loops by design, so a summary body opening with a thematic-break-delimited section (--- / heading / ---) had that section silently deleted. The old pre-#2703 regex preserved it, so shipping the loop would have traded one silent corruption for another. Adds an explicit { once } option to the canonical primitive -- default behavior and the two existing callers are unchanged -- and has buildSummaryMd opt in. A GSD-2 summary is an arbitrary user document, not a GSD artifact with a known doubling failure mode, so a second block there is body content. This also makes the acceptance criterion exact: CRLF now produces the same result LF already produced, rather than a new result for both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2703): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-03 15:21:30 -04:00
Dennis Kim	178ec00040	fix(#2787): clarify broken-windows ship blocking enforcement (#2814) ... * fix(#2787): clarify broken-windows ship blocking enforcement * fix(#2787): update renderLedger header to clarify opt-in enforcement * fix(#2787): address maintainer scope and wording review --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-03 12:39:16 -04:00
Dennis Kim	d3ddcaba1c	fix(#2785): implement missing gate predicate evaluators (#2816) ... * fix(#2785): implement missing gate predicate evaluators * fix(#2785): gate predicate numerical coercion * fix(#2785): address evaluator review findings * fix(#2785): use safe frontmatter read seam --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-03 12:16:56 -04:00
𝚌𝚕𝚎𝚣𝚌𝚘𝚍𝚒𝚗𝚐	88f6d9bd1b	fix(#2644): deduplicate Cursor slash menu (#2812) ... * fix(#2644): deduplicate Cursor slash menu * fix: preserve installer executable mode * chore: add changeset for PR #2812 * test(#2644): acknowledge Cursor emission changes * test(#2644): drop spent emitted drift acknowledgments * fix(#2644): remove retired Cursor command converter --------- Co-authored-by: clezcoding <clezcoding@users.noreply.github.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-03 12:05:45 -04:00
Tom Boucher	067a4d1c6c	fix(#2650): bound and auto-recover plan-phase planner/plan-checker stalls (#3015) ... * test(#2650): add failing-first regression for plan-phase stall detection Regression test for gsd_stall_should_recover / gsd_stall_watch and the planner.stall_* config keys, none of which exist yet — proves RED before the fix lands in the next commit. * fix(#2650): bound and auto-recover plan-phase planner/plan-checker stalls Mirrors the already-shipped executor.stall_* pattern (execute-phase.md, bug #3212) but with a dispatch change the executor's prose-only surveillance lacks: the standard planner spawn, chunked-outline planner spawn, chunked-per-plan planner spawn, plan-checker spawn, and revision-loop planner respawn now dispatch with run_in_background=true and are followed by a real, bounded bash poll (gsd_stall_watch) that returns control to the orchestrator on its own schedule instead of waiting indefinitely on a subagent that may never return. On stall, the existing accept-plans/retry/ stop recovery menu (9a/11a) is auto-surfaced instead of requiring a manual interrupt. New config keys planner.stall_detect_interval_minutes (default 5) / planner.stall_threshold_minutes (default 10) mirror executor.stall_*. The helper functions (gsd_stall_should_recover, gsd_stall_watch) live in a new lazily-loaded gsd-core/workflows/plan-phase/steps/stall-detection- helpers.md rather than inline, and per-site prose is kept minimal, because plan-phase.md is frozen under the ADR-857 Phase 6 PRE_PHASE6 gate (tests/phase6-capstone-conformance.test.cjs) with ~36 bytes of headroom at baseline; the net effect is plan-phase.md.md ships slightly SMALLER than before (the old unconditional-wait ORCHESTRATOR RULE sentences are gone at the five touched sites, superseded by the bounded watcher). Also fixes a stale doc comment in tests/workflow-size-budget.test.cjs that still described the per-file workflow-size-baseline.json guard removed by #2724 (ADR-2719 Phase 4) as if it were still the enforcement mechanism — discovered while verifying this fix's own byte budget. Researcher and pattern-mapper spawns are untouched (out of scope per the issue's Agent Brief). * fix(#2650): make gsd_stall_watch single-cycle; harden numeric config inputs Two review findings addressed on top of the prior commit: 1. gsd_stall_watch previously looped internally for the full threshold+interval duration inside ONE Bash tool call (up to 15 min at defaults) — a single call blocking that long risks the host tool's own timeout killing it before it ever prints a result, silently defeating the fix. Redesigned to a single sleep-and-check cycle per call, taking an explicit dispatch_ts so the orchestrator prose can repeat the (short, default 5 min) call until it resolves; the outer threshold is now enforced by dispatch_ts accumulating across calls, not by one call's duration. Documented the resulting trade-off (up to one interval of added latency on the success path) in the changeset and reference doc. 2. PLANNER_STALL_INTERVAL_MINUTES/THRESHOLD_MINUTES are config-controlled values that flow into bash arithmetic ($(( ))). A review flagged this as command injection; empirically verified against both macOS bash 3.2.57 and Docker bash:5 that this is NOT actually exploitable (bash hard-errors on a `$(cmd)`-shaped arithmetic operand rather than invoking it) — but an unvalidated malformed value WOULD abort the stall-watcher itself with that bash error, silently defeating the exact hang-recovery this issue ships. Added integer validation with safe-default fallback, both at the config-resolution point and defensively inside gsd_stall_should_recover. Also adds the previously-missing integration coverage for gsd_stall_watch's real execution (grep/find/date plumbing), not just the pure classifier. * fix(#2650): correct AC2 self-test — helpers doc may name teams-status in prose The AC2 regression test asserted the stall-detection-helpers.md step file never contains the substring "teams-status" at all, but the file's own prose explicitly documents its independence from that guard (containing the word by design). Narrowed the assertion to what actually matters: no second `query teams-status` call site and no gating on it, not a blanket absence of the word. * test(#2650): regenerate golden install-tree fixtures for the new step file gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md is an emitted file (installed for every runtime), so adding it changes the install tree even though it is invisible to docs/INVENTORY.md and docs/INVENTORY-MANIFEST.json (both explicitly scope to non-recursive gsd-core/workflows/*.md — verified against the execute-phase #2930 and pre-existing plan-phase step-file precedent, which are equally absent from both inventory artifacts). The golden install tree snapshots the sorted list of emitted relative paths per runtime, so a file invisible to the inventory is still visible here. Regenerated via `npm run gen:install-tree` — one line added per runtime fixture (19 files), no other drift. * fix(#2650): restore 7 ORCHESTRATOR RULE labels; sync runtime-launcher preamble Two more consequences of extracting helper bodies out of plan-phase.md, both caught by verification (0017e1a78, 9 unique failures): 1. tests/plan-phase-drift-guard.test.cjs (#913) requires at least 7 "ORCHESTRATOR RULE — ALL RUNTIMES" labels in plan-phase.md itself, one per agent spawn site. Moving the full explanatory blocks to plan-phase/steps/stall-detection-helpers.md carried 5 of the 7 labels out with them (only the untouched researcher/pattern-mapper sites kept theirs). Restored a short label at each of the 5 stall-watch sites, trimmed a few more redundant words ("Per 7.99, " — already established by the adjacent step-7.99 pointer) to stay under the frozen PRE_PHASE6 cap (94497 bytes, 21 bytes headroom). 2. tests/runtime-launcher-parity.test.cjs (#373) requires exactly one canonical gsd_run preamble, byte-equal to gsd-core/workflows/_runtime-launcher.snippet.sh, before the first gsd_run call in any workflow .md that calls it (recursive scan under gsd-core/workflows/, unlike the non-recursive inventory/step-tag-balance checks). The new step file's config-get calls use gsd_run without one. Fixed via `node scripts/sync-runtime-launcher.cjs`, verified: exactly 1 preamble occurrence, before the first call, including the .claude/ and .codex/ home fallback arms. Also verified (no fix needed, evidence recorded): the generic `gsd-core-verbatim` identity rule in tests/helpers/emitted-provenance.cjs (roots: ['gsd-core'], pattern matching workflows/.+) self-attributes any new gsd-core/workflows/** path to itself, so the new step file needs no drift-ack entry — consistent with plan-phase.md's own net shrinkage requiring none either. * test(#2650): acknowledge plan-phase.md's +14 byte drift Restoring the 5 ORCHESTRATOR RULE — ALL RUNTIMES labels (#913) flipped plan-phase.md from -142 bytes (post-extraction) to +14 bytes net growth against baseline (94483 -> 94497), which the differential attribution size ratchet (tests/emitted-attribution.test.cjs) correctly flags as unacknowledged growth. Added tests/emitted-drift-acks/2650-plan-phase- stall-detection.json, keyed on the bare filename plan-phase.md per the existing fragment schema (see tests/emitted-drift-acks/2649-diagnose- execute-plan-base-check.json), explaining the growth as exactly the 5 restored labels — still verified under the PRE_PHASE6 cap (94497 < 94519) and satisfying #913's 7-label requirement. * fix(#2650): bind {outputFile} from the real Agent() return — was dead code Independent review blocker: PLANNER_OUTPUT_FILE/CHECKER_OUTPUT_FILE were read by every gsd_stall_watch call but never assigned anywhere in the diff. With the variable permanently empty, `[ -f "$output_file" ]` was always false, marker_found could never become true, and marker_received was unreachable — the marker-based detection path was permanently dead. Worse for the plan-checker spawn specifically: a checker that PASSES touches no *-PLAN.md files, so it had no working completion signal at all without the marker path. A healthy plan-checker finishing cleanly in two minutes would be declared stalled once planner.stall_threshold_minutes elapsed and the recovery menu would fire on an already-succeeded agent — worse than the original unbounded hang. Fixed by replacing the dead bash variable with the `{outputFile}` orchestrator-substitution token, the same convention docs-update.md:471 already uses for a real run_in_background=true Agent() return ("Read tool: file_path: `{outputFile from README agent result}`"). This is a net BYTE SAVING at each site (`"{outputFile}"` is shorter than `"$PLANNER_OUTPUT_FILE"`), which funded moving the full binding explanation — including why plan-checker's *-PLAN.md glob alone is not a working completion signal — into the lazily-loaded reference file to stay under the frozen PRE_PHASE6 cap (94496 bytes, 22 headroom; net +13 over baseline, acknowledged in tests/emitted-drift-acks/2650-plan-phase- stall-detection.json). Added a regression test asserting plan-phase.md itself binds {outputFile} at all 5 spawn sites and contains no dangling $PLANNER_OUTPUT_FILE / $CHECKER_OUTPUT_FILE reference — the previous test suite only exercised gsd_stall_watch's behavior when handed a valid argument, which is why the dead production wiring survived two rounds of review. Also fixed tests/fix-2650-plan-phase-stall-detection.test.cjs:170-195's raw try/finally to use t.after(), per CONTRIBUTING's test-cleanup convention. * chore(#2650): backfill changeset PR number to 3015 * fix: normalize CRLF at the read boundary in all .md-bash-extraction tests Maintainer-authorized scope expansion, folded into this PR rather than deferred: the Windows CI lane on this PR's own tests/fix-2650-plan-phase- stall-detection.test.cjs exposed DEFECT.TEST-SHELL-PIPELINE-NONPORTABLE (CONTEXT.md; recurring since #1700) as a repo-wide latent class, not a one-off. Ten test files parse a fenced ```bash block out of a workflow .md file and execute it via spawnSync/execFileSync; a Windows checkout can yield CRLF line endings despite .gitattributes eol=lf, and bash then treats the trailing \r on every extracted line as part of the token — "unexpected EOF while looking for matching `"'" or a bare syntax error, partway through the script. Added tests/helpers.cjs:readFileNormalized() — strips \r\n -> \n at the read boundary, before any fence-slicing or regex runs, so every downstream operation is correct by construction. Migrated all ten call sites to it: Previously broken (fs.readFileSync with no normalization anywhere between read and spawn): - tests/worktree-cleanup.test.cjs (extractCwdGuardBash) — also fixes a misleading comment claiming the fence regex alone was "CRLF-safe"; it protected only the fence delimiters, never the captured body. - tests/new-milestone-clear-phases.test.cjs (extractFenceBetween, extractFenceContaining) - tests/code-review-pipeline-regression.test.cjs (extractPostProcessingScript) - tests/drift-detection.test.cjs (readGate/bashBlock, plus the snippet-file comparison read in the same test) - tests/graphify-visualization.test.cjs (extractStep3Block) - tests/pause-work-improvements.test.cjs (extractCheckBlock) - tests/plan-review-convergence.test.cjs (extractReviewerFlagsParseBlock and the inline post-config-gate resolution-block slices) Already correct (split(/\r?\n/) then join('\n')), migrated to the shared helper for consistency rather than a fourth/fifth/sixth copy of the same fix: - tests/git-base-branch.test.cjs (extractHandleBranchingBash) - tests/quick-branching.test.cjs (extractStep25Bash) - tests/runtime-launcher-parity.test.cjs (extractResolverSnippet) Verified against a simulated Windows CRLF checkout (not assumed): for both the worktree-cleanup.test.cjs and new-milestone-clear-phases.test.cjs extraction shapes, confirmed the pre-fix code produces a real bash syntax error on CRLF input and the post-fix code does not. One eslint follow-up: local/no-crlf-fragile-split statically flags any bare `\n` inside a markdown-fence-shaped regex, regardless of whether the receiver was already normalized — it cannot see the readFileNormalized() data-flow. Kept `\r?\n` in extractCwdGuardBash's fence regex (redundant but harmless on pre-normalized input) rather than fight the rule. Scope note: this diff is broader than issue #2650's own change (plan- phase.md stall detection) because the Windows lane surfaced a genuine repo-wide defect class while verifying that fix, and the maintainer authorized fixing it here rather than filing it separately and shipping a known-broken pattern. Runtime impact: none — this is a test-harness-only defect. The live orchestrator (Claude Code or another runtime) does not do a byte-exact extract-and-pipe of .md content into a shell the way these tests do; it reads the instructions and generates its own bash invocation text, which does not reproduce a raw CRLF pass-through the same way. Not touched: tests/plan-review-convergence.test.cjs's separate, tracked spawnSync ETIMEDOUT flake under bench load (#3005, reproduced on unmodified next) — unrelated load-sensitivity, not a CRLF symptom. * fix(#2650): remove stale drift-ack fragment — plan-phase.md is self-explaining tests/emitted-drift-acks/2650-plan-phase-stall-detection.json acknowledged plan-phase.md's own emitted-path hash move, but plan-phase.md is directly edited in this diff. Per the emitted-attribution law (ADR-2719, tests/emitted-attribution.test.cjs), a workflow's emitted key equals its own source path (gsd-core-verbatim identity rule), so a direct edit to the source is self-explaining and auto-attributed — no ack was ever needed. Verified via the pre-merge lint (scripts/lint-emitted-drift-ack.cjs, run through npm run lint:ci with a fully cleared eslint cache): it passes clean with the fragment removed, confirming no contradiction between the lint and the runtime attribution gate — this was simply an unnecessary fragment. * fix(#2650): restore plan-phase.md drift-ack — size ratchet demands it against next tests/emitted-drift-acks/2650-plan-phase-stall-detection.json was deleted in the previous commit because, against an earlier verification base, it was inert: it explained a moved emitted hash that a direct edit to plan-phase.md already self-attributes. Against origin/next@f1af47766a the demand is different: plan-phase.md is 13 bytes larger than the base copy, which trips the emitted-attribution size ratchet — a job this same ack also performs. Recreated in the documented shape, keyed on the bare filename plan-phase.md (not the full path, and not restating the byte delta per review guidance), describing the actual change: the {outputFile} binding fix for the dead PLANNER_OUTPUT_FILE/CHECKER_OUTPUT_FILE variables and the 5 restored ORCHESTRATOR RULE labels required by #913, both at the stall-watch spawn sites, with explanatory bodies living in the lazily-loaded gsd-core/workflows/plan-phase/steps/stall-detection-helpers.md reference. Confirmed no other fragment (on this branch or on next) claims the bare key "plan-phase.md" before recreating — scripts/lint-emitted-drift-ack.cjs's duplicate check is an exact string match, and the only other mention of plan-phase.md in tests/emitted-drift-acks/ (2658-trae-instruction-file-path.json) uses the full path as its key, so there is no collision. * fix(#2650): real cause of Windows CI failure — bash -c argv-transport, not CRLF The CRLF diagnosis for PR #3015's Windows failure was wrong. Proven wrong, not assumed: .gitattributes' blanket `* text=auto eol=lf` means a Windows checkout never receives CRLF for stall-detection-helpers.md, and the extracted fence's line 64 is byte-identical and correctly balanced on every platform. The real cause: runShouldRecover() passed a 70+ line, quote-dense script as ONE argv element to `spawnSync('bash', ['-c', script, arg0, ...])` PLUS four more positional args. Windows has no execve — Node serializes that whole argv into a single CreateProcess command-line string, and Git Bash's MSYS layer re-splits and unescapes it with its own rules. The boundary between the script and the trailing args was not stable across that round trip (live evidence: one failure's stderr was prefixed `gsd_stall_should_recover_test:` — arg0 arrived — another `/usr/bin/bash:` — arg0 did not). Fixed by writing the script to a temp file and running `bash <file> <args>` instead — the four values are now normal, quote-free positional args, and the script itself never enters argv transport at all. Mirrors tests/quick-branching.test.cjs's extractStep25Bash/runStep, which already uses this exact shape and is green on Windows on `next`. tests/worktree-cleanup.test.cjs's extractCwdGuardBash/runGuard stays on `bash -c` but never appends extra positional args beyond the script itself, so it never hits the same boundary — checked both siblings per review, not assumed. Corrected the now-actively-misleading CRLF comment in extractStallHelpersBash(), and corrected the changeset's claim that the repo-wide CRLF-normalization fix (folded into this branch, maintainer- authorized) explains this PR's own Windows failure — it doesn't, though it remains defensible on its own merits as general test-portability hardening. Separately, while auditing the shipped (non-test) gsd_stall_watch for Windows portability per review request, found and fixed a second, real user-facing defect: the artifact-freshness check used GNU find's `-newermt "@<epoch>"` shorthand, which the BSD find(1) actually shipped on macOS does NOT understand ("Can't parse date/time: @<epoch>", verified live against /usr/bin/find on both a stale and a genuinely fresh file). With the adjacent `2>/dev/null`, that failed silently and permanently degraded artifact_fresh to false on every macOS run — a plan-checker or planner actively writing plan files could still be reported "stalled." Replaced with `find $glob -mmin -N` ("modified less than N minutes ago"), which needs no date-string parsing and is supported identically by GNU find and BSD find; verified live that the old shape fails and the new shape passes against the same real fresh file. Added a real-execution regression test (gsd_stall_watch with `sleep` stubbed to a no-op so the test doesn't actually wait, but the real `find ... -mmin` line still runs) proving the fix, replacing the prior "not integration-tested" note for that path. Note: the remote gsd-test runner is Linux-only, so it cannot itself confirm the Windows fix — only the actual windows-latest CI lane can. * fix(#2650): route the third bash -c call site through the same temp-file seam runWatch() and a `-mmin` regression test still passed their script via `bash -c <script>` after the previous commit only converted runShouldRecover() — live Windows CI on 4b86cc57f confirmed the mechanism: failures went 11 -> 4, and `full test (windows-latest, 22, shard 1/3)` and `shard 2/3` flipped from fail to pass, but the remaining 4 failures (all in this file, all still `bash: -c:`) were exactly the gsd_stall_watch describe block, which runWatch() serves. runWatch() passes NO extra positional args at all, so this also rules out the trailing-args theory from the prior commit: the ~73-line, quote-dense script itself is what does not survive Windows argv serialization when passed as a single `-c` element, regardless of how many (if any) further argv elements follow it. Extracted one shared runBashScript(script, args, opts) helper — write to a fs.mkdtempSync'd file, run `bash <file> [args...]`, clean up in `finally` — and routed all three bash-invoking call sites in this file through it (runShouldRecover, runWatch, and the -mmin freshness test that builds its own script inline for the `sleep` stub). One transport seam means a fourth call site in this file cannot silently reintroduce the bug in isolation, which is exactly what happened here with a second call site. Corrected extractStallHelpersBash()'s doc comment a second time to state the mechanism precisely (script content, not argv-element count) and cite the live evidence (11->4 failures, shards 1 and 2 flipping green) so the next reader does not have to rediscover it. Audited every other bash-invoking call site in files this branch touches, per review request: - tests/code-review-pipeline-regression.test.cjs (runPostProcessing), tests/graphify-visualization.test.cjs (runBlock), and tests/drift-detection.test.cjs (two execFileSync('bash', ['-c', ...]) sites, one of them carrying the same giant runtime-launcher preamble text) — all pre-existing, UNCHANGED by this branch (only touched for the readFileNormalized() CRLF swap), and already exercised on `next`'s last six Windows CI runs per the reviewer's own citation. Left as-is: no evidence of failure, and converting untested pre-existing code outside #2650's scope on an unverifiable guess would be its own risk. - tests/git-base-branch.test.cjs (runHandleBranchingStep) and tests/quick-branching.test.cjs (runStep) already use the same temp-file pattern. No action needed. - tests/runtime-launcher-parity.test.cjs (runResolver) uses `bash -c` but is explicitly `if (process.platform === 'win32') return '';` guarded off on Windows entirely, for an unrelated extension-less-PATH-stub reason — never reaches Windows argv transport at all. No action needed. - tests/worktree-cleanup.test.cjs (runGuard) confirmed by the reviewer as correct and verified; not touched, per instruction. Do not touch: the -mmin fix, the drift-ack fragment, the changeset — all three confirmed correct in prior rounds and left untouched here. Note: the remote gsd-test runner is Linux-only and cannot confirm this; only the windows-latest lanes on #3015 can. * fix(#2650): give runBashScript a default timeout runShouldRecover() was the only one of the three call sites through runBashScript() with no timeout — runWatch() and the -mmin test both pass timeout: 10000 explicitly. Not a regression (this path never had a bound before), but CONTEXT.md's unbounded-subprocess guidance applies directly, and runShouldRecover() is driven repeatedly by a fast-check property test: one pathological input that fails to terminate would hang CI indefinitely instead of failing. timeout: 10000 is now the helper's own default, with ...opts spread after it so the two existing explicit timeout: 10000 call sites are unchanged and any future caller inherits a bound automatically. * fix(#2650): build the -mmin freshness test's glob with forward slashes Windows CI on d6ddda6ea reported the last failure: the -mmin regression test expected 'active' but got 'waiting' — find matched nothing, the same silent-degradation shape as the macOS -newermt defect, but this time in the test's own fixture rather than the shipped bash. Traced what production actually passes: every gsd_stall_watch call site in plan-phase.md builds artifact_glob as `"${PHASE_DIR}"'/*-PLAN.md'` — PHASE_DIR is a POSIX-style .planning/phases/NN-slug value, and the whole thing runs under Git Bash regardless of host OS, so production's glob is always forward-slash. The test instead built it with `path.join(tmp, '*-PLAN.md')`, which on Windows yields a backslash path (C:\Users\RUNNER~1\...\*-PLAN.md). In bash pathname expansion a backslash escapes the next character, so that pattern can never match a real path — find silently returns empty under the existing 2>/dev/null, same shape as the macOS bug. Confirmed as a test artifact, not a production defect: production never constructs the glob this way, so no Windows user is affected. Fixed by forward-slashing the tmp dir before appending the glob suffix, matching production's own convention, with a comment recording why (so a future "simplify this back to path.join" edit doesn't silently reintroduce the failure). The shipped bash's unquoted $artifact_glob is untouched — quoting it would break the multi-file glob expansion it exists for. Note: the remote runner is Linux-only and already passed clean at d6ddda6ea (0/29,603, both node lanes); only the windows-latest lanes on #3015 can confirm this fix. * fix(#2650): forward-slash the three remaining runWatch globs (vacuous-pass CR) The :353 fix (833c11da9) only converted the -mmin freshness test's glob. Three sibling tests in the same describe block still built theirs with path.join(tmp, '*-PLAN.md'), which yields a backslash path on Windows. Two of those three were silently passing for the wrong reason: the '-> stalled' and '-> waiting' tests both expect the glob to match nothing, and on Windows a backslash path matches nothing regardless of whether the directory is actually empty (bash eats each backslash as an escape before the pattern is even evaluated). They would have passed identically with glob expansion completely broken, which is a vacuous pass — not exercising what they claim to. The third ('-> marker_received') is outcome-independent of the glob, so it was merely inconsistent rather than wrong. Converted all three to the same `${tmp.replace(/\\/g, '/')}/*-PLAN.md` construction already used at the -mmin test, so every glob in the file now matches production's own forward-slash `"${PHASE_DIR}"'/*-PLAN.md'` shape, and the two negative tests are meaningful on Windows instead of accidentally correct. Reworded the trailing comment on the 'stalled' test's glob line: it now describes the fixture (the tmp dir contains no *-PLAN.md files) rather than the pattern, since "matches nothing" read as a property of the glob syntax when it's a property of what's on disk. No assertion, the sleep stub, runBashScript, or the shipped bash changed. Smoke-tested all three updated tests manually before committing (not via node --test): marker_received / stalled / waiting, all correct. * fix(#2650): fix own regression tests for #2993's plan-phase.md relocation 531101843's merge with origin/next brought in #2993 (unrelated, epic #1671 Phase 6.2), which extracted plan-phase.md's whole "Chunked Planning Mode" section into gsd-core/workflows/plan-phase/steps/chunked-planning-mode.md, leaving a <!-- gsd:section --> pointer behind. tests/plan-phase-drift-guard. test.cjs (#913) was already updated to read the combined surface (host file + every steps/*.md) so its label count didn't go blind — my own #2650 regression tests were not, and searched plan-phase.md alone for the two chunked spawn sites' headings, which no longer exist there. Two tests failed outright (indexOf returning -1); a third ("standard planner spawn") was silently weakened to an unbounded slice-to-EOF by the same relocation, since its own end-boundary heading also moved — passing by accident rather than by testing what it claimed. Promoted the drift guard's local readPlanPhaseCombined() to a shared, exported tests/helpers.cjs readWorkflowCombined(workflowPath) (host file + sorted steps/*.md, CRLF-normalized at the read boundary) so a second, divergent implementation is never written — the drift guard now delegates to it via a same-named local wrapper, unchanged at every existing call site. Fixed the three affected tests in tests/fix-2650-plan-phase-stall-detection. test.cjs: - "standard planner spawn (step 8)": end boundary changed from the now-gone "## 8.5. Chunked Planning Mode" heading to "## 9. Handle Planner Return", which still exists in plan-phase.md. - "chunked outline spawn (8.5.1)" / "chunked per-plan spawn (8.5.2)": now read gsd-core/workflows/plan-phase/steps/chunked-planning-mode.md directly (not the generic multi-file combined blob, whose file-sort ordering would put unrelated step files between 8.5.2's slice and any downstream anchor) — the same heading-to-heading slicing as before still works because the file is small and self-contained. - Extended the "no unbound $PLANNER_OUTPUT_FILE/$CHECKER_OUTPUT_FILE" check to also scan chunked-planning-mode.md, since two of the five spawn sites now live there. - Added a new count-based test asserting exactly 5 (not "at least one") `gsd_stall_watch "$TS" "{outputFile}"` invocations across the combined surface, mirroring #913's own label-count guard, so every one of the five spawns stays provably bounded and a future relocation can't silently drop one without a test noticing. Also added a small positive test that plan-phase.md's <!-- gsd:section --> pointer to chunked-planning-mode.md exists (#2993 is unrelated to #2650 but its presence is now load-bearing for where 2 of the 5 spawn sites live). Audited every other test file in the repo for a stale reference to content #2993 relocated (searched for the moved headings/prose and for "chunked-planning-mode"/"CHUNKED_MODE" across all *.test.cjs): only this file and the drift guard needed changes. tests/issue-2762-plan-reviews-chunked.test.cjs already reads chunked-planning-mode.md directly (brought in correct by the same merge). gen-section-manifest.test.cjs, init.test.cjs, and workflow-fragments.test.cjs reference "chunked-planning-mode" only as a manifest/section-id fixture value for #2993 itself, not as a stale pointer to relocated content. Did not touch: the ported ORCHESTRATOR RULE lines, run_in_background=true, the glob constructions, runBashScript, the -mmin change, the timeout default, or the drift-ack fragment (confirmed correct against the stale local `next` ref two rounds ago and left alone). --------- Co-authored-by: sim <sim@local>	2026-08-03 10:46:22 -04:00
Tom Boucher	ad3b9ec486	chore(#1671): fragmentize plan-phase.md and repair flag forwarding to the init bundle — Phase 6.2 (#3019) ... * chore(#2993): fragmentize plan-phase.md onto the fragment model Epic #1671 Phase 6.2. plan-phase.md is the largest workflow in the repo and carried zero markers; it was deferred out of the Phase 3 pilot for two reasons, both now dead. The 36-byte PRE_PHASE6 headroom was never the blocker it looked like — fragmentizing is net-negative on host source, so the trim is what creates the room. The --mvp interleaving was resolved by measurement in #2992 and no sub-line mechanism is built. - widen WHEN_VOCABULARY 14 -> 19 via a second coordinated ADR-1671 amendment: flag:--ingest, flag:--prd, flag:--research-phase, flag:--reviews, state:chunked-mode - state:chunked-mode is `--chunked` OR config workflow.plan_chunked, and that disjunction is resolved in the FACT, never in the grammar, so a compound condition never becomes an operator - parse the new flags on the plan-phase route; extract six gated bodies to gsd-core/workflows/plan-phase/steps/ behind manifest-gated stubs - prd-express-path.md was already extracted but read unconditionally; its wrapper is now gated, so the existing extraction finally pays off plan-phase.md 94,483 -> 87,575 bytes (cap 94,519): headroom goes from 36 bytes to 6,944. Also closes a surfaced docs gap: five real plan-phase flags (--chunked, --skip-ui, --bounce, --skip-bounce, --granularity) were documented in neither the argument-hint nor help. Making --chunked load-bearing without fixing its siblings would leave the defect class half-open. Refs #2993 * fix(#2993): forward flags to the init bundle so section gating actually fires Blocker found by the correctness review, confirmed directly, and missed by both the isolated reviewer and every test in this branch. Neither workflow forwarded its flags to the init CLI: plan-phase.md:71 INIT=$(gsd_run query init.plan-phase "$PHASE" $GRAN_PARAM) execute-phase.md:84 INIT=$(gsd_run query init.execute-phase "${PHASE_ARG}") So every flag: atom was permanently false in production and its section permanently excluded. For plan-phase that made the PRD express path UNREACHABLE — a regression, since it was an unconditional read before. For execute-phase this is PRE-EXISTING: #2932 shipped `flag:--wave` gating that has never once been true, so `--wave` silently dropped its own wave-filtering guidance. Fixed here under the no-defer rule. Why every test missed it: they drive the init CLI directly with flags, which works. Production goes through the workflow's bash line, which did not pass them — the exact "assert against the shape production uses" trap this branch's own test matrix warns about. - parse and forward --prd/--ingest/--research-phase/--reviews/--chunked (plan-phase) and --wave (execute-phase), using the anchored regex idiom the neighbouring GRAN_PARAM line already uses - add a regression guard DERIVED FROM THE MANIFEST: for every flag:--X section, the owning workflow's init line must forward --X. It fails against the pre-fix files and covers any future atom, rather than spot-checking today's six. Verified through the workflow shape, not the CLI shape: `3 --prd spec.md` now yields ["prd-express-gate"] (was []), `2 --wave 2` yields ["partial-wave"] (was []). Refs #2993 * test(#2993): acknowledge the execute-phase ripple and regenerate install-tree fixtures Remote matrix was red with 46 unique failures, identical on both lanes. Both causes are mechanical consequences of changing shipped workflow content, and neither is visible to any local gate. - emitted-attribution: execute-phase.md grew 163 bytes from the WAVE_PARAM forwarding fix and was unacknowledged, while the ack fragment named plan-phase.md, which SHRANK and therefore needed no ack at all — a stale entry is itself a failure. The reason now names the real ripple. The entry had to merge into the existing 2930 fragment: the ack linter does unconditional cross-fragment duplicate-key detection with no spent/live exception, so a second fragment declaring execute-phase.md collides even when the first is already merged and inert. Resolved per the linter's own guidance and that file's precedent of appending successive ripple reasons to one entry. - golden-install-tree: tests/fixtures/install-tree/*.json are committed and deliberately excluded from the ADR-2719 attribution cutover, so they must be regenerated when shipped tree content changes. Regenerated after build:lib per the ordering landmine. 19 runtimes each gained exactly the six new plan-phase step files; zero paths removed, which is the absolute failure shape those fixtures exist to catch. Refs #2993 * fix(#2993): restore the launcher preamble in an extracted step and follow moved content in its drift guards Second red run: 26 unique failures, identical on both lanes, in two classes. RUNTIME BUG (runtime-launcher-parity, 7 failures) — chunked-planning-mode.md calls gsd_run but carried no canonical launcher preamble, which is what DEFINES gsd_run(). On any non-Claude runtime that step would fail outright. The preamble is now copied verbatim from the canonical source of truth, gsd-core/workflows/_runtime-launcher.snippet.sh, and the fence dedented to column 0 to match the prd-express-path.md sibling (a list-continuation indent breaks the byte-equal preamble match). prd-express-path.md already had a correct one. This is the same defect #2932 hit when it extracted steps; the parity test caught a real bug, not a stale assertion. DRIFT GUARDS (plan-phase-drift-guard, issue-2762-plan-reviews-chunked, skill-frontmatter-contract) — these assert plan-phase.md contains content this branch moved into step files. Retargeted at where the content now lives, with the asserted property unchanged; the ALL-RUNTIMES label COUNT test now reads host + every step file so the count is preserved across the split rather than reduced. Each retargeted guard was verified to still fail when its step file is stripped, so none was weakened into vacuity. No emitted-drift ack was needed: currentSizes() enumerates gsd-core/workflows/*.md non-recursively, so files under plan-phase/steps/ are never in the size ratchet's scope. Refs #2993 * chore(#2993): backfill changeset pr number to 3019 --------- Co-authored-by: sim <sim@local>	2026-08-03 09:38:58 -04:00
Tom Boucher	9640968f8e	fix(#2847): require gap_closure value in plan-gap-closure schema and bind validate_plan to it (#3018) ... * test(#2847): add failing-first regression tests for gap-closure frontmatter schema gap --gaps did not load a machine-checked requirement for gap_closure: true. The planner's only validation gate (frontmatter.validate --schema plan) never required it, and plan-phase.md's downstream_consumer contract never mentioned it either, so gap-closure plans could pass validation while missing the field that /gsd:execute-phase --gaps-only filters on. These tests are RED against current production code: no plan-gap-closure schema exists yet, and neither agents/gsd-planner.md's validate_plan step nor plan-phase.md's downstream_consumer block references gap_closure conditionally. * fix(#2847): enforce gap_closure via plan-gap-closure schema --gaps did not load a machine-checked requirement for gap_closure: true. The planner's only validation gate (frontmatter.validate --schema plan) never required it, so a gap-closure plan could pass validation while missing the field /gsd:execute-phase --gaps-only filters on, silently spawning zero executors. Add a plan-gap-closure schema (every plan-required field plus gap_closure) and make the planner's validate_plan step select it when gap_closure mode is active, plan otherwise. Standard/reviews-mode plans are unaffected: plan's required fields are unchanged. plan-phase.md's downstream_consumer block was investigated for a symmetric mention but deliberately left untouched: it sits 36 bytes under the frozen ADR-857 PRE_PHASE6 ceiling and the validate_plan step in gsd-planner.md is the actual call site, needing no help from plan-phase.md's prose. * fix(#2847): compact validate_plan edit under gsd-planner.md size caps Merging origin/next (7 commits, including #2775's gsd-planner.md STRIDE-row edit) left only 22 chars of headroom under four separate hard-coded 49152-char caps on gsd-planner.md (planner-decomposition, precondition-element, reversibility-tagging, security.test.cjs). The verbose validate_plan prose from the previous commit overran all four. Compact the edit to a single line (net +17 chars vs origin/next) while keeping the functional content: schema name, mode condition, and the unchanged base required-fields list. Also: - Fix a real bug in the fix-2847 negative-assertion test: plan-phase.md mentions the literal string "<downstream_consumer>" twice in backtick-quoted prose before the actual opening tag, so a plain indexOf() grabbed the wrong start position and swallowed ~10KB of unrelated content (including a "gap_closure" hit in a Mode: enum line), producing a false failure. Anchor on the tag starting its own line instead. - Merge the emitted-drift-ack fragment for gsd-planner.md with the #2775 fragment brought in by the merge (both named the same path; two ack sources may never name the same path) and correct its byte delta to the actual final number. * fix(#2847): drop stale merge-inherited emitted-drift-ack fragments Merging origin/next brought in three new emitted-drift-ack fragments (1700, 2658, 2775) relative to this branch's fork point. #2775 collided with my own gsd-planner.md key and was already consolidated. #1700 and #2658 don't collide, but none of their entries name a path this branch's actual diff touches (git diff --name-only origin/next...HEAD) — the ripples they explain are already baked into the current next baseline, so they explain nothing here and the emitted-attribution gate correctly reports them as stale (verified live: spike-wrap-up.md from #1700). Delete both fragment files. Neither is referenced by any test beyond a stray comment pointing at an unrelated diagnosis artifact path, not the ack fragment itself. * fix(#2847): restore merge-inherited ack fragments deleted in error 1700-spike-manifest-idea-scoping.json and 2658-trae-instruction-file-path.json exist on origin/next (landed via other, already-merged PRs) and arrived on this branch unchanged via the origin/next merge. The previous commit deleted them to satisfy a stale-acknowledgment finding, but the finding was about the acks being MODIFIED in this diff, not about needing to stop existing — deleting them would have silently reverted two other PRs' already-merged, already-justified byte growth. Restored byte-identical to origin/next (git diff origin/next -- <path> empty for both). 2775-planner-package-legitimacy-gate.json stays consolidated into 2847-gap-closure-validate-plan-step.json: that one was a genuine hard key-collision (two fragments naming the same gsd-planner.md path, which lint-emitted-drift-ack hard-blocks), not a pass-through case. * fix(#2847): bind --schema to gap_closure mode, not hardcode it Prior revision left the validate_plan bash invocation unconditional (--schema plan)) while only the prose sentence above it described the gap_closure-mode branch. An agent executing the shown line literally always validated with the plan schema, so a gap-closure plan missing gap_closure: true still reported valid:true — #2847 reproducing unchanged. Existing tests didn't catch it: they checked for substring presence anywhere in the step, which the prose alone satisfied. Change the bash line to --schema "$SCHEMA" — a real shell-variable reference in the same placeholder convention this file already uses for "$PLAN_PATH" (never literally assigned; the agent resolves it from context, same as PLAN_PATH). A genuine if/then bash conditional already exists elsewhere in this file (load_project_state's INIT @file: check), confirming executed conditionals, not merely descriptive prose, are the established pattern here. Rewrite the regression test to assert on the bash block's literal --schema argument: reject a hardcoded plan) or plan-gap-closure) literal, require a variable reference, and require the step's prose to bind that same variable name. Verified RED against the prior revision and GREEN against this one before committing either state. * fix(#2847): CRLF-safe tests, drop unexplained ack, require gap_closure=true Four items from independent review, all landing together per request: 1. The #2847 regression test file had two CRLF-fragile regexes (local/no-crlf-fragile-split): a bare \n on readFileSync content means a real \r\n checkout returns invocationLine === null and all four executable-content assertions stop asserting anything while still reporting green. Both now use \r?\n. Prior lint report of exit 0 was a false green from a stale eslint cache. 2. The 2847 drift-ack fragment explained nothing: a direct edit to agents/gsd-planner.md is self-explaining, drift-acks exist for emitted-artifact ripple that cannot be traced to a changed source path. Deleted. Restored the 2775 fragment byte-identical to next (git diff --name-status next...HEAD -- tests/emitted-drift-acks/ now prints nothing) — it only conflicted with the now-deleted 2847 fragment, never needed touching itself. 3. plan-gap-closure validated gap_closure by PRESENCE only (unchanged since the original #2847 fix), so gap_closure: false satisfied it — --gaps-only filters strictly on gap_closure === true, so a false-valued plan still validates green and still spawns zero executors: #2847's exact reported symptom, one value away. Added an optional requiredValues map to FRONTMATTER_SCHEMAS; plan-gap-closure now requires gap_closure to equal the string "true" (extractFrontmatter parses every scalar as a string) in addition to being present. Every other schema/field keeps the original presence-only contract. The row that had documented the hole instead of closing it now asserts the fix; a matching unit test locks requiredValues on FRONTMATTER_SCHEMAS. 4. The "names the plain plan schema" assertion matched the bare substring "plan" anywhere in the step, which verify.plan-structure satisfies incidentally a few lines below — the assertion could not fail even if the plain-plan branch were deleted from the prose. Changed to match the standalone backtick-quoted plan token. * fix(#2847): remove contradictory leftover assertion in Row 6 test The gap_closure:false test asserted !present.includes('gap_closure') (correct — matches the implementation's fold-wrong-value-into-missing semantics) immediately followed by a stale, unedited leftover from an earlier draft of the same test asserting the opposite: present.includes('gap_closure'). The second could never pass once the first did; both were in the same diff. Verified before committing: searched every consumer of frontmatter.validate output (agents/gsd-planner.md, docs/CLI-TOOLS.md, all other test files) for any read of the present field — none exist. Nothing depends on "present" meaning "physically exists regardless of value correctness", so the implementation's fold (present/missing stay a full partition of required) is the right call; the test needed to agree with it, not the other way around. Manually replayed all six rows in the plan-gap-closure describe block against the built CLI to confirm each now passes. * fix(#2847): prototype-key guard, wrong-value diagnostic, doc fixes, vacuous tests Six items from an independent SHIP_VERDICT:no review, landing together per request: 1. Prototype-key crash (src/frontmatter.cts): FRONTMATTER_SCHEMAS[schemaName] was an unguarded lookup, so --schema __proto__ (also constructor, toString, hasOwnProperty, valueOf) resolved to an Object.prototype member instead of undefined, the `!schema` check never fired, and the command crashed with an uncaught TypeError and a stack trace instead of "Unknown schema". Now reachable from prompt state (--schema is an agent-bound $SCHEMA), not just an unreachable literal. Guarded with Object.prototype.hasOwnProperty.call before the lookup, checked and rejected before assignment so `schema`'s type stays non-optional. Added a test for all five prototype keys. 2. Wrong-value diagnostic (src/frontmatter.cts, agents/gsd-planner.md): the strict gap_closure === "true" check from the previous fix was correct (fail-closed) but silent about WHY — a plan with gap_closure: True got "missing", indistinguishable from genuinely absent, even though the field is plainly in the file. Added an `invalidValue` field to the validate JSON (present but wrong-valued, disjoint from missing/present) and updated validate_plan's prose to state the exact required literal and explain invalidValue, within the remaining byte budget (49130/49152). 3. docs/reference/plan-md.md: fixed three inaccuracies in the gap_closure row — "this field plus every field above" implied `requirements` (documented Required: Yes) is schema-enforced, it is not; "Type: boolean" implied YAML True/TRUE/yes/1 are accepted, they are rejected (exact string match on literal lowercase true); "must never carry it" stated an unenforced rule as fact. Also switched /gsd:plan-phase and /gsd:execute-phase to the house-style hyphen form for docs/. 4. Vacuous negative assertions (tests/fix-2847-gap-closure-frontmatter.test.cjs): RegExp#test coerces a null invocationLine to the string "null", so both hardcoded-literal checks passed vacuously even if the step or its bash block were deleted entirely. Added a truthy precondition check first. 5. Deleted vacuous/pass-always tests: four in tests/frontmatter.unit.test.cjs strictly subsumed by (or, for the "superset" test, tautologically guaranteed by the same spread as) the deepEqual exact-list test; two describe blocks in the #2847 regression file that were already GREEN at the RED commit (5e5897cd2f17ebf2fc55757bae651bbbeb236289) and pinned untouched files rather than covering anything this change altered — one of them additionally forbade any future legitimate gap_closure mention in plan-phase.md, a trap for whoever frees up that file's byte budget later. 6. .changeset/clever-newts-wake.md: switched /gsd:plan-phase and /gsd:execute-phase to /gsd-plan-phase and /gsd-execute-phase — changesets render verbatim into CHANGELOG.md with no converter in the path, so the colon form would have reached readers naming a command no runtime registers. * chore(#2847): backfill changeset pr number (#3018) --------- Co-authored-by: sim <sim@local>	2026-08-03 09:16:29 -04:00
Tom Boucher	77dbfb961d	fix(#2645): persist verification verdicts so deletion cannot raise completeness (#3016) ... * test(#2645): pin failing-first regression coverage for verification-deletion ledger Deleting a *-VERIFICATION.md file after a failing gaps_found/human_needed verdict was recorded silently raises reported workstream completion — the deletion is indistinguishable from "verifier never ran" at the verdict-lookup layer. These tests pin the correct behavior (deletion must never raise completed_phases/progress_percent) and fail against the current code, which has no such protection. * fix(#2645): persist verification verdicts across deletion in workstream rollup FAILING_VERIFICATION_STATUSES gated phase completeness on a verdict read fresh from *-VERIFICATION.md on every call. Deleting that file made "verifier found gaps, report later deleted" indistinguishable from "verifier never ran" (both read the internal 'missing' sentinel), so removing evidence silently raised completed_phases/progress_percent. Persist the last real verdict observed per phase key in a ledger file at the workstream directory level (outside every phase directory, so the same deletion that triggers the hole cannot also erase the memory of it). The ledger is consulted only when a live read comes back 'missing' and is updated whenever a real verdict is observed, so a genuinely re-verified phase is never permanently pinned, and verifier-disabled or not-yet-verified phases are unaffected (no ledger entry is ever created for them). Ledger-winner selection walks phase directories in the same sorted order buildWorkstreamInventory's own duplicate-directory tie-break uses, so a stale same-numbered directory can never clobber the live directory's remembered verdict on an exact mtime tie. Adds fault-injection coverage for the ledger read/write per CONTRIBUTING.md's filesystem-writes rules. * fix(#2645): redesign verification ledger to fail closed, not open The two-state ledger read (any read/parse failure -> "nothing remembered") failed OPEN: deleting the ledger alongside the report, or simply corrupting it while the report was already gone, degraded to the same 'missing' sentinel this issue exists to stop trusting -- silently reopening the completion-inflation hole one level up. Redesigned as a three-state read distinguishing 'absent' (no ledger file at all -- ENOENT specifically, disambiguated from a broken symlink via lstatSync) from 'corrupt' (file exists but unreadable/unparseable/ wrong shape) from 'ok'. Only 'absent' behaves as pre-fix (ungated) -- deliberate, since every existing project is in that state for every workstream on the day this ships. 'corrupt' and 'ok' both fail closed for a phase with no trustworthy entry, via a new internal sentinel 'unrecorded' added to FAILING_VERIFICATION_STATUSES. A corrupt ledger is not a permanent wedge: it is only overwritten when a real verdict is actually observed (never patched with an empty object), so re-verifying even one phase repairs the file. The ledger write is now atomic (temp file + rename, mirroring broken-windows.cts's writeLedgerAtomic/renameWithRetry shape) so this fix does not itself produce the corrupt files it now treats as security-relevant. Disclosed, accepted residual gap, pinned as an explicit test: deleting the ledger file itself (not just the report) still returns a workstream to the pre-adoption 'absent' state. This is inherent to any design where a wholly-absent store must be safe by default -- the alternative is gating every never-verified phase in every project on upgrade. Rail B is prospective only; a phase deleted before this fix shipped cannot be retroactively recovered. Also: dropped 'stale' from the Row 9 property test's REAL_STATUSES (it does not round-trip through readVerificationStatus as written, so including it claimed coverage the test did not have), converted two try/finally test bodies to t.after(), and added the remaining CONTRIBUTING.md fault-injection cases (broken symlink, missing parent directory, rename failure, temp-file cleanup). * fix(#2645): share the rollup winner selection to close a scoping gap BLOCKER: the ledger-winner selection in workstream-inventory.cts compared raw mtimes with no milestone-scoping filter, while the builder's own rollupDirByKey filters out-of-milestone directories before comparing. In a scoped workstream, a stale out-of-milestone duplicate-key directory with a newer mtime could win the ledger's selection while losing the builder's -- so deleting the LIVE directory's report never consulted the ledger, reopening #2645's hole for the phase that actually counts toward completed_phases, reachable with a plain rm. Extracted pickRollupWinners as the single shared implementation both rollupDirByKey and the ledger's winner selection now call, with the identical scoping filter -- two independent hand-written copies of "pick the winner" is what produced the divergence; one implementation makes the bug class structurally impossible rather than merely tested against. Added a unit-level proof (synthetic same-key entries with opposing inclusion/mtime) and an integration-level proof (a scoped workstream asserting an out-of-milestone verdict is never written into the ledger). Also: disclosed a third residual limitation in the changeset (editing a ledger entry by hand plants a permanent false verdict -- worse than deleting the ledger, since it looks like genuine history; not made tamper-proof, that's scope creep here); fixed a non-ENOENT lstatSync failure falling open to 'absent' instead of failing closed like every other path in that function; and fixed two test bugs a real gsd-test run caught -- Row 11's write-failure mock matched only the final ledger path, but the atomic-write refactor moved the real write target to a temp file, so the mock silently stopped intercepting anything and the test's own assertion caught its own staleness. * test(#2645): relabel Row 19 honestly and pin the lstat fail-closed fix Row 19 used two DIFFERENT phase keys (1-old, 2-new), so it never exercised the same-key collision the milestone-scoping blocker fix addresses -- the pre-fix, unshared ledger-winner code would have satisfied it too. Its docstring called it the integration-level proof of the blocker; it is not. Relabeled both rows accurately: Row 18 (synthetic same-key data) is now stated as the only row that proves the collision end to end, and Row 19 is described for what it genuinely covers -- a distinctly-keyed out-of-milestone phase's verdict never reaching the ledger, real coverage but not the collision case. Extensive probing (documented in 10-diagnosis.md) could not construct a natural directory-naming pair that shares a rollup key while diverging in milestone membership under the current roadmap-parser implementation, so the collision proof stays unit-level by necessity, not convenience. Also added a test pinning the lstat fail-closed fix: readVerificationLedger disambiguates a broken symlink (ENOENT from readFileSync) from genuine absence via a follow-up lstatSync call, and only lstatSync itself reporting ENOENT is proof of absence. A double-fault (readFileSync ENOENT, lstatSync a DIFFERENT code) cannot occur on a real filesystem, so it's monkeypatched directly -- without a test, a future edit could re-widen that catch back to "any lstat failure means absent" and fall open again silently. * chore(#2645): backfill changeset PR number to 3016 --------- Co-authored-by: sim <sim@local>	2026-08-02 23:43:32 -04:00
Tom Boucher	f1af47766a	chore(#1671): widen the when= grammar and key the section manifest per workflow — Phase 6.1 (#3013) ... * chore(#2992): widen the when= grammar and key the section manifest per workflow Epic #1671 Phase 6.1. Two blockers stopped the fragment model reaching any file beyond execute-phase.md: the when= vocabulary was frozen at 4 atoms (3 execute-phase-specific), and the section manifest was single-workflow by construction with 'execute-phase' hardcoded into buildSectionManifestField. - widen WHEN_VOCABULARY 4 -> 14 via a coordinated ADR-1671 amendment; the grammar stays CLOSED (one atom, no operators, negation or nesting) and WHEN_PREDICATES stays a hand-written literal map, never deriving a predicate from its atom string - InvocationFacts gains flags: ReadonlySet<string> plus three computed state booleans; add the missing reverse vocabulary/predicate parity guard - key the manifest artifact per workflow; a stale flat {sections:[...]} artifact now fails shape validation instead of being misattributed - wire the field into six init entry points and parse the flags each needs An atom ships only with both a real consuming section and a fact the init seam actually computes. Six surveyed atoms are withheld because their workflows have no dedicated init entry point; an atom without a computed fact evaluates false forever and silently disables its own section. Fixes a defect found while wiring: parseNamedArgs always materializes a boolean flag key, so folding its false into the absent sentinel is required or every flag reads as present and gating is silently always-on. Also resolves ADR-1671:194 by measurement: --mvp stays unmarkable, because its interleaved sites are always-run flag resolution and a ~340 byte block that already delegates lazily. Refs #2992 * fix(#2992): treat any falsy option value as an absent flag and reject unsafe manifest read paths Findings from two orthogonal reviews (Claude /code-review + an isolated adversarial pass); both independently reproduced the first one. - MAJOR: the flags-builder treated only `undefined` as absent, but parseNamedArgs yields `null` for an absent value-flag and `false` for an absent boolean-flag, so `--granularity` read as present on every plan-phase invocation. Fixed at the root: a flag is present iff its option value is truthy. The six per-handler `|| undefined` folds are now redundant and removed, which also closes the duplicate-translation and missed-onboard-handler findings. - MAJOR: state:needs-codebase-map had zero coverage. Added unit, property and real-CLI integration tests. - MINOR: reject absolute, UNC/drive and `..`-traversing `read` paths in the manifest, degrading the whole load to null like every other shape violation. Verified: `/etc/passwd` previously reached section_manifest.read. - MINOR: corrected a stale "4 to 20" doc comment; the vocabulary is 14. Refs #2992 * test(#2992): update the generator suite for the per-workflow manifest shape The remote matrix went red with 5 unique failures, identical on linux-node22 and linux-node24, all in tests/gen-section-manifest.test.cjs. Re-keying the artifact to {workflows:{...}} left this suite asserting the old flat {sections:[...]} shape; nothing else in the tree still does. - three tests read manifest.sections.length, now undefined; retargeted at workflows.<name> with their original intent preserved (a fenced or loop-host marker still asserts NO section is produced, not merely a changed count) - the stale-manifest test wrote its fixture in the OLD shape, so it tripped shape validation and stopped exercising staleness at all. Its fixture is now valid-but-mismatched so FAIL_STALE is genuinely reached again. - added the coverage that exposed: a pre-6.1 flat artifact must report FAIL_MANIFEST_MALFORMED_SHAPE. That is the real upgrade path for an installed tree and nothing covered it. Refs #2992 * chore(#2992): backfill changeset pr number to 3013 --------- Co-authored-by: sim <sim@local>	2026-08-02 22:36:45 -04:00
Tom Boucher	fd07e1a357	fix(#2850): resolve the active workstream in the statusline GSD-state segment (#3012) ... * test(#2850): add failing-first tests for workstream statusline state readGsdState only ever reads the flat .planning/STATE.md via a directory walk-up; it has no path for .planning/workstreams/<ws>/STATE.md and never consults GSD_WORKSTREAM or the stored active-workstream pointer, so the GSD-state segment silently disappears in workstream mode. These tests prove the RED before the fix lands. Uses shared saveSessionEnv/restoreSessionEnv/clearSessionEnv helpers now added to tests/helpers.cjs (single source of truth for the session-env-var save/clear/restore pattern also used by tests/active-workstream-store.unit.test.cjs, which is updated here to consume the same shared helpers instead of its own local, already-diverged copy). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2850): resolve active workstream in the statusline readGsdState only ever walked up looking for a flat .planning/STATE.md; it had no branch for .planning/workstreams/<ws>/STATE.md and never consulted GSD_WORKSTREAM or the stored active-workstream pointer, so the GSD-state segment silently vanished in workstream-mode projects with no root STATE.md (exit 0, no diagnostic). Reuses the existing CLI>env>store resolution seam (resolveActiveWorkstream, active-workstream-store.cts) and the existing mode-detection/path-building seam (listAvailableWorkstreams/planningPaths, planning-workspace.cts) rather than re-implementing either inline. When workstream mode is detected but nothing resolves, readGsdState now returns a {noActiveWorkstream:true} sentinel that formatGsdState/formatGsdStateCompact render as "no active workstream" -- observable, never silent emptiness. Flat-mode behavior and the case where a resolved workstream has no STATE.md yet are both unchanged. Adds active-workstream-store.cts's peekActiveWorkstream: a read-only sibling of getActiveWorkstream. resolveActiveWorkstream's default store lookup self-heals a stale/invalid pointer by deleting it (adapter.clear()) -- correct for a command, but not for a renderer invoked once per prompt, which must never mutate persistent, possibly cross-session state as a side effect of drawing a screen. The statusline now injects peekActiveWorkstream via resolveActiveWorkstream's own getStored override, keeping the env>store precedence itself fully reused while removing only the store tier's write side effect. This satisfies the issue's AC4 ("the fix is purely additive to what's displayed"). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2850): backfill changeset PR number to 3012 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-02 21:44:04 -04:00
Tom Boucher	97f2af29da	fix(#2852): isolate wave-cleanup blocks to their own entry (#3009) ... * test(#2852): add failing-first regression coverage for wave-cleanup isolation Adds the #2852 test matrix to executeWorktreeWaveCleanupPlan: per-entry block reasons must isolate to the blocked entry instead of aborting the rest of the wave, and a deletion must only block when another wave member's branch still touches the deleted path. These fail against the current implementation (RED) — the fix lands in the next commit. * fix(#2852): isolate wave-cleanup blocks to their own entry and scope the deletions guard to real dependents executeWorktreeWaveCleanupPlan aborted the rest of a cleanup wave on the first blocked entry (branch_mismatch, base_mismatch, worktree_dirty, merge_failed, etc.), dumping every remaining entry into `pending` untouched instead of evaluating it. Every per-entry block reason now isolates via `continue` instead of `break` + bulk pending push. The one exception is a failed --no-ff merge, which can leave repoRoot itself mid-merge: that path now attempts `git merge --abort` and only halts the remaining wave if the abort itself fails (an unrecoverable repo-level failure), matching every other block reason's isolation. The `branch_contains_deletions` guard also blocked any deletion unconditionally, even one nothing else in the wave depends on (the reported repro: folding a test file into a sibling and deleting the original). It now blocks only when another wave member's branch still touches the deleted path — computed lazily per wave so a run with no deletions pays no extra git call, and fails closed (still blocks) when a sibling's diff cannot be determined. * fix(#2852): eagerly cache each entry's own diff to fix an ordering bug in the deletions-overlap check The deletions cross-entry overlap check (previous commit) computed each "other" entry's touched-files set lazily, the first time some later entry's overlap check needed it. That is wrong: once an entry has already been merged earlier in the same loop pass, its branch becomes an ancestor of HEAD, and `git diff --name-only HEAD...branch` silently collapses to empty. A dependent entry that appears BEFORE the deleting entry in the manifest (and has therefore already merged by the time the deletion check runs) would be missed, letting a genuinely-depended-on deletion through undetected — a live violation of the negative-space acceptance criterion, caught by /code-review's Spec-axis before this shipped. Fixed by populating each entry's touched-files cache eagerly, during that entry's own turn in the loop, immediately before its own merge attempt (the only step that can move HEAD) — so every entry's diff is captured before it could possibly have been merged, regardless of manifest order. Adds a regression test reproducing the exact broken ordering (dependent merges first, then a later entry tries to delete the file it depends on). * refactor(#2852): extract shared git name-only line parser /code-review's Standards axis flagged duplicated parsing logic: `stdout.split('\n').map((l) => l.trim()).filter(Boolean)` appeared at both the per-entry deletion list and the cross-entry touched-files cache added by this fix. Extracted into parseGitNameOnlyLines(), used by both call sites, so the two can't silently drift apart. * revert(#2852): scope the fix to wave-isolation only, restore unconditional deletions guard #2852's own triage comment explicitly deferred the deletions-guard policy question as a separate product decision ("Policy/enhancement ask, not a defect ... Out of scope: deciding or implementing an opt-in mechanism for intentional deletions"). All four of the issue's actual acceptance criteria concern wave isolation only. The prior two commits on this branch built a cross-entry deletion-dependency heuristic that substituted a derived judgment for that deferred product decision — out of scope for a confirmed-bug fix. Reverts: getEntryChangedFiles, touchedFilesCache, the overlapUnknown fail-closed branch, parseGitNameOnlyLines, and the eager per-turn cache-population call. `branch_contains_deletions` now blocks unconditionally again (byte-identical trigger condition to pre-fix); the only change is `continue` instead of `break` + bulk `pending.push`, same as the other 7 block reasons. Keeps: the full wave-isolation fix (all 8 sites) and the merge_failed / git merge --abort recovery-and-carve-out, both squarely inside the issue's actual acceptance criteria. The deferred opt-in-for-intentional-deletions decision is filed as #3003, citing #2852's triage as origin. * refactor(#2852): extract blockEntry() helper to remove duplicated block-assembly across 8 sites /code-review's Standards axis flagged the repeated "result.status='blocked'; result.reason=...; result.stderr=...; results.push(result); ok=false;" shape at every one of the 8 per-entry block sites this fix touches. Extracted into blockEntry(), called at each site; each call site still owns its own continue/break decision. No behavior change. * fix(#2852): check actual repo state instead of git merge --abort's exit code The merge_failed recovery path decided "genuinely unrecoverable, halt the wave" based on whether `git merge --abort` itself exited successfully. That is not a reliable signal: git refuses many merges (e.g. "your local changes would be overwritten by merge") WITHOUT ever creating a MERGE_HEAD, in which case repoRoot's tree was never touched — but `git merge --abort` still fails with "There is no merge to abort (MERGE_HEAD missing)?" in that exact safe case. Trusting that exit code alone misclassified an ordinary per-entry merge failure as a repo-level one and stranded the rest of the wave — the exact defect #2852 exists to fix, reintroduced through the recovery path (caught in review). Fixed by checking repoRoot's actual state directly via `git rev-parse --verify -q MERGE_HEAD` after the abort attempt: MERGE_HEAD present means genuinely still mid-merge (unrecoverable, halt); absent means safe (isolate and continue), whether because no merge state was ever entered or because abort successfully cleared it. An unexpected git error or timeout degrades to the conservative "still mid-merge" answer rather than throwing or guessing. Rewrites the "unrecoverable merge_failed" test, which previously used the safe "There is no merge to abort" string as its unrecoverable example — that pinned the defect as correct behavior. Adds the missing case: an ordinary merge_failed that never entered a merge state must not abort the wave. * test(#2852): cover repoRootStillMidMerge's fail-closed branches /code-review flagged that the two conservative fail-closed branches of repoRootStillMidMerge (a timeout on the post-abort MERGE_HEAD check, and an unexpected non-0/1 exit code such as a fatal git error) had no test coverage — exactly the branches most likely to hide a mutation survivor (e.g. a flipped `timedOut` check or a flipped final `return true`). Adds both cases: each must halt the wave (fail closed) rather than assume repoRoot is safe when its state cannot be verified. * chore(#2852): backfill changeset PR number to 3009 --------- Co-authored-by: sim <sim@local>	2026-08-02 19:51:43 -04:00
Tom Boucher	67e2ff7b25	fix(#2855): scope the phase-locator archived-milestone fallback to the active workstream (#3008) ... * test(#2855): add failing-first regression test for cross-workstream archive leak Covers findPhaseInternal/getArchivedPhaseDirs in src/phase-locator.cts resolving a pending workstream phase to an unrelated workstream's (or flat-mode's) archived phase because the archive fallback hardcodes the project-root .planning/milestones/ tree. Fails against the current implementation; the fix lands in a follow-up commit. * fix(#2855): scope phase-locator archived-milestone fallback to the active workstream findPhaseInternal and getArchivedPhaseDirs in src/phase-locator.cts hardcoded the project-root .planning/milestones/ tree when falling back to search archived phases, ignoring GSD_WORKSTREAM. A pending phase in one workstream whose own phases/ directory didn't exist yet would silently resolve to a same-numbered phase archived under an unrelated workstream's (or flat-mode's) history, complete with stale plan/summary counts and an archived status. Route the archive fallback through planningDir(cwd) instead — the same workstream-aware helper the active-phase search (three lines above) and the archive-write path (archivePhaseDirectories in milestone.cts) already use. Flat/non-workstream projects are unaffected: planningDir(cwd) with no GSD_WORKSTREAM resolves to the same root .planning path as before. Also switch the reported relBase/basePath from a hardcoded '.planning/milestones/...' literal to path.relative(cwd, archivePath), so the paths returned to callers stay consistent with wherever the archive actually resolved to (root or workstream-scoped). * chore(#2855): add changeset for phase-locator workstream archive fix * fix(#2855): normalize getArchivedPhaseDirs basePath to posix separators Orthogonal code-review finding: findPhaseInternal's relBase/directory field was explicitly toPosixPath-normalized, but getArchivedPhaseDirs's basePath used a bare path.relative() call, leaving it native-separator on Windows — an inconsistency between two sibling "relative path from cwd" report fields introduced by the same #2855 fix. Wrap basePath in toPosixPath to match, and update the two existing assertions that compared basePath against path.join output (which would break on Windows now that the field is guaranteed posix) to compare against forward-slash literals instead, matching how the sibling `directory` field is already asserted elsewhere in this suite. * refactor(#2855): share archive-directory resolution between findPhaseInternal and getArchivedPhaseDirs Orthogonal code-review finding: the two functions carried independent copies of the same resolve-milestonesDir-then-enumerate-archive-dirs logic — the exact shape that let the original #2855 bug (hardcoded root path) exist in one copy while the workstream-aware active-phase search sat three lines above it. Extract listArchiveVersionDirs(cwd) as the single seam both functions now consume, so a future change to how the archive tree is located only needs to happen once. Byte-for-behaviour preserved: readSubdirectories and searchPhaseInDir already self-contain their own try/catch and never throw, so moving the iteration outside the old inline try block changes nothing observable (verified via manual repro scripts covering leak prevention, positive resolution, flat-mode parity, and multi-milestone reverse-sort ordering). * test(#2855): demonstrate ROADMAP.md presence does not affect the archive-leak guard Orthogonal code-review (spec axis) finding: issue #2855's AC1 states the guard must hold "regardless of whether workstream A's roadmap already lists the phase and when it doesn't yet" — an explicit two-value dimension that had no direct test coverage; it was only inferable by reading findPhaseInternal's source and confirming it never touches ROADMAP.md. Add a parametrized test creating the workstream's ROADMAP.md with and without a matching Phase heading, asserting the archive-leak guard resolves identically (null) either way. * chore(#2855): backfill changeset PR number to 3008 --------- Co-authored-by: sim <sim@local>	2026-08-02 19:24:53 -04:00
Tom Boucher	51f32d2d40	fix(#2658): detect trae runtime and resolve its instruction file to a concrete rules file (#3006) ... * test(#2658): add failing-first regression for trae runtime detection and instruction path Covers all three collided defects reported in #2658 plus a fourth instance of defect 1 (ingest-docs.md) found while diagnosing it: missing trae detection in workflow runtime-detection blocks, the CLAUDE.md path-mutilation bug in both the js/cjs and md install-time converters, and the missing projectInstructionFile capability declaration. Fails against current source; the next commit fixes it. * fix(#2658): detect trae runtime and resolve its instruction file to a concrete rules file Three defects collided to produce the reported ".claude/.trae/rules/" path: 1. new-project.md and ingest-docs.md's runtime-detection blocks only recognized codex/gemini/opencode and fell through to RUNTIME=claude for trae. Both now recognize the /.trae/ execution-context path and the TRAE_CONFIG_DIR env var before the claude fallback. 2. RUNTIME_CONTENT_DISPATCH.trae.js (bin/install.js) replaced bare "CLAUDE.md" before the ".claude/" prefix was handled, mutilating ".claude/CLAUDE.md" into ".claude/.trae/rules/". Now replaces the full ".claude/CLAUDE.md" path first, and targets a concrete file. 3. convertClaudeToTraeMarkdown (mirrored in bin/install.js and src/runtime-artifact-conversion.cts per the #2094 output-parity test) had the same class of bug with a different wrong output (".trae/.trae/rules/", from its generic ".claude/" rewrite firing after the bare CLAUDE.md rewrite). Both mirrors now match full-path forms before the bare/generic patterns, converging on the same concrete file as the js/cjs converter. 4. capabilities/trae/capability.json didn't declare hostBehaviors.projectInstructionFile, so getProjectInstructionFile fell through to the generic AGENTS.md default even when RUNTIME=trae was resolved correctly. Now declares ".trae/rules/rules.md", regenerated into gsd-core/bin/lib/capability-registry.cjs via npm run gen:capability-registry. Closes #2658 * chore(#2658): add changeset * fix(#2658): preserve arbitrary runtime-dir prefixes in the trae path rewrite Found by the end-to-end --trae install regression test (not by static trace) across two verification runs: 1. copyWithPathReplacement runs a generic ~/.claude/, $HOME/.claude/, and ./.claude/ -> runtime-dir rewrite on every .md file BEFORE calling convertClaudeToTraeMarkdown. The prior fix's .claude/CLAUDE.md-specific patterns never fire on that already-rewritten text, and the bare fallback still doubled whatever prefix the generic pass substituted. A first attempt handled only the fixed "./.trae/" shape and missed the $HOME/.claude/ and ~/.claude/ forms gsd-core/workflows/profile-user.md actually uses, which post-rewrite become an arbitrary absolute local-install-root path, not the fixed relative shape. Fixed with a prefix-preserving pattern that captures whatever precedes a ".trae/" tail and fixes only the filename suffix, instead of assuming one fixed shape. 2. The fix's own explanatory comments literally spelled out the malformed strings and the instruction filename as contiguous text. Since these two files ship verbatim into local --trae installs, where they are themselves run through the same find/replace, the comments got "fixed" right along with the real code, leaking the malformed string into the installed tree. Rewrote every comment in both mirror copies to never spell either the instruction filename or a malformed shape as one contiguous token. Adds an emitted-drift-ack fragment: the corrected replacement target for every CLAUDE.md mention (bare directory -> concrete file) changes trae-emitted output for every repo file that mentions CLAUDE.md, not only the ones that hit the originally reported bug. * test(#2658): extend parity test with arbitrary-prefix .trae/ inputs The bin/install.js vs runtime-artifact-conversion.cjs parity assertion for convertClaudeToTraeMarkdown only fed the pre-existing bare .claude/CLAUDE.md input through both implementations. Feed the prefix-preserving cases (relative, nested-absolute, tilde, $HOME, backtick-wrapped) plus a property-based check through both, so a future edit to only one copy of the .trae/-tail regex fails this test instead of silently diverging. * chore(#2658): backfill changeset PR number to 3006 --------- Co-authored-by: sim <sim@local>	2026-08-02 18:29:47 -04:00
Tom Boucher	a987cf2731	chore(#2932): emit a per-invocation section manifest from the init bundle (#2987) ... * chore(#2932): emit a per-invocation section manifest from init Extends the init bundle with a typed per-invocation section manifest so an invocation loads only the branch guidance it will actually take. The three flag/state-gated branches in execute-phase.md move into their own step files; the parent keeps its gsd:section markers wrapping a one-line on-demand reference, so each section's prose lives in exactly one file and the parent shrinks 93369 -> 89507 bytes. A new drift-guarded generator derives the shipped section manifest from those markers, and a new pure evaluator maps invocation facts to applicable section ids. The evaluator is a lookup over the frozen WHEN_VOCABULARY, never a parser (Greenspun's Tenth Rule, ADR-1671:69); a parity test asserts the vocabulary and the predicate map stay exhaustively in sync. Closes #2932 * fix(#2932): fail closed on prototype-chain when values An isolated adversarial review found WHEN_PREDICATES[section.when] was a bracket lookup on a plain-prototype object, so inherited Object.prototype members resolved as predicates: "constructor"/"toString"/"valueOf"/ "hasOwnProperty" returned truthy and SILENTLY INCLUDED the section, and "__proto__" threw an untyped TypeError carrying no .reason. Both violate the module's documented fail-closed contract, and the manifest is read from disk at run time so it cannot be assumed trustworthy. Builds the predicate map on a null prototype and guards the lookup with an explicit Object.hasOwn check. Adds table-driven coverage for nine Object.prototype-shaped keys asserting the TYPED reason (asserting only that it throws would still pass while broken) plus a fast-check property injecting a hostile value at an arbitrary document position. * test(#2932): retarget execute-phase step assertions at extracted step files * fix(#2932): emit typed reasons for generator lib-load and write failures * fix(#2932): restore launcher preamble in extracted steps and refresh derived fixtures * chore(#2932): backfill changeset pr number to 2987 --------- Co-authored-by: sim <sim@local>	2026-08-02 12:34:41 -04:00
Adnan	137f3fbb9c	fix(#2562): scope workstream progress/status to the current milestone (#2588) ... * fix(#2562): scope workstream progress/status to the current milestone `workstream progress` / `workstream status` / `workstream list` share one derivation that could report a workstream's CURRENT milestone as "milestone complete" / 100% while phases in that milestone were unstarted, in progress, or failing verification. Three coupled defects: 1. The shipped signal was project-lifetime, not milestone-scoped: workstreamMilestoneShipped() returned true if ANY *-ROADMAP.md snapshot existed or "SHIPPED" appeared anywhere in ROADMAP.md. Every prior shipped milestone leaves a permanent collapsed <summary>✅ … SHIPPED</summary> block, so any post-v1.0 workstream was pinned to "milestone complete" forever (over-correction from #1913). 2. The denominator dropped declared-but-unscaffolded phases, and completed PRIOR-milestone phase directories inflated the numerator, letting progress_percent round to 100 while real work remained. 3. Phase completeness ignored the VERIFICATION verdict — SUMMARY >= PLAN count alone marked a phase complete even with a human_needed verdict. Fix: derive both numerator and denominator from artifacts scoped to the current milestone. The current version comes from the workstream STATE.md `milestone:` field (ROADMAP in-progress markers can be stale); the ROADMAP `## Progress` table maps every phase — including dirless ones — to its milestone, and the matching set is both the denominator and the directory membership filter. The shipped signal now requires the CURRENT version's archived ROADMAP snapshot (REQUIREMENTS snapshots are not accepted; they can be written at milestone start) or the current milestone's own line marked shipped. Phases with an explicit failing verdict (gaps_found/human_needed) count as in_progress; missing/unknown/stale are left untouched so verifier-disabled projects do not regress to never-complete. Greenfield roadmaps with no versioned Progress table, and projects whose current version cannot be determined, keep the prior behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#2562): add changeset for workstream milestone-scoping fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(#2562): parse the Progress table via findTableWithColumns The ad-hoc pipe-table regex tripped the local/no-adhoc-markdown-parsing ESLint rule. Use the canonical markdown-table helper instead: the milestone-grouped RoadmapProgress variant is located by its required `Phase` + `Milestone` columns and cells are addressed by column NAME, so the parser tolerates column reordering and injected columns. The `flat` variant (no Milestone column) yields no attribution, which is the intended fallback to legacy counting. Behaviour is unchanged: verified against a real multi-workstream project (same status/percent/phase and plan counts before and after). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2562): count table-only phases in the unscoped denominator Addresses the reporter's repro detail: a phase declared as a `## Progress` table row with no `### Phase N` heading is missed by countRoadmapPhases EVEN WHEN other headings exist — the heading regex counts 1 for a "1 heading + 1 table-only" roadmap — not just in the zero-heading fallback path. Milestone scoping did not cover this, because a flat Progress table (no Milestone column) carries no per-phase attribution, so greenfield and single-milestone projects kept the old heading-only denominator and the declared phase silently vanished from it. When milestone scoping cannot engage, the denominator is now the union of the Progress table's declared phase numbers and the phase directories, so neither source can shrink it. Verified against the reporter's minimal fixture (phase 1: 1 PLAN + 1 SUMMARY + gaps_found; phase 2: table row only, no heading, no dir), which now reports 0/2 at 0% across all four table/STATE permutations instead of 1/1 at 100%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2562): attribute dir-only sub-phases to their parent's milestone A sub-phase directory inserted mid-milestone (e.g. `30.1-…` under a table-declared phase 30) usually has no ROADMAP Progress-table row, so it had no milestone attribution and was scoped out of the rollup entirely — its completed work was invisible and it could never hold the percentage below 100. It now inherits its parent phase's milestone and joins BOTH sides of the calculation. Both sides is the load-bearing part: adding it to the numerator alone would let completed_phases exceed a denominator that never counted it, cap back to 100% via Math.min, and reintroduce exactly the defect this issue reports. A regression test pins that failure mode (all declared phases complete + an in-progress dir-only sub-phase → 75%, not 100%). Attribution is deliberately one-directional: a sub-phase counts only when its PARENT is in the current milestone, so a follow-up created in a later milestone under an older parent is excluded rather than misattributed — conservative (under-count) rather than falsely inflating. Verified on a real project: the reported workstream moves from 2/6 (33%) to 3/7 (43%), the 3/7 being the honest figure — a completed sub-phase that was previously invisible now counts, and so does its plan total. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#2562): describe the denominator + sub-phase fixes in the changeset The fragment was written at the first commit and only covered the three original defects. Bring it up to date with what actually ships: the table-only-phase denominator union (heading-only counting dropped a declared phase even when other headings existed) and sub-phase milestone inheritance across both sides of the calculation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(#2562): promote the canonical phase-key surface to phase-id state.cts kept `phaseKeyFromToken`/`phaseKeyFromDir` private, so every other module that had to compare two independently-derived phase references — a ROADMAP table cell against a phase directory, say — wrote its own regex. That is the defect class #2562 reports: a padded `01` and an unpadded `1-slug` land in different key spaces and the comparison silently yields nothing. Move the pair to the phase-id owner module and add `phaseKeyFromProse` (for ROADMAP/STATE prose, markdown emphasis stripped) and `parentPhaseKey` (a sub-phase's parent). state.cts imports them; its call sites are unchanged. * fix(#2562): own milestone-shipped detection and accept a workstream scope Three changes to the module that owns milestone parsing, so its consumers stop reimplementing it: - `isMilestoneShippedInRoadmap(content, version)` answers "does the ROADMAP mark THIS milestone shipped" from heading and `<summary>` lines only. A bullet that merely names the version (`- [x] 03-01: ship the v2.0 login endpoint`) is prose about a phase, not a milestone verdict. The version token is boundary-matched with `(?![\w.-])` — `\b` does not bound it, since `.` is a non-word character, so a shipped `v2.0.1` heading would otherwise close `v2.0`. - `extractCurrentMilestone` and `getMilestonePhaseFilter` take an optional trailing workstream name and thread it to `planningDir(cwd, ws)`. A caller iterating workstreams cannot set `GSD_WORKSTREAM` per iteration, which is what the existing resolution falls back to. Omitted, resolution is unchanged. - `getMilestonePhaseFilter` exposes `versionScoped`, true only when the phase set really is one milestone's. On an unversioned roadmap `phaseCount` spans the project's lifetime and must not be read as a current-milestone denominator. The closed/active milestone-marker patterns were kept in three byte-identical copies; they are hoisted to module scope as one `isClosedMilestoneHeading`. * fix(#2562): derive membership and denominator from one phase-key space The milestone scoping added earlier in this PR derived the ROADMAP table key and the phase-directory key with two different regexes, and dropped rows it could not attribute. Each of those was another way to reproduce the symptom this issue reports — a rollup contradicting its own `phases[]` listing: - a padded `| 01. … |` row never matched a `1-slug` directory (and a bespoke `^0*(\d+…)` never matched `PROJ-05-…` at all), so phases fell out of the milestone entirely and the percentage collapsed or pinned; - a blank or malformed Milestone cell deleted the phase from BOTH sides, letting an unstarted phase vanish and the remainder round to 100%; - shipped detection scanned bullets, so any checkmarked line naming the version closed the milestone; - the numerator counted per-directory while the denominator counted distinct phases, so a stale same-numbered directory (Bug #2445's scenario) pushed `completed_phases` past the denominator, where `Math.min` capped it to 100% and hid the unstarted phase. Both sides now key off the phase-id owner module (`phaseKeyFromDir` / `phaseKeyFromProse`), directory membership additionally consults `getMilestonePhaseFilter` when that filter is genuinely version-scoped, and the denominator is the union of the roadmap's declarations with the member directories' keys — so `completed_phases <= denominator` holds by construction. The Builder asserts it and throws; the `Math.min` cap survives only on the legacy unscoped path, where the denominator is a heading count that cannot bound the numerator. An unattributable row degrades over-inclusively (kept, never dropped), matching the degrade direction roadmap-parser already commits to. * test(#2562): boundary coverage for each milestone-scoping reproduction One test per way the scoping could still report "milestone complete"/100% while phases are incomplete: zero-padded rows vs padded dirs (and the mirror), project-code-prefixed dirs, a blank/malformed Milestone cell, a checkmarked bullet naming the version, a shipped `v2.0.1` heading against a current `v2.0`, and a stale same-numbered directory. Plus the current milestone's own shipped heading (the signal must survive the boundary fix), the Builder's numerator-above-denominator throw, a parity check that every non-`passed` verifier status blocks completeness, and a guard that scoping reads the workstream's ROADMAP rather than the project root's. Reverting only `src/` reddens six of them. * docs(#2562): record the milestone-scoped semantics and its consumer impact CONTEXT.md: the Workstream Inventory Module's completion fields now describe the current milestone, not the workstream's lifetime; phase-id owns the canonical phase-key surface; roadmap-parser owns milestone shipped/active classification and takes an optional workstream scope. Changeset: name the behaviour change explicitly — `roadmap_phase_count`, `completed_phases` and `progress_percent` change meaning with no schema signal, and `getOtherActiveWorkstreamInventories` filters on the derived status, so consumers see real movement. * fix(#2562): collapse every zero-padding spelling to one phase key A property test over the key surface — table cell and directory decorated INDEPENDENTLY, which is the point — found a divergence neither review named: `padStart(2, '0')` is a no-op once the input is already ≥2 characters, so `5` normalised to `05` while `005` stayed `005`. A `| 5. … |` row and a `005-slug` directory therefore never compared equal, which is the same failure mode as the padded-vs-unpadded blocker, one level down. The strip belongs in `phaseKeyFromToken`, not in `normalizePhaseName`: applying it to the latter regressed multi-decimal leading-zero plan IDs (`001.10-PLAN.md` capture + wave assignment), which rely on its verbatim rendering. Confining it to the key surface fixes the comparison and leaves rendering untouched. Also tightens `isMilestoneShippedInRoadmap`'s patterns to anchored, complementary character classes so an untrusted ROADMAP cannot drive backtracking, and makes the project-code test discriminating — it previously passed pre-fix, because an unresolvable key collapsed scoping to the whole roadmap and happened to land on the same number. It now carries a prior-milestone directory that a collapse would wrongly admit. * fix(#2562): prefer the milestone-attributing Progress table; pin the seams Three gaps the earlier self-check missed: - Both RoadmapProgress variants carry a `Plans Complete` column, so probing it first picked a FLAT table appearing earlier in the document over the milestone-grouped one that actually carries the attribution. Every row came back unattributed, was treated as current-milestone, and silently re-admitted prior-milestone phases. The attributing shape is probed first; flipping the order reddens the new test. - `lint-phase-id-drift` exempts phase-id.cts by design, so it is silent on `phaseKeyFromToken`'s own segment strip by construction — not evidence. Its interaction with `stripProjectCodePrefix` (which runs AFTER) is pinned across project codes and hyphenated ids, including the pre-existing `M1-46-6` vs `M1-46-6-rs` asymmetry, which is `extractPhaseToken`'s #2043/#2232 slug-word rule and not something to "fix" by accident. - `listWorkstreamInventories` loops every workstream with no try/catch, so a REACHABLE Builder-invariant throw would take down `workstream list`/`status`/ `progress` for all of them. The invariant test only exercised the pure Builder with hand-built inputs. A test now drives `inspectWorkstream` over every adversarial shape at once (prior-milestone dirs, three colliding duplicates, a dirless declaration, an unattributed row, a project-code prefix, a dir-only sub-phase) and asserts it does not throw and the invariant holds — so the throw stays a contract assertion for external callers, not a runtime path. Also covers the active-marker-wins rule (`## v2.0 — 🚧 IN PROGRESS … ✅` must not mark shipped), which nothing exercised. * fix(#2562): scope a declared-but-empty current milestone instead of falling back to history The review's open MAJOR. `STATE.md`'s `milestone:` field updates the moment `/gsd-new-milestone` writes the heading, while the `## Progress` table and phase sections land later. In that window nothing attributes a phase to the current milestone, `scoped` went false, and the fallback counted the project's ENTIRE phase history as both numerator and denominator — a milestone with zero work done reported 100% off its predecessors'. That is #2562's own symptom reached by a different precondition, and none of the 16 tests covered it. Reproduced first, four ROADMAP shapes, at `inspectWorkstream` rather than the Builder — the Builder takes the scoping decision as an input, so a Builder-level test proves it honours a flag, not that the derivation sets it. Three of the four reported 2/2 100% with no phase of the current milestone begun. Which signal witnesses the state depends on the ROADMAP's shape, and no single one covers all three: - `## v3.0` exists but declares no phases. `getMilestonePhaseFilter` DOES locate the section and sets `versionScoped`, then the zero-phase pass-all degrade resets it to false — erasing the only evidence the milestone exists. Neither existing flag survives that path, so this adds `versionSectionFound`, set beside `versionScoped` and deliberately preserved through the degrade. - No section for this version at all, in a roadmap that versions its others — the existing `missingExplicitVersion`, already exposed and tested. - Unversioned headings, but a Progress table attributing every row elsewhere: neither filter flag fires and the table is the only witness. A ROADMAP that attributes NO versions anywhere matches none of them, which is the point. Its rows parse with `version: null`, land in `currentMilestoneKeys`, and never reach the new branch. `readCurrentMilestoneVersion` returns a non-null version for very nearly every project (`getMilestoneInfo` defaults to `v1.0`), so keying off `currentVersion` alone would have zeroed out every free-form legacy project — the condition looks fussy for that reason. A test pins it. Within an empty milestone, membership inverts: a directory belongs unless another milestone's row claims it. Excluding everything would have dropped a phase scaffolded before the roadmap caught up from BOTH sides of the rollup, and hiding real work is the same class of defect as inventing it — this codebase degrades over-inclusive, never under. Scoping is now stated by the caller (`milestoneScoped`) rather than inferred from `currentMilestonePhaseCount > 0`. That inference was the root cause: it cannot represent a milestone that is scoped AND legitimately zero-phase, so the Builder read "no phases yet" as "no scoping" and reopened the whole-history path. The count-derived value stays the default for callers that say nothing. A regression test also pins that a zero denominator does not trip the Builder's `completed_phases <= denominator` throw, since `listWorkstreamInventories` has no try/catch and a crash on every freshly-declared milestone would be worse than a wrong percentage. The changeset and CONTEXT.md no longer claim membership is derived in "ONE" / "a SINGLE" phase-key space. `getMilestonePhaseFilter` still runs its own `normalizePhaseIdSegments`; the signals are OR'd so a divergence can only widen membership, but two normalisers coexist and the docs now say so. * fix(#2562): cross-validate the shipped marker against the milestone's artifacts `status: "milestone complete"` was asserted from the shipped marker alone, so a single payload could report it beside `progress_percent: 67` — this issue's own symptom, reached through `status` rather than the percentage. The marker is now a claim checked against the milestone's own artifacts, and the two signals are checked at DIFFERENT strengths because one check cannot serve both. A `heading` marker (operator-typed, live ROADMAP) is refused on a short completion ratio, which also catches phases declared but never scaffolded. A `snapshot` marker is NOT ratio-gated: `milestone complete` moves the milestone's phase dirs into `milestones/<version>-phases/` (milestone.cts:755-762) while copying — never truncating — the live ROADMAP (:671-674), so a correctly archived milestone reads 0/N by construction and a ratio gate would strip `milestone complete` from every archived milestone. It is refused instead when an in-milestone phase dir is still live and unfinished, reachable because `milestone complete` does not advance STATE's `milestone:` field (state-transition.cts:1335 vs :1224). `legacy` stays ungated — only reachable when scoping is off. A refused marker does not fall through to a STATE field claiming the same thing; against contradicting artifacts neither source may report completion. The refusal surfaces as `milestone_shipped_unverified` rather than staying silent, distinct from `status_conflict` (derived-vs-field only). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * test(#2562): pin both marker strengths, the archived guard, and the owner modules workstream-inventory: four tests, all four red against the prior src and green with it. A live-ROADMAP SHIPPED heading over an incomplete milestone is refused; an archived snapshot SURVIVES its phase dirs being moved out (the regression the obvious single ratio-gate would cause — swapping the snapshot branch to that gate reddens this AND the pre-existing `CURRENT-version snapshot marks the milestone complete` at :321); an archived snapshot is refused once a phase is reopened under it; and a refused marker is not re-asserted by a STATE field claiming the same. roadmap-parser: `isMilestoneShippedInRoadmap` gets unit coverage at its owner module rather than only through the inventory that consumes it, plus two characterisation tests for `getMilestonePhaseFilter`'s legacy call surface — omitting the new trailing `ws` param is indistinguishable from `undefined`/`null`, and the `GSD_WORKSTREAM` env fallback still resolves. These characterise the call surface; they do not stand in for coverage of its individual callers. phase-id: the `phaseKeyFrom*` / `parentPhaseKey` one-key-space contract, incl. a property that padding a directory number never changes its key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * docs(#2562): record the two-strength shipped cross-check + milestone_shipped_unverified Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * fix(#2562): refuse an archived snapshot on a DIRTY archive, not just a live-unfinished dir The snapshot arm checked `liveIncompletePhases > 0`, which misses the shape @davesienkowski reproduced: a COMPLETE live dir beside a phase declared in the Progress table with no directory. Nothing is live-and-unfinished, the marker sails through, and `cmdWorkstreamProgress` returns `{"status":"milestone complete","progress_percent":50}` — the reported symptom verbatim, from one payload. Reproduced at 483a3ba30 before changing anything. His diagnosis is the right one and better than mine: an in-milestone directory outliving the archive means the archive is not CLEAN, and once that is true the completion ratio is meaningful again. So the check is the conjunction — any live in-milestone dir AND `completedPhases < effectivePhaseCount`. That strictly subsumes the old predicate (an incomplete member dir is in the denominator and not the numerator, so the ratio is always short when one exists) and leaves the clean-archive guard green, since a clean archive has no live dirs at all. Also corrects the module comment: the `scoped &&` prefix ungates all three signals, not just `legacy`. That is correct behaviour — unscoped, the denominator is the whole-roadmap count and membership is everything, so there is no current-milestone artifact set to check a current-milestone claim against — but the comment claimed otherwise. And the `milestone.cts` citations were ~28 lines stale after the rebase; they are now :700-702 (copy) and :783-790 (move), re-verified against this head. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * fix(#2562): project milestone_shipped_unverified from list, status and progress The inventory carried the field and every renderer dropped it — `workstream.cts` was not in this PR's diff at all — so at the CLI a refused marker looked exactly like no marker: a fallback `status` and nothing saying one was seen and rejected. That is the silent collapse this issue is about, reintroduced one layer up, and it made the changeset's "visible rather than silent" claim false at every surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * test(#2562): pin the dirty-archive shape and the CLI projection Five tests, all five red against the prior src and green with it. The reviewer's repro at the builder: an archived snapshot with a COMPLETE live dir beside a dirless declared phase must be refused, and status must not contradict the percentage. Four at the CLI via runGsdTools, the surface that was dropping the field rather than the builder that already had it: `workstream progress`/`status`/`list` each project `milestone_shipped_unverified: true` for that workstream, and a clean archive still reports `false` with `status: "milestone complete"`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * docs(#2562): correct the snapshot check, the scoped-only caveat and the CLI claim Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-01 23:06:43 -04:00
0xdhx	f4d6747d21	fix(#2738): report graphify query budget outcome and stop between-tier over-trimming (#2819) ... * fix(#2738): report budget outcome from graphify query and stop over-trimming between tiers applyBudget retains seed nodes unconditionally, so the seed set is a floor the edge-tier reduction cannot go below — a --budget 500 request could return the full seed payload (~119k tokens measured) with no signal of the miss. Add budget_met + budget_estimate to the budget result and surface them through graphifyQuery when a budget was requested. Secondary: the tier loop estimated against the full pre-filter node set, so a tier removal that already satisfied the budget (once its orphaned nodes were excluded) still triggered the next, higher-confidence tier drop. Recompute reachability and the estimate after each tier and break as soon as the pruned result fits. Adjacent same-class instance from pre-submit review: the CLI forwards --budget 0 but truthiness checks silently treated it as no budget and returned the unbounded result. Test budget presence with != null so a zero budget is honored and reported as an unmeetable miss. * docs(changeset): backfill PR number for #2738 fragment * fix(#2738): estimate the payload as emitted, not a private compact form budget_met measured a different payload than the caller receives. The estimator serialized a compact `{nodes, edges}`, while output() emits the whole response pretty-printed (2-space indent, plus the term/total_*/trimmed wrapper keys). Measured on the repo's own SAMPLE_GRAPH fixture: reported 183 tokens against 302 actually emitted — 1.65x — so `--budget 200` returned budget_met: true while handing back 302 tokens. That is worse than the old silent miss: an automated consumer stops checking a signal that is confidently wrong. Fix the basis rather than the number: - io.cts gains serializeForOutput(), the single definition of the wire form. output() now calls it, so the estimator and the emitter cannot drift on indentation or shape. Pure extraction; output()'s behaviour is unchanged. - graphify builds its response through one buildQueryResponse() used by both the emitter and the estimator, so the estimate describes exactly the bytes returned. - The tier loop estimates on that same basis, so it keeps trimming until the real payload fits instead of stopping at a smaller internal measure. This makes budget_met === (budget_estimate <= budget) true by construction. - Drop the module-private chars/4 helper for prompt-budget's estimateTokens — the repo's single token scale, per the rule phase-estimation.cts documents. budget_estimate is self-referential (its own digits are part of the emitted bytes), resolved by iterating to a fixed point; the sequence only ever grows, so it settles in a couple of passes and errs toward over-reporting. Tests pin estimator to emitter so this cannot silently re-diverge if output() ever changes its indentation. Both new tests fail against the pre-fix source. * test(#2738): property-test the budget-limit and reporting contract RULESET.TESTS.property-based-testing names budget-limit contracts, and this module is the literal case: #2819 turns it into a *reporting* contract, which is what properties express well. Five invariants over arbitrary small graphs and any budget >= 0: - budget_met === (budget_estimate <= budget) - budget_estimate === the tokens actually emitted - the seed set is a floor the reduction never goes below (the changeset's "seeds are a floor" claim, previously asserted only for one hand-built fixture) - total_nodes/total_edges match the returned arrays - a larger budget never yields a smaller payload Two notes on what these do and do not prove. The emitted-payload property fails against the pre-fix source; the budget_met/budget_estimate agreement property does NOT — pre-fix both derived from the same wrong number, so it is a contract guard, not a regression proof. Monotonicity is asserted over the payload (node/edge counts), not over budget_estimate: the estimate measures emitted bytes exactly, and budget_met renders as "false" (5 chars) or "true" (4), so an identical payload can measure one token larger when the budget is missed. That is the estimate being honest, not a monotonicity break. The generator seeds on `label` — seedAndExpand matches label/description, never id/name, and a fixture that gets this wrong expands to nothing and passes vacuously. * test(#2738): pin the budget boundary and label the forward-guard test Two test-coverage gaps from review. RULESET.TESTS.boundary-coverage wants limit-1 / limit / limit+1. The added tests used 1, 0, 200, 100000, 50 — all far from the decision point. The branch that matters is `estimate <= budgetTokens`, so the input that decides it is budget === estimate exactly: that is the one value where an off-by-one in the comparison flips budget_met, and nothing else in the suite would catch it. Pinned at the limit and either side of it. The "omits budget fields when no budget was requested" test passes unchanged on next — graphifyQuery never set those keys before the fix, so both assertions already held. It has value as a forward guard against the spread leaking budget fields, but it is not failing-first and should not be counted toward RULESET.TESTS.regression-must-fail-first. Said so in a comment, so a later reader does not mistake it for the regression proof. * fix(#2738): keep a non-finite budget out of the budget path Switching `!budgetTokens` to `budgetTokens == null` widened the internal contract to admit NaN, where every `estimate <= NaN` is false: the loop strips all three tiers and returns a seeds-only payload that is indistinguishable from a legitimate aggressive trim. Unreachable through the CLI — graphify-command-router rejects a non-numeric --budget with makeInvalidArgs before graphifyQuery is called — so this is hardening, not a live defect. It is still worth guarding: graphifyQuery and applyBudget are module-level entry points a future caller could reach without the router's validation, and the failure mode is silent. Number.isFinite also routes Infinity to the no-budget path, deliberately: an unbounded budget is not a budget, and parseInt cannot produce one anyway. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-01 21:36:48 -04:00
Adnan	fc3fde05ee	feat(#2646): surface unresolved deferred-items.md at milestone close (#2983) ... * feat(#2646): surface unresolved deferred-items.md at milestone close auditOpenArtifacts scanned eight categories; deferred-items.md was not among them. #2287 made that file readable at the PHASE boundary (audit-uat, /gsd-progress check 7), but one boundary up it stayed invisible — and phase directories archive to milestones/vX.Y-phases/ by default (#1871), so an out-of-scope discovery a phase agent correctly recorded rather than fixed left the live tree at milestone close having never reached the [R]/[A]/[C] prompt that exists to catch exactly this. Adds deferred_items as a ninth scanner plus its count, its items entry and its report section. The workflow needed no change: complete-milestone branches on "any section with count > 0", so the new category flows through the existing prompt. The resolved/unresolved predicate is NOT reimplemented. uat.cjs already exports parseDeferredItems, which owns the parsing rule (entries under a `## Deferred Items` level-2 heading, else the whole file fail-safe; RESOLVED only on an explicit case-insensitive `status: resolved` field). The scanner requires it lazily, inside the scan, so audit-command-router's property that a route never loads the module it does not need is preserved. Two readers of one file sharing one predicate is the point — duplicating the inequality is how they drift into disagreeing about what "open" means. Regression test proves fail-first: 9 of its 10 cases go red against the pre-change tree. The tenth is the deliberate no-regression boundary (a clean tree emits no section) and is green both ways. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TQU48ETJjEmGLJjA6hdQ4 * docs(#2646): document the pre-close artifact audit and its nine categories The /gsd-complete-milestone entry did not mention the audit at all, so the gate that can stop a close was undocumented — and this change adds a category to it. Tabulates all nine with their source artifact and what makes each one "open", plus the [R]/[A]/[C] outcomes. Also disambiguates the one genuinely confusing thing: the per-phase deferred-items.md scanned here is NOT the `## Deferred Items` section the [A] path writes into STATE.md. Same name, different artifact, opposite ends of the flow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TQU48ETJjEmGLJjA6hdQ4 * chore(#2646): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TQU48ETJjEmGLJjA6hdQ4 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-01 21:27:33 -04:00
0xdhx	c61dd49d95	enhance(#2255): blocking catastrophic-shrink guard for curated .planning/ writes (#2301) ... * feat(#2255): blocking catastrophic-shrink guard for .planning writes Adds hooks/gsd-write-guard.js, a PreToolUse hook that hard-blocks (decision: 'block', exit 2) a whole-file Write collapsing a curated .planning/ artifact (ROADMAP.md, .planning/milestones/*-ROADMAP.md, STATE.md) below 40% of its on-disk line count. Files under 40 lines are exempt; GSD_ALLOW_PLANNING_SHRINK=1 (named in the block message) bypasses for legitimate milestone resets. Fix 3 of #973 — the only defense independent of per-agent tool config. Registered on the Claude plugin surface (hooks.json), settings-json runtimes (runtime-hooks-surface.cts, self-contained pattern), Kimi spec, and the OpenCode/Kilo plugin buses. Golden install fixtures and INVENTORY regenerated; regression tests negative-controlled (16/16 RED with the hook absent, 16/16 GREEN with it present). * chore(#2255): backfill changeset pr number to 2301 * enhance(#2255): address review — fail-closed reads, typed block output, registration, property test Review fixes for trek-e's CHANGES_REQUESTED on PR #2301: - Blocker 2: register gsd-write-guard.js in BUNDLED_GSD_HOOK_FILES (no-shipping-drift test). - Blocker 3: update the always-on hook enumerations in ADR-766 and CONTEXT.md from six to seven. - Major 4: fail CLOSED on non-ENOENT read errors — only a missing file (new-file Write) passes; EACCES/EISDIR/ELOOP/etc now block, with a typed readError field and the override still honored. Tested, with a negative control against the pre-fix hook. - Major 5: fast-check property test for the SHRINK_RATIO/FLOOR_LINES budget contract (blocked ⟺ newLines < oldLines*SHRINK_RATIO above the floor; sub-floor always exempt), boundary examples pinned. - Major 6: block output now carries typed oldLines/newLines/ overrideEnvVar fields; tests assert on those instead of regexing the free-form reason string. - Minor: CURATED_PATTERNS are case-insensitive (case-insensitive-FS bypass on macOS/Windows); limit+1 boundary tests added for both the floor and the ratio. * enhance(#2255): engage the write guard on Kimi's native payload shape The guard shipped with Claude-vocabulary checks (tool_name 'Write', tool_input.file_path), which #2304 showed leaves a guard dormant on Kimi: the [[hooks]] matcher is registered pre-translated but kimi-cli forwards its native payload verbatim — tool_name 'WriteFile' (bare or module-qualified) and tool_input.path per its tool schemas (src/kimi_cli/tools/file/write.py). The guard matched, saw an unknown name, and exited 0. Apply the same per-guard normalization PR #2326 gives the three sibling guards (name + field mapping, inlined — hook scripts stage as standalone files), and write the block reason to stderr as well as stdout JSON: Kimi feeds stderr, not stdout, back to the model on exit 2, so a stdout-only reason blocks without telling the model why or naming the documented override. Regression tests pipe Kimi-shaped payloads (engage, qualified-name, stderr-reason) plus exemption pins (StrReplaceFile stays out of scope by design; non-curated paths pass) — verified red against the pre-fix guard, green after. * enhance(#2255): rebase onto next; regenerate golden-parity fixtures * enhance(#2255): wire the escape hatch into complete-milestone's reorganize step Review Blocker 1: the guard hard-blocked /gsd:complete-milestone's ROADMAP reorganize — the tree's only legitimate milestone reset and the exact caller GSD_ALLOW_PLANNING_SHRINK was built for. The reorganize step now performs the rewrite through a shell write with the hatch set on the command (a hook inherits the runtime env, so a bare Write cannot carry a per-step override), and a binding test derives the env var name from the guard's typed output and asserts (a) the workflow step sets it and (b) the guard passes the identical catastrophic payload under it — so the next complete-milestone.md edit cannot silently re-break the wiring. * enhance(#2255): drop dead Edit-class mapping from normalizeKimiPayload Review Major 1: StrReplaceFile -> 'Edit' and the old_string/new_string reconstruction were unreachable-by-effect — the guard exits 0 for any tool_name !== 'Write', so nothing ever read the fields they set, leaving guaranteed-surviving mutants against the Stryker bar. The map now carries only WriteFile -> 'Write'; the StrReplaceFile exemption test message states the fall-through it actually exercises. * enhance(#2255): review minors — American spellings; writeSync before exit(2) Minor 1: normalised/normalise -> American house style. Minor 2: the two block paths wrote stdout+stderr via async pipe writes then exit(2) — async-on-Windows, unflushed at exit; fs.writeSync(1/2, ...) makes the block payload durable. * enhance(#2255): assert stderr equals the typed reason, not raw prose Minor 3: the last raw-text match in the suite pinned override-name prose on stderr. The contract is "stderr carries the reason Kimi feeds back" — now asserted as stderr non-empty and byte-equal to the parsed stdout.reason. * enhance(#2255): bind the write-guard's Kimi normalization into the parity test Review Major 2: the guard's normalizeKimiPayload is a 4th inlined copy with nothing binding it. This extends PR #2326's kimi-guard-normalization-parity test (same path and helpers, authored as a superset so either merge order resolves cleanly): sibling byte-parity is existence-gated zero-or-all — trivially green until #2326 lands, full-strength after — and the write-guard copy is bound semantically (map is the value-inverse of convertKimiToolName; the Kimi name for Write must map, or the guard is dormant on Kimi; the path -> file_path half must be present). Byte-parity is deliberately not asserted for this copy: it legitimately omits the Edit-class mapping (Major 1 — dead code in a Write-only guard). * enhance(#2255): refresh golden-parity fixtures for revised guard + workflow * chore(#2255): regenerate golden fixtures after rebase onto next The committed fixture hashes were generated against a tree predating next's latest 11 commits, which independently modified the same install-parity surface. Rebased onto next and regenerated with `npm run gen:golden`. Verified: against upstream/next the regenerated fixtures differ by exactly this PR's own entries -- hooks/gsd-write-guard.js (new), hooks/managed-hooks-registry.cjs, plugins/gsd-core.js, and gsd-core/workflows/complete-milestone.md. No unrelated drift. * fix(#2255): regenerate workflow size baseline for complete-milestone `complete-milestone.md` grew 31071 -> 32061 (+990) when the round-2 review fix bound GSD_ALLOW_PLANNING_SHRINK=1 into the reorganize step, but tests/workflow-size-baseline.json was never regenerated. The per-file workflow baseline test (issue #1074) failed on ubuntu-latest/22 and both macOS shard 1/3 jobs. The growth is justified: it is the escape-hatch binding requested in review round 2 (the guard must not hard-block the tree's only legitimate milestone reset), not incidental bloat. Regenerated via `npm run size:baseline`; the diff is exactly the one entry. * chore(#2255): regenerate golden fixtures and size baseline after rebase onto next * enhance(#2255): bind the shrink escape hatch mechanically — single-use sentinel the guard consumes Round-5 M1: the per-step `GSD_ALLOW_PLANNING_SHRINK=1 tee` prefix was inert (no PreToolUse hook exists on Bash in this family; the write succeeded by dodging the guard, not by the override firing) and the protection was prose. The hatch is now a transport code consults: complete-milestone's reorganize step arms `.planning/.gsd-allow-shrink` with the target's path, keeps the Write tool as the sanctioned path, and the guard — at the block point only — verifies the sentinel is fresh (15 min) and names the pending target, then CONSUMES it and allows that one write. Path-bound + single-use + freshness keep it from becoming a standing unlock. The env var remains as the interactive transport, where it can actually reach the hook. Regression tests written first (negative control: 3 failed pre-fix): the armed-sentinel Write passes and consumes; stale does not exempt; a token for a different file neither exempts nor is consumed; the binding test now takes the sentinel name from the guard's typed output (overrideSentinel), asserts the step arms it, and asserts the step no longer routes the rewrite around Write via a shell pipe. Also in this commit, same file: - m2: block emission is exception-safe — emitBlock() wraps both writeSync sites in their own try/catch that still exits 2, so an EPIPE can no longer convert fail-closed into the outer catch's fail-open. - Header discloses the two reviewed design limits (cumulative sequential shrink; lexical match vs symlinked paths) per round-5 scoping. * docs(#2255): document the sentinel transport across guard surfaces; changeset ends with the (#2255) parenthetical (m4) USER-GUIDE bullet, INVENTORY row (en + ja/ko/pt/zh), the runtime-hooks-surface registration comment, and the changeset now describe both hatches — the single-use sentinel for workflow steps and the env var for interactive use — instead of implying a per-step env can reach a hook. The changeset's trailing `Resolves #2255.` prose becomes the `(#2255)` parenthetical the repo's fragments use (round-5 m4). * chore(#2255): regenerate derived families on the rebased tree (full sweep) Full generator sweep after rebasing onto next @ the body-parser-patched lockfile: build, gen-inventory-manifest, gen:golden, size:baseline. Every regen delta verified to be either a PR-owned entry (gsd-write-guard.js, complete-milestone.md, INVENTORY/USER-GUIDE) or exact convergence to next's committed value for entries our arbitrary-side conflict resolution had left stale (all 18 runtime fixtures checked mechanically). * test(#2255): use helpers.cleanup for sentinel teardown, not raw fs.rmSync The repo's local/no-raw-rmsync-in-tests rule exists for the Windows-EBUSY retry budget; the sentinel disarm now rides it like every other teardown. * chore(#2255): regenerate derived families after rebase onto next Full sweep on the rebased tree (build -> gen-inventory-manifest -> gen:golden -> size:baseline). Every delta is either a PR-owned entry (hooks/gsd-write-guard.js, its registration surfaces hooks/managed-hooks-registry.cjs and the two plugin buses, gsd-core/workflows/complete-milestone.md) or exact convergence to next's committed value across all 18 runtime fixtures. * chore(#2255): regenerate derived families after rebase onto next @ a5180d96 Rebase onto current `next` (a5180d96) resolved 12 conflicting golden-install-parity fixtures; all regenerated via the full generator sweep (build, gen:golden, size:baseline) rather than a single generator. `lint:generated-sync` reports every generated artifact in sync. All 45 differing fixture keys and the single workflow-size-baseline entry map to files this PR actually touches; no foreign drift. * fix(#2255): remove the stale unguarded reorganize_roadmap step (round-8 blocker) complete-milestone.md carried a second ROADMAP-collapsing step, `reorganize_roadmap`, distinct from the sentinel-armed `reorganize_roadmap_and_delete_originals` this PR wired. It is a vestige of the pre-archive-then-reorganize design: it sits BEFORE archive_milestone, so executing it as written would collapse ROADMAP.md before the archive snapshots the full phase detail — and its Write is exactly the shape gsd-write-guard hard-blocks, with no hatch armed. The file's own success criteria describe only one reorganize outcome (Backlog-preserving, overwrite-in-place — the later step's properties), and archive_milestone points forward to "the reorganize step". Removed rather than wired, per the round-8 review's confirm-and-remove option. A new binding test asserts the sentinel-armed step is the ONLY reorganize step in the workflow, so an unguarded collapse step cannot be silently reintroduced (negative-controlled: fails against the pre-fix tree). Golden-parity fixtures and the size baseline regenerate for the shrunk file; every changed fixture key is complete-milestone.md's own. * test(#2255): document why the read-error injection is a path collision, not an fs monkeypatch Round-8 nit: the non-ENOENT tests inject via a directory-at-target-path collision instead of the repo's fs-method monkeypatch pattern. That is deliberate, not drift — runHook exercises the hook as a spawnSync child process, so an in-process fs.readFileSync patch (the pattern the cited siblings use on require'd, in-process code) can never reach the code under test. Record the reasoning at the injection site. * chore(#2255): regenerate derived families after rebase onto next @ 0d08c320 Rebase onto current next (0d08c320) for the CONFLICTING/DIRTY state. All 32 conflicts were generated artifacts (19 golden-install-parity, 12 install-tree, workflow-size-baseline); resolved arbitrarily and regenerated via a full generator sweep (build, gen:golden, size:baseline, gen-inventory-manifest) rather than hand-merged. No source conflicts. Regen diff verified against the PR's changed-file set: 7 distinct differing keys, all PR-owned (gsd-write-guard.js, managed-hooks-registry.cjs, plugins/gsd-core.js, complete-milestone.md, and their .kimi mirrors). lint:generated-sync clean. * chore(#2255): regenerate derived families after rebase onto next @ 9138271b Conflict set was 20 paths, every one a generated artifact, zero source conflicts — resolved arbitrarily during the replay and regenerated here, per the maintainer's round-9 recipe (never hand-merged). Generator sweep (not just gen:golden): npm run build, gen:golden, size:baseline, gen-inventory-manifest, gen:registry. INVENTORY-MANIFEST came back byte-identical, so the merged value was already correct. Regen diff verified == PR-touched entries: every differing leaf key attributes to a file this PR changes (complete-milestone.md, the guard, managed-hooks-registry.cjs, both plugin buses) — derived from the changed-file set, not a feature-name filter. * fix(#2255): port the #2595 Kimi-payload hardening the base landed for the sibling guards Self-found via the round-10 base sweep: a8b40fa5 (#2547/PR #2595) hardened gsd-prompt/read/worktree-path-guard's normalizeKimiPayload after this PR was authored, and this guard carried all three pre-fix shapes: - `path` now wins outright over a model-supplied `file_path` — kimi-cli's WriteFile schema carries no `file_path`, so a payload pairing a curated `path` with a spurious `file_path: ""` had the guard reading '' and exiting 0 while kimi-cli wrote to `path` (one-key bypass, no crash needed). - null/primitive payloads pass through deliberately instead of throwing into the fail-open catch. - the `file_path` read is typed (`typeof === 'string'`), closing the crash-to-allow on `[]`/`{}` — this is what the base-added kimi-guard-typed-payload-reads scan flags. Negative-controlled: the shadowing payload exits 0 against the pre-fix guard and 2 against this one. * fix(#2255): round 9 Major 2 — stop printing the sentinel recipe in the agent-facing denial The block message ended by telling the blocked agent exactly how to arm the single-use sentinel — handing the #973 actor a mechanical self-authorization in the moment it is blocked. The recipe is gone from the prose; the corrective advice and the env override's name stay (the latter is a #2255 acceptance criterion, and a per-step env prefix cannot reach a hook anyway), and the typed overrideSentinel field stays for the binding tests. The hatch remains documented in USER-GUIDE.md and complete-milestone.md, where humans and the workflow engine read. * fix(#2255): round 9 Minors 1-2 — realpath-resolve the target before the curated match; disclose the /i Linux cost Minor 1: a Write to a non-curated path that symlinks into a curated file was not matched while writeFileSync followed the link — the target is now realpath-resolved before the curated match (ENOENT keeps the lexical resolution so new-file Writes still pass; any other realpath error falls through to the read, which fails closed). Negative-controlled: the symlink payload exits 0 against the pre-fix guard, 2 against this one. Test skips on win32, where symlink creation needs privilege. Minor 2: the header's design-limits block now names the unconditional /i cost on case-sensitive Linux (a genuinely distinct .planning/roadmap.md is also treated as curated) next to the stateless limit, and drops the closed symlink limit. * test(#2255): round 9 Minors 3-4 — CRLF counting pin + a passing Write leaves a fresh sentinel unburned Minor 3: countLines' split('\n') is CRLF-safe for a count (the \r rides along), confirmed by trace in the review — this pins it against this repo's recurring CRLF regressions, on both sides of the compare and at the 40% boundary. Minor 4: consumeSentinelFor runs only after the ratio check would block, so a within-tolerance Write never burns the workflow's token — true by construction, previously un-asserted. * fix(#2255): round 9 Major 3 — correct the stale env-var line in archive_milestone's summary complete-milestone.md's "After archival" bullet still said the reorganize happens "under GSD_ALLOW_PLANNING_SHRINK=1" — the wording from the round-2 design this PR's own history rejected in round 5 (a per-step env var cannot reach a hook; setting it in a Bash step silently does nothing). It now points at the sentinel mechanics the reorganize step actually documents, matching that step and USER-GUIDE.md. * docs(#2255): round 9 Major 1 — user-facing docs state the stateless per-Write limit The changeset and USER-GUIDE described the guard as covering "catastrophically shrinks" with no caveat, while the stateless design was disclosed only in the hook header — an operator reading the shipped docs would conclude iterative erosion is covered. Both surfaces now state the per-Write comparison and the erosion non-goal explicitly, in line with what the guard does. * fix(#2255): round 10 Blocker 1 — arm the hatch in the roadmapper's curated writes gsd-roadmapper Step 7 Writes both .planning/ROADMAP.md and .planning/STATE.md wholesale, and /gsd:new-milestone spawns it against the OUTGOING milestone's files. new-milestone's `phases.clear` archives phase DIRECTORIES, never ROADMAP.md, so nothing compacts it first, and no ordering rule forces /gsd:complete-milestone to run before /gsd:new-milestone. Measured against the shipped hook at the #973 file size (292 lines): a new 4-phase roadmap lands at 18.2% and an 8-phase one at 31.8% — both blocked; only a 12-phase replacement (45.5%) clears. So the collapse is reachable on a first-party flow, which is the failure mode #2255 para 4 says the guard must not have. Arms the single-use sentinel immediately before each Write, one per target (the token is path-bound and single-use, so one arming cannot cover both), and gates each arming on `[ -f ]` so the /gsd:new-project path — where the guard already exempts via ENOENT — strands no unconsumed token. Binding test derives the sentinel name from the guard's own typed output, so a rename on either side fails the suite instead of silently unwiring the hatch. Negative-controlled: 2 assertions fail against the pre-fix roadmapper. * docs(#2255): round 10 Major 2 — scope the claim to the guarantee that ships The sentinel is a plain file, so an agent that would reason past an advisory can arm one with a single Bash call it is already permitted to make. The "only defense independent of per-agent tool config" framing outran that, and the changeset was on its way into CHANGELOG.md. Retitles the claim on all three surfaces (changeset, guard header, USER-GUIDE) to what the guard actually delivers: it blocks accidental and single-shot collapse and is not a defense against a determined agent; what it converts is "ignore a sentence" into "take one deliberate, path-bound, single-use, auditable action". Pinned by test on the DURABLE surfaces only — the guard header and USER-GUIDE. The changeset fragment is deliberately not pinned: it is consumed at release, so a test reading it would start failing the moment the release lands. The bound-statement assertion normalizes comment markers and whitespace first, so it pins the claim rather than the paragraph's line wrapping. Negative-controlled: both assertions fail against the pre-fix surfaces. * test(#2255): acknowledge the roadmapper growth from the round 10 Blocker 1 wiring The emitted-attribution gate (#2719/#2767) flags gsd-roadmapper.md growing 1130 bytes without an acknowledgment. The growth is the Blocker 1 sentinel wiring plus the rationale a future editor needs to keep it, so it gets an ack fragment rather than a silencing regen — the gate's own message is explicit that there is nothing left to regenerate. Fragment is PR-scoped (2301-…) per the gate's naming instruction, and uses the plain-string reason form the shipped fragments use. Verified against the TRUE upstream tip, not the fork's origin/next: a stale origin made this same gate report unrelated phantom drift (1 emitted path + 6 grown files + 5 stale acks) that vanishes when GSD_EMITTED_BASE is pinned. * test(#2255): renumber the roadmapper PROSE_ALLOWLIST pin after the Step 7 wiring CI red on shard 2/3, all four platforms. The #2751 gate keys PROSE_ALLOWLIST on {file, line}; the Blocker 1 wiring added 18 lines above the allowlisted parenthetical in agents/gsd-roadmapper.md, moving it 624 -> 642. Both halves of the gate then fired: the moved line reads as a new offender, and the stale entry no longer matches anything. Line content at 642 is byte-identical to what the entry describes — a descriptive "e.g." naming SDK queries a user could run — so this is a renumber, not a re-classification. Swept the defect class rather than the instance: agents/gsd-roadmapper.md is the only line-pinned reference to any file this round changed. Negative-controlled: both assertions fail against the un-renumbered allowlist. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-01 21:19:49 -04:00
0xdhx	cc3ee301a7	fix(#2544): stage the CommonJS marker in GSD-owned dirs, not the config root (#2593) ... * fix(#2544): stage the CommonJS marker in GSD-owned dirs, not the config root installSharedHooksBundle wrote `{"type":"commonjs"}` over <configRoot>/package.json unconditionally — no existence check, no merge, no backup — on every install and every /gsd-update re-install. On the 11 affected runtimes that file is often user-owned; on OpenCode and Kilo it is the documented place to declare local-plugin npm dependencies, so a user's name/type/dependencies/scripts were destroyed on each run. The uninstall path already read the file and unlinked it only on an exact content match. That asymmetry was the defect: the discipline existed in the codebase, it just was not applied on the write side. Move the marker into the directories GSD creates and fills with its own .js files — hooks/ (all shared-hooks runtimes, incl. Kimi's own root) and the nativePlugin dir (plugins/ for OpenCode+Kilo, extensions/ for pi) — and stop writing the config root entirely. New src/commonjs-marker.cts owns the marker string plus one ownership predicate (absent / gsd-owned / foreign, fail-closed on an unreadable file) shared by ensureCommonJsMarker and removeCommonJsMarker, so install and uninstall cannot drift apart again. Nothing else depended on the config-root marker: package identity is baked at build time (#378/#498) and version resolution prefers gsd-core/VERSION and already tolerates a missing root package.json (#1383) — Codex has installed without one all along. A package.json in plugins/ or extensions/ is inert to plugin discovery, which globs *.{ts,js} only (see installer-migration 006). Uninstall retires the pre-fix config-root marker, so upgrading users are cleaned up on removal, and still never touches a file it did not write. * fix(#2544): point the changeset fragment at the filed PR The fragment's `pr:` field is only knowable after `gh pr create` returns. * fix(#2544): register commonjs-marker.cjs in the tsc-generated ESLint ignore set bin/lib/commonjs-marker.cjs is tsc output (src/commonjs-marker.cts is the linted source), so it belongs in the ADR-457 ignore list like its siblings. Clears the lint-tests no-var failure and the repo-invariants "linted xor ignored" migration-state test. * fix(#2544): pin the kimi CommonJS marker to hooks/, not the ~/.kimi root The UPGRADE 1 test still asserted the pre-#2544 marker location (~/.kimi/package.json). The marker now lives inside ~/.kimi/hooks — the directory GSD itself creates — matching the updated golden-install-parity and install-tree fixtures. Also asserts the root marker is NOT written. * fix(#2544): make the CommonJS marker write path non-fatal Review round 2, Major 3 + Minor 1 + the stagedHooks nit. ensureCommonJsMarker rethrew any non-EEXIST write error and neither call site caught it, so EACCES on a read-only hooks/, EROFS, or ENOSPC aborted the whole install with a raw stack trace. Every other marker interaction in the module is best-effort — removeCommonJsMarker swallows unlink failures, classifyMarker swallows read failures — and this was the write path, i.e. the one most likely to fail on a locked-down config dir. It now returns a new 'failed' outcome and both call sites warn and continue. Sibling found while sweeping for the same defect class: fs.mkdirSync sat OUTSIDE the try block, so an unwritable parent threw past the guard entirely. Creating the directory is the same environmental hazard as writing into it, so it moved inside. Also in this file: - The hooks marker is now gated on `stagedHooks && hooksOk`, not stagedHooks alone. stagedHooks is computed from the SOURCE listing before the copy loop, so it stays true when the copies land but verifyInstalled() then fails — marking a hooks/ GSD did not successfully populate claims an ownership the install did not earn. - The uninstall rmdir of the native plugin dir is gated on GSD having actually removed something from it. Hoisting it out of the adapter-exists guard (so the marker-only case could prune) had silently widened it into deleting a user-created but empty plugins/ or extensions/ dir — the same "don't touch territory GSD didn't fill" principle this issue is about, inverted. - Kimi's pre-#2544 marker at its native hook root (~/.kimi) is retired at the same call site that writes its replacement. That path is outside kimi's configDir, so installer-migration 007 structurally cannot reach it. * fix(#2544): retire the stale config-root marker via installer-migration 007 Review round 2, Major 1 — the PR's headline claim was false for existing installs. Upgraders kept BOTH markers: the new one under hooks/ and the stale {"type":"commonjs"} at the config root, so their config root stayed pinned to CommonJS and their dependency manifest stayed gone until they uninstalled. The migration is unusual in one way, and it is the part worth reviewing: the config-root marker was never recorded in gsd-file-manifest.json (writeManifest records hooks/, agents/, commands/, scripts/ and the native plugin, never a root package.json), so classifyArtifact answers 'unknown' for it and the planner's own guard downgrades a remove-managed on an 'unknown' classification to preserve-user. 007 therefore supplies the "purpose-built detector for an old GSD-owned shape" that docs/installer-migrations.md#remove-managed sanctions — exact content match, the same predicate removeCommonJsMarker has always used — and declares the resulting classification on the action. A package.json with any other content is left untouched, and there is deliberately no backup-and-remove branch: a non-matching file here is not a patched GSD artifact, it is somebody else's file. Scope is all runtimes. The `runtimes` field is OMITTED rather than `[]`: validateStringArray requires the field to be non-empty WHEN PRESENT, while the runtime filter treats an empty array as "all" — so `runtimes: []` throws at plan time and the migration never runs. The metadata test pins this. Kimi is a deliberate carve-out, named in the migration's own header: its marker lived at ~/.kimi, outside kimi's configDir, and migration relPaths are structurally confined to configDir. It is retired by the installer instead. Registration: shipped-migrations table, .gitignore for the emitted .cjs, the EXPECTED_CHECKSUMS baseline, and the ESLint ignore set. That last one is not copied from migration 006 by rote — 006 needs no entry because it imports nothing, while 007 imports node builtins, so tsc emits its __importDefault helper and the `var` in it trips no-var. This is the same lint gate that made round 1 red. * test(#2544): fault-injection and multi-runtime marker coverage Review round 2, Major 2 + Minors 4 and 5. Major 2 — CONTRIBUTING.md:514-531 is mandatory for install/uninstall flows and the suite had no fs monkeypatching at all. Every branch now covered is one whose doc comment claims it as the module's safety posture: - classifyMarker non-ENOENT lstat error -> 'foreign' (the fail-closed rule), with an ENOENT control alongside it so the test discriminates rather than just asserting one side - classifyMarker readFileSync throw -> 'foreign' (present-but-unreadable never downgrades to the permissive answer) — the fixture's bytes are exactly GSD's marker, so the test fails if the code ever answers on content it could not read - a DIRECTORY at the marker path (CONTRIBUTING:521; the symlink case was already covered with a real symlink, the directory case needs no injection at all) - the ensureCommonJsMarker TOCTOU EEXIST branch — the entire reason for flag:'wx' - the new 'failed' outcome, for both writeFileSync (EACCES/EROFS/ENOSPC) and the mkdirSync that used to sit outside the guard - removeCommonJsMarker unlink throw -> false These save and restore fs methods in `finally` rather than using chmod 0o000, which does not fault under root and would pass vacuously in root Docker and CI. Minor 4 — uninstall was driven for opencode only. pi's extensions/ and both kimi locations now have behavioral coverage, install and uninstall, each paired with a user-authored-file case proving GSD leaves it alone. Minor 5 — the stagedHooks gate had no assertion behind its stated reason. A pre-existing, GSD-untouched hooks/ directory is now driven through a runtime that declares skipSharedHooksInstall and asserted to stay marker-free, with its user content intact. Also regression-tests the uninstall rmdir gate from the previous commit: an empty plugin dir GSD removed nothing from must survive. * docs(#2544): correct stale marker prose, register the module, document the trade-off Review round 2, Minors 2, 3 and 6. Minor 2 — six files asserted the installed ROOT ships the synthetic marker. None was load-bearing (all three walk-up consumers are VERSION-first with try/catch and the marker never carried a `version`), but ADR-457:52 is the rationale for keeping a generated module, so a future reader would mis-derive the constraint from it. Each site is corrected to what is now true: the installed tree carries no package.json with a .name at all, because the only ones GSD stages are {"type":"commonjs"} markers and they now live in GSD's own directories. Two of the six needed more than a location swap. hooks/gsd-check-update-worker.js and the platform-gate test both described `require('../package.json').name` resolving to undefined; post-#2544 that require does not resolve at all, so the history is kept accurate and the present-tense claim corrected rather than just moved. And src/runtime-artifact-conversion.cts described the no-root-package.json case as Codex-only — it is now every runtime, which strengthens that comment's own argument for lazy resolution. The generated .cjs sibling needs no edit: it is gitignored build output, not a tracked file. Minor 3 — src/commonjs-marker.cts had no CONTEXT.md entry, unlike every peer module, and CONTEXT.md is the #2 co-change partner of bin/install.js. Added, including the fail-closed posture and the never-throws contract. Minor 6 — the plugins//extensions/ marker shadows the config root for all .js siblings, so an OpenCode/Kilo user's ESM plugin/*.js stays broken. That is exactly what #2544's Fix section prescribed and it is disclosed in the PR body, but the PR body is not documentation. It now lives in the OpenCode section of docs/how-to/install-on-your-runtime.md, stated as a real constraint rather than a pure improvement, with the .ts mitigation and a fallback for ESM plugins. * test(#2544): attribute the CommonJS marker in the emitted-provenance rules The differential emitted-attribution gate (#2723, landed on `next` after this branch was cut) went red on the macOS shards once this PR rebased onto it. Two distinct causes, both real gaps rather than noise: 1. `plugins/package.json` and `extensions/package.json` matched NO rule — the `native-plugin` rule covers `*.{js,cjs,mjs}` only, so the marker read as an unattributed emitted family. 2. `hooks/package.json` fell through to `hooks-built`, which attributes an emitted `hooks/<X>` to a repo source `hooks/<X>`. There is no `hooks/package.json` in the repo, so it resolved to a nonexistent path. Cause 2 is exactly the failure already documented three lines above it for Copilot's `gsd-session.json` — "a code literal, not a built script" — so the fix follows that precedent rather than inventing one: `package.json` is excluded from `hooks-built` the same way, and a dedicated `commonjs-marker` rule attributes the family across all four roots it can appear in (both hooks roots plus `plugins`/`extensions`) to the sources that actually emit it. Deliberately a RULE, not an entry in tests/emitted-drift-ack.json. An ack is for a one-off ripple and goes stale by design — the gate fails a stale ack precisely so it cannot pre-clear the next change on that path. These markers are a permanent part of the emitted tree from #2544 onward, so they need standing attribution. Verified by reproducing the CI failure locally with GSD_EMITTED_BASE: 3 provenance errors + 12 unattributed paths before, 35/35 green after. * fix(#2544): route the #2717 hooks-surface marker helpers through commonjs-marker #2717 landed a second copy of ensureCommonJsMarker/removeCommonJsMarkerIfGsdOwned in src/runtime-hooks-surface.cts for the runtimes that stage .js hooks via dedicated paths (cursor/windsurf/codex). That copy had drifted from this PR's module on the two properties that matter: - ownership probe: `fs.existsSync` FOLLOWS symlinks and reports false for a DANGLING one, so a dangling package.json symlink classified as absent and the write went straight through it. Demonstrated: against the pre-fix copy, ensureCommonJsMarker() on a hooks/ dir holding a dangling package.json symlink returns true and creates {"type":"commonjs"} OUTSIDE that directory. - create: a plain writeFileSync leaves the classify->write window open, where commonjs-marker creates with flag:'wx' (O_EXCL). Both helpers now delegate to src/commonjs-marker.cts, which is what this PR's own docstring already claimed was the single place these rules are enforced. Exported signatures are unchanged (still boolean), so bin/install.js and the #2717 tests are unaffected. The new subtest is the only coverage that fails if the duplicate is ever reintroduced — the two implementations agree on every non-adversarial input, so the existing suites pass against both. * test(#2544): pin the stagedHooks gate on zcode, not windsurf The Minor-5 coverage picked windsurf because hostBehaviors.skipSharedHooksInstall kept it out of the shared hooks bundle, so GSD staged nothing into hooks/ and the marker was correctly absent. #2717 changed that premise: cursor/windsurf/codex now stage their .js hooks via dedicated paths and get the marker beside those scripts. Measured on this tree, windsurf stages 2 .js hooks and receives a marker — so the assertion was pinning behaviour that is now wrong, not the gate it was written for. ZCode is the durable choice: per #1821 it has hooksSurface:'none' AND no plugin surface to spawn hooks, so GSD stages no .js there by either route (measured: 0 staged, no marker). The property under test is unchanged — a user-created hooks/ directory GSD never fills stays marker-free. * test(#2544): use the shared cleanup helper in the migration test Addresses the review's Major 1. The suppression's stated reason — "no helpers import available" — was not correct: tests/helpers.cjs exports cleanup, and the other test file added in this same PR imports it (tests/commonjs-marker.test.cjs). The local reimplementation dropped two protections that are live on this repo's windows-latest lane: the CWD guard (Windows cannot remove a directory that is the current working directory) and the 20 x 250ms retry budget that absorbs the deferred-scan handle Windows Defender holds on newly-written files. Local function and suppression both removed; local/no-raw-rmsync-in-tests now passes without one. * test(#2544): expect hooks/package.json for the #2717 runtimes The fresh-install contract table predates #2717, which stages cursor/windsurf/ codex .js hooks via dedicated paths and writes the CommonJS marker beside them. All three therefore now receive hooks/package.json legitimately. Measured on this tree: codex stages 3 .js hooks, cursor 6, windsurf 2 — each with the marker; cline/copilot/trae/zcode stage none and get none, so their contracts are unchanged. * fix(#2544): gate the #2717 marker writes on having staged something The three dedicated marker writers #2717 added ran unconditionally. Each one mkdirs hooks/ up front and stages its scripts conditionally on the source existing, so with an absent or empty hook source they created a directory, filled it with nothing, and marked it as GSD's anyway. That is the same write-into-someone-else's-territory this issue is about, and installSharedHooksBundle already guards the identical case with `stagedHooks`. The dedicated paths now carry the matching gate: - cursor / windsurf: `installedScripts.size > 0` - codex: a new `codexStagedHooks` flag. The enclosing guard only proves that hooks/dist EXISTS; it says nothing about whether any CODEX_HOOKS_TO_COPY entry landed. Covered for cursor and windsurf by driving each writer against a src tree whose hooks/ dir is empty. The codex leg is defensive and deliberately uncovered: its trigger state needs a package tree where hooks/dist exists but holds none of the allowlist, which is not constructible from a real checkout. * test(#2544): scope the commonjs-marker sources per root The rule declared one flat source list for every marker root, so `extensions/package.json` was attributed to runtime-hooks-surface.cts (which never writes there) and `.kimi/hooks/package.json` to install-engine.cts. That is not merely untidy. emitted-diff.cjs accepts the FIRST satisfied source, so a flat list containing bin/install.js let any change anywhere in that 13k-line file authorise marker drift for every root — the blanket escape hatch this file's own agents-verbatim comment refuses for exactly the same reason. Sources are now derived per root from ctx.rel. Note the rule ctx is `{ rel, runtime }` and carries no `root`, so keying on ctx.root would have sent every path down one branch silently. * test(#2544): state precisely what the zcode assertion pins The comment claimed the test pinned installSharedHooksBundle's `stagedHooks` gate. It does not, and neither did the windsurf version it replaced: zcode declares skipSharedHooksInstall, so the outer guard skips that helper entirely and the gate is never evaluated. The test passes on the runtime exclusion. What it does pin — the outcome a pre-existing, GSD-untouched hooks/ stays marker-free — is still worth having, and is what the review asked for. The two `staging zero hook scripts` tests are the ones that pin a real staged-nothing gate. Comment corrected rather than left implying coverage that is not there. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-01 21:00:23 -04:00
Tom Boucher	b62589b73f	fix(#2840): exclude runtime from defaults.json spread into project config (#2985) ... * test(#2840): add regression for runtime poisoning from defaults.json * fix(#2840): exclude runtime from defaults.json spread into project config runtime is host-specific (written by whichever installer ran last). On a machine with 2+ runtimes, it poisons every new project config — e.g. a Codex install's runtime:'codex' leaks into Claude Code projects. Now excluded from the userDefaults spread, mirroring the resolve_model_ids guard (#2297). * chore(#2840): add changeset fragment * fix(#2840): add new test file to lint-test-file-count allowlist * chore(#2840): backfill changeset PR number 2985 --------- Co-authored-by: sim <sim@local>	2026-08-01 16:33:43 -04:00
Tom Boucher	628648d63a	chore(#2931): cap emitted per-runtime bytes and single-source windsurf (#2984) ... * fix(#2931): preserve protected regions and cap emitted per-runtime bytes Route every runtime brand swap through applyClaudeCodeBrandSwap so "Claude Code" survives verbatim inside <runtime_compatibility> regions (#2284b). The fix existed only in bin/install.js's local copies; the src/*.cts exports still used a naive replace, so binding install.js to the single source -- as this phase does for the Windsurf family -- would have silently regressed those runtimes. A table-driven parity guard now covers all nine brand-swapping converters. De-duplicate the Windsurf converter family: delete the six local copies in bin/install.js and bind the four exported ones by reference, guarded by reference-identity assertions (the ADR-1508/#1675 pattern). The two unexported helpers and an unused tool table go with them. Replace the Windsurf 12,000-byte throw with description truncation, matching the bound its sibling skill converter already applied. The throw could only fire on an ~11.7 KB frontmatter description: the largest emitted workflow is 311 bytes. Truncation makes the cap unreachable by construction and leaves 12,000 in exactly one place, eliminating the dual-surface duplication rather than testing for it. Add the emitted-byte cap gate: buildEmittedSizes captures LF- and <HOME>-normalized bytes from the walk buildParityManifest already performs, and evaluateEmittedCaps asserts them against a per-runtime cap table with dead-rule detection. buildParityManifest's return shape is deliberately unchanged -- diffEmitted compares its values with ===, so making them objects would report all 8,529 emitted paths as moved. A regression test pins the values as strings. Add a deterministic trim-safety gate over composeWithinBudget's omitted/shrunk/floored/isolatePrefix metadata, with an anti-vacuity rule, replacing the model-graded eval gate the issue described. * docs(#2931): correct ADR-1671 windsurf premise and trim-safety contract * fix(#2931): bound the windsurf command name and single-source the brand swap Review findings from the orthogonal passes, all fixed inline. The claim that removing the 12,000-byte throw left total emission "bounded by construction" was false. The #1615 regex constrains the character class but not the length, and commandName is interpolated three times into the emitted workflow: a 20,000-character name emitted 60,162 bytes silently. Add WINDSURF_COMMAND_NAME_MAX=128 as a separate, clearly-labelled size control that THROWS -- commandName is the @-ref path target, so truncating it would point the workflow at a file that does not exist (DEFECT.WORKFLOW-DELEGATION-TARGET-NOT-INSTALLED). The #1615 security regex is untouched and still runs first. 128 is generous: the longest shipped name is gsd-plan-review-convergence at 27. Harmonize convertClaudeCommandToWindsurfSkill onto the code-point-safe truncation helper. It still used a UTF-16 slice(0,177) -- the exact surrogate-splitting bug the helper was written to avoid, in the very sibling the helper's comment cites as its model. Bounds are unchanged, so output is byte-identical for every shipped command (descriptions max out at 99 chars). Export applyClaudeCodeBrandSwap and bind it in bin/install.js, deleting the local copy. Adding it to the .cts left two unlinked implementations of identical logic -- the drift class this change exists to remove. Verified byte-identical across eight fixtures and five sequential calls before merging, and guarded by a reference-identity assertion. Convert three try/finally test bodies to t.after (CONTRIBUTING.md:344), add fast-check property coverage for the trim-safety contract, and use fc.pre instead of a bare return in a property callback. * test(#2931): fix three test-authoring bugs the remote matrix caught The remote runner returned 8 unique failures on 6f15cdeb8. All three causes were in the test files, not the modules under test -- local harnesses exercise the modules directly, so nothing executed the test bodies until the matrix did. `{ __proto__: [...] }` in an object literal sets the prototype instead of an own key, so the JSON round-trip erased it and the cap table never saw a reserved runtime key. The production rejection was already correct; the test could not reach it. Use a computed key. Two cap fixtures tripped orthogonal error paths rather than the paths they name: one declared windsurf in the cap table but omitted it from sizes (UNKNOWN_RUNTIME), the other left the sole windsurf pattern matching nothing (a genuine dead rule). Both now include a compliant artifact so the intended branch is what is asserted. The dead-rule and unknown-runtime contracts are deliberate and unchanged. `const { root } = makeSyntheticConfig({ ... `${root}` })` referenced `root` from inside its own initializer -- a temporal dead zone error. makeSyntheticConfig now optionally takes a (root) => files factory. Also raise the npm pack --dry-run bound 60s -> 120s in the shipped- scripts packaging test. That failure is NOT from this branch: the file is byte-identical to next, a fresh tsc measures 1.98s there vs 2.14s here, and the run recorded 60,637ms against a 60,000ms bound -- a timeout under 28,948-test parallel contention, not a slowdown. Fixed rather than deferred because a bound that tight is fragile regardless of which branch trips it. * chore(#2931): backfill changeset pr number to 2984 --------- Co-authored-by: sim <sim@local>	2026-08-01 16:00:14 -04:00
Tom Boucher	34633fa4ec	fix(#2893): preserve prose below the JSON ledger on windows append/waive/fixed (#2975) ... * test(#2893): add regression for append destroying prose below JSON ledger writeLedgerAtomic overwrites the entire file with renderLedger(ledger), dropping any prose below the JSON closing fence. The test creates a WINDOWS.md with prose sections below the ledger, appends an entry, and asserts the prose survives. * fix(#2893): preserve prose below the JSON ledger on append/waive/fixed writeLedgerAtomic was overwriting the entire WINDOWS.md with renderLedger(ledger), which reconstructs only the frontmatter + header + table + JSON block — silently destroying any prose a user wrote below the JSON closing fence. Now the writer reads the existing file before overwriting, extracts content after the closing fence, and appends it to the rendered ledger. First-write (no existing file) proceeds normally with no prose to preserve. * fix(#2893): address review — correct fence search + idempotency test BLOCKER from isolated adversarial review: indexOf(JSON_FENCE_CLOSE) matched the OPENING fence ('json' starts with ''), duplicating the entire JSON body as prose on every write. Now searches for the closing fence starting AFTER the opening fence, mirroring parseJsonBlock. Test hardened: non-empty initial ledger, second append (idempotency — prose appears exactly once, exactly one JSON fence open), parseLedger round-trip. * chore(#2893): add changeset fragment * chore(#2893): backfill changeset PR number 2975 --------- Co-authored-by: sim <sim@local>	2026-08-01 12:59:09 -04:00
Tom Boucher	640eaee16e	chore(#2930): fragmentize execute-phase.md and prove per-runtime composed emission (#2972) ... * feat(#2930): fragmentize plan-phase.md workflow into per-runtime-composed sections Adds src/workflow-fragments.cts (in-file <!-- gsd:section --> marker parser/composer, ADR-1671 epic #1671 Phase 3), wires it into bin/install.js's copyWithPathReplacement emission path, and pilots the marker grammar on gsd-core/workflows/plan-phase.md. Bookkeeping ripple for the new src/*.cts module: .gitignore, eslint.config.mjs, docs/INVENTORY.md + docs/INVENTORY-MANIFEST.json, and a CONTEXT.md glossary entry. Amends ADR-1671 with open questions 1 and 2 resolutions and records the closed when= applicability grammar. Adds docs/reference/workflow-fragments.md and an ARCHITECTURE.md section documenting the marker authoring model. * fix(#2930): put allow-test-rule issue ref on the same line as the marker lint-allow-test-rule-refs.cjs requires the #NNN issue reference on the same source line as `allow-test-rule:`; it was one line below and read as an unreferenced novel exemption. * docs(#2930): link the orphaned gate-predicates reference from the docs index Found while adding the workflow-fragments reference doc: docs/reference/gate-predicates.md shipped without an entry in docs/README.md, so it was unreachable from the docs index. Fixed inline rather than deferred. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2930): scope composition to workflows, add typed failure reasons Review findings from two orthogonal passes: - Scope composeWorkflow to gsd-core/workflows/ only. It previously ran on every .md the installer copied, so a future agent/command/reference doc documenting the marker syntax with an unfenced example would have been mis-parsed and silently stripped — a lossy drop the phase forbids. - Add a frozen REASON enum; failures attach a typed .reason and tests assert on it instead of matching free-form message text (CONTRIBUTING.md:635-694). - Derive the property generator's when= values from WHEN_VOCABULARY instead of duplicating them (DEFECT.GENERATIVE-FIX). - Add adversarial parser fixtures: Unicode headings, NUL, U+FFFD, BOM, fence-within-fence, tilde and indented fences, lone-CR marker line. - Document why --mvp is structurally unmarkable: its content is interleaved, not sectioned, so the whole-line grammar cannot reach it. Also fixes two stale tests on this branch, each reproduced on the unmodified tree before correction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2930): retarget the pilot from plan-phase to execute-phase The full remote matrix went red on both Linux lanes. Root cause was ours: tests/phase6-capstone-conformance.test.cjs holds a PRE_PHASE6 ceiling of 94519 bytes for plan-phase.md, asserting an ADR-857 Phase-6 completion property. That is a third size gate beyond the tier caps and the differential ratchet, and it left plan-phase.md just 36 bytes of headroom rather than the 3821 computed from the XL cap. The 330 marker bytes overran it by 294. Raising the ceiling is not an option: it is a red line certifying another ADR's completion. plan-phase.md is reverted to byte-identical origin/next and the pilot moves to execute-phase.md, which has 728 bytes of headroom under its own ceiling and lands at 93147 with 3 marker pairs. The vocabulary narrows to the atoms actually used: always, flag:--wave, state:gap-closure-phase, state:has-prior-phases. Recorded in the ADR: every branch the epic names lives in plan-phase.md, which cannot be fragmentized until caps move from source to emitted bytes. That is direct evidence for the epic's premise and may reorder phases 3-4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#2930): backfill changeset PR number (#2972) * fix(#2930): make the emission install tests portable on Windows The windows-latest lane went red on three tests in the new install suite; Linux was green. Both causes were in the test harness, not the module. Root normalization: the opencode converter always embeds the install root forward-slashed, but the tests stripped it with the native-separator string from mkdtemp. On Windows that never matched, so the root leaked through unstripped — and because the real and stub install roots have different prefix lengths, that length difference landed directly in the byte-delta assertion (344 observed vs 275 expected). Normalize both text and root to one separator form before stripping. @-ref resolution: the helper stripped only the @~/ and @$HOME/ forms, so a Windows absolute ref (@C:/Users/...) fell through and was joined onto the root, producing ...\@C:\Users\... Strip the @ first, then detect absoluteness from the token's own shape (POSIX, drive-letter, or UNC) with no platform branching, so every OS takes the same path. Neither assertion was weakened; the exact-equality byte check is the point of the test and still holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(#2930): document every REASON member and guard the doc/enum parity Code review found the reference doc's 'Fails closed' list covering 10 of the 11 frozen REASON members — MALFORMED_ATTRIBUTES (parseAttrs rejects malformed key="value" syntax) had no bullet, and it is distinct from UNRECOGNIZED_ATTRIBUTE, which is valid syntax with an unknown key. Two parallel surfaces sharing one constant with nothing asserting they agree is the DEFECT.GENERATIVE-FIX class, so the same commit adds the parity assertion: the test derives the enum side from the built module and the doc side by parsing the reference page, keyed on the reason IDENTIFIER rather than prose so a reworded bullet does not break it, and reports set differences in both directions by name. Proven non-vacuous: removing the MALFORMED_ATTRIBUTES bullet turns the suite red naming that exact member; restoring it returns 44/44. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-01 12:12:20 -04:00
Tom Boucher	f0bb0787c9	fix(#2640): report truthful state_updated + keep progress frontmatter in sync after phase remove (#2974) ... * test(#2640): add regression for state_updated false positive + stale progress Three cases: (1) state_updated reflects actual content change, not just file existence; (2) progress.total_phases resync'd even when the body lacks 'Total Phases:' (the no-op guard was skipping syncStateFrontmatter); (3) state_updated is false when STATE.md doesn't exist. * fix(#2640): report truthful state_updated + force frontmatter resync Two defects in cmdPhaseRemove: 1. state_updated was fs.existsSync(statePath) — trivially true, since the file existed before and readModifyWriteStateMd never deletes it. Now captures the boolean return from readModifyWriteStateMd (changed from void to boolean: true when content was written, false on no-op). 2. progress.* frontmatter stayed stale when the body lacked 'Total Phases:' or 'of N' — readModifyWriteStateMd's no-op guard (#948) skipped syncStateFrontmatter when the body transform was unchanged. Now the transform forces a body diff when a phase was actually removed, so the guard passes and syncStateFrontmatter rebuilds progress.* from the post-deletion disk/ROADMAP state. * fix(#2640): address review — gate forced-diff on targetDir, strengthen assertions Two MAJOR findings from isolated adversarial review: 1. Forced-diff injected a spurious 'Total Phases:' line even when no directory was removed (targetDir === null). Now gated on targetDir !== null. 2. Test #2 asserted 'not 3' instead of '2' — would pass for any wrong count. Now asserts exact value. Test #1 strengthened to assert body Total Phases and frontmatter total_phases both equal 1. * chore(#2640): add changeset fragment * chore(#2640): backfill changeset PR number 2974 --------- Co-authored-by: sim <sim@local>	2026-08-01 11:56:34 -04:00
Tom Boucher	000a322489	fix(#2941): hint at global: prefix when bare skill name matches a global skill (#2973) ... * test(#2941): add regression for bare-skill-name global: hint When a bare skill name matches an existing global skill, the skip warning must hint at the global: prefix. When no global skill matches, the original 'Skill not found' warning is unchanged. * fix(#2941): hint at global: prefix when a bare skill name matches a global skill When buildAgentSkillsBlock skips a bare skill name that doesn't exist as a project-relative path, check if it matches an existing global skill. If so, append a hint to the warning: 'a global skill named X exists; use global:X to reference it'. When no global skill matches, the warning is unchanged. getGlobalSkillDir and getGlobalSkillsBase are already imported in this module for the global: branch. The hint is guarded on globalSkillsBase being non-null (runtimes without a skills directory don't support the prefix). * chore(#2941): add changeset fragment * chore(#2941): backfill changeset PR number 2973 --------- Co-authored-by: sim <sim@local>	2026-08-01 10:58:50 -04:00
Tom Boucher	1db7dcd9bf	fix(#2849): strip trailing hyphen after 60-char slug truncation (#2967) ... * test(#2849): add failing regression for trailing-hyphen-after-truncation The strip ran before .substring(0, 60), so a cut landing on a separator produced a slug ending in '-'. Four cases: the exact issue repro (59 a's + space + tail), a boundary landing before a separator, leading-hyphen survival, and a long-Cyrillic transliteration+truncation case. * fix(#2849): strip trailing hyphen after 60-char truncation generateSlugInternal ran the ^-+|-+$ hyphen strip BEFORE .substring(0, 60), so a title whose 60-character cut landed on a separator yielded a slug ending in '-' — the very thing the strip step exists to prevent. Reorder so the strip runs after truncation. Truncation cannot introduce a leading hyphen, so the full ^-+|-+$ pass last is equivalent for leading hyphens and fixes the trailing-hyphen-after-truncation case. Latin-script output for titles ≤ 60 chars is byte-identical; only titles whose truncation boundary lands on a separator change (from broken to clean). * test(#2849): add all-separator collapses-to-empty boundary case Surfaced by isolated adversarial review: pin the contract that input which is entirely separators ('!!!', '!'.repeat(70)) reduces to '' — not null, not a stray hyphen — both short and past the 60-char truncation. * chore(#2849): add changeset fragment pr:0 placeholder; will backfill the real PR number after the PR exists. * chore(#2849): backfill changeset PR number 2967 --------- Co-authored-by: sim <sim@local>	2026-08-01 08:24:05 -04:00
Tom Boucher	73418c516f	fix(#2956): scope Phase extraction to ## Current Position (3rd gen of #2444/#2567) (#2961) ... * test(#2956): fail-first regressions for Phase scoped to ## Current Position Third generation of #2444 / #2567. Stopped At / Paused At were scoped to ## Session; Phase (canonically in ## Current Position per templates/state.md) was left unscoped, so a historical Phase: / **Phase:** line in an archive section overwrites current_phase on every write. Since current_phase is routing input for gsd-progress / --next, the rewind routes work to the wrong phase. Six failing-first regressions + one round-trip: - shape B: bold **Phase:** 19 archive BELOW the section - shape C: plain archive Phase: 19 ABOVE the section - bootstrap h3 ### Current Position variant - CRLF variant - Phase token in decisions prose (over-broad-fix guard) - Paused At read-path parity with the write seam (## Session) - write-then-read round trip stays at 22 (read/write agreement) Folded into tests/state.test.cjs (lint:regression-test-names bans a new tests/bug-NNNN-*.test.cjs file). * fix(#2956): scope Phase extraction to ## Current Position at both seams Third generation of #2444 / #2567. Stopped At / Paused At were scoped to ## Session by those fixes; Phase (canonically in ## Current Position per templates/state.md) was left unscoped, so a historical Phase: / **Phase:** line in an archive section silently overwrote current_phase on every write. Because current_phase is routing input for gsd-progress / --next, the rewind routes work to the wrong phase. Fix mirrors the proven #2444 seam exactly: - new matchCurrentPositionSection helper (collectSection-based, CRLF-tolerant, level-flexible for the bootstrap ### Current Position h3 variant), sited next to matchSessionSection. - read path (cmdStateSnapshot): extract Phase from matchCurrentPositionSection ?? body. Also scope Paused At to matchSessionSection ?? body so the read seam agrees with the write seam (which already scoped Paused At to ## Session). - write path (buildStateFrontmatter): extract Phase from matchCurrentPositionSection ?? bodyContent. stateExtractField itself is untouched (its bold/plain precedence is load-bearing for other fields — the #3265 test depends on it), and preferNewerLastActivity is untouched (Last Activity has no canonical section; its date-direction guard is deliberate). Fall back to full-body when no ## Current Position section exists so files without the heading keep current behaviour. * chore(#2956): add changeset fragment (pr:0 placeholder, backfill after PR) * test(#2956): make round-trip test actually trigger the write-path resync The write-then-read round-trip test used 'state update Status "Executing"' on a fixture with no Status field, so the update was a no-op (updated:false) and no frontmatter resync ran through buildStateFrontmatter — the assertion on the written frontmatter then failed not because the fix is wrong, but because no write happened. Add a **Status:** field so the update performs a real field update (updated:true) and forces the resync. Verified locally: pre-fix this writes current_phase:19 (the archive value); post-fix it writes 22. The code fix is correct (5 of 7 RED tests passed; the 2 failures were this defective test). This is the 'fix the bad test' half of the TDD-loop rule. * chore(#2956): backfill changeset PR number 2961 --------- Co-authored-by: sim <sim@local>	2026-08-01 00:02:41 -04:00
Tom Boucher	9ac0dfad58	chore(#2929): generalize prompt-budget into the shared context-composer seam (#2958) ... * test(#2929): capture prompt-budget parity corpus pre-refactor Phase 2 of epic #1671 generalizes prompt-budget's trim ladder into a shared context-composer seam. Its success condition is that review-prompt output does not change, and the only authority on "did not change" is the behavior that shipped before the refactor. Capture that behavior now, while it is still the live implementation. 47 characterization cases, every `expected` value computed by executing the current implementation rather than hand-authored — the independence CONTRIBUTING.md "Fixture provenance (#2371)" asks for. A corpus is only worth what it can detect, so this one was validated by mutation rather than assumed. Five deliberate defects were injected and each must be caught by at least one case: - the note reserve deducted unconditionally instead of only under pressure - the pressure test relaxed from `>` to `>=` - a no-op head-shrink still setting the shrunk flag - the per-plan floor dropped from the proportional share - drop order reversed Two of those exposed real holes in the first cut of this corpus, and the cases that close them exist because of it: - `>=` was caught by NOTHING. At exact cap the only trimmable fragment was a floored plan group, and the 1024-char floor absorbed the entire trim, so the mutation was byte-invisible. A3b/A3c put a droppable at exactly the cap, which makes the strict inequality observable as context kept vs omitted. - No case reached proportional-truncate at all — B6 and B7 both hard-failed the min-set pre-check first, leaving planTruncationPct at 0 across every case and the floor semantics entirely unexercised. Rebudgeted to 700 and 1100 so the min-set fits and the truncate step is actually reached; they now record 40.20% and 48.80%. The A4/A10 families sweep the pressure boundary from both sides, which is where this function has regressed before: CONTEXT.md's LEARNING.prompt-budget.boundary-gap records PR #3708 shipping two regressions that only fired when the baseline sat inside the NOTE_RESERVE_TOKENS band, because the suite paired a trivially-fitting budget with a trivially-overflowing one and never sampled between them. A4 pins that nothing is trimmed from the cap down to 81 tokens under it; A10 pins that pressure fires at +1. Together with A3b/A3c they satisfy row (d) of RULESET.TESTS.boundary-coverage.fixtures. Two facts the corpus establishes that the design notes had wrong: - "" and null sections are NOT distinguished. applyBudget uses truthy checks throughout, so an empty-string section is treated as absent: not rendered, not dropped, never recorded in `omitted`. B13b pins this while the ladder is actively trimming, where only the non-empty `research` is dropped. - Sizing matters. B12/B13 were first written at a budget where both hard-failed the min-set check and returned "", so comparing them compared two empty strings and proved nothing. Committed as its own commit, ahead of the refactor, and regenerated against the pre-refactor implementation, so the oracle is demonstrably independent of the change it will adjudicate. Refs #2929 * refactor(#2929): extract the context-composer seam from prompt-budget Epic #1671 needs prompt-budget's budget-trimming logic for a second consumer — per-runtime artifact emission — but it is walled inside the cross-AI review pipeline. Lift it into a shared seam so later phases can call it, without changing what the review pipeline emits. ADR-1671 specifies the composer as "priority + binary-search cutoff to a per-runtime budget". Read against the code it generalizes, that contract cannot express the thing being generalized. applyBudget is not a cutoff: it is a fixed five-step ladder in which each section carries its own shrink strategy, and only three of its eight sections are ever dropped. PROJECT.md is head-shrunk to N lines; plans are proportionally tail-truncated with a per-plan 1024-byte floor; instructions and roadmap are never touched at all. A cutoff composer sorts by priority and discards the tail — it has no way to say "shrink this one", "truncate that one but never below 1 KB each", or "these three are the only droppables, in this order". Building to the literal contract and routing prompt-budget through it would have silently changed review-prompt output, which is the one outcome this phase forbids. So shrink strategies are the core abstraction here, and cutoff becomes one strategy among them — the right one for per-runtime emission in Phases 3-4, not for this ladder. That is an elaboration of the ADR's intent, not a departure from it, and ADR-1671 is updated to say so. Three decisions worth stating: - The composer DECIDES; the caller RENDERS. composeWithinBudget returns a plan of surviving fragments and never a string. assemblePrompt's rendering is prompt-shaped (`## Roadmap`, `### <file>`, the note in position two), and owning it in the composer would force emission to adopt prompt-shaped rendering. The split is what lets one seam serve both consumers. - The budget unit is INJECTED via `measure(text)`. prompt-budget passes its chars/4 estimator; emission will pass a byte counter, which ADR-1671 requires for emission caps. The existing code converts a token budget to a character budget with a hardcoded `* 4`; that assumption is now an explicit `charsPerUnit` inverse, which is precisely what a byte unit needs in order to reuse this. - The entry point is `composeWithinBudget`, not `applyBudget`. That name already exists twice — src/prompt-budget.cts and src/graphify.cts, the latter being an unrelated graph-edge budget. A third would make every symbol search in this repo ambiguous, and it already misresolves: preflight and impact queries for "applyBudget" return graphify's. Behavior is unchanged and proven so: all 47 characterization cases reproduce byte-identically, and the corpus is mutation-validated rather than merely green (see the preceding commit). prompt-budget.cts drops from 436 to 343 lines and from eighteen mutable accumulators to two, both inside a helper copied verbatim. estimateTokens deliberately stays in prompt-budget and keeps its exact math: src/phase-estimation.cts re-exports it as measureTokens, and CONTEXT.md pins plan estimates and recorded actuals to that same scale, so moving or changing it would silently break the calibration loop. Refs #2929 * docs(#2929): document the context-composer seam and amend ADR-1671 Adds the INVENTORY row, the CONTEXT.md glossary entry (a PR gate for new domain modules), and a mutation-matrix entry for the new module. The ADR amendment is the substantive part. ADR-1671 specified the composer as "priority + binary-search cutoff to a per-runtime budget". Implementing Phase 2 established that a cutoff alone cannot express the function the platform generalizes, so the ADR now records shrink strategies as the core abstraction with cutoff as one strategy among them, reserved for per-runtime emission in Phases 3-4. Recording it in the ADR matters because Phases 3-6 are planned against that contract and would otherwise be planned against a mechanism that does not work. The mutation-matrix entry is not bookkeeping. Stryker scores per module against a named .cjs, so relocating the ladder out of prompt-budget.cjs would leave the extracted code unmeasured while prompt-budget's own score floated free of the logic it used to cover. context-composer gets its own entry at the same floor. Refs #2929 * test(#2929): pin the effectiveBudget rounding mode in the parity corpus An isolated correctness review found a real blind spot: mutating `Math.floor` to `Math.round` in the effectiveBudget calculation failed ZERO of the 47 corpus cases. Every (budget, safetyMarginPct) pair in the generator happened to produce a whole number, so floor, round and ceil all agreed and the rounding mode was entirely unpinned by a corpus whose whole job is to pin observable behavior. Three cases fix that by straddling the .5 boundary: A11 95 * 0.90 = 85.5 floor 85, round 86 -> the two disagree A12 97 * 0.90 = 87.3 floor and round agree; ceil (88) does not A13 93 * 0.85 = 79.05 same guard at a non-multiple-of-10 margin, so the margin arithmetic is exercised and not just the budget A11 alone catches the round mutation; all three catch ceil. Regenerated against the pre-refactor implementation (`git show 9557f8552:src/prompt-budget.cts`), so the expanded corpus keeps the independence property the original capture had. The corpus is now mutation-validated against seven injected defects, every one caught: unconditional note reserve, `>` relaxed to `>=`, no-op head-shrink setting its flag, the truncate floor ignored, drop order reversed, and both rounding-mode changes. Refs #2929 * feat(#2929): flexReserve floors and the byte-stable isolate prefix Two of issue #2929's "Done when" items were unimplemented rather than deferred, and an isolated review flagged them alongside my own audit. Both are part of ADR-1671's composer contract, so shipping the seam without them would have left Phases 3-4 building against a contract that does not exist yet. flexReserve is a per-fragment floor in measure units that every strategy must respect, which is what makes it different from the pre-existing floorChars: that one is a chars-denominated detail of proportional-truncate alone and is retained unchanged. A floored fragment is never dropped, is never head-shrunk below its floor, and raises its own proportional cap. A fragment already smaller than its floor is untouchable outright. Metadata gains `floored`, listing the ids whose floor actually prevented a trim — a guarantee no caller can observe is a guarantee no test can hold you to. isolate marks the byte-stable canonical prefix the ADR calls for: never trimmed, never dropped, but still counted, because a prefix excluded from accounting would silently under-count real context. Metadata gains `isolatePrefix` so a caller can hash or assert on the exact bytes. Declaring an isolate fragment after a non-isolate one throws: a prefix that is not at the front is not a prefix, and accepting it would make the cross-runtime stability claim meaningless. Adds tests/context-composer.test.cjs for the exact new semantics and tests/context-composer.property.test.cjs for the five invariants, including the budget-monotonicity property the issue names explicitly. Both are registered in the mutation matrix, since coverage does not migrate with relocated code. prompt-budget uses neither feature, and its output is unchanged: all 50 corpus cases still reproduce byte-identically. Refs #2929 * chore(#2929): allowlist the prompt-budget parity suite The parity corpus needs its own test file and that makes prompt-budget a three-file module against a limit of two. The lint offers consolidation or an allowlist entry with justification; the entry is the right call here. Consolidation would mean folding the characterization suite into prompt-budget.test.cjs, which is the one thing that should not happen to it. The parity suite is a distinct concern with a distinct lifecycle: it is generated rather than hand-written, it is named by scripts/mutation-matrix.cjs as its own scoring target, and its failure means something categorically different from a unit-test failure — not "this behavior is wrong" but "observable output moved". Burying it inside a general unit file would obscure exactly that signal. The allowlist is an identity ratchet, so this entry pins today's three exact filenames: adding a fourth still fails, and dropping back to two requires removing the entry. Refs #2929 * fix(#2929): register the new module with two gates it was missing The remote matrix caught three defects that no local check could, because the local runner is blocked in this repo and these suites had therefore never executed. Eight failures, identical on node22 and node24, so nothing environment-shaped. Two are the new-module ripple. A net-new src/*.cts lands in six places and this change had reached four of them — .gitignore, INVENTORY, the manifest, and the CONTEXT.md glossary — while missing the ESLint ignore list (tsc OUTPUTS must not be linted; repo-invariants asserts linted-xor-ignored) and the mutation ratchet baseline (a deliberate review-visible mirror of the matrix floors, which every COVERED module must carry). Both are now registered, the ratchet at the same floor of 66 the matrix declares. The third was a test asserting an outcome it had made impossible. It set budget:1 alongside a 400-char required fragment, so the group budget came out at -99 and the proportional-truncate step was skipped entirely — the deliberate "non-positive group budget is skipped, never clamped" rule inherited from the original ladder. Nothing was trimmed, and the test then asserted a truncation. Rebudgeted so the step actually runs, with the arithmetic written out in a comment so the next reader does not have to re-derive why 120 rather than 80. Fixing that surfaced a genuine bug in the composer. `floored` is documented as recording fragments whose flexReserve prevented a trim that would otherwise have happened, but the push sat in the else-branch of "content did not change", so it only fired when nothing was trimmed at all. A fragment truncated to a reserve-raised cap has also had a trim prevented — 40 characters' worth in the test above — and was silently absent from the field that exists to make the guarantee observable. The condition was already right; it was in the wrong branch. Now recorded on both paths: a drop prevented outright, and a truncation capped higher than the share alone would have allowed. Parity is unaffected — prompt-budget never sets flexReserve, so the branch is unreachable from every corpus path, and all 50 cases still match. Refs #2929 * chore(#2929): backfill changeset PR number (#2958) * chore(#2929): correct the corpus case count in the changeset fragment --------- Co-authored-by: sim <sim@local>	2026-07-31 23:03:13 -04:00
Daniel Einspanjer	f0ff23635e	fix(#2602): discover project-local Codex agents (#2623) ... * fix(#2602): discover project-local Codex agents - Select an existing local Codex agents directory before global fallback - Prove init reports the canonical local installation through compiled CJS * test(#2602): lock Codex agent precedence - Cover override, local authority, global fallback, and runtime compatibility - Exercise installed state through the compiled resolver * fix(#2602): resolve local Codex agent skills - Pass the canonical project root to the non-Claude persona fallback - Cover nested-Codex fallback and Claude compatibility through the CLI * test(#2602): cover local Codex validation status - Assert emitted validate and health commands use the project-local install - Preserve empty local-directory authority beside complete global agents * fix(#2602): align validation with local Codex discovery - Pass the resolved runtime and project root to health W010 - Resolve the validate-agents runtime before checking installation status * test(#2602): cover local Codex docs status - Assert docs-init reports an authoritative empty local install as unhealthy * fix(#2602): align docs with local Codex discovery - Pass the resolved runtime and canonical project root to the shared agent checker * fix(#2602): honor agent-skills runtime override - Resolve agent-skills fallback runtime through the canonical project resolver - Cover conflicting config and GSD_RUNTIME values through the emitted CLI * fix(#2602): ignore non-directory local agents paths - Treat only a local Codex agents directory as authoritative - Cover regular-file fallback through the emitted install checker * chore(#2602): add changelog fragment - record the user-visible local Codex agent discovery fix for PR #2623 * fix(#2602): align local agent discovery with runtime policy - Resolve Codex's local config directory through the canonical runtime policy - Use test-managed cleanup for local-agent discovery coverage * fix(#2602): discover local agents across runtimes - Prefer manifest-backed project-local installs for non-Claude runtimes - Respect runtime-specific local install roots and preserve global fallback behavior - Cover native, partial, cross-runtime, and project-root local discovery * fix(#2602): preserve agent discovery fallback - Fall back globally when local-install probes fail - Document and test symlink rejection - Align the changeset with repository format * fix(#2602): reuse local directory policy - Resolve runtimes without local config through the canonical sentinel - Document the manifest gate and refresh the context index --------- Co-authored-by: Daniel E. <daniel.e@teachingstrategies.com> Co-authored-by: Rezolv <dave@sienkowski.com>	2026-07-31 21:20:46 -04:00
JusticeWay	7b204ad2ac	enhance(#2530): extend UAT checkpoint frame language pack (9 more languages) (#2564) ... * feat: extend UAT checkpoint frame language pack (9 more languages) response_language is a free-form config value, but CHECKPOINT_FRAMES only covered 9 languages — any other configured language silently fell back to the English frame. Add Dutch, Polish, Russian, Ukrainian, Turkish, Hindi, Arabic, Vietnamese, and Indonesian frames plus their aliases, with a regression test asserting each resolves instead of falling back. Follow-up to #2402 (PR #2457). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: add changeset for #2527 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(#2530): list UAT checkpoint frame languages in CONFIGURATION.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(#2530): point changeset fragment at PR #2557 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2530): address Unicode language-pack review * fix: address checkpoint language review * fix: count spacing combining marks in checkpoint width * test: verify checkpoint aliases structurally * fix: isolate RTL checkpoint frames * fix: isolate RTL checkpoint frames correctly * test(#2530): assert checkpoint aliases neither collide nor go unreachable Review Minor #1. A duplicate alias key was invisible to the existing catalog tests: the runtime object is well-formed after JS collapses the literal, the self-alias assertion still holds, and the losing language just stops resolving. tsc catches the byte-equal case (TS1117), but not the two that survive compilation — an alias whose NFC-lowercase form already belongs to another language, and an alias not in lookup form at all, which resolveCheckpointFrame() can never produce. The check reads the source literal rather than the object, since the object no longer records what was written. Both assertions are independently load-bearing: an NFD twin of an existing alias trips the collision check, an uppercase alias trips the unreachability check. Review Minor #2: changeset retyped Changed -> Added. Nine wholly new supported response_language values are an addition under Keep a Changelog, not a modification of existing behavior. * test(#2530): check alias collisions on the catalog, not its source --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> Co-authored-by: Rezolv <dave@sienkowski.com>	2026-07-31 21:01:50 -04:00