900504f9852862c33c71b0d2691fe2d047d8d6ea
2958 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
0xdhx
|
900504f985 |
fix(#3776): decide nothing-to-commit from the staged diff, not from staging success (#3859)
* fix(#3776): decide nothing-to-commit from the staged diff, not from staging success
`cmdCommit`'s empty-diff guard tested `stagedPaths.length === 0`, but
`stagedPaths` records paths whose `git add` exited 0 — "did staging
succeed", not "is there anything to commit". Staging an already-committed,
unmodified file succeeds while contributing no diff, so the guard was
reachable only when every named path was missing from disk.
The ordinary empty-diff case therefore fell through to `git commit`, where
the only thing converting the failure back to `nothing_to_commit` was a
string match on git's output. Git runs the pre-commit hook before it
decides there is nothing to commit, so a rejecting hook pre-empted that
match and the caller was handed `commit_failed` carrying a gate message
about a commit that had nothing to gate.
Ask git whether the staged paths actually differ instead. Two conjuncts
are load-bearing: the `length === 0` short-circuit keeps the
all-missing-paths case exact (a pathspec-less `diff --cached` would test
the whole index, so unrelated staged work would suppress the guard), and
`!isMergeInProgress` keeps a merge from being abandoned — during a merge
git refuses a partial commit, so the pathspec describes nothing about what
would land.
Nine regression cases in tests/commands.test.cjs cover the brief's six
acceptance criteria plus the merge interaction. Against the pre-fix build
exactly one fails; the other eight pin behaviour that was already correct.
Residual sibling of #2608/#2693, which covered `git add` failing; this
covers `git add` succeeding and contributing nothing.
* fix(#3776): exempt a cherry-pick too, but never a revert
The empty-diff guard must not decide from a pathspec git will not honour.
That test was merge-only; git refuses a partial commit during a cherry-pick
for the same reason, so the guard would have fired there and reported a
silent `nothing_to_commit` where the pre-fix code surfaced git's refusal.
The three sequencer states do not agree, so this is driven rather than
reasoned by analogy (git 2.54):
MERGE_HEAD fatal: cannot do a partial commit during a merge.
CHERRY_PICK_HEAD fatal: cannot do a partial commit during a cherry-pick.
REVERT_HEAD permitted; behaves like an ordinary commit.
REVERT_HEAD is therefore deliberately excluded: enumerating it alongside the
other two — the obvious move — would suppress this fix during a revert and
reintroduce the very misreport it removes. Both new states are pinned by a
test, and the revert arm fails against the pre-fix build exactly as AC1 does.
`canScope` keeps its narrower merge-only test on purpose; widening it would
change pre-existing cherry-pick behaviour, which is outside this fix.
* fix(#3776): probe the working tree, not the index
`git commit -- <paths>` is a PARTIAL commit: it records the working-tree
content of those paths and ignores what is staged. The guard was probing
`git diff --cached` — the index — which answers a different question than
the commit asks.
Driven on the same path, in this order: `git add` an unmodified file, then
write to it, then probe.
git diff --cached --quiet -- p rc 0 ("nothing staged")
git diff --quiet HEAD -- p rc 1 ("the tree differs")
git commit -m m -- p committed the new content
So a working-tree write landing between the `git add` above and the probe —
another process in a shared checkout, which this project explicitly supports
— would let the guard report `nothing_to_commit` for a call that would have
recorded that content. Probing `HEAD` asks the question the commit answers.
Not reachable through this function single-threaded, because the staging
loop re-adds every named path immediately beforehand, so index and working
tree agree at the probe. The change is correctness by construction rather
than a fix for an observed miscommit.
An unborn HEAD makes `diff HEAD` fatal; that falls through to the commit as
any other probe error does, and is now pinned by a test — the first commit
in a repo must not be swallowed by an empty-diff guard.
Both added probes are now gated on the guard being able to fire at all, so
an unscoped commit and an `--amend` pay for neither.
Found by adversarial pre-filing review; the index/worktree distinction was
not something my own path-shape probes could have surfaced.
* test(#3776): pin the assume-unchanged boundary; correct a stale comment
`git update-index --assume-unchanged` makes `git add` stage nothing and makes
BOTH diff forms — `--cached` and `HEAD` — report no difference, so no
diff-based guard can see a change to such a path. `git commit -- <path>` is
the odd one out: it reads the working tree directly and records it.
So a modified assume-unchanged path now reports `nothing_to_commit` where it
previously committed. That is the answer consistent with this function's own
staging step, which honoured the flag one loop earlier — but it is a
behaviour change, and it belongs on the record as a decision rather than
surfacing later as a surprise.
Also corrects an AC3 comment still describing the `diff --cached` whole-index
form that the previous commit replaced.
* chore(#3776): set changeset fragment pr to 3859
The fragment carries the PR's own number, which is unknowable before the PR
exists. Backfilled post-create; the repo's changeset lint rejects the `pr: 0`
placeholder.
* fix(#3776): do not read an unanswered sequencer probe as "no merge"
`execGit` surfaces a spawn timeout as `exitCode: 1` (`_spawnResult`:
`result.status ?? 1`) — the same code `rev-parse --verify` returns for a ref
that does not exist. So the MERGE_HEAD and CHERRY_PICK_HEAD probes could not
tell "not in that state" from "never answered", and the empty-diff guard read
both as "not in that state". That is the one path in #3776 that did not fail
toward the previous behaviour: a timeout during a real merge decided
`nothing_to_commit` from a pathspec git will not honour and left the merge
unconcluded, where before it was a loud `commit_failed`.
Treat an unanswered probe as "assume the partial commit would be refused" —
which falls through to `git commit` and lets git speak for itself.
Routed into `partialCommitRefused` only, deliberately never into
`isMergeInProgress`. That flag also feeds the pre-existing `canScope`, and
widening it there is worse than the misreport it fixes: with `canScope` false
the commit runs bare, and a bare commit during a merge is PERMITTED — git
concludes the merge with the whole index under a message naming one file.
Driven: the whole-flag form reports `committed` where this form reports
`commit_failed`, and it drops the pathspec on the ordinary timeout, re-opening
the #2112 scope leak.
* fix(#3776): pin the empty-diff probe against diff-only configuration
`git diff` is porcelain and honours settings `git commit -- <paths>` does not,
so an unpinned probe let a caller's configuration decide whether the guard
fires. Driven against git 2.54, each with the paired `git commit -- <path>`
confirmed to record the change the unpinned probe reported as absent:
diff.ignoreSubmodules=all a gitlink bump is invisible to the probe
.gitmodules ignore = all the same, and it needs NO local config at
all — it is checked in, so it arrives with
a clone
diff=<driver> + textconv two different blobs converge to one text,
so the probe sees no change; no submodule
involved
`--ignore-submodules=dirty` rather than `=none`, because `dirty` is what a
partial commit of a submodule path actually means: it records the gitlink,
which moves only when the submodule's HEAD does. Under `=none` a merely dirty
submodule work tree reports a difference the commit would not record, sending
an empty call back to `git commit` — the same misreport, re-entered from the
other side. `dirty` still overrides both `diff.ignoreSubmodules` and a
checked-in `.gitmodules` `ignore`, so the gitlink vectors stay closed.
`--no-ext-diff` is deliberately absent: `--quiet` short-circuits ahead of an
external diff driver, so `diff.<driver>.command` cannot invert the probe
(driven: rc 1 with and without the flag).
* docs(#3776): disclose the two outcome changes the changeset omitted
The body listed what stays unchanged and never named the arms whose
user-visible outcome moves, so neither would have reached the changelog:
- a modified path under `git update-index --assume-unchanged` now reports
`nothing_to_commit` where it was previously committed. `git add` already
honoured the flag one loop earlier; the guard reports what staging did.
Documented in a code comment and pinned by a test since the first round,
but absent from the fragment.
- naming a submodule whose work tree is dirty while its recorded commit has
not moved now reports `nothing_to_commit` rather than `commit_failed`,
because nothing would have landed. New in this round, from the
`--ignore-submodules=dirty` pin.
* test(#3776): use helpers.cleanup() for the submodule fixture teardown
`local/no-raw-rmsync-in-tests` rejects a bare `fs.rmSync` in a test: the
helper carries the Windows-EBUSY retry budget (`maxRetries`/`retryDelay`)
that a raw call does not, and a submodule work tree is exactly the shape
that holds handles open on Windows.
Caught by CI, not locally — the round ran the two affected suites but not
`npm run lint:ci`, so the repo's own rule never fired until the push. The
chain now exits 0 locally against this tree.
* fix(#3776): never drop a named assume-unchanged path
`--assume-unchanged` is the one state where `git diff` and
`git commit -- <paths>` genuinely disagree: `git add` stages nothing,
both diff forms report no difference, and `git commit -- <path>` still
reads the working tree and records it. The empty-diff guard therefore
reported `nothing_to_commit` about content the caller named in `--files`
and git would have written.
commit is made — so suppressing its misreport must not be paid for by a
silent drop. Same rule the timeout routing already follows: a fix for a
misreport may not cost content.
The guard now asks `git commit --dry-run --porcelain` whether the commit
would record anything, and stands aside on rc 0. That is the same
decision the real commit makes, so there is no second implementation of
it to drift. It does not run the `pre-commit` hook (driven: a rejecting
one neither fires nor writes a marker), which is what matters — a firing
`pre-commit` is the whole of #3776. It is NOT hook-free in general: git
2.54 fires `post-index-change` here, so a repo using that hook sees it
once for the probe and once for the commit. Stated rather than claimed
away.
Asking git rather than reconstructing its answer was reached by
measurement. Comparing `git hash-object` against `HEAD:<path>` was tried
and is wrong three ways, each a silent drop of named content: it misses a
mode-only change (`chmod +x` leaves the blob identical while the commit
records `100755`); it cannot hash a submodule path at all (`fatal: Unable
to hash sub`, while the commit advances the gitlink); and the path it
needs must be parsed out of `ls-files` output, which `core.quotePath`
renders as `"caf\303\251.md"` by default. Each has its own arm, and the
non-ASCII arm pins `core.quotePath` so it cannot go vacuous.
Falling through on the `ls-files` tag alone — without asking whether
anything would land — is also wrong: an UNMODIFIED assume-unchanged path
would reach `git commit`, which with any unrelated modified file present
prints `no changes added to commit`, a string the fallback does not
match, and returns `commit_failed`. That is #3776 re-entered from the
other side, the same shape `--ignore-submodules=none` would have
re-entered it. Pinned by its own arm.
The `ls-files` read is an optimisation, not a gate: it keeps the dry run
off the hot path when no assume-unchanged entry is present, and when it
cannot answer the dry run simply runs, because the dry run needs nothing
from it. Failing closed there would drop content and failing open would
re-enter #3776 — both are wrong answers to a question that can be asked
directly.
`--skip-worktree` is not a second instance. A present, modified one exits
1 from `git add` and fails closed as `staging_failed` above the guard; an
absent one is skipped before `git add` runs (#2014) and is answered by
the `stagedPaths.length === 0` arm, exactly as it was pre-fix. Both
shapes pinned, because the shorter claim ("never reaches the guard") is
too strong.
* test(#3776): register fixture teardown so a failed assertion cannot leak
`bumpedSubmodule()` creates its sub-repo as a SIBLING of `tmpDir`, and
the unborn-HEAD arm creates `fresh` outside it too, so the describe's
`afterEach(() => cleanup(tmpDir))` reaches neither. Both were cleaned by
a trailing statement in the test body, which any failing assertion above
it skips — leaking a git repo into the temp root.
`bumpedSubmodule()` now records the path and a describe-scoped
`afterEach` drains it, which covers all three of its callers at once;
the unborn-HEAD arm takes `t.after`, the form already used elsewhere in
this file.
Negative-controlled both ways with a deliberate assertion failure
injected into the dirty-submodule arm, under an overridden TMPDIR:
before, one `*-sub` repo survives the run; after, none.
* fix(#3776): never read an unanswered dry-run probe as "nothing to record"
The `git commit --dry-run --porcelain` probe that decides the assume-unchanged
boundary is the one probe in the guard whose rc 0 is the reassuring answer, so
it inverts the diff probe's safety: `execGit` collapses a spawn timeout (or any
spawn error) to `exitCode: 1`, byte-identical to git's own "nothing to record",
and the guard then reported `nothing_to_commit` about content named in
`--files` that git was never asked to write. Same conflation the sequencer
probes already defend against.
Only a CONFIRMED rc 1 with no spawn error closes the path now; a timeout, a
spawn error, or rc 128 falls toward the real commit, where git speaks for
itself. Five arms in tests/commit-files-pathspec.test.cjs pin it (posix +
windows timeout shapes, rc 128, the ls-files optimisation's own timeout, and a
negative control on an unmodified path); the injection helper gains an optional
`matchArg` so the dry run can be targeted without intercepting the real commit.
Also corrects the comment that claimed both sequencer probes are gated on
`guardApplies` — the MERGE_HEAD probe predates this fix and is unconditional.
* fix(#3776): probe with --no-verify so a hook-firing git cannot close the guard
Round 4, review finding 3 (Minor). The `git commit --dry-run --porcelain`
probe's safety rested on an empirical claim about one git version: that
`--dry-run` does not run `pre-commit`. git 2.54 satisfies it, but the failure a
differing version would produce is silent and lands in exactly #3776's own
configuration.
A `pre-commit` that fires and rejects exits 1 — the same code git returns for
"nothing to record" — so the closure would read it as a CONFIRMED empty answer,
drop the content the caller named in `--files`, and report `nothing_to_commit`.
That is #3776 re-entered through the probe the fix added.
`--no-verify` forecloses it structurally rather than documenting the version
dependency. Driven on git 2.54: rc-identical in both directions (rc 0
would-record, rc 1 nothing) with and without the flag, so it is behaviour-
neutral where the version already agrees.
Two claims deliberately NOT widened: `--no-verify` does not suppress
`post-index-change`, which still fires on this call with or without it (driven
both ways); and the real `git commit` is untouched — #3776 is a bug about a
hook's message reaching the caller wrongly, never a licence to skip hooks.
The new arm pins the FLAG rather than an outcome, because the outcome it
protects is unobservable on a git that already declines to run the hook. It is
a seam assertion over the argv the guard actually issued, not a source grep.
* test(#3776): pin all-missing --files during a merge or cherry-pick
Round 4, review finding 1 (Major) and finding 7 (Nit, its coverage half). The
review asks for the `stagedPaths.length === 0` disjunct to be gated on
`!partialCommitRefused`, or for the combination to be documented and tested.
Documented and tested — the gating is refused, with cause.
The premise is confirmed: the state is reachable exactly as described, and
during a merge the `nothing_to_commit` report does not tell the caller the merge
is still open. The prescription is not. With every named path missing,
`stagedPaths` is empty, so `canScope` is false and the fall-through reaches a
BARE `git commit`, which git PERMITS during a merge and which then CONCLUDES it.
Driven, git 2.54, through cmdCommit with the prescription applied:
cmdCommit(cwd, 'add the thing', ['.planning/never-produced.md'])
-> { "committed": true, "hash": "8e6bf45", "reason": "committed" }
MERGE_HEAD gone; HEAD is a 2-parent merge commit recording
.planning/shared.md with the caller's resolution content.
So the gating trades a report that writes nothing for one that silently writes
the whole index under a message naming a path that does not exist, and reports
success. That is the same trade the timeout routing already refuses one block
up, which is why the sequencer states gate the DIFF branch only.
The behaviour is also pre-existing and unchanged by this PR: at
|
||
|
BeeHiggs
|
41466e8e88 |
fix(#4023): preserve decimal phase ids in init progress ordering and smart-entry output (#4110)
* test(#4023): reproduce decimal phase-id coercions * fix(#4023): preserve decimal phase ids in progress signals * test(#4023): align phase token contract expectations * chore(#4023): point the changeset at PR #4110 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
80d2236ca5 |
fix(#4112): drop redundant $(( subshell wrapping in pause-work.md Context Detection (#4140)
* test(#4112): add failing-first regression coverage for pause-work.md Context Detection Extracts and executes the shipped Context Detection bash block to prove the $(( arithmetic-context misparse (SC1102/SC1106/SC2205) as a hard syntax error, plus boundary/independence coverage for the phase/spike/sketch/ deliberation resolution the fix must preserve. * test(#4112): fix false-green regression test — assert dash/sh failure, not bash -n The prior version asserted bash -n syntax validity and wrapped bash -c execution, neither of which detects this bug: bash/zsh silently fall back to a working command-substitution reading of the ambiguous $(( construct, and bash -n never evaluates arithmetic-context content at parse time. A gsd-test run against the still-broken file passed 41594/41594 with this test in place — a false green. POSIX sh/dash does not implement that fallback and throws a real "Syntax error: Missing '))'", which is what this version asserts against. * fix(#4112): remove leaked tool-call tags corrupting the regression test file A subagent's Write introduced trailing </content>/</invoke> markup at the end of tests/pause-work-context-detection.test.cjs. gsd-test's own tests/portability-rule-disable-ban.test.cjs (which parses every test file) correctly caught this as a parse error. Strips the garbage lines; no behavioral change. * fix(#4112): drop redundant $(( subshell wrapping in pause-work.md Context Detection phase=/spike=/sketch= opened with $((, which POSIX sh/dash parses as arithmetic expansion and rejects (Syntax error: Missing '))') since the enclosed text isn't valid arithmetic. bash/zsh silently retry it as command substitution, which masked the defect there. Dropping the inner grouping parens (matching the deliberation= line already in the same block) makes the construct valid under bash, zsh, and dash alike, with identical fallback-to-empty-string behavior when nothing matches. Once the surrounding syntax parses, ShellCheck can now also analyze the inner ls -lt calls it previously couldn't see past the parse failure, newly surfacing the same pre-existing SC2012 ("use find instead of ls") suggestion already baselined for the file's other ls usage. Baseline updated to reflect the true current count; no ls-vs-find behavior change made, as that is a separate, pre-existing, out-of-scope question. * fix(#4112): verify shell discrimination instead of trusting the binary name Code review finding: falling back from dash to sh could silently produce a non-discriminating test on a host where /bin/sh is bash-compatible (e.g. macOS) — such a shell never throws on the broken construct either, so the test would pass whether the bug were present or not. resolvePosixShell now verifies the candidate actually rejects a known-ambiguous $(( snippet before using it, and skips with an explicit reason when none does. The gsd-test Linux bench (dash as /bin/sh) is unaffected either way. * fix(#4112): route regression test through process-seam, splitLines, cleanup npm run lint:ci flagged 7 violations gsd-test's node:test run doesn't check: local/no-adhoc-markdown-parsing (single fence-spanning regex), local/no-crlf-fragile-split (bare \n split), local/no-unbounded-spawn (x4, hand-rolled execFileSync with no timeout), and local/no-raw-rmsync-in-tests. Rewrites the test to route every subprocess call through tests/helpers/process-seam.cjs's runHook() (bounded by construction), tests/helpers.cjs's cleanup() for temp-dir removal, and a line-by-line fence scan using the text-lines.cts splitLines() seam, matching this repo's established extraction idiom (tests/no-hardcoded-home-gsd-tools.test.cjs). No behavioral change to what is asserted. * docs(#4112): add changeset for the pause-work Context Detection fix * docs(#4112): backfill changeset PR number (pr:0 -> pr:4140) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
cad70f4f3e |
fix(#4120): replace shellcheck npm dep with dependency-free downloader (#4121)
* fix(#4120): replace shellcheck npm dep with dependency-free downloader The `shellcheck` devDependency (added in #4109) pulled in decompress@4.2.1 for archive extraction, which carries an unpatched CRITICAL zip-slip vulnerability (GHSA-mp2f-45pm-3cg9, CVSS 9.1) plus two moderate findings. decompress's latest published version IS the vulnerable one -- no patched release exists upstream, so npm audit fix cannot resolve this by upgrading. Removes the shellcheck package entirely and replaces its role with scripts/lib/shellcheck-fetch.cjs: a small downloader using only Node's built-in https/zlib plus a hand-written tar-entry reader, fetching a pinned koalaman/shellcheck release directly from GitHub releases. The reader never uses an archive-supplied name as a filesystem path (the exact defect class decompress had) -- it only returns the matched entry's bytes; the caller writes those bytes to a path it constructs itself. Bounds the download with a 30s-per-hop timeout, consistent with the ShellCheck subprocess's own timeout. Covers linux/darwin on x86_64/aarch64, matching this repo's actual CI (lint-tests runs only on ubuntu-latest) and local dev needs; Windows fails with a clear, honest error rather than silently misbehaving. Adds tests/lint-workflow-shellcheck-fetch.test.cjs covering the tar-parser (unit cases plus a fast-check property test per CLAUDE.md's parser-testing requirement), a security behavioral pin confirming traversal-style entry names are treated as opaque strings never filesystem paths, and boundary coverage for the redirect-following logic's MAX_REDIRECTS limit (limit-1/limit/limit+1, via an injectable transport, no real network I/O). npm audit: 0 vulnerabilities (was 1 critical + 5 moderate). The lint script reproduces the identical result against the current tree: "212 pre-existing finding(s) from baseline, 0 new" -- no behavior regression, no baseline changes needed. * fix(#4120): register shellcheck-fetch.cjs with the installer scripts/lib/shellcheck-fetch.cjs shipped without being added to GSD_SCRIPTS_LIB_FILES in bin/install.js, which would have left it orphaned on uninstall and broken the golden install-tree fixtures for every runtime. Adds the entry and regenerates the 19 affected fixtures via npm run gen:install-tree. * docs(#4120): add changeset for the decompress CVE fix --------- Co-authored-by: sim <sim@local> |
||
|
Cody Anderson
|
8c9265d4e5 |
fix(#3724): warning-only Dimension 3b findings no longer force the revision loop (#3758)
* fix(#3724): stop advisory Dimension 3b findings from forcing the revision loop Dimension 3b (undeclared/temporal coupling, #1954) is spec'd "never a blocker" but tagged severity: warning — the tier plan-phase's revision loop counts as must-fix — and the planner is never taught the rule, so every multi-wave phase touching shared mutable state replans at least once, and intentionally coupled plans re-flag identically every iteration to the stall prompt. Three coordinated changes: - gsd-plan-checker: retag 3b to severity: info, the tier references/revision-loop.md already exempts by design; recognize a coupling_justified frontmatter declaration in the Do-NOT-flag list so deliberate pairs converge. Additions are offset by trimming 3b motivation prose — the checker sits 45 bytes under its LARGE hard cap. - plan-phase step 12: INFO-only accept — an issues block with zero BLOCKER/WARNING entries accepts the plan and surfaces the advisories instead of re-entering the revision loop. Real blockers and warnings still gate unconditionally. - gsd-planner: slim pointer in assign_waves to the new progressive-disclosure reference gsd-core/references/planner-coupling.md (the planner sits 19 chars under its own cap), which carries the shared-mutable-state rule and the coupling_justified escape hatch so first-pass plans avoid the finding when the coupling is unintentional. Documented the coupling_justified field in docs/reference/plan-md.md. Growth acks per #2914; inventory manifest and install-tree fixtures regenerated for the new reference file. Closes #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): pin Dimension 3b at severity: info The severity retag makes the old assertion (severity: warning) stale; lock the advisory tier from both directions — info must be present, warning must not — so a future edit cannot silently re-arm the revision-loop trigger. Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * chore(#3724): changeset fragment for PR #3758 Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * docs(#3724): roster planner-coupling.md in docs/INVENTORY.md The new reference was enumerated in the manifest and all 19 install-tree fixtures but missing its row in the Modular Planner Decomposition table — the roster half the manifest-sync test cannot check. (Review Blocker.) Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): cover all four acceptance criteria (review round 1) - plan-checker-coupling: the 3b severity assertion is now a PARITY check deriving the exempt tier from revision-loop.md's flow instead of hardcoding info — editing either side alone reds the suite. New describe pins the other three criteria: plan-phase's INFO-only accept clause (proven failing-first), the BLOCKER + WARNING count staying intact, the coupling_justified Do-NOT-flag exemption + fix_hint, and the planner pointer + planner-coupling.md content. - ack fragment: $comment's plan-phase figure corrected to +79B; the 2775 pin note carried forward into the gsd-planner.md entry, updated for upstream's #3761/#3764 Rule-paragraph anchor (which this diff leaves verbatim). The parallel-dependent-plans re-anchor this commit originally carried was superseded by upstream #3764 during review; this branch no longer touches that file. Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 2 — align the stance enumeration, complete the template contract MAJOR: <adversarial_stance>'s severity enumeration gains the INFO bullet so it agrees with Dimension 3b's 'ALWAYS INFO' mandate instead of contradicting it. Funded by extracting the inline <examples> block to the new progressive- disclosure reference gsd-core/references/plan-checker-examples.md (@-inlined from the same spot; #1949 precedent), which also restores the 3b motivation clause round 1 traded away (Nit 4) and nets the agent file SMALLER than base (49107 -> 48486) — the extraction the byte pressure was owed. MINOR: gsd-core/templates/phase-prompt.md now carries coupling_justified, and the field's shape becomes one 'plan-id: reason' string per coupled peer so a plan justified against two peers can express it; docs/reference/plan-md.md's Type column names the shape. NIT: the 3409 ack's plan-phase entry no longer calls the #1168 workflow ratchet an 'XL tier'. Acks and derived artifacts updated accordingly (checker entry removed — a shrink needs no ack; INVENTORY roster row + regen:derived for the new file). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): derive the 3b negative severity assertion (review round 2) Every severity token in the 3b span must BE the tier revision-loop.md exempts, replacing the hardcoded severity:warning negative — if the loop's exemption ever moves, the failure names the real conflict instead of blaming the agent file with a mutually-unsatisfiable pair. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): refit the planner coupling pointer under the char cap Upstream #3299 (PR #3390) grew agents/gsd-planner.md to 49146 chars at the base, leaving 5 chars of headroom where the +16-char pointer was measured against 13 more. The pointer prose shortens to 'Non-file coupling:' — 49150 chars, back under the strict 49152-char cap — and the ack figures follow. The @-path the tests pin is unchanged. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): re-home the plan-phase ack after the #3823 spent-fragment sweep Upstream #3078/#3823 deleted all fully-spent ack fragments, including 3409-unreachable-guard-arms.json, which carried this PR's plan-phase.md +79B append. Per the collision remedy that sweep added: take the deletion and home the still-live entry in this PR's own fragment. Figures re-measured at this merge base (90871 -> 90950 LF bytes). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): absorb the spent #3172 plan-phase fragment into this PR's ack Upstream #3825 shipped 3172-stated-failing-direction.json naming only plan-phase.md, now spent at the base — colliding with this PR's live plan-phase entry. Per the #3003 pattern the fully-spent single-path fragment is deleted and this fragment stays the path's one source; figures re-measured at this base (93073 -> 93152 LF bytes). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 3 — true up the ack figures, restore the wave comment The fragment's absolute sizes are re-measured and anchored to base |
||
|
Tom Boucher
|
e242d85c92 |
fix(#4109): rewrap unquoted DISPATCH_SLUGS-shaped consumption to survive zsh (#4116)
* test(#4109): add zsh regression coverage for gate-check and invoke_reviewers dispatch Extends tests/review-plan-coverage-manifest.test.cjs to cover the two DISPATCH_SLUGS consumption sites the existing #3301 coverage-check tests don't reach: the write_reviews gate-check block (ALL_LANES_SKIPPED / TOTAL_LANE_FAILURE counting) and invoke_reviewers' dispatch + join loops. Both extract the real shipped bash from review.md and run it under bash and zsh, same convention as the existing coverage-check rows. Expected RED under zsh pre-fix, GREEN post-fix. * fix(#4109): rewrap unquoted DISPATCH_SLUGS-shaped consumption to survive zsh Bash word-splits an unquoted scalar on IFS by default; zsh does not (no setopt SH_WORD_SPLIT anywhere in these files), so a scalar accumulated from multiple space-separated tokens and then consumed via bare `for x in $VAR` collapses onto one bogus iteration under zsh whenever it holds 2+ tokens. Fixes all four review.md sites reported by #4108/#4109 (invoke_reviewers dispatch + join loops, the gate-check block, and the #3301 coverage-check block), plus the identical pattern found by a repo-wide sweep in complete-milestone.md, code-review.md, pr-branch.md, sync-skills.md, and execute-phase/steps/per-plan-worktree-gate.md. Each site is rewrapped in unquoted command substitution (`$(printf '%s' "$VAR")`), which re-splits identically under both shells regardless of SH_WORD_SPLIT — the same mechanism that already made the accumulator-building loops in these files shell-safe. * test(#4109): warn loudly when the zsh probe fails instead of skipping silently detectShells() drops the zsh test lane whenever a live zsh probe fails (e.g. on a CI runner without zsh installed), and a dropped lane reads identically to a passing one in the suite's own output — exactly the blind spot that let #4109's bug class ship undetected. Both copies of this helper (this file and its byte-identical duplicate in review-build-prompt-optional-sections.test.cjs) now emit a greppable console.warn when the probe fails, so a run without zsh reads as "zsh coverage unknown" rather than "all lanes green". * ci(#4109): gate workflows/*.md's embedded bash blocks in lint:ci Adds scripts/lint-workflow-shellcheck.cjs, wired into npm run lint:ci, so this bug class can't land undetected a third time. Two independent checks run over every ```bash block in gsd-core/workflows/**/*.md: - ShellCheck (new `shellcheck` devDependency, downloads and caches the real koalaman/shellcheck binary) catches the general unquoted-expansion/ word-splitting family in argument position. 212 pre-existing findings across the tree are absorbed into scripts/lint-workflow-shellcheck-baseline.json (matched on {file, code, message}, not line number, so unrelated edits elsewhere in a file can't spuriously un-baseline anything) — fixing all of them is out of scope for this issue; only NEW findings fail the build. - A custom structural check specifically for #4109's own shape: ShellCheck does not flag a bare `for x in $VAR` word-list — it treats that as an intentional idiom under any ruleset (confirmed empirically). This check does, and gates the build on any occurrence, with zero tolerance (no baseline) since every known site was already swept and fixed on this branch. * test(#4109): update pr-branch cherry-pick loop test anchor for the zsh fix extractPickLoop()'s PICK_LOOP_MARKER located the create_pr_branch cherry-pick loop by the literal substring "for HASH in $INCLUDED_COMMITS", which no longer appears verbatim after this issue's fix rewrapped that loop in unquoted command substitution. Updates the anchor (and its error message, now derived from the same constant instead of duplicating stale text) to the new literal form. No behavior change to the extraction logic itself. Emitted-Drift-Ack-Growth: review.md — #4109's fix adds explanatory comments at 4 sites; net code is functionally equivalent, comment expansion grows the file Emitted-Drift-Ack-Growth: complete-milestone.md — #4109's fix adds explanatory comments at 3 sites documenting the zsh word-splitting divergence Emitted-Drift-Ack-Growth: code-review.md — #4109's fix adds an explanatory comment documenting the zsh word-splitting divergence Emitted-Drift-Ack-Growth: pr-branch.md — #4109's fix adds explanatory comments at 3 sites (TRANSIENT_DIRS, INCLUDED_COMMITS, FILTER_PATHS) Emitted-Drift-Ack-Growth: sync-skills.md — #4109's fix adds explanatory comments at 2 sites (CREATE_LIST/UPDATE_LIST, REMOVE_LIST) * test(#4109): cover lint-workflow-shellcheck parsers + add timeout Adds tests/lint-workflow-shellcheck.test.cjs covering the hand-rolled parser/logic functions exported by scripts/lint-workflow-shellcheck.cjs (stripCommandSubstitutions, stripShellComments, substitutePlaceholders, extractForLoops, findBareForLoopSplits, findingKey, partitionAgainstBaseline) that shipped with zero coverage — CLAUDE.md requires at least one fast-check property test for parsers, included here (bare/braced forms always flagged naming the variable; quoted/substituted/literal forms never are). Also bounds runShellcheck's subprocess with a 60s timeout: the `shellcheck` npm package's own API has no timeout option and internally blocks on a synchronous spawnSync, so this reimplements the binary resolve/download step via the package's own exported config/download and calls spawnSync directly with a native timeout. And guards the CLI entry point with `if (require.main === module)`, matching this repo's sibling dual-purpose lint scripts — without it, requiring the module for its exported functions (as the new test file does) also triggered a live ShellCheck run as a side effect. * docs(#4109): add changeset for the zsh word-splitting fix * chore(#4109): backfill changeset PR number (#4116) --------- Co-authored-by: sim <sim@local> |
||
|
BeeHiggs
|
1a358ce0fd |
feat(#2761): bracket-tolerant read path — roadmap/validate/verify/state recognize bracket ids (epic #612 PR-2) (#2867)
* feat(#2761): gated heading-intro selection + one bracket identity grammar Foundation. Two owner-level changes plus a federated convention resolver; no reader consumes them yet. 1. GATED SELECTION, not an ungated widening. Widening every heading matcher requires the claim "no legacy ROADMAP contains a `[CODE.MM]` bracket followed by a digit", and that is false: `### [RFC.2119] 5:`, `### [v1.0] 2024:`, `### [ADR.612] 3:` and `### [ISO.8601] 2026:` are ordinary headings, and a widened reader claims each as a phase — moving phase_count and total_phases and adding W006 on projects that never opted in. No narrowing rescues it: the premise is about documents we do not control. `phaseHeadingPrefixSrcFor(baseline, convention, capturing?)` selects the pattern SOURCE at construction time. A project whose resolved `phase_id_convention` is not exactly 'bracket' compiles the same source string it compiled before. `baseline` is explicit because whether a site spells the any-bracket prefix or a bare `Phase\s+` is a fact about that site's history: handing the wider grammar to a bare site retro-grants tolerance it never had, in both directions — warnings appear, and a warning that fires today vanishes. Both bracket forms CAPTURE. `[GSD.999] Phase 07:` previously matched through the base alternative, which captures nothing, so a reader saw no bracket, fell back to the legacy token rule, and counted a labeled icebox heading while excluding the label-less one beside it — two derivations of one ROADMAP disagreeing. 2. ONE bracket identity grammar, one width rule. The milestone width is reconciled with the emit validator: pad2 output, so two digits or 3+ with no leading zero. Earlier spellings diverged in both directions — admitting `002`, which the validator rejects, and a bare `0` pad2 never produces — and the section recognizers accepted `[GSD.2]`, which SCOPED a milestone no phase heading could then resolve into, recreating the on-disk-count fallback this epic removes. An unpadded bracket is now uniformly malformed: it scopes nothing, bounds nothing, sections nothing. W005 on its directories is the surfacing signal. The milestone field is boundary-anchored, so a malformed run cannot match by its prefix (`GSD.002-01` read as sentinel `00`). Recognition stays case-insensitive because readers compile `/i`, but identity helpers match `[A-Z]`, so a captured id is folded first — otherwise `### [gsd.999] 07:` failed every sentinel test. The qualified key shares the width, the `(?=-|$)` boundary and the single-sub-phase shape of the directory token, because phaseTokenMatches returns unconditionally on a qualified hit: a key matching a directory isPhaseDirName rejects would be a final wrong answer. 3. resolvePhaseIdConvention federates workstream -> root exactly as config-loader does — including that root is a fallback only when a WORKSTREAM is active, so a project-scoped directory stands alone. loadConfig cannot serve this: it merges against CONFIG_DEFAULTS and drops keys it does not know, and this key is not among them. It governs the bracket-selection reads ONLY. PHASE_HEADING_PREFIX_SRC is left byte-identical: PR-1 shipped it, nothing consumes it, and it is superseded rather than redefined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): roadmap.cts selects its heading grammar from the convention Six matchers build their intro through the gated selector, and cmdRoadmapAnalyze / cmdRoadmapGetPhase / getRoadmapPhaseWithFallback each resolve the convention ONCE per command and thread it down. Three sites take the any-bracket baseline (they already tolerated `[anything] Phase N`); three take label-only (they spelled a bare `Phase\s+`). Handing the wider grammar to a label-only site retro-grants tolerance it never had — and not only by adding matches: on a legacy repo an unchecked `- [ ] **[v1.0] Phase 05: Thing**` bullet would start SUPPRESSING the W006 that fires today. Sentinel handling under bracket ADDS a rule rather than replacing one: a bracketed heading is a sentinel when its bracket milestone is reserved (`### [GSD.999] 01:`) OR when its token is, so the engine-wide 0/999 backlog convention keeps applying to `### [GSD.02] 999:`. Replacing the token rule let a mid-migration ROADMAP — bracket headings plus a legacy backlog block, exactly the content this epic targets — add entries to the progress denominator. The captured id is folded before the identity test, so a lowercase `### [gsd.999] 07:` is excluded too. The DIRECTORY read is threaded too. `cmdRoadmapAnalyze` resolves the convention once and hands it to all four of its heading/checklist patterns, but the single `phaseTokenMatches` call that decides `disk_status`, `plan_count`, `summary_count`, `has_context` and `has_research` was left two-argument — so every canonical `{CODE}.{MM}-{PP}-slug` directory read as `no_directory` with zero counts, on the PR's own headline verb, while the SAME build resolved those same directories correctly in three other places on the same repo (W006/W007 via phaseTokenFromDir, `state json` via the milestone filter, and the W021 milestone-complete read through this very helper's three-argument form). It failed ONLY for the directory shape the convention exists to name: a mid-migration bracket repo carrying legacy `01-one` dirs resolved fine, which is why nothing caught it. Measured, bracket vs its flat-legacy twin: `[["01","no_directory",0,0],["02","no_directory",0,0]]` against `[["01","complete",1,1],["02","planned",1,0]]`. The oracle is the twin, computed in the same test run, plus exact literals — `grep disk_status tests/adr-612-*` was zero hits before this, so neither the fix nor a future regression had any gate at all. Disclosed: a ROADMAP written in bracket form before config.json is switched reads as empty rather than mis-counted. Silent invisibility during the migration window is the deliberate trade against claiming phases on projects that never opted in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): validate.cts selects its grammar; gated directory recognition The W006/W007 feeders take the resolved convention as a threaded parameter. These sites carry the letter-tolerant `[\w][\w.-]*` capture, which makes them where an ungated widening does the most damage: `### [RFC.2119] 5:` enters roadmapPhases as a phantom and becomes a W007 "in ROADMAP.md but no directory on disk" on a project that never opted in. buildRoadmapPhaseVariants also surfaces the tokens borne ONLY by sentinel-bracket headings. Surfaced rather than filtered in place because roadmapPhases feeds both a membership check and a missing-directory warning, and only the latter should ignore an icebox item. That set is OCCURRENCE-AWARE, and the subtlety is load-bearing: roadmapPhases is a TOKEN set, so `[GSD.999] 01` and `[GSD.02] 01` collapse to one entry. Keying suppression on the token alone let an icebox heading silence a REAL phase that happens to share its number — a false negative strictly worse than the warning it removed. A token is suppressed only when no non-sentinel heading bears it. Directory recognition is added as gated FUNCTIONS beside the exported RegExp constants, which stay byte-identical: the `{CODE}.{MM}-` prefix is string-indistinguishable from the letter-prefixed-decimal family this repo documents as ambiguous, and folding a branch in changes those constants' answers on exactly that family. A RegExp constant has nowhere to attach a gate. The recognizer mirrors the emit grammar and delegates the token to the canonical owner, so recognizer and resolver agree on rejected input as well as accepted. Both functions throw on a non-string, matching the call pattern they replace. buildRoadmapPhaseVariants' CHECKLIST scan is capturing, like its heading twin and like the sibling checklist scan in roadmap.cts, and for the reason that one states: the bracket id has to ride along or the sentinel filter is blind to `- [ ] **[GSD.999] 01: Icebox**`. Left un-capturing, the scan called every checklist token REAL, and the occurrence-aware un-suppression loop then deleted the icebox token the HEADING scan had correctly marked sentinel — so `validate consistency` warned that a bracket ICEBOX phase had no directory, in the HOUSE ROADMAP shape where an icebox appears as both a bold bullet and a detail heading. `validate health` stayed silent on that same repo, so the two verbs disagreed — which is the disagreement `sentinelPhases` exists to close. Both directions are pinned, because the failure mode of a careless fix here is the opposite one: a real phase sharing a sentinel's token must still warn. It does, in all four shapes that attack it (sentinel heading + real bullet, lowercase sentinel, sentinel after the real heading, colon-less bullet). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2761): count bracket headings, and retire them, in both derivations Both `total_phases` derivations select their grammar from the resolved convention, in one commit — cmdStateSync already carries the comment that it mirrors buildStateFrontmatter "so both report consistent percents (#3242 Bug B)", so teaching one and not the other ships that divergence. The #1514 retirement filter widens WITH the counter it protects. The canonical gesture strikes the checklist BULLET and leaves the detail heading intact, so a bracket-form retirement went undetected and the phase stayed in the denominator forever. That is half a fix alone: the retired key is compared against phaseKeyFromDir, which called extractPhaseToken with no convention. Both halves land here. Under bracket the sentinel token rule composes as the full engine set {0, 999}, so this counter agrees with `roadmap analyze`, which has always excluded both — otherwise the two derivations report different numbers for one ROADMAP and the changeset's "excluded from every count" is false as written. The LEGACY path keeps its pre-existing 999-only rule: widening it there would move legacy totals, so the two stay split off the bracket path exactly as they are today. The sync-side assertion reads the PERCENT sync writes into the STATE.md body, not the frontmatter total_phases. Sync's own counter never reaches that field — the read derivation writes it — so asserting the frontmatter after a sync measures the read path twice and lets a mutation to the write-path guard survive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): verify.cts bracket-coherence W021 + selected milestone-complete read The shipped milestone-prefixed W021 gate keeps its ROOT-only config read, verbatim base semantics. Federating it silently moved a legacy convention's answer in BOTH directions on workstream repos — a W021 that fires at base vanishing, and one that is silent at base firing. resolvePhaseIdConvention governs the new bracket-selection reads only. B6, the milestone-complete check, keeps its ungated POSTURE (bug-557 pins it with an empty config) but selects its grammar from the convention. Inferring 'bracket' from the shape of a matched bracket ran a repo-failing check against a legacy ROADMAP that merely contained `### [RFC.2119] 5:`. Directory resolution widens with the heading read, so a bracket repo whose phases are on disk stays silent, and a bracket sentinel is not reported as unstarted. checkBracketCoherence is advisory and gated. Anchored to tokenizeHeadings so fenced examples cannot warn and heading level is structural. Its scope rules each close a way it silently did nothing or fired wrongly: only a genuine MILESTONE heading opens or closes a section (a `### Notes` used to reset scope and disable both sub-checks); a legacy `## v3.0` DOES close it; an M-NN or letter-suffixed phase heading raises missing-bracket and CONTINUES; a bare `#### 2026:` is not a phase; the full h2-h6 range is processed. Its section recognizer shares the one milestone width, so an unpadded `### [GSD.3] 05:` can no longer be a phase to the id grammar and a section to the section grammar at once, silently re-scoping every warning after it. validate consistency suppresses bracket sentinels in its missing-directory warning — the two verbs disagreed, health suppressing via notStartedPhases while consistency did not. The legacy reading is untouched, including its pre-existing wart that `### Phase 999:` still warns there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2761): scope the milestone by its bracket; select the disk-side filter Two roadmap-parser reads, both of which made a bracket project's totals track the disk instead of the ROADMAP. The ADR pins the bracket milestone heading as `## [GSD.02] Foundation` — a name, no version — but scoping matched STATE's `milestone: v2.0` STRING against a heading, so the canonical form matched nothing and total_phases fell back to the directory count. The rule was re-derived in THREE places: extractCurrentMilestone plus two `milestoneBounded` guards; fixing one left the others falling back regardless, so they are now one gated helper. It matches the CANONICAL padded spelling only — accepting `0*N` bounded a milestone whose phases were invisible, which un-suppressed a progress percent computed off an unscoped disk count. getMilestonePhaseFilter's heading scan becomes the 14th selected read. On a bracket ROADMAP it collected nothing, so the filter degraded to pass-all and buildStateFrontmatter counted every other milestone's directories — making the bracket convention strictly worse than the M-NN one it supersedes on the property that matters most: totals must track the ROADMAP, not the disk. The DIRECTORY side of that same filter is selected with it. Teaching only the heading scan was half a fix and a worse one: `milestonePhaseNums` became non-empty, so the pass-all degrade stopped firing, but no bracket directory could satisfy the three legacy dir checks (numericRe fails on `GSD.02-05-five`, the custom-id match captures the project code `GSD`, and stripProjectCodePrefix does not strip a dotted prefix). Every bracket directory was rejected, and completed_phases / total_plans / completed_plans / percent all collapsed to 0 while `state sync` went on writing a percent off the unfiltered disk — `state json` reporting 0% on the same repo, in the same second, that STATE.md's body called 67%. That is the #3242 Bug B divergence this PR exists to avoid, and total_phases could not show it: `Math.max(phaseDirs.length, roadmapPhaseCount)` floors it at the ROADMAP count no matter how many directories are rejected. The dir side matches on the milestone-QUALIFIED id, delegated to the owner's gated `phaseTokenMatches(dir, id, 'bracket')`, not on the bare token: READING-B puts the milestone in the bracket, so `GSD.01-01-old-one` and `GSD.02-01-one` share the token `01` and only the qualified key separates them. The qualified ids are kept in their own set — a hyphen in `milestonePhaseNums` would flip `roadmapUsesHyphenedIds` and silently move the LEGACY dir path on a bracket repo — and the branch is ADDITIVE: on a miss it falls through to the three legacy checks, so a bracket project carrying legacy-shaped directories reads unchanged. Both are resolved lazily and gated, so the legacy path pays neither a config read nor a second scan and cannot change answer. The scoping call is also GUARDED: resolvePhaseIdConvention reaches planningDir, which throws a plain Error for a GSD_PROJECT/GSD_WORKSTREAM segment carrying `/`, `\` or `..`. At base the only planningDir call in extractCurrentMilestone sits inside the STATE-read try, so the function returned normally on such an environment; an unguarded one here let that escape and broke the never-throws invariant that getRoadmapPhaseInternal and getMilestoneInfo three hundred lines below carry #2245 / ADR-227 notes about. Unreachable through the CLI — GSD_WORKSTREAM is rejected up front by the workstream-name policy and GSD_PROJECT throws identically at base — but reachable by any in-process embedder, which is precisely who that invariant is for. The filter's own resolve call was already inside its try and is unaffected. The milestone-qualified key is formed only for a token that is itself a bracket phase token. `${bracketId}-${token}` is a string SPLICE, so a mid-migration heading carrying an M-NN label — `### [GSD.02] Phase 02-01:` — spliced to `GSD.02-02-01`, which the qualified-key grammar reads as milestone 02 / phase 02: the `-01` truncated, both such headings collapsing to one key, and the heading claiming `GSD.02-02-two`, the directory it does NOT name, while rejecting `GSD.02-01-one`, the one it does. The guard drops those headings back to the unqualified legacy path, restoring the base ACCEPTANCE VECTOR exactly — pinned against the milestone-prefixed reading of the same ROADMAP, which is base-identical on this shape. Scoped precisely, because the fixture moves one number that the guard does not touch: `total_phases` on it reads 1 at base and 2 here. That is the bracket heading COUNT this PR exists to add, not the splice — measured identical with and without the guard, and identical to what the canonical `### [GSD.02] 01:` spelling does on the same fixture (both read 2 with zero directories on disk, where base reads 0). The claim is base-equivalent ACCEPTANCE, not a base-equivalent reading. One consequence is stated rather than fixed: a heading whose token carries a hyphen still puts that hyphen into milestonePhaseNums and so still flips `roadmapUsesHyphenedIds`. Base does the same for that spelling, so preserving it is what keeps the shape base-equivalent; excluding the token would have moved answers versus base on malformed input. The comment at the qualified-set declaration is corrected to claim only what is true — it keeps QUALIFIED IDS out of that flag's input, not hyphens in general. The oracles ship with it, and they are the five numbers, not the one: the parity gate now asserts total_phases, completed_phases, total_plans, completed_plans AND percent, on both derivations, on two fixture shapes (one milestone; two milestones with stale prior-milestone directories on disk). The oracle is the flat-legacy twin, built in the same test run and compared number for number, plus exact literals so a shared wrong answer cannot pass. The oracle SUBSTITUTION is itself pinned. The M-NN spelling of these shapes could not serve, because buildStateFrontmatter's #2445 de-dup key captures only a directory's leading integer and collapses `02-01-one` / `02-02-two` / `02-03-three` to one — measured [3,0,1,0,0] against the flat-legacy twin's [3,2,3,2,67], identically at base and before this fix, and structurally unreachable from the bracket key space. That reasoning is only sound while it stays true, so a characterization test holds the M-NN reading down on the two numbers that do not depend on which directory wins the mtime race. Widen the de-dup key and it fails, instead of quietly invalidating the changeset's disclosure. Also adds the call-site pin. The structural table pins transcription against the selector; it cannot see a call site whose BASELINE ARGUMENT is wrong. Flipping verify.cts's milestone-complete site to the wider baseline grants a fires-on-every-repo check tolerance it has never had, and every behavioural test still passed. The pin reads the shipped sources and asserts the mode at each of the 14 sites, count-exact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2761): pin the bracket read surfaces in the parity gate This gate exists because #2043 fixed one bug across five hand-edited copies of a rule and #2232 was the residual that survived, because a later reader could not tell the copies were one rule. PR-2 adds two consumers, so they belong here. Surface 7 — the heading read and the directory read must agree about WHICH phase a `MM-<seg>` pair names, across the shared width corpus, and the bracket and legacy spellings of one heading must yield the same token. Surface 8 — the two bracket directory readers, in BOTH directions. Agreement on ACCEPTED input was already pinned; agreement on REJECTED input is where they actually diverged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2761): changeset Disclosures for the PR body (deliberate, not defects): - phase_id_convention is not a CONFIG_DEFAULTS key, so loadConfig drops it and cannot serve as the convention resolver however the file is federated. This PR ships its own workstream->root resolver; adding the key and its value enum is later-slice work. - Convention matching is strictly === 'bracket'. A misspelled value reads as not-configured and the project keeps legacy behaviour silently. - An UNPADDED bracket milestone (`[GSD.2]`) is malformed: it scopes nothing, bounds nothing, sections nothing, and is not a phase id. W005 on its directories is the surfacing signal. - WIDTH UNIFICATION MOVED FOUR MERGED PR-1 EXPORT ANSWERS on non-canonical inputs, none of which toDir can emit and none of which had a bracket caller at base: isSentinelPhaseId('GSD.0-01', 'bracket') true -> false isSentinelPhaseId('GSD.0999-01', 'bracket') true -> false getMilestoneFromPhaseId('GSD.2-01', 'bracket') 'v2.0' -> null getMilestoneFromPhaseId('GSD.002-01', 'bracket') 'v2.0' -> null The canonical pad2 sentinel spelling `[GSD.00]` still tests true. - FLAG TO MAINTAINER: docs/adr/612:132 reads "Sentinel behavior (0.x / 999.x -> milestone null) is preserved". After the unification that holds for the canonical `00` spelling only, not for a bare `[GSD.0]`. ADR wording is yours; flagging the tension rather than editing it. - The bracket sentinel rule COMPOSES with the legacy one — a bracketed heading is a sentinel when its bracket milestone OR its token is reserved. Under bracket the state-side token rule is the full {0, 999} set so both derivations agree; the LEGACY path keeps its pre-existing 999-only rule, unchanged. - validate consistency's legacy reading is untouched, including the pre-existing wart that `### Phase 999:` warns there while validate health suppresses it. - find-phase still cannot resolve a bracket phase directory. phase-locator.cts is outside this PR's module set. Sibling PR #2559's matchPhaseDirs calls phaseTokenMatches without a convention, so whichever slice lands second must thread it through. - Four of the five bracket readers scan raw ROADMAP content, so a bracket heading inside a fenced code block is read as a phase. Pre-existing for the legacy spelling; parity, not a new class. - roadmapPhaseLookupSources gained no bracket source: nothing emits a milestone-qualified query into it yet. - roadmap validate remains a separate, unfederated convention reader. Pre-existing and base-identical, but two verbs can disagree about the active convention on one project. - _diskScanCache keys on cwd while the values it caches are now convention-dependent. Not reproducible through the CLI; pre-existing for the workstream dimension, widened here. Stated as inconclusive. - A ROADMAP written in bracket form before config.json is switched reads as empty rather than mis-counted — the deliberate migration-window trade. - THE READ AND WRITE PERCENTS STILL DIVERGE ON A MULTI-MILESTONE REPO, and that divergence is MIRRORED under bracket rather than closed. buildStateFrontmatter applies the milestone filter; cmdStateSync does its own fs.readdirSync and never calls it, so on a repo carrying prior-milestone directories the read path reports the SCOPED percent and the sync body reports the WHOLE-DISK one. Measured on the true base build ( |
||
|
0xdhx
|
ef9ce3e598 |
fix(#3702): count asterisk, plus and ordered markers as deferred-items entries (#3739)
* fix(#3702): deferred-items counts `*`, `+` and ordered markers as list items
`deferred-items.md` has no template and no mandated shape, but its parser
recognised only the `- ` hyphen marker. Asterisk bullets, plus bullets and
dot-terminated ordered lists — all lists in CommonMark and GFM — contributed
ZERO entries on both the headless and the heading-delimited path, and a mixed
file dropped its non-hyphen entries while keeping their hyphenated siblings,
under-reporting without ever looking empty.
The restriction was a regex literal inherited from the Gaps seam, where the
template genuinely mandates the hyphen YAML-lite form; nothing in the module's
stated rationale distinguishes `*` from `-`.
Widened on the deferred path only:
- `splitGapsEntriesCore`'s entry opener, `extractGapEntryFields`' line-0 strip
and `rawGapEntryText`'s line-0 strip take a `BulletMarkers` parameter that
DEFAULTS to the hyphen-only set, so `## Gaps` keeps its template-mandated
grammar byte-for-byte and the module still has exactly one grouping pass.
- `splitDeferredHeadingEntries`' body-bullet test, `stripLeadingBulletMarker`
and `acknowledgeDeferredItem`'s status-field regexes move in lockstep —
widening what OPENS an entry without widening what is STRIPPED before field
extraction would surface an entry that can never resolve.
Unchanged, and pinned by tests: prose-only and bare headings still contribute
nothing ("prose is not an item"); a table under a leaf heading still yields
exactly its rows, since table lines are skipped before the body-marker flag can
be set and a `|` row is not a list marker; the paren-terminated ordered form
`1)` is out of this fix's scope.
* docs(#3702): changeset fragment (pr: 0 placeholder pre-create)
* fix(#3702): widen the forensic-audit prose entry rule to match the parser
Sibling site of the same defect class, found by a defect-class sweep of the
deferred-items consumers. `/gsd-progress` check 7 does NOT go through
`gsd-tools query` — it globs `deferred-items.md` and has the model read entries
by a prose rule that mandated "one entry per top-level `- ` line". Left as-is,
the marker widening would hold on the CLI path while the one consumer that
bypasses the parser kept reporting "No unresolved deferred items" for a file
written with `*`, `+` or an ordered marker: the same false negative, surviving
in the only place the fix could not reach by code.
Also pass DEFERRED_BULLET_MARKERS explicitly where the heading path extracts
fields. It was already correct — stripLeadingBulletMarker pre-strips the widened
set from every line, so the default hyphen strip is a no-op there — but relying
on that leaves a detection site and a strip site nominally on different marker
sets, which is exactly the asymmetry the BulletMarkers doc comment warns about.
Explicit is local; inferred is a trap for whoever edits the strip next.
Out of scope, noted rather than fixed: forensic-audit.md globs only
`.planning/phases/*/` and so misses archived milestone phases that
`scanDeferredItems` covers. Pre-existing, a different defect, and not this
issue's ruling.
* docs(#3702): note the milestone-close halt for heading-shape non-hyphen files in the changeset
A heading-delimited deferred-items.md written with */+/ordered markers
previously parsed to zero and closed silently; it now yields entries whose
heading shape acknowledgeDeferredItem refuses, halting complete-milestone
until hand-edited. User-visible, so the fragment states it.
* chore(#3702): set changeset fragment pr to 3739
* fix(#3702): CR-normalise the heading path and the acknowledge writer (review B1, M4, m2)
B1 — `splitDeferredHeadingEntries` stored RAW lines; on a CRLF file every
body line but the last still carried its `\r`, the `$`-anchored marker
strip failed on it, the marker survived into field extraction and the
field was lost — a `**Status:** resolved` that was not the file's final
line resurfaced its entry as open. The heading path now stores CR-stripped
lines like the headless path already did, and the strip regex tolerates a
trailing CR on its own. Round 1's CRLF test put `**Status:**` on the last
line, the one position `collectSection`'s `.trimEnd()` had already
de-CR'd; the new tests put it first and mid-body.
M4 (pre-existing on `next`) — `acknowledgeDeferredItem` found the status
line on a CR-stripped copy but rewrote the raw line with a `$`-anchored
`.*`, which cannot consume `\r`; `replace` returned its input, and the
writer reported `ok` over byte-identical content. The rewrite now runs on
a CR-stripped line. The comment that claimed `.*$` consumed the `\r` is
corrected — it was the bug, stated as the design.
m2 — the indent probe for an inserted `status:` line ran on the raw line
and fell back to indent 0 on CRLF; it is CR-stripped too.
* fix(#3702): derive every deferred-items marker regex from one source (review M3, N1, N2)
M3 — round 1 carried the marker alternation in FOUR places: the
`BulletMarkers` pair and two inline literals inside
`acknowledgeDeferredItem`, under a doc comment saying the interface
existed so a detection site and its strip site could not drift. All four
now derive from `DEFERRED_MARKER_ALT`; drift is impossible rather than
discouraged. A parity test pins the vocabulary against
`markdown-sectionizer`'s `iterateBullets` on everything the two grammars
are meant to agree on, and names the two points they deliberately differ.
N1 — the ordered marker is `\d{1,9}\.` (CommonMark §5.2), not `\d+\.`.
N2 — the marker is followed by `[ \t]`, not `\s`, which also accepted
`\r`; the tab remains accepted (CommonMark-legal) and the divergence from
`iterateBullets`' literal space is pinned rather than papered over.
The four regexes are exported for the parity test only.
* fix(#3702): an ordered marker opens an entry only from `1.` or inside a run (review B2, m1)
B2 — `\d+\.` alone read ordinary prose as a list: "2026. was a bad year
for this module" and, under `### Notes`, "3. is the number of retries we
settled on." both opened an entry on round 1, the second straight through
the "prose is not an item" contract that round's AC4 claimed to preserve.
CommonMark §5.3 faces the same ambiguity when an ordered list would
interrupt a paragraph and resolves it by requiring the list to start with
1; `matchListOpener` applies that rule wherever an ordered marker is seen,
with the run carried per list (headless) or per leaf-heading body. Numbers
after the first are ignored, as CommonMark ignores them. Stated cost,
pinned: a hand-numbered list starting at 2 reads as prose — every ordered
record in the #3702 scan starts at 1.
Both reviewer cases are pinned as prose; the ruling's `1. alpha / 2. beta`
shape still counts.
m1 — the 9-digit boundary is pinned at both sides (`999999999.` opens,
ten digits is not a marker), and the 3-vs-4-space indentation cliff is
pinned as deliberately NOT applied: the parser is indent-lenient because
surfacing a questionable hand-written entry beats dropping a real one.
* fix(#3702): thematic breaks close the list and fenced code never opens an entry (review M1, M2)
M1 — `- - -` was a phantom `"- -"` entry on base; widening the marker set
added `* * *` and `+ + +` to the class, and `* * *` is the separator an
author writing in the `*` style is most likely to use. A CommonMark §4.1
thematic break (plus the `+ + +` gesture, which is the same garbage as an
entry name) now closes the open entry on the headless path and is dropped
from the body on the heading path — neither an item nor a continuation.
M2 — neither splitter was fence-aware, so `+ `-prefixed diff lines and
`1.`-numbered repro steps inside a code block counted as entries; #3702's
wild records carry exactly those blocks. Both splitters now classify lines
by the sectionizer's own `scanFencedBlocks` (so `~~~`, indented and
unterminated fences behave as `stripFencedCode` would): fence content
never opens an entry, is continuation inside an open one — keeping the
span invariant `acknowledgeDeferredItem` re-verifies — and is discarded
before the first.
* test(#3702): range the #2287 deferred-items property over marker × shape × line ending (review B3)
The `#2287` property hard-coded `- ` and filtered `\r\n` out of its
arbitraries, so the widened marker set — an enumerated domain, exactly
what a property is for — was never under it. It now ranges over
`{-, *, +, ordered}` × `{headless, heading}` × `{LF, CRLF}`, with the
heading shape placing `**Status:**` first or last: the review's
prescription (markers × line endings) would not have reached B1, which
lives on the heading path only, so the shape axis is the load-bearing
addition. Ordered entries are numbered from 1, so the B2 run rule is
under the property too.
A second property drives `acknowledgeDeferredItem` over every unresolved
headless entry across the same marker × line-ending grid — the one that
reaches M4 (a CRLF rewrite that reported `ok` and wrote nothing) and m2.
* test(#3702): pin the milestone-close halt on a heading-delimited `*`/`+`/`1.` file (review m3)
A heading-delimited `deferred-items.md` written with a non-hyphen marker
previously parsed to zero entries and let `complete-milestone` close
silently; it now yields entries whose heading shape `acknowledgeDeferredItem`
refuses, which the milestone loop turns into `record_ack_failure` → exit 1.
The loop is prose in a workflow, so the test drives the two CLI calls it
makes: `audit-open --json` must list the entry, and
`audit-open acknowledge --text <the audit's own text>` must refuse with the
heading-delimited message and write nothing.
* docs(#3702): changeset and forensic-audit prose carry the round-2 grammar
The changeset names the CRLF fixes, the ordered start-at-1 rule, thematic
breaks and fences. The `/gsd-progress` forensic-audit step is the one
prose parser of this file and must state the same grammar the code has.
* fix(#3702): round-review refinements — run ends at a paragraph, rejected ordinals unstripped, breaks at any indent, fenced fields, `## Gaps` scope
Findings from the pre-push adversarial review of round 2, each pinned:
- An ordered run ENDS at a paragraph that follows a blank line (CommonMark
§5.3); a non-indented line with no blank before it is lazy continuation
and keeps the run open. `1. a` / blank / `paragraph` / blank / `5. x` is
one entry, not two.
- The heading path strips the marker off every body line before field
extraction (#3457); a line whose ordinal `matchListOpener` REJECTED must
not be stripped, or "3. status: resolved" as prose loses its `3. ` and
reads as a resolved field. `splitDeferredHeadingEntriesDetailed` now
carries a per-line opener flag and only accepted openers are stripped —
in headless regions of a heading-shaped file too.
- A thematic break is recognised at any indent, matching the parser's
indent-lenient reading of items; ` * * *` was a phantom `* *`.
- Fenced lines carry no FIELDS either: a `status: resolved` quoted inside a
code block no longer resolves its entry on either path.
- Block structure (breaks, fences) is a property of the GRAMMAR, carried as
`BulletMarkers.blockStructure`: the deferred set opts in, the Gaps set
does not, so `## Gaps` is byte-for-byte on its `next` behaviour — the
round-2 M1/M2 change had reached it through the shared splitter.
* test(#3702): the property exercises the rejected-ordinal branch; the N2 control is independent
Round review: the widened #2287 property numbered every ordered run from 1
and so never generated an ordinal the start-at-1 rule rejects — it could
not tell round 1 from round 2 on B2. Each entry may now carry a decoy prose
line beginning with a non-1 ordinal, placed where it cannot end a run
(before the first headless entry; first in a heading body), followed by a
`status: resolved` that must never become a field; and a decoy-only
heading body must yield no entry.
The N2 assertion accepted a tab, which round 1's `\s` accepted too, so a
`[ \t]` → `\s` revert alone stayed green. NBSP, form-feed and vertical-tab
are now asserted refused — the assertion that fails on that revert on its
own, and the disclosure that `[ \t]` narrows what round 1 accepted.
* fix(#3702): the splitter records its own opener flags; an opener clears the blank-line memory
Round-review continuation, two state defects in the ordered-run logic:
- `blankSeen` survived the headless splitter's opener branch, so an opener
followed by a lazy continuation line read as "paragraph after a blank" and
ended the run — `1. a` / blank / `2. b` / lazy / `3. c` folded `c` into `b`.
The opener branch now clears it.
- The heading path re-derived per-line opener flags for headless regions
without the paragraph reset, re-accepting a rejected `3. status: resolved`
under a stale run and stripping it into a field. `GapsEntrySpan` now
carries the flags the splitter itself computed, and the heading path reads
them; the re-derivation is deleted.
* fix(#3702): ordered-run memory is per indent — nested runs resolve, nested ordinals never inherit the top-level run
Round-review continuation 2: nested openers consulted the TOP-LEVEL run
flag and never wrote their own, so a nested `1. / 2.` run under a hyphen
entry rejected its `2. status: resolved` (round 1 resolved it), while a
nested `3. status: resolved` under a nested `- ` bullet inherited an open
top-level run and was stripped into a false field.
`OrderedRuns` keys the memory by indent: a new opener at indent d resets
every deeper level, a paragraph after a blank at indent d ends the runs at
d and deeper, a thematic break or a heading clears all. Both splitters use
it; the top level still decides entry boundaries, nested levels decide
only which continuation lines are accepted openers for field stripping.
Pinned for LF and CRLF.
* fix(#3702): run levels — one top level at or above the base, CommonMark column indents, a fence ends its level's runs
Round-review continuation 3:
- A dedenting top-level list (` 1.` / ` 2.` / `3.`) lost its entry
boundaries: the exact-indent run lookup rejected the shallower ordinals
before the boundary check ran. Every indent at or shallower than the
list's base is now ONE level, in both splitters.
- `indentOf` counted characters, so a tab and a space aliased to one level
and `\t1. nested` / ` 2. status: resolved` resolved falsely. Indent is now
measured in CommonMark columns (§2.2: a tab advances to the next multiple
of 4), for the run level and the entry-boundary check alike.
- A nested run survived a fenced block. A fence is a non-list block: its
opening delimiter ends the runs at its level and deeper, exactly as a
paragraph after a blank does.
* fix(#3702): the indent measure is grammar-scoped — Gaps keeps next's character count
`blockStructure: false` promised the Gaps grammar byte-for-byte parity with
`next`, but the CommonMark-column indent measure added for the deferred
grammar was shared by the whole splitter core, so tab-indented Gaps input
changed entry boundaries in BOTH directions:
`\t- a` / ` - b` — next folded into one entry, HEAD split into two
` - a` / `\t- b` — next split into two, HEAD folded into one
`indentWidth` now keys the measure on the grammar: columns for the deferred
set, raw character count for Gaps. The opt-out covers indent semantics, not
only fences and thematic breaks.
Four cases pin both halves — the two flipped Gaps pairs, the two Gaps pairs
that never moved, and the same tab/space pairs on the deferred path returning
the opposite (column-measured) verdict by design.
* fix(#3702): the acknowledge path reads and writes through one classifier
Round 3, Blockers 1 and 3, and Minors 7 and 8 — one mechanism, so one commit.
Every consumer of an entry's lines now reads the splitter's own per-line
verdict instead of a re-derivation of it.
B1. Round 2 widened the WRITER's status-line finder to the deferred marker set
while `extractGapEntryFields` still de-bulleted line 0 only. A nested
` * status: pending` was therefore selectable by the writer and invisible to
the reader: acknowledge rewrote it in place, returned `ok`, and the item stayed
outstanding on every later audit. Measured against a `next` build, `*`, `+` and
`1.` each resolved on base and stopped resolving at round 2's head — a
regression, not a gap in new behaviour. The hyphen form of the same shape was
already broken on `next` and is fixed here too: one classifier cannot be right
for three markers and wrong for the fourth.
`parseGapEntryFieldLine` is now the single place a line is classified as a
field, and it reports the offset at which the VALUE begins. The rewrite happens
at that offset rather than through a second regex, so a line the classifier can
select is one whose rewrite it has already located — the selection and the
rewrite cannot disagree. Both `DEFERRED_STATUS_FIELD_RE` and
`DEFERRED_STATUS_REWRITE_RE` are deleted rather than widened. A read-back guard
returns `rewrite_not_readable` rather than `ok`; it is unreachable by
construction today and is the fail-loud floor under the next divergence.
B3. This is the end state the round-3 review prescribed on both #3739 and
#3773: #3773's shared classifier, parameterised by this PR's marker set, with
this PR's two status regexes deleted. #3773 lands first. Its hyphen-only strip
is consistent with `next`'s hyphen-only splitter today, so the writer/reader
divergence is created by THIS merge, which is why widening every consumer
belongs to the PR that widens the domain.
m7. The heading path marker-stripped its lines before calling the reader, so
the reader's fence scan ran over text the splitter never saw: `- ```sh` is an
ordinary bullet to the splitter but strips to a fence opener, and a
`**Status:** resolved` after it was suppressed as fence content — a resolved
entry resurfaced as open. Stripping now happens inside the reader, after the
fence scan.
m8. `rawGapEntryText` stripped a marker off line 0 unconditionally, but on the
heading shape line 0 is the heading TEXT: `### 1. Race in the writer` was
silently renamed to `Race in the writer`, and the name is the key acknowledge
matches on. Line 0 is stripped only when the splitter accepted it as an opener.
Also removed: `splitDeferredHeadingEntries`, whose sole caller only null-checked
it (round 3, M4 — the claim was zero callers, which was wrong; the wrapper's
`.map` was waste at the one call site), and `stripLeadingBulletMarker`, which
this change leaves with no callers at all. The export surface narrows to the two
splitter regexes the behavioural parity test reads (M6).
[PEER-ASK pr-order-12d5]
q: Reviewer blocked both on merge order. I'm declaring #3773 lands first and
building the end-state shape into #3739 now (both my status regexes
deleted). Does that match your plan?
reply: CONFIRMED - same order, derived independently. #3773 cannot carry the
fold: `DEFERRED_BULLET_MARKERS`/`BulletMarkers` have zero occurrences at
`next` (verified), so the prescribed end state is not executable inside
#3773 without absorbing this PR's work.
deadline: 03:55 UTC (answered before it)
fallback: declare #3773 first, adopt end-state shape in #3739, push+comment
decision: proceeded as stated; #3773 lands first, this PR carries the widening
of every consumer.
Refs #3740
* test(#3702): pin the detect/strip symmetry, and drop a white-box test that could not reach it
Round 3, Blocker 2 and Minors 6 and 9.
B2. The regression shipped green because no fixture put a marker on a nested
status line. Four markers x {nested status line}, each asserting the entry
READS BACK as acknowledged rather than that acknowledge merely reported `ok` —
reporting `ok` over a line the reader skips is the whole defect. Plus the bare
capitalised `Status:` case (the reader stores it case-sensitively, so the
writer must not select it), and an idempotence test, which is the failure the
defect actually produced: the item resurfaces, is acknowledged again, and never
settles.
Each of these was run against the pre-fix build first: all five fail there and
pass here. Two further assertions in the block are labelled CONTROL because
they held pre-fix — they guard the new offset-based rewrite and the opener-flag
threading against regressing, and calling them regression tests for a reported
defect would overclaim.
M6. The round-2 parity test asserted that four writer-side regexes embedded the
same source string. That is true of a detect/read asymmetry too, so it could
not have caught B1 — and two of the four regexes were widened into `export =`
purely to let it read them. Replaced with a behavioural test that drives the
real seam: every marker that opens an entry must also resolve it through
acknowledge. The structural assertion is kept for the two splitter regexes,
which really are two copies of one alternation.
m9. `expectedResolved` was computed and immediately voided; the loop beneath it
already asserts both polarities.
m7/m8 coverage lands here too: a bullet whose content is a fence opener must
not suppress the entry's fields, and a heading beginning with a list marker
must keep it in the entry name.
* docs(#3702): document the deferred-items entry shape where the file is written
Round 3, Major 5, and #3702's own item 2. The widened grammar was documented in
the reader (`forensic-audit.md`) but not at the write site, where
`executor-examples.md` still said only "log to deferred-items.md" — so the
question the issue actually raised, which shapes count, remained unanswered
anywhere a human writes the file.
States what opens an entry (`-`, `*`, `+`, and `1.` when the list starts at
`1.`), that `1)` is not a marker here, that a separator closes the list and
fenced content is never an entry or a field, and that an entry without an
explicit `status: resolved` stays open by design.
* chore(#3702): regenerate the changeset through the generator
Round 3, Minor 10. The fragment was hand-named against 64 generated names on
`next`, and its body ran ~250 words against CONTRIBUTING's one-sentence form.
Regenerated via `npm run changeset`, which is also what the random three-word
name is for: concurrent PRs never collide.
* fix(#3702): the fence gate lives on the seam both sides call, not just the reader
Found by the pre-push adversarial review of this round, and it is a regression
this round introduced rather than a pre-existing one.
`extractGapEntryFields` applied `fencedLineSet` before classifying; the
acknowledge writer's status-line search did not. So a `status:` line inside a
fenced block was SELECTED by the writer and SKIPPED by the reader — the write
produced a line nothing reads, the read-back guard refused it, and the entry
became impossible to acknowledge at all: `audit acknowledge` raised an internal
error and `complete-milestone` halted on it.
Measured, `- alpha` / fence / ` status: pending` / fence:
next ack=ok -> reads back "acknowledged"
round-2 head ack=ok -> reads back "" (the B1 defect)
before this ack=rewrite_not_readable -> refuses entirely (worse than next)
`entryFieldLines` is now the seam — per line of an entry, the field it declares
or `null`, fences included — and the reader and the writer both go through it.
That makes "the writer cannot select a line the reader will not read back"
structural rather than asserted, which is what the previous commit's message
claimed while a second read-side filter still lived outside the classifier.
Two comments corrected with it. The read-back guard is NOT "unreachable by
construction": this round shipped a reachable path to it, which is precisely
what an invariant asserted in a comment is worth. And the M6 replacement test
put its marker only on the entry opener, so it passed against the defective
build — the exact weakness it was introduced to fix in round 2's test. It now
marks the nested status line too, and fails pre-fix like the rest.
Round-3 tests against the pre-fix build: 10 of 12 fail there, and the 2 that
hold are labelled CONTROL because they guard this round's new code rather than
pin a reported defect.
* fix(#3702): one end-of-file CRLF algorithm, adopting #3773's with its B4 closed
Round-4 M1. Two open PRs shipped two different answers to "what line ending
does an entry that ENDS THE FILE get?", and the review's ruling was that the
disagreement needs one answer, not two. Neither shipped answer was that one.
Measured on builds of both heads:
case #3739 r3 #3773 here
undelimited single entry, CRLF preamble pass FAIL pass
LF-dominant list, one stray CRLF at EOF FAIL pass pass
(the other five) pass pass pass
This PR's content.endsWith('\r\n', matchIndexInContent) reads the terminator of
the PREVIOUS line, so it propagated an isolated CRLF into an LF-dominant list --
refuted by #3773's own LF-dominant fixture, ported here. Withdrawn.
#3773's crlfAtEof asks the right question -- does anything before the entry,
within scope, contradict CRLF -- and fails closed. But its scope goes EMPTY for
an undelimited single-entry list, because the entry-list region runs from the
first entry's start to the insertion point and those coincide; crlfAtEof('') is
false by its own before.length > 0 guard, so 'preamble\r\n\r\n- alpha' gained a
bare \n in a CRLF document. That is #3773's B4, verified by driving its head.
Adopted here with the scope widened to everything preceding the insertion point
where the preferred region is empty, rather than asserting LF from no evidence.
That only ever loosens a scope carrying zero information, and the predicate
stays fail-closed over the wider one. An entry at offset 0 of an undelimited
document has no evidence under either scope and stays LF.
Tests: 10 added. Negative control, driven -- 1 of the 10 fails against this
branch's own pre-fix head (the stray-CRLF fixture); B4 fails against #3773's
head; the remaining 8 are the scope counterexamples ported with the function,
which were regression pins in #3773 and are guards here. Each still kills a
simpler algorithm: drop any one and a refuted scope passes again.
Four deferred-items suites 450/450, 0 skipped. npm run lint:ci exit 0.
* fix(#3702): drop the unreachable rewrite_not_readable guard (B3)
Round-4 B3: the status had zero test coverage in either file. The review
offered two branches -- drive it from a test, or delete it and stop carrying an
untested terminal status. Taking the second, with the reason stated rather than
assumed.
Why it cannot be driven. Round 3 added the guard after a fenced `status:` line
proved the writer could select a line the reader would not read back. Round 3
then closed that divergence STRUCTURALLY, by routing the writer's line selection
and the reader's field extraction through one entryFieldLines seam. The guard
now detects a state construction prevents: 21 document shapes were driven
against it -- fence openers on the bullet line for every marker in the widened
set, duplicate and triplicate status lines, bolded and nested variants, fences
between duplicates -- and none reached it. The only seam that would is routing
the internal call through the module's exports so a test could stub it, which
reshapes production surface for a test.
Why leaving it undriven is not free. RULESET.TESTS.mutation-score runs Stryker
incrementally over changed files at an 80% threshold and says to treat a
surviving mutant as a failing test specification. An undriven `if` on a changed
file is exactly that, on both the condition and the .toLowerCase() comparison.
What this gives up, stated rather than hidden: if a future change re-splits the
writer's selection from the reader's extraction, acknowledgeDeferredItem returns
ok over an item that stays outstanding -- the original #3702 defect class. One
correction to the review's framing: match_verification_failed does NOT backfill
it. That check runs BEFORE the write and compares the matched span to the
target, so it cannot see a post-write read-back failure. The protection against
re-splitting is the shared seam and the round-3 tests that pin it, not a runtime
assertion. A comment at the removal site records all of this.
Removing it also drops the union member from both files, which resolves the PR
body's internal contradiction (it claimed no type-signature changes while adding
one) and the duplicate-status surface #3773 collides on.
No test changed behaviour: 450/450 across the four deferred-items suites, 149/149
across the audit suites, npm run lint:ci exit 0 -- the same figures as before the
removal, which is itself the evidence that nothing exercised the branch.
* fix(#3702): the deferred fence gate is indent-unbounded, like the rest of the grammar (M2)
Round-4 M2. scanFencedBlocks is CommonMark, which caps a fence delimiter's
indent at three spaces -- a fourth makes it an indented code block instead. This
grammar had already opted out of that cliff for entry openers ([ \t]*) and for
THEMATIC_BREAK_RE (^[ \t]*), but not for fences. So a fence at four spaces was
not a fence to the gate, and a `status: resolved` line inside it RESOLVED the
entry containing it.
That is not an exotic shape. A fenced block written under a NESTED bullet sits
at four spaces, so ordinary hand-written deferred-items.md files reach it.
Driven before the fix at indents 4, 5, 8 and a leading tab: all four silently
resolved. It is the #3702 silent-resolution defect class in a new place.
gsd-core/references/executor-examples.md, added by this PR, states flatly that
"nothing inside a fenced code block is an entry or a field". The review offered
fixing the parser or bounding that claim in three places. Fixing it -- the claim
is the one users will rely on, and the grammar had already chosen unbounded
indent everywhere else.
NO second fence dialect (the rule blankIndentedFenceDelimiters states). The
classification is still done by scanFencedBlocks, the one exported CommonMark
state machine, over a de-indented VIEW of the same lines. Run lengths, backtick
vs tilde, closer-must-match-and-not-trail, info-string rules and the
unterminated-at-EOF case remain that engine's answers. Indent is the only
dimension hidden from it, and it is exactly the dimension this grammar has
already declared it does not measure. Index alignment is 1:1 -- map preserves
length -- so every returned line index still addresses the original line.
Scope is the deferred grammar only. Both marker-parameterised call sites gate on
markers.blockStructure, which the Gaps set does not set, so Gaps reaches an empty
set. Verified, not asserted: the 47-fixture Gaps differential (marker x
line-ending x separator x fence x break x key-shape x list-shape) is
BYTE-IDENTICAL across this change, 8033 bytes both sides.
Tests: 14 added, of which 8 fail against the pre-fix source and pass here; the
other 6 are the deliberate controls -- indents 0 through 3, which must NOT move,
and the Gaps opt-out guard.
Four deferred-items suites green; the 58 suites touching uat/deferred/sectionizer
run 6045 tests with an IDENTICAL failing set before and after this change (17
pre-existing environment failures -- installs and an unpinned GSD_EMITTED_BASE;
emitted-attribution passes 259/259 in isolation with its base pinned). lint:ci
exit 0.
* fix(#3702): changeset, both prose parsers, and the minors (M3, M4, m1-m3, m5, n1-n2)
M3 -- the changeset omitted a user-BREAKING change. Measured against next: a
heading-delimited deferred-items.md written with `*`, `+` or `1.` went from
"0 entries, so complete-milestone has nothing to acknowledge and closes" to
"1 entry, the CLI writer refuses the heading shape, ACK_FAILURES accumulates,
exit 1". The `-` form already halted and is unchanged. That is release-note
material: a close that used to succeed now fails, and the correct response is to
fix the file, not revert. Also names the fence-indent fix below, and adds #3740
so #3773's issue is attributed here as it is absorbed.
M4 -- gsd-core/workflows/progress/steps/forensic-audit.md is a SECOND,
model-executed parser of the same grammar, and prose cannot carry a parity test.
Its widened text stated the start-at-1 rule, fences and separators but not the
`1)` exclusion nor the nine-digit ordinal cap, both enforced in code with pinned
tests. Both stated now, along with the round-4 fence-indent rule. (No ack
fragment: the size ratchet's currentSizes does a NON-recursive readdirSync of
gsd-core/workflows and agents, so a file under workflows/progress/steps/ is
outside its scope -- verified by reading the helper, not by the green.)
n1 -- executor-examples.md documented that the BOLDED status key is matched
case-insensitively and left the bare key's rule to inference. Driven: bare
`Status: resolved` is NOT read, so the entry stays open with no warning, while
`**Status:**` is. Stated explicitly, with the digit cap and the any-indent fence
rule (n2).
m1 -- boundary coverage was 2/3. limit (999999999.) and limit+1 (1234567890.)
were pinned; limit-1 (12345678.) added, per RULESET.TESTS.boundary-coverage.
m2 -- THEMATIC_BREAK_RE and the tab-expanding indent counter are hand-rolled
CommonMark rules with no in-repo peer to compare against, so the parity
assertion is against the SPEC: eight positive and five negative fixtures, plus
the two DELIBERATE divergences pinned as deliberate (`+` is a separator here but
not in CommonMark, because `+` is a list marker in this grammar and `+ + +`
would otherwise be a phantom entry; indent is unbounded). One fixture was
initially wrong -- `-- -` IS a CommonMark break, since the spec allows free
spacing between the three characters -- and the parser was right.
m3 -- the result union is hand-duplicated in audit.cts as part of a deliberate
structural view of uat.cjs, so the fix is not to delete a copy but to make drift
observable. Every REACHABLE status is now driven from a fixture; four of the six
(ambiguous, unsupported_heading_shape, already_resolved, match_verification_failed)
had no assertion anywhere in the suite before this. match_verification_failed is
still undriven and the test says so rather than omitting it.
m5 -- DECLINED, with the measurement. The review is right that `(\s*)` in the
opener and `/^[ \t]*/` in the reader disagree about \f, \v and NBSP, but its
prescribed narrowing was implemented, driven and REVERTED: as shipped, an entry
indented with any of those surfaces, parses its status field, acknowledges, and
reads back acknowledged -- a complete round-trip. Narrowing turns all three into
SILENTLY DROPPED entries, which is the #3702 defect class itself and the opposite
of this file's stated fail-safe rule. A latent inconsistency in the safe
direction is not worth a live regression in the unsafe one. Pinned by three
round-trip tests so the prescription cannot be re-applied silently; if it is ever
closed, the direction is to make the readers agree with the opener, not to make
the opener reject lines it accepts today.
Four deferred-items suites 475/475, 0 skipped. lint:ci and lint:changeset exit 0.
The 47-fixture Gaps differential is byte-identical at 8033 bytes.
* fix(#3702): the pinned `## Gaps` phantom now cites its issue (m4)
Round-4 m4. The second assertion in the Gaps byte-for-byte test pins a real
defect as expected output: a spaced hyphen thematic break in `## Gaps` is read
as an ITEM, so `- - -` surfaces a phantom open gap named `- -`. Reproduced on
pristine next at
|
||
|
Tom Boucher
|
6beaa66b25 |
enhance(#3304): gate re-verification blockers on deterministic evidence (#4085)
* test(#3304): add failing-first suite for the convergence evidence gate Content-assertion suite for the Step 7 re-verification evidence gate (agents/gsd-verifier.md / gsd-core/references/verifier-evidence-gate.md). Committed before the implementation to prove RED via gsd-test. * enhance(#3304): gate re-verification blockers on deterministic evidence Step 7's anti-pattern scan re-runs at full, unbounded scope on every re-verification pass, independent of the must-haves established in Step 2. A blocker it finds — other than the self-evidencing debt-marker check — previously reverted a completed gap-closure round and started another --gaps cycle on nothing more than the verifier's own new judgment call, with no bound on how many times that could repeat. A Step 7 blocker now blocks unconditionally in re-verification mode only if it is a carried-forward gap (present in the prior VERIFICATION.md's gaps: list) or the flagged file was git-modified since the prior pass (a regression; fails closed toward blocking when history is unresolvable). Otherwise it predates the gap-closure round unflagged and needs deterministic evidence — a named test run red, or another concrete reproducible artifact — to stay blocking. Unevidenced, it downgrades to a new advisory: frontmatter list and report section instead of setting status: gaps_found, and never reverts a completed must-have. Maintainer approval was narrowed to this evidence condition only, explicitly rejecting the broader "advisory whenever untraceable to a requirement/decision/prior-gap" proposal — implemented and pinned by tests/verifier-evidence-gate.test.cjs and documented as rejected in gsd-core/references/verifier-evidence-gate.md so it can't silently re-expand. Closes #3304 * fix(#3304): correct window-truncation and indentation bugs in evidence-gate tests gsd-test's GREEN checkpoint caught 3 real bugs in the test file itself (not the production prose): a {0,600} match window was shorter than the 724-char paragraph it was scanning (the "exclude from Step 9 Rule 1" phrase starts at offset 662), and two regexes assumed no indentation after a markdown list-continuation line break. All three phrases are confirmed unique across agents/gsd-verifier.md, so the windowed submatches are replaced with direct whole-string assertions instead of just widening the window. Also acknowledges the deliberate byte growth in agents/gsd-verifier.md that the differential-attribution check (ADR-2719) correctly flagged. Emitted-Drift-Ack-Growth: gsd-verifier.md — adds the #3304 re-verification evidence gate (Step 7 rule, Advisory bucket, advisory: frontmatter, report section); 1488 bytes, still within the LARGE-tier 48 KiB cap (48751/49152). * docs(#3304): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
62b0d939b6 |
feat(#3274): make reviewer-lane timeout configurable via timeoutConfigKey (#4083)
* feat(#3274): make reviewer-lane timeout configurable via timeoutConfigKey Add an optional `timeoutConfigKey` field to the reviewer lane descriptor, resolved in `resolveLanePlan` at invocation time and falling back to the frozen `timeoutFloorMs` when unset or invalid, in the same spirit as the existing `promptBudgetKey`/`modelConfigKey` fields. All 12 shipped lanes declare `review.timeouts.<slug>` on both surfaces (the descriptor and their capability.json manifest), validated by capability-validator.cjs. For the antigravity lane, the native `agy --print-timeout` flag — previously a second hardcoded literal (`540s`) independent of the outer cap — is now derived from the same resolved outer timeout in `antigravityArgv`, preserving the existing 60-second buffer relationship (ADR-2782 D6: the outer bound is declared data, the inner one is handler-owned). The antigravity default timeoutFloorMs stays at 600s per the maintainer's disposition; users raise it through the new config key instead. * docs(#3274): document review.timeouts.* and extract resolveTimeoutMs helper Address code-review findings on the timeoutConfigKey change: extract the inline timeout-resolution logic into a named, exported, directly-tested resolveTimeoutMs helper (matching the file's existing configString/ normalizeHost convention); document the new review.timeouts.* federated config keys in docs/CONFIGURATION.md, docs/reference/capability-manifest.md, and docs/how-to/ship-a-reviewer-lane.md; add the changeset fragment. * fix(#3274): resolve native antigravity timeout in resolveLanePlan, not the runner gsd-test caught two design mistakes in the prior commits: 1. SpawnPlan.argv is documented and tested as fully resolved by resolveLanePlan (model/effort/output/prompt already folded in) — leaving the antigravity '{{nativeTimeout}}' marker unresolved until the runner's antigravityArgv violated that contract and broke tests that read plan.argv directly (tests/antigravity-reviewer.test.cjs, tests/review-default-reviewers-workflow.test.cjs). Fix: '{{nativeTimeout}}' is now a fifth ARGV_PLACEHOLDER member, resolved by resolveLanePlan itself via the new nativeTimeoutToken() helper, exactly like the other four. antigravityArgv reverts to its pre-#3274 four-argument form. Also missed updating capabilities/antigravity/capability.json's invoke.args to match the descriptor, which broke the manifest/descriptor parity test. 2. tests/reviewer-config-federation.test.cjs enforces a deliberate, narrow invariant (#3691 narrows #2797): qwen, cursor, and coderabbit — the three lanes with neither a model flag nor a host — may own no config key beyond their own prompt-budget key. Adding review.timeouts.<slug> to all 12 lanes violated it. Fix: those three keep timeoutConfigKey: null and own no review.timeouts.* key, matching their existing modelConfigKey: null. The other 9 lanes are unaffected. * chore(#3274): backfill changeset PR number (pr:0 -> 4083) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
6eea00b707 |
enhance(#3301): tell reviewers the plan ids and total count, grade coverage (#4084)
* test(#3301): add failing-first plan coverage manifest tests Failing-first regression tests for the plan-id manifest, the updated Review Instructions, and the mechanical per-reviewer coverage check, ahead of the review.md implementation. RED baseline before the fix lands. * test(#3301): raise allow-test-rule-refs unverified ceiling for new marker Adding tests/review-plan-coverage-manifest.test.cjs's source-text-is-the-product marker grows the unverified-exemption pool by one (282 -> 283), the same documented growth path scripts/lint-allow-test-rule-refs.cjs's own failure output names. Confirmed clean via 'npm run lint:allow-test-rule-refs' locally. * feat(#3301): tell reviewers the plan ids and total count, grade coverage build_prompt now derives a plan-id manifest from each *-PLAN.md filename (stripping the -PLAN.md suffix) and appends it, with the total plan count, to both gsd-review-instructions.md and gsd-review-prompt.md. The Review Instructions prose requires one heading-verbatim section per id before any cross-plan or overall-risk content. write_reviews grades each dispatched lane's real (non-stub, non-empty) review against that same manifest and records an optional plan_coverage: frontmatter block, present only when a lane is incomplete. The match escapes regex metacharacters in the id and excludes a preceding/trailing hyphen or word character as a boundary, closing the two traps named in the issue (a decimal phase like 12.6 satisfied by 12X6-01; a threat id like T-04-07 registering as coverage of plan 04-07). CodeRabbit is exempt, since it never receives the source-grounding prompt carrying the manifest. This closes the gap where a review that silently covers only some plans in a multi-plan phase is indistinguishable from one that covers all of them. * docs(#3301): add changeset fragment * test(#3301): use t.after() instead of try/finally for cleanup CONTRIBUTING.md bans try/finally inside test bodies. Code review caught this in the new coverage-manifest test file; switch every fixture-cleanup site to the approved t.after() pattern. * test: use t.after() instead of try/finally in #3300's build_prompt tests Pre-existing try/finally-for-cleanup pattern in this file (landed for #3300) violates CONTRIBUTING.md's explicit ban on try/finally inside test bodies. Surfaced incidentally while reviewing #3301's diff, which cites this file as its extraction-pattern precedent; fixed inline per the no-defer rule rather than deferred to a separate PR. * test(#3301): anchor coverage-check extraction on the fence line, not prose `.plans-manifest.md` also appears in write_reviews' own prose ahead of the ```bash fence, so indexOf found that occurrence first and the backward-walk-to-fence-open landed on the earlier, unrelated gate-check block instead. gsd-test caught this: coverage-check tests expecting a real verdict got null, because the wrong block ran and never writes .plan-coverage-<slug>.json. Anchor on the fence-only bash assignment line instead. Emitted-Drift-Ack-Growth: review.md — #3301 adds the plan-coverage manifest and mechanical coverage check to build_prompt/write_reviews. * fix(#3301): route id escaping through the canonical pattern seam ADR-3212 (epic #3212) consolidated ~44 hand-rolled regex-escape copies into one owner, src/pattern.cts's escapeRegex, specifically to stop this exact class of duplication. My coverage-check node -e script hand-rolled the identical metachar-escape regex — invisible to eslint-rules/no-adhoc-regex-escape.cjs only because it lives inside a workflow markdown file, not a .cts/.cjs source file the shape-matching guard scans. Require the compiled seam (gsd-core/bin/lib/pattern.cjs) instead, matching the established node -e-requires-a-compiled-lib idiom already used elsewhere in this workflow (code-review.md's code-review-flags.cjs/code-review-depth.cjs calls). Verified both named traps from the issue still resolve correctly under escapeRegex's RegExp.escape-backed implementation, which differs in escaped-text shape (hex-escapes hyphens/leading chars) but not match result. * test(#3301): run coverage-check block with cwd at the repo root The block's node -e now requires ./gsd-core/bin/lib/pattern.cjs, a path relative to the repo root (correct for production, which always runs from there). The test harness ran it with cwd at the fixture's own temp dir instead, so the require failed. Add an optional cwd param to runScript (default: root, unchanged for the plan-copy-block tests) and pass the real repo root for every coverage-check call site. Manually verified end-to-end before spending another remote run: the extracted block now produces the expected {complete:true} verdict. * docs(#3301): backfill changeset pr number (pr:0 -> pr:4084) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
86452da7cb |
fix(#4070): reserve shard 1's aux-suite cost out of the LPT unit-test packer (#4072)
* test(#4070): failing-first regression for shard1 aux-suite budget imbalance - selectShard has no way to reserve virtual weight on a bin, so the LPT unit-test packer cannot account for shard 1's fixed aux-suite cost (integration/security/install/slow all pinned to shard 1/3). - test.yml wires no such reserve into the workflow. - ci-test-job-timeout-budget.test.cjs's LANE_COSTS entry for job `test` was stale (7m12s from run 30677442953, predating the aux-suite growth); corrected to the real evidence cited in #4070 (13m48s / cancelled at ~14m51s), which now honestly fails the file's own 1.5x headroom policy against the current 15-minute cap. All three are expected RED on this commit; see .gsd/bug/fix-4070-shard1-aux-suite-budget/50-test-matrix.md. * fix(#4070): reserve shard 1's aux-suite cost out of the LPT unit-test packer selectShard now accepts an optional initialWeights array giving one or more bins a virtual head start before any file is placed, so LPT converges each bin's FINAL total (assigned weight + head start) toward equal instead of raw assigned weight alone. test.yml wires RUN_TESTS_SHARD_RESERVE=1:77 into the full-scope unit-test step (gated on matrix.scope == 'full', so the unrelated windows lane is unaffected) -- 77 weight units is the empirical conversion of the aux suites' ~220s measured fixed cost, derived against the real tests/test-timings.json (see the diagnosis artifact for the full computation). Also corrects two pieces of now-stale bookkeeping this issue exposed: - ci-test-job-timeout-budget.test.cjs's LANE_COSTS entry for job `test` carried a 7m12s figure that predated the aux-suite growth; corrected to the real pre-fix evidence (13m48s / cancelled at ~14m51s, issue #4070), which requires raising timeout-minutes from 15 to 21 (1.5x headroom over the real worst-case measurement) to satisfy the file's own policy. - test.yml's job-header and matrix comments, which still claimed the aux suites cost "~1m35s combined" (they now measure ~216-224s). Closes the gap the previous commit's failing-first tests proved: selectShard had no reserve-capacity mechanism and test.yml wired none in. * fix(#4070): correct the reserved-weight property bound; cover main()'s reserve bounds check Isolated code review found a genuine gap and gsd-test's real run confirmed a real test bug it exposed: - The fast-check property "no shard exceeds average(+reserve) + heaviest file" was falsified by gsd-test itself (weights=[1,1,1], total=2, reserve=6 on bin 0): selectShard is correct, the BOUND was wrong. A reserve large enough that its bin never receives a real item stays at exactly that reserve forever -- no amount of routing real items elsewhere can dilute a fixed head start below itself -- so the true bound is max(reserve, the classic Graham term), not the Graham term alone. Verified the corrected bound against the exact counterexample plus 20,000 additional random trials (zero violations) before re-running gsd-test. - Isolated review (MAJOR): the shard-total bounds check on RUN_TESTS_SHARD_RESERVE and its console.error fallback in main() were untested end-to-end -- parseShardReserve itself has no concept of the shard total, so only main() enforces that guard, and nothing exercised it through the subprocess seam. Added an E2E harness test that sets RUN_TESTS_SHARD_RESERVE to an out-of-range index via the real CLI, asserts the fallback warning fires, AND asserts the resulting file selection is byte-identical to a no-reserve control run against the same injected timings table -- proving the reserve was actually ignored, not just that a warning printed. * chore(#4070): backfill changeset PR number pr:0 -> pr:4072 * fix(#4070): strip leaked RUN_TESTS_SHARD_RESERVE from the harness test's child env Real GH Actions CI on this PR (run 33288554040, ubuntu shard 2/3) failed 7 tests in the shard-partitioning describe block, all with the same symptom: `run-tests: no tests in suite "all"` where a real file count was expected. gsd-test's own dockerized bench run never showed this, and ubuntu shards 1/3 and 3/3 (which run the same test.yml step) passed clean -- the discrepancy is the tell: only shard 2/3 happened to schedule this specific test FILE for that run, and the outer CI job's own environment is where the leak lives. Root cause: test.yml's "Run unit tests" step now sets RUN_TESTS_SHARD_RESERVE=1:77 (this issue's own reserve mechanism) on the OUTER job that runs `npm run test:coverage:unit:raw -- --shard N`. The harness test file's runHarness() helper spawns run-tests.cjs as a CHILD of that same job via `{...process.env, ...extraEnv}`, so every pre-existing --shard test in this describe block silently inherited the ambient reserve -- even though none of them know it exists. A reserve of 77 weight units utterly dwarfs the ~0.3 total weight of the 9-file synthetic fixtures these tests use (none are in the real timings table, so all fall back to the same tiny median weight), so shard index 1 is routed zero files every time -- exactly the observed "no tests" failures, and exactly the skewed 5/4 split observed on the shard-2 test that expected a plain 3/3/3 round-robin. Reproduced locally end to end (set RUN_TESTS_SHARD_RESERVE=1:77, spawn the old runHarness against a synthetic 9-file fixture, --shard 1/3 -- reproduces the exact "no tests in suite \"all\"" stderr) and confirmed the fix (env stripped unless a test opts in via extraEnv, as the #4070 E2E bounds-check test already does) resolves it, before re-running gsd-test. This is a genuine bug this PR introduced -- a new ambient env var that a pre-existing subprocess-spawning test helper didn't know to isolate against -- not a pre-existing flake and not resource contention. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
4d70b4dc43 |
fix(#4068): add --merge-async to c8 coverage-merge invocations (#4069)
* test(#4068): failing-first regression guard for c8 --merge-async flag Adds a config-invariant test asserting test:coverage:unit and test:coverage:report pass --merge-async to c8, guarding against the coverage-merge OOM (release.yml finalize dry-run, exit 134/SIGABRT) regressing a third time (prior stopgap: #199). Also commits the sourced research memo backing the diagnosis. * test(#4068): rename regression test to avoid lint-test-file-count collision coverage-merge-async-flag.test.cjs's effective prefix (coverage-merge- async-flag) matched the unrelated gsd-core/bin/lib/coverage.cjs module's 2-file cap under lint-test-file-count.cjs's startsWith bucketing, failing FAIL_EXCEEDS_LIMIT (confirmed via the RED gsd-test run on 7b6e6ca9e). Renamed to c8-merge-async-flag.test.cjs -- no colliding prefix. * fix(#4068): add --merge-async to c8 coverage-merge invocations The `test:coverage:unit` script OOM-crashed (exit 134, SIGABRT) in the release.yml finalize dry-run of 1.12.0: all 1785 unit tests pass, then c8's report/merge phase crashes ~76s later against the 6144 MB heap ceiling. Root cause (verified against this repo's pinned c8@11.0.0 source, node_modules/c8/lib/report.js): the default sync merge path, Report._getMergedProcessCov(), loads every raw V8 coverage file for the whole run into memory as one array before merging. This is a recurrence of #199 (same OOM class at ~466 tests, "fixed" by raising the heap ceiling) -- the suite outgrew 6144 MB as it grew to 1785 tests, and test.yml's own coverage-gate job already independently hit and stopgap-fixed the identical class once (test.yml:614-620, 4096->8192 MB). c8 ships the upstream fix for exactly this: --merge-async (c8 v7.14.0, already inside the pinned c8@^11.0.0 range -- no dependency bump), which switches to Report._getMergedProcessCovAsync(), reading and merging one raw file at a time instead of loading them all at once. The merge arithmetic (mergeProcessCovs()) and --all's zero-coverage-file inclusion are unchanged between the two paths, so the coverage gate's accuracy is unaffected. Verified empirically (not just by source inspection): against a 223 MB synthetic raw-coverage corpus derived from this repo's actual 235 gsd-core/bin/lib/**/*.cjs files (same order of magnitude as the 358 MB figure documented in test.yml), c8's real Report class crashes with the same "Ineffective mark-compacts near heap limit" signature via the sync path at a 300 MB heap ceiling, while the async path completes at a flat ~40-50 MB peak down to a 100 MB ceiling. Full repro steps and numbers: .gsd/bug/fix-4068-coverage-merge-oom/10-diagnosis.md. Only `test:coverage:unit` and `test:coverage:report` are changed. `test:coverage` (line 150) is a non-CI dev-convenience script, never invoked as a command by any workflow. `test:coverage:unit:raw` (line 153) already runs --reporter none, which skips the merge/report phase this flag affects, entirely -- adding it there would be a no-op. Fixes #4068 * fix(#4068): add changeset fragment for the coverage-merge OOM fix The changeset fragment was generated locally (npm run changeset) but never committed -- isolated code review caught the gap (no .changeset/*.md on the branch, confirmed via git diff --name-status). * chore(#4068): backfill changeset PR number pr:0 -> pr:4069 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8793d307f6 |
fix(#4060): drive repo-baseline lint check in-process, not via a subprocess timeout race (#4065)
* test(#4060): failing-first regression for repo-baseline subtest timeout race The "repo baseline passes" subtest in lint-allow-test-rule-refs.test.cjs drives the script under test via a spawnSync subprocess with a fixed 30s timeout, which races the script's real wall-clock completion against unbounded CI-load contention -- it has already died at this race twice (#4060, and once before at a lower bound). Rewrites the subtest to call the script's `main` directly, in-process, removing the subprocess timeout race entirely. This commit only changes the test (main is not yet exported), so it fails first with `TypeError: scriptUnderTest.main is not a function`. * fix(#4060): export lint-allow-test-rule-refs main() for in-process drive The "repo baseline passes" subtest previously drove this script via a spawnSync subprocess with a fixed 30s timeout, racing the script's real completion time against unbounded CI-load contention -- it has now died at that race twice (#4060, and once before at a lower bound). A fixed timeout racing unbounded contention has no value that is both tight and safe, so raising it again would not fix the mechanism, only its odds. Parameterizes main() to accept an explicit argv (defaulting to process.argv.slice(2) only when omitted, so the CLI entrypoint is unaffected) and exports it, so the test can call it directly, in-process -- removing the subprocess and its spawnSync timeout kill race entirely for this one row. * fix(#4060): capture stderr too in the in-process repo-baseline subtest Code-review finding: the in-process rewrite captured only console.log, but main()'s real failure path throws a bare, messageless ExitError -- all diagnostic detail goes to process.stderr.write. The old subprocess-based assertion embedded both stdout and stderr in its failure message; this silently dropped that debuggability. Captures process.stderr.write the same way (restored in finally) and surfaces both streams in assertion failure messages and in a wrapped re-thrown error on an unexpected throw from main(). --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
6e681cb36c |
fix(#3901): commit-docs-guard suites isolate children from a global core.hooksPath (#4063)
* test(#3901): the guard suite must survive a developer's global core.hooksPath (failing first) * fix(#3901): the commit-docs-guard suite isolates children from the host git config A developer's GLOBAL core.hooksPath (~/.gitconfig) applies to every fresh repo, so the guard's correct refusal — it will not install a hook git would never run — failed this suite's beforeEach and 18 tests read as a commit-docs-guard regression on any machine that centralizes commit hooks. CI never saw it (runners have no global git config). before() pins GIT_CONFIG_GLOBAL to an empty file for the whole suite (runGsdTools and the git helpers propagate process.env to every child). A LOCAL repo value cannot isolate this: any non-empty local value trips the same refusal, and an empty local value makes rev-parse --git-path hooks resolve to ./ instead of .git/hooks. The regression test pins the seam: the isolation file is armed and empty, a child git resolves no hooksPath from the host, and the beforeEach enable installed the hook at the repo-local default path. A child given an explicitly hostile GIT_CONFIG_GLOBAL still refuses — that is the guard being correct. * fix(#3901): review fold-ins — isolation hoisted to all three guard suites; changeset Adversarial review: the pin covered suite A only — the B (#3588 B1-B15) and D (real git commit wiring) suites run enable against fresh repos too and failed identically on hostile-global machines; the issue's 18 failures could not have come from A's five tests alone. The pin is extracted to isolateGlobalGitConfig() and armed in all three suites (B9's LOCAL hooksPath test is unaffected — the pin neutralizes only the global scope; D's premise is the hook firing from .git/hooks, which the isolation preserves). * chore(#3901): backfill changeset PR number (4063) * fix(#3901): drop the now-unused before import — max-warnings 0 fails CI --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
73919526e9 |
fix(#3902): packaging guard resolves npm 12's pack --json shape — stays armed on Node 26 (#4064)
* test(#3902): packListToPathSet must resolve npm 12's object-keyed pack --json shape (failing first) * fix(#3902): resolve both npm pack --json shapes — the packaging guard stays armed on Node 26 npm 12 (bundled with Node 26) emits an object keyed by package name where npm <=11 emitted an array; parsed[0] was undefined, the before() hook threw, and all 6 tests in the packaging guard — including the two 'does NOT ship' guards that stop repo-only CI tooling leaking into the published package — went dark for contributors on Node 26. CONTRIBUTING pins Node 24 as the floor but requires Node 26 compatibility for code AND tests; this was the test's parse, not the code. packListToPathSet now resolves either shape and fails loud on anything else — a silently-empty set would be the exact dark-guard failure mode this guard exists to prevent. * fix(#3902): review fold-in — a multi-key object fails loud Single-package assumption documented and enforced: npm 12 emits exactly one key for this repo (no workspaces — verified); if workspaces were ever adopted, first-key selection would silently validate one package's file list and recreate the exact silent-disarm this fix kills. * chore(#3902): changeset fragment (pr number backfilled after PR creation) * chore(#3902): backfill changeset PR number (4064) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
686c05a740 |
fix(#4058): raise MAX_ACK_TRAILERS from 64 to 128 (#4059)
* fix(#4058): raise MAX_ACK_TRAILERS from 64 to 128 64 had no principled derivation (no git trailer limit, no CI resource bound) and a wide-touching maintenance PR can legitimately accumulate more than 64 distinct emitted-drift acknowledgments, tripping the cap and failing Required tests even though nothing is actually wrong. The cap's underlying purpose is unchanged: parseAckTrailers() still throws rather than truncates on overflow, and still forces pruning of stale acknowledgments at some ceiling. Only the ceiling moves. * chore(#4058): backfill changeset PR number (4059) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
90fff40f5a |
fix(#3900): overlay walker skips non-regular files — a repo-root socket no longer fails 32 tests (#4062)
* test(#3900): a unix socket under the repo root must not break the overlay (failing first) * fix(#3900): the overlay walker skips non-regular files (sockets, FIFOs, device nodes) The walker classified every not-a-directory entry as a file, so a unix socket anywhere under the repo root (an MCP server, a language-server daemon, a watcher) made copyFileSync throw ENXIO — the reporter measured 32 local test failures across 6 files, all in before() hooks, none about the code under test; a FIFO would block until a writer appeared. The hard-link path does not save it: the overlay sits under tmpfs, so cross-device EXDEV always falls through to the copy. One guard at the classification site (srcStat.isFile() before the copy/link branches) — sockets, FIFOs, and device nodes are not repository content; CI never saw this because a fresh clone has no working-tree daemons, which is exactly why it ate local debugging time. Also adds opts.root (test-only root override, default REPO_ROOT) so the regression tests build the overlay from a tiny synthetic root in milliseconds instead of paying a full repo walk. * chore(#3900): changeset fragment (pr number backfilled after PR creation) * fix(#3900): review fold-ins — sibling cold-fixture guard, changeset format, root doc buildColdInstallTree enumerated the live repo's hooks/ with a NAME-only filter — the same class #3900 documents: a daemon socket or FIFO inside hooks/ would reach cpSync and throw ENXIO (or block). Dirent type guard added. The changeset gains the mandated bold user-visible lead, and opts.root is documented at its definition (mirroring cold-runtime-lib- fixture's repoRoot convention). * chore(#3900): backfill changeset PR number (4062) --------- Co-authored-by: sim <sim@local> |
||
|
Dennis Kim
|
8487f0ed42 |
enhance(#3552): warn on additional protected branches beyond the resolved base branch (#3648)
* test(01-01): add failing protected-branch warning coverage - pin configured, absent, and malformed branch-list behavior - require opposite CLI and execute warning outcomes * feat(01-01): warn on configured protected branches - resolve the base branch union configured protected branch names - expose exact boolean CLI comparison output for workflow callers - keep execute-phase warning advisory and within its byte budget * test(01-01): add failing protected branch config coverage - cover valid list persistence and null unset - reject hostile shapes while preserving the prior value * feat(01-01): validate protected branch configuration - register git.protected_branches as a canonical config key - require a non-empty array of non-blank branch names * test(01-02): add failing ship protected-branch controls - Execute both workflow warning blocks with exact predicate arguments - Require true and false results to produce opposite warning outcomes - Preserve the none-strategy feature-branch offer contract * feat(01-02): warn at ship on protected branches - Reuse the typed protected-branch predicate in ship preflight - Keep raw base resolution for PR targeting and advisory branch creation - Prove execute and ship warning blocks with opposite-result controls * test(01-02): add failing protected-branch docs parity - Require the canonical schema key in both English config references - Pin the non-empty string-array type and absent default - Require synchronized multi-branch examples and advisory semantics * feat(01-02): publish protected branch configuration contract - Document the optional non-empty string-array field in both references - Explain resolved-base union and absent-field compatibility - Keep execute and ship warnings advisory under branching_strategy none * fix(01): CR-01 honor active workstream branch policy * fix(01): WR-01 assert protected config path selection * docs: add changeset fragment for #3648 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CteVPJt4BkPmroMPGajYx * fix(#3648): resolve base_branch precedence inversion and round-1 findings Blocker 1/2: production config resolution was flat-first, so a project that migrated to git.base_branch but still carried a stale flat base_branch got the old value back. Add base_branch to normalizeLegacyKeys (mirrors the existing branching_strategy/sub_repos pattern: canonical nested wins) and route readEffectiveGitConfig's test seam through the same normalization so it can't silently diverge from production again. Adds a regression test with both keys set that fails without the fix. Blocker 3/4/5: restore the handle_branching case-selector prose and "none" contract sentence that #3389's tests anchor on, and revert the unrelated prose/comment compaction in the same step — both were drive-by edits outside #3552's scope. Also addresses review majors/minors: delete readConfigBaseBranch and readConfigProtectedBranches (dead in production, only self-tested); --is-protected now fails closed (reports protected) instead of silently answering false when the base branch can't be verified; trim configured protected-branch names; fix HOME-without-USERPROFILE vacuous isolation on Windows; correct the drift-ack's byte accounting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S44stkuQbhD3jTCtKzte5N * test(#3648): add failing legacy-key hoist safety coverage Round-2 review found normalizeLegacyKeys block 5 records a normalization carrying the DISCARDED flat value on the canonical-wins branch. Probing that turned up a second, unreported defect in the same helper shape: blocks 1, 2 and 5 all spread result['git'] / result['planning'] with no object guard, so a config whose section key holds a string is spread into index keys — {"git":"main","base_branch":"release"} -> {"git":{"0":"m","1":"a","2":"i","3":"n","base_branch":"release"}} The resolved value is accidentally still correct, so nothing fails and no diagnostic fires. But normalizations.length > 0 sets configDirty, and config-loader then serializes that shape back into the user's config.json — a read that silently corrupts config. The deleted #3057 W3 suite covered {"git":"main","base_branch":"release"} explicitly; this is the input it would have caught. Covers both defects across blocks 1 and 5, with object/array/null negative controls that must stay green in both phases, and a fast-check property over arbitrary `git` values. * test(#3648): pin fail-closed handling of malformed protected_branches Replaces the test that pinned the fail-OPEN behaviour. The old assertion — ['develop', 42] yields isProtected === false for 'develop' — locked in the exact failure #3552 exists to close: config-set validation is bypassable by a direct edit of .planning/config.json, so a user who believes 'develop' is protected got a silent false and no warning. It was also inconsistent with the fail-CLOSED direction twelve lines away, where an unverified base reports protected and writes a diagnostic. A protection predicate must not have two opposite failure directions depending on which input is bad (#3648 review Blocker 3). New coverage: a bad element drops only itself, a non-array contributes no names, an empty list is well-formed rather than malformed, and --is-protected surfaces the rejection. Both negative controls — a clean list reports nothing rejected and writes no diagnostic — must stay green in either phase, so the reject channel cannot fire unconditionally. * fix(#3648): drop only invalid protected_branches and report them Partition git.protected_branches instead of discarding the whole list on one bad element, and carry the rejections out through ProtectedBranchStatus so --is-protected can name them on stderr. Valid names keep protecting; the user finds out the rest were ignored. A non-array value still contributes no names — a bare string is not a list of branch names — but is now reported rather than swallowed. An empty array stays silent: declaring no extra protected branches is a valid choice, not a misconfiguration. writeDiagnostic is hoisted out of the unverified-base branch since both arms now use it. * test(#3648): prove the predicate diagnostic survives both call sites The workflow bash stub now emits a stderr diagnostic the way the real command does, which is what makes a swallowed `2>/dev/null` visible to a test — previously the stub was silent on stderr, so discarding it changed no observable behaviour and the call sites could drop the explanation undetected. Adds the Minor 2 binding check as well: ship must expose the predicate result as IS_PROTECTED rather than only echoing a warning, asserted by running the extracted bash and reading the bound value, not by grepping the workflow source. Both tests carry opposite-outcome controls — an empty diagnostic must leave the text absent, and a false predicate must bind false. * fix(#3648): surface the predicate diagnostic and bind ship's result Drop `2>/dev/null` from the --is-protected call at both call sites. The fail-closed explanation and the new rejected-entry warning both go to stderr, so discarding it left the user with a bare "protected branch" warning on a branch that is not protected and no way to tell a real match from a degraded-git guess. `git branch --show-current` keeps its own redirect — that one is genuine noise. ship.md binds IS_PROTECTED and its prose now branches on the variable, so the following steps have evaluable state instead of having to infer it from warning text in tool output. execute-phase.md byte accounting refreshed: 92326 -> 92645, net growth 319 bytes (was 331 before the redirect came out). Baseline re-verified against the current rebase base by blob id; the ceiling check passes with 755 bytes of margin. * test(#3648): restore negative space for the readFile config seam The #3057 W3 suite was deleted with readConfigBaseBranch, but every arm it pinned survives verbatim in readEffectiveGitConfig's readFile branch — the JSON.parse catch, the non-object guard, the git-section object guard, .trim() and blank-string rejection — and the four surviving readFile injections were positive-path only. protected_branches was never driven through this seam at all. Restores nine cases against the seam, including protected_branches partitioning, plus a control proving loadConfig still wins when both seams are supplied. Records honestly what the suite pins. Mutating the built lib shows .trim() is KILLED, while the non-object guard and the blank-string rejection SURVIVE — both are unreachable through this entry point for the same reasons the deleted suite documented against its own equivalents: a JSON-parsed non-object carries no relevant own-property either way, and a blank value is rejected a second time downstream by the resolver's truthiness check. They stay as defence-in-depth and are labelled known-unkillable rather than left looking like coverage this suite does not provide. * test(#3648): distinguish detached HEAD from a missing branch argument `args[1] ?? ''` collapsed two different situations into one: a detached HEAD, where `git branch --show-current` legitimately prints nothing, and the flag being called with no argument at all. Both answered false, so the right outcome arrived by an unintentional path and a caller bug was indistinguishable from normal operation. Asserts the detached case stays silent and the missing-argument case reports, with a control that the two diagnostics differ. * fix(#3648): report a missing --is-protected branch argument Answer false either way, but say so when the flag arrives with no argument. A detached HEAD passes an explicit empty string and stays silent, since that is a normal state rather than a misconfiguration. * docs(#3648): state exact-name matching and per-entry rejection isProtected is exact string equality, so a git-flow project must enumerate every release/* and hotfix/* by name. #3552 only asked for an integration-branch field, so the implementation satisfies the letter of the issue while leaving its git-flow motivation partly unserved — say so where users will meet it rather than leaving them to discover it. Also documents the Blocker 3 behaviour change: an invalid entry is ignored with a warning naming it and the remaining names still apply. Both statements land in docs/CONFIGURATION.md and gsd-core/references/planning-config.md, and the config-field-docs parity test asserts each in both so the two cannot drift. * refactor(#3648): extract isValidProtectedBranches for cross-surface pinning The `git.protected_branches` check inside `cmdConfigSet` and the resolver's per-entry filter in `git-base-branch.cts` are deliberately different shapes — all-or-nothing on write, per-entry on read, so a hand-edited config.json cannot fail the guard open. Nothing structural keeps their two definitions of "usable branch name" in step. Lifting the write-side check into a named, exported predicate lets a property test ask both surfaces about the same value and assert they agree, which is the fast-check gap the round-2 review flagged. No behaviour change: the predicate is the same expression, called from the same place. * fix(#3648): stop --is-protected rewriting the config it is asking about `gsd_run query git.base-branch --is-protected` runs on every execute-phase and every ship. It resolved config through `loadConfig`, whose normalize-then-write path rewrites `.planning/config.json` whenever any legacy key normalizes — so a boolean question was silently editing the user's checked-in config. This PR had widened the trigger by adding a fifth normalization block (top-level `base_branch` -> `git.base_branch`), making it fire for exactly the projects the feature targets. `loadConfigResolved` gains `options.persist` (opt-OUT, default true): resolution is unchanged, only the two write-back side effects are suppressed. The predicate passes `persist: false`; the ~30 other callers are untouched, so a legacy config is still migrated by ordinary use. Asserted on BYTES rather than parsed shape, because the rewrite reorders keys and reflows whitespace even when the values are equivalent. Three tests, each with its own control: the end-to-end CLI leaves the file byte-identical while still answering `true` from the legacy key (proving the config WAS read); an ordinary persisting load of the same fixture DOES change the bytes (proving the fixture is live rather than inert); and `persist:false` vs default over one directory returns deep-equal config while differing on the write. Reverting the one-line `persist: false` fails the first of those and only that one. Also from the review: - `readEffectiveGitConfig`'s comment claimed the readFile branch routed "through the same precedence authority production uses". It does not, and cannot — it reproduces two of production's steps over a single file. The comment now names what the seam covers and what it does NOT (root/workstream deep merge, builtin and global defaults, federated merge), and the seam now applies production's flat-then-nested lookup so it stops disagreeing about a surviving flat key. - The missing-argument diagnostic promised "answering false", which the fail-closed guard on the same call can contradict by printing `true`. It now states what it did with the argument and leaves the answer to stdout. * test(#3648): re-pin block 5 on #3760's refusal contract #3767 landed on next while this PR was in review and fixed the non-object config-section defect properly: a present-but-non-object section now BLOCKS its own migration — value preserved, no Normalization pushed, refusal reported via `skipped[]` — rather than being rebuilt from a plain-object view. That supersedes this branch's round-2 `hoistLegacyKey`, which prevented the character-key spread but still dropped the section value silently, and which the round-3 review correctly called out as destruction in place of corruption. The rebase drops that commit and routes block 5 through the upstream helper. This file's tests asserted the superseded design, so they are rewritten to pin block 5 — `base_branch` -> `git.base_branch`, which did not exist when #3760's suite was written — against the contract that now governs it: ordinary hoist into an absent/null/object section, canonical-nested-wins, and refusal for each of string/number/boolean/array sections with the exact `skipped` entry. Two controls keep it from passing vacuously: the refusal must be scoped to block 5 (an unrelated block still normalizes in the same call), and a property over arbitrary `git` values asserts hoist and refusal are exhaustive AND mutually exclusive per key, that a refusal leaves both the section and the legacy key untouched, and that a hoist manufactures no index key the input did not carry. * docs(#3648): correct the Git Query and Config Loader module contracts CONTEXT.md's Git Query Module still described base-branch tier 1 as a direct `.planning/config.json` read. Since this PR it is the EFFECTIVE configuration resolved by the Config Loader — a materially different authority, carrying the root/workstream deep merge, flat-then-nested lookup and builtin/federated defaults. The `--is-protected` predicate, `git.protected_branches`, and the two invariants that distinguish the predicate from the plain query (fails closed on an unverified base; must not write) were undocumented entirely. The Config Loader entry now states that loading is not side-effect-free by default and documents `options.persist`. docs/INVENTORY.md's `git-base-branch.cjs` row carried the same stale ladder and no mention of the predicate. `node scripts/gen-inventory-manifest.cjs --write` was run and produced no diff: the manifest indexes roster NAMES, not row prose, so a description edit cannot move it. Also closes the global-defaults minor: `git.protected_branches` is inert in `~/.gsd/defaults.json`, but so is every other `git.*` key — no branch-policy key appears in `_globalBaseCfg` or `GLOBAL_DEFAULTS_RESOLUTION_KEYS`. That is section-wide and predates this PR, so the fix is to state the scope where users meet it rather than to quietly extend the resolution set for two new keys. * fix(#3648): close four defects found by the round-4 external review Two external reviewers (codex, antigravity/Gemini 3.1 Pro) were run adversarially against this branch. Four findings reproduced against source; each is fixed with a failing-first test and a control, and each fix was verified by reverting it and watching exactly the intended test fail. 1. `persist:false` was DROPPED by the workstream fallback (codex). Blocker 1 was only half closed. `loadConfigResolved` re-enters itself with a bare `{ workstream: null }` when a workstream has no config.json of its own, and that literal discarded every other option — so the recursive pass ran at the DEFAULT persistence and rewrote the ROOT config. Reproduced: with GSD_WORKSTREAM=alpha and a legacy flat `base_branch`, `--is-protected` rewrote `.planning/config.json` despite `persist:false`. Both recursions now forward `options` and override only `workstream`; the explicit override still wins the hasOwnProperty check, so spreading cannot let `workstreamContext` reintroduce a workstream. 2. Both workflow call sites failed OPEN, and aborted under `set -e` (both reviewers, independently). `IS_PROTECTED=$(gsd_run ...)` yields an empty string when the query fails, so `[ "$X" = true ]` was simply false: no warning, no trace — a silent hole in the guard whose only job is to warn. The bare assignment also aborted the step under `set -e`. Both sites now degrade VISIBLY: `|| IS_PROTECTED=""`, then an explicit empty-string arm that says the check did not run. Deliberately not fail-closed — claiming "protected" on no evidence would warn on every branch whenever gsd-tools is unavailable. 3. `isValidProtectedBranches` and the resolver disagreed on a sparse array (antigravity). `.every()` skips holes; the resolver's `for...of` yields `undefined` for them, so `["main", , "develop"]` was accepted by config-set and rejected by the resolver. The cross-surface property passed only because `fc.array` cannot generate a hole. The predicate now indexes, and the generator punches holes so that axis is actually falsifiable. JSON cannot express a hole, so this is unreachable in production — but two definitions of one predicate must not contradict each other. 4. A top-level `protected_branches` silently outranked `git.protected_branches` (antigravity). Routing the key through `get(key, {section, field})` gave it flat-then-nested precedence, which is back-compat for keys `normalizeLegacyKeys` migrates. `protected_branches` is new in #3552 and has no legacy form, so that invented an undocumented alias. It now resolves nested-only through a new `getNested`, in production and in the test seam. `base_branch` keeps flat-then-nested — it HAS a legacy spelling that #3760's refusal path can leave behind — and a control pins that distinction. Also narrows a CONTEXT.md claim this round introduced. The predicate fails closed only when a git query TIMED OUT or could not be spawned (#3057 B4's `verified`); a git command that runs and exits non-zero counts as a clean negative, so a cwd that is not a repository answers `false`, not `true`. Verified pre-existing on next @ |
||
|
0xdhx
|
472f585f7c |
fix(#3726)!: require --confirm before milestone complete mutates (#3774)
* fix(#3726): require --confirm before milestone complete mutates `milestone complete <version>` is a one-way door — ROADMAP.md and REQUIREMENTS.md archived, every phase directory in the milestone MOVED, STATE.md rewritten — and ran unconditionally on first invocation through every invocation path, including `query milestone.complete <version>`, whose `query` meta-prefix reads as a read-only namespace but performs no filtering (#167's invocation-compatibility shim + #3243's dotted-form normalization). The gate lives on the destructive command itself, not on the `query` prefix (the prefix is an intentional invocation mechanism, not a permission boundary — restricting it would break dozens of shipped workflow callers). Without --confirm and without --dry-run the command now refuses via error() before reading anything beyond its arg checks, so an unconfirmed invocation is a guaranteed no-op on disk. --dry-run still previews with no confirmation needed and is now documented in the usage block (it was only documented for the sibling archive-quick). --force keeps its narrow meaning — bypassing the TRUNCATED-scope and unstarted-phase guards — and does not double as the mutation opt-in. --confirm follows the existing `phases clear --confirm` idiom in the same module. complete-milestone.md's two invocations pass --confirm (the workflow has gathered explicit user intent by that step). Existing tests get --confirm appended — pre-change behavior is exactly confirmed behavior — and a #3726 regression block covers: refusal + full-tree byte-identity on both invocation forms, --force not satisfying the gate, --dry-run still passing without confirmation, and --confirm proceeding. The refusal tests fail against pre-fix code (negative control run). Fixes #3726 * docs(#3726): document the --confirm requirement in CLI-TOOLS and COMMANDS Cross-AI review of the fix diff (codex, pre-create) caught three shipped doc sites still instructing the now-refused bare invocation: the CLI-TOOLS.md milestone-complete synopsis + flag table, and COMMANDS.md's two guard-override instructions (`--force` alone now refuses without --confirm). Localized CLI-TOOLS copies already lag the English synopsis (no --force/--dry-run either) and follow the translation pipeline, not this fix. * chore(#3726): set changeset fragment pr to 3774 * test(#3726): confirm-gate CI repairs — QA scenario caller + growth ack Two CI reds from the --confirm gate, both this branch's own misses: - tests/qa/scenarios/milestone-rollover.json invoked `milestone complete 1.0 --force` as a JSON arg-array fixture — a caller shape the test sweep (which grepped runGsdTools/runSdkQuery in tests/*.cjs) never enumerated. Adds --confirm; the scenario's boundary-crossing contract is otherwise untouched. - complete-milestone.md's +420-byte --confirm note trips the emitted-attribution growth ratchet. Acknowledged as a #3726 append to the existing complete-milestone.md entry in 3409-unreachable-guard-arms.json (two ack sources may never name the same path, per that fragment's own precedent). Local: lint-emitted-drift-ack ok; loop-walk.qa 115/115 green sandboxed. * docs(#3726): CLI-TOOLS.md guard-override sentences say --force --confirm Review Major 1: the truncated-window and unstarted-phase guard paragraphs still told the reader to "Pass `--force` to override", which now refuses (--force alone does not satisfy the confirmation gate), while the flag table 470 lines later said the opposite. Mirror the docs/COMMANDS.md pair so the file no longer contradicts itself. * docs(#3726): synopsis renders --confirm and --dry-run as alternatives Review Nit 1: `milestone complete <version> --confirm [--dry-run]` read as "a dry run still needs --confirm", the opposite of AC 3. Render the pair as `(--confirm | --dry-run)` in the CLI-TOOLS.md synopsis and the usage docblock, and let the flag rows carry the rule. * test(#3726): pass --confirm in base-added milestone fixtures; re-file the growth ack Rebase onto next (26 commits) surfaced three tests the gate now refuses: the #3685 write-flag contract pair in tests/milestone.test.cjs and the `milestone complete` boundary fixture in tests/state-contract.test.cjs all invoke the command bare. Each now passes --confirm (a mutating run is exactly what they assert on). The +420 byte complete-milestone.md growth ack rode on 3409-unreachable-guard-arms.json, which #3078 swept from next as fully spent — hence the modify/delete conflict. Re-filed under a fresh fragment named for this issue, never resurrecting the swept one. * test(#3726): pin the present-but-falsy arm of the confirmation gate Review Minor 1: the boundary triple covered absent and present but not present-but-falsy. The gate is an exact-token match, so --confirm=false and --confirm=0 refuse today — pinned (canonical + query forms, whole .planning/ tree byte-identical) so a future `=`-aware or prefix-matching parser cannot silently turn --confirm=false into a confirmed run of an irreversible command. * test(#3726): drop --confirm from dry-run-only invocations Review Nit 2: --confirm was mass-appended to 14 pre-existing --dry-run invocations that never needed it, so each stopped standing as incidental proof that a preview needs no confirmation. Reverted to the pre-PR form; the dedicated AC-3 test carries the explicit assertion. * docs(#3726): sync the localized CLI-TOOLS synopsis with the confirm gate REQ-I18N-02 (docs/features/internationalized-documentation.md) requires translations to stay synchronized with the English source. The four localized CLI-TOOLS.md guides still advertised a bare `milestone complete <version>`, which now exits 1. Render the English synopsis verbatim — `(--confirm | --dry-run)` plus the `[--force]` and `[--archive-quick]` flags the translations had also fallen behind on. * test(#3726): drop --confirm from the remaining preview-only invocations Round 2 reverted the --confirm appends on --dry-run-only invocations in tests/milestone.test.cjs, but four more sat in two files the sweep missed: tests/milestone-archive.test.cjs (three) and tests/milestone-window-single-owner.test.cjs (one). Each is a preview run whose whole purpose is to document that a preview mutates nothing, so `--dry-run ... --confirm` contradicted the semantics the test exists to pin. Dropping the token restores each as incidental proof that a preview needs no confirmation; the dedicated AC-3 test keeps the explicit assertion. No assertion added, relaxed, or removed — the change is four tokens. * chore(#3726): migrate the emitted-drift ack from a fragment to a commit trailer #3954 (ADR-3942) moved emitted-drift acknowledgments out of tests/emitted-drift-acks/ and into git commit trailers, and the fragment directory no longer exists on next. The reason this PR's fragment carried moves verbatim into the Emitted-Drift-Ack-Growth trailer on this commit; the fragment file is removed rather than resurrected. Emitted-Drift-Ack-Growth: complete-milestone.md — #3726: +420 bytes (40186 -> 40606). The archive_milestone step's two `milestone complete` invocations now pass the required --confirm flag (the command refuses to mutate without it — the archive is irreversible), with a note explaining the flag and pointing at --dry-run for previews. Deliberate runtime-loaded workflow text for the new gate, not converter drift. * fix(#3726): name --confirm in the version-required refusal The documented arg-discovery path (gsd-tools.cjs top-level usage: invoke the command without args and the error lists what is required) stopped at `version required for milestone complete (e.g., v1.0)` — one required argument short. Discovering --confirm took a second round trip through the gate. The refusal now reads `… — and --confirm to mutate`, pinned by a test that also asserts the version-less invocation leaves .planning/ untouched. * test(#3726): pin the milestone complete docs against a silent regression The changeset is `type: Fixed`, which the docs-required lint exempts, so nothing in CI would notice a later edit that reinstated the bare-`--force` override prose or dropped `--confirm` from the synopsis. Four tests in tests/milestone.test.cjs now pin: the synopsis line in docs/CLI-TOOLS.md and its four localized mirrors; the `--confirm` flag row; both guard-override instructions in docs/CLI-TOOLS.md and docs/COMMANDS.md, by guard name (a substring match on each instruction's `--force --confirm` text); and — as an identity ratchet over the milestone-complete sections — every `--force` sentence or clause that lacks `--confirm`, so a new bare instruction in its own sentence or clause fails whatever its wording. Named residual: a bare instruction spliced into the same clause as a compliant one coalesces with it and passes the ratchet; the by-name pins are what keep the four known instructions from losing the pairing that way. The file is registered in scripts/docs-guard-registry.cjs so the pin runs on the PR that changes those docs, not only after merge. --------- Co-authored-by: CI Rebase Check <ci@gsd-redux> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
370cfc6680 |
enhance(#4036): persist CI shard/job timeout-vs-cap trending, warn at 90% (#4043)
* feat(#4036): persist CI shard/job timeout-vs-cap trending, warn at 90% Adds two new mechanisms plus an audit-coverage extension: - scripts/lib/ci-job-timing.cjs: shared elapsed-vs-cap arithmetic - scripts/ci-check-job-near-cap.cjs: in-job advisory near-cap check, wired into test/test-full/mutate/smoke as each job's last step - scripts/ci-timeout-report.cjs + .github/workflows/ci-timeout-report.yml: scheduled REST-API poll that appends new records to tests/ci-timeout-budget-history.jsonl and opens a small data-only PR - tests/ci-test-job-timeout-budget.test.cjs: extended to cover mutate (mutation.yml) and smoke (install-smoke.yml), which previously had no headroom-factor gate coverage at all Does not change any timeout-minutes value, shard composition, or shard-1 contents — those stay maintainer policy calls per the issue's own scope. * fix(#4036): address two-orthogonal-review findings - Parity tests guarding the two hand-duplicated literals this design cannot single-source through GH Actions YAML: CI_JOB_TIMEOUT_MINUTES vs each job's own timeout-minutes, and ci-timeout-report.cjs's JOB_RULES name-prefixes vs each job's actual name: template. - Thread run.event through as runEvent on every persisted record, so PR-context and push-context install-smoke timings (genuinely different matrix shape) are distinguishable in the history rather than silently conflated under one job name. - Replace the Windows near-cap start-time step's ambiguous PowerShell +/>> precedence with GitHub's documented string-interpolation form. - Move github.run_id out of direct ${{ }} shell interpolation into an env: var in the new scheduled workflow, per this repo's own expression-injection-safe convention. * test(#4036): regenerate golden install-tree fixtures for scripts/lib/ci-job-timing.cjs npm run gen:install-tree — scripts/ ships wholesale into the installed package (per ADR/known-defect precedent from #4012's own PR history: a new scripts/lib/*.cjs file needs its golden entry regenerated or every runtime's install-tree test fails). Confirmed via gsd-test: this was the sole cause of the first real verification run's 25 failures (all in tests/golden-install-tree.test.cjs, one per runtime). Top-level scripts/*.cjs files (ci-check-job-near-cap.cjs, ci-timeout-report.cjs) are not individually tracked in these fixtures — consistent with every other existing top-level scripts/*.cjs file, so no entry was expected or added for those two. * fix(#4036): register new lib file with installer, fix H1 shell policy - bin/install.js: add ci-job-timing.cjs to GSD_SCRIPTS_LIB_FILES (a hand-maintained registry, not generated — tests/install.test.cjs asserts every scripts/lib/ file is enumerated here) - test.yml: replace the two OS-conditional "Record job start time" step pairs (test + test-full jobs) with a single unconditional `node -e` step. The prior pair's Windows variant declared an explicit shell: pwsh, which scripts/workflow-policy.cjs's H1 checker statically flags against every OS a job's matrix can realize, independent of the step's own if: gate. A single Node one-liner needs no shell override at all — it's syntactically valid and behaves identically under bash, zsh, and pwsh — which is both H1 compliant and removes the last OS-specific shell syntax from this change entirely. Both defects were found by a real gsd-test run, not local gates — lint:ci and build:lib were clean throughout because neither the scripts/lib/ install-manifest parity check nor the H1 shell-policy baseline runs as part of lint:ci; both are gsd-test-only suites. * docs(#4036): how-to for reading CI timeout budget signals The phase-gate docs check correctly flagged the enablement sequence as 3 real steps (read the near-cap warning, find the accumulated trend file, pick the right maintainer lever) — a reference table can't carry a sequence. Adds docs/how-to/read-ci-timeout-signals.md, indexed from docs/README.md. * chore(#4036): backfill changeset PR number (4043) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
b431ae9f0d |
fix(#3898): a separator-shaped line in ## Gaps is skipped, not made an entry (#4057)
* test(#3898): a spaced-hyphen thematic break in ## Gaps is not an entry (failing first) * fix(#3898): a separator-shaped line in ## Gaps is skipped, not made an entry splitGapsEntriesCore's opener regex (/^(\s*)-\s/) matched a spaced hyphen thematic break, fabricating a gap named '- -' with result 'unknown' — surfaced by audit-uat as an outstanding finding that cannot be cleared by editing any entry, because there is no entry, only the separator the author wrote for readability. Five shapes were affected (- - -, -, and wider/indented variants); the unaffected ones (---, ----, * * *, ___) were safe only by accident — the path never matched them, not because it understood breaks. A line whose content after the opening marker is solely hyphens and whitespace (with at least one further hyphen) is now skipped entirely — neither an opener nor a continuation. Deliberately option 2 from the issue, not a full thematic-break concept: a break does not close the Gaps list, entries after it keep parsing, and the deliberately-frozen byte-for-byte Gaps behavior changes ONLY for documents carrying such a separator (which previously produced a phantom). A real entry whose truth begins with a hyphen (- truth: "-5 error budget...") is untouched — its remainder contains non-hyphen characters. * fix(#3898): review fold-ins — span-contiguous skip, property coverage The skip is narrowed to where the phantom came from: a separator-shaped line BETWEEN entries (nothing open, or it would open a top-level entry). One landing strictly inside a live entry (indent > baseIndent) folds back as a continuation line, so entry lines and the GapsEntrySpan agree byte-for-byte — the span invariant and the #3805 ack writer's identity re-verification both hold (the review traced the unconditional skip to a match_verification_failed refusal in that corner). Adds the parser- convention property test (arbitrary hyphen counts/indents/spacings) and a span-contiguity pin. * chore(#3898): changeset fragment (pr number backfilled after PR creation) * chore(#3898): backfill changeset PR number (4057) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
44ddc6dc46 |
fix(#3865): init.* phase queries accept --phase <N> as the positional alias (#4054)
* test(#3865): init.* phase queries accept --phase as positional alias (failing first) * fix(#3865): init.* phase queries accept --phase <N> as the positional alias The phase-taking init.* queries read their phase token at args[2] blindly: '--phase 60' made the literal '--phase' the phase, which the locator can never resolve — the reported incident answered a well-formed phase_found:false, plan_count:0 with exit 0 for a phase holding seven committed plans (ADR-3473 §8.4's later strict validation turned the paired form into a usage error instead, still not the alias). normalizePhaseAlias (shared by all eight phase-taking handlers — the issue's four plus phase-op, review, discuss-phase-assumptions, todos, the same class) rewrites '--phase N'/'--phase=N' into the caller-owned positional slot before flag parsing, so handlers and strict validation see exactly the argv the positional form produces. A valueless --phase is a usage error naming the flag. Any other flag-shaped args[2] now resolves to undefined (the commands' designed use-the-current-phase input) instead of passing flag text down as a phase name. isFlagToken exported from command-arg-projection (single owner of the flag-shape predicate). * fix(#3865): review fold-ins — honest no-position-given comment, fail-closed throws The helper's comment claimed flag-shaped args[2] resolves to 'the commands' designed use-the-current-phase input' — no such cross-module behavior exists: execute-phase/plan-phase/verify-work usage-error 'phase required', the find-based queries answer phase_found:false, and todos drops its area filter. Reworded to what actually happens, so the contract-grade comment cannot mislead a future edit. Adds the parseNamedArgsOrExit-style throw after each error() call as a fail-closed backstop against a returning fail(). * chore(#3865): changeset fragment (pr number backfilled after PR creation) * chore(#3865): backfill changeset PR number (4054) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
d9e906a744 |
fix(#3864): smart-entry classify() matches the verif* status stem (#4052)
* test(#3864): smart-entry classify must match the verif* status stem (failing first) * fix(#3864): classify() matches the verif* status stem, aligning with normalizeStateStatus "verified"/"verification" contain no "verify" substring, so the exact-word branch never matched: a STATE.md declaring status: verified fell through to situation "unknown" (or idle-stranded on a clean tree with unpushed commits — differently wrong, which made it look intermittent). state-document's normalizeStateStatus already matches any verif* stem; the classifier now uses the same stem, so the two owners agree on the invariant. verify_failed is tested earlier in classify(), so failed verification still wins. Negative controls (completed/executing/planning/paused) unchanged and pinned. * test(#3864): review fold-in — the real handler-written verification status pinned both ways 'Phase complete — ready for verification' (state-document.cts's own Status default) carries the verif stem but names completion: at 5/5 it must stay complete (isComplete beats the stem), mid-project it must route to verify-pending (pre-fix it fell through to unknown/idle- stranded — a real behavior change beyond the literal verified repro, sanctioned by the issue's own verification→verify-pending table). * chore(#3864): changeset fragment (pr number backfilled after PR creation) * chore(#3864): backfill changeset PR number (4052) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
4048ba2e80 |
fix(#3860): Quick Tasks lookup accepts milestone-suffixed headings; schema-aware section selection (#4050)
* test(#3860): quick-tasks heading tolerance for milestone suffixes (failing first) * fix(#3860): prefix-match Quick Tasks heading + schema-aware section selection The exact-anchored predicate (^quick tasks completed$) never matched a milestone-suffixed heading ('Quick Tasks Completed (v1.1+)'), so every /gsd-fast append AND every reset failed with QUICK_TASKS_SECTION_ABSENT before the columns were ever looked at — the reported table was already canonical. A heading is a section label, not data: the predicate is now prefix-anchored with a word boundary (Completedness still excluded), hoisted to one shared isQuickTasksHeading beside QUICK_TASKS_SECTION_ABSENT so the two call sites cannot drift. Section selection is now schema-aware (the issue's deliberate-choice ask): among ALL matching sections, the first whose table parses with a recognized Quick Tasks schema wins — a legacy (v1.0) table first in document order no longer shadows a usable (v1.1+) one below it. When no section is usable, the FIRST is returned so the error names the real problem (unrecognized schema) instead of a false 'no section'. Fixes one over-assertion in the new tests (v1.1+'s own next ordinal IS 2; pins v1.0 rows byte-identical instead). * fix(#3860): review fold-ins — level-bounded bodies, splice guard, convention pin Adversarial review caught that collectSections ends a candidate's body only at the next MATCHING heading: an intervening '## Deferred Items' table (canonical STATE.md layout — templates/state.md puts it after the Quick Tasks section) would be swallowed into the body, and the append's last-table-line scan would splice the quick-task row into that WRONG table — silent corruption on the primary /gsd-fast path. Each candidate is now re-collected through collectSection with an offset-precise predicate, restoring the pre-#3860 level-bounded stop (next same-or- higher heading) for both probing and splicing. Also pins the both-schema-valid tie-break (document order, newest-on-top — the layout the issue itself demonstrates; this layer has no active-milestone signal) with a test, guards the Deferred Items layout with a dedicated splice-target test, and updates resetQuickTaskRows's doc comment to name the new pipeline. * chore(#3860): changeset fragment (pr number backfilled after PR creation) * chore(#3860): backfill changeset PR number (4050) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
fd63889d1f |
fix(#3854): write normalization preserves tight multi-line lists (#4049)
* test(#3854): write normalization must preserve tight multi-line lists (failing first) * fix(#3854): no blank before a bullet whose previous line is an indented continuation _normalizeMd's 'separate a list from a preceding paragraph' rule inserted a blank before any bullet whose previous line wasn't a bullet — but an INDENTED CONTINUATION of the previous multi-line item also isn't a bullet. Every .md write (phase.complete in the report, but any write through platformWriteSync) therefore converted tight lists to loose ones: +61 blank lines on the reporter's 1015-line ROADMAP, one before each bullet following a wrapped item. Tight and loose lists render differently, so this was a rendering change plus misleading diff noise; one-shot (idempotent afterwards), which is why integrity checks on headings/content passed. The guard is the mirror image of the after-a-bullet rule two lines below, which already excludes indented next lines. Paragraph→list and heading→list separations — the rule's purpose — are pinned unchanged by the new suite. * fix(#3854): review fold-ins — ceiling tracks next's 281 + this branch's marker (282), header/require nits The ceiling is not ratcheted but must track the tree: origin/next raised it to 281 (sibling branch's marker file); this tree adds one more (shell-command-projection-md-normalize), so 282/282. Also fixes the test header's stale pre-rename filename and hoists the inline require to the file's single import. * chore(#3854): changeset fragment (pr number backfilled after PR creation) * chore(#3854): backfill changeset PR number (4049) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
529480b4a5 |
fix(#3895): delete the mempalace-curator's model frontmatter pin — the fleet's only hardcoded model (#4048)
* test(#3895): no shipped agent may hardcode a model frontmatter pin (failing first) * fix(#3895): delete the mempalace-curator's model frontmatter pin — the fleet's only hardcoded model Exactly one of the 34 shipped agents carried 'model: sonnet' in its frontmatter; every other agent resolves through the model-profile system. The ship:post dispatch (#2684) resolves per-hook and — per #2517 — deliberately OMITS model= on inherit so the agent inherits the orchestrator's model; the frontmatter pin intercepted that inherit case, silently forcing sonnet where all 33 siblings would inherit, and operators could not durably remove it (install rewrites live copies wholesale). Deleting the line changes nothing for default profiles — the catalog entry (model-catalog.json agents.gsd-mempalace-curator: golden/balanced sonnet, budget haiku) preserves today's behavior — while restoring model_overrides and inherit authority. Pinned by a new agent-frontmatter guard: no shipped agent may hardcode a model pin, and the catalog entry must keep existing so the pin's deletion can never orphan the agent. * chore(#3895): changeset fragment (pr number backfilled after PR creation) * chore(#3895): backfill changeset PR number (4048) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
400db94e02 |
fix(#3894): quick path honors workflow.research_before_questions; key resolves from global defaults (#4047)
* test(#3894): research_before_questions must resolve globally and order quick.md (failing first) * fix(#3894): quick path honors workflow.research_before_questions; key resolves from global defaults Two layers, one key. The quick workflow ran its discussion phase before its research phase unconditionally — neither quick.md nor its steps ever read workflow.research_before_questions, though the key is documented, schema-registered, /gsd-settings-writable, and honored by /gsd-discuss-phase and /gsd-new-project. A gray-area answer given without research is then written to <quick_id>-CONTEXT.md as a locked decision downstream agents are told not to revisit — an evidence-free choice made unfalsifiable (the reporter's #3714 misresolution). - quick.md Step 4 now carries the same research-before-questions check the two honoring paths make: when enabled, research-phase executes before discussion-phase; false/unset keeps the written order. Both sections stay section-manifest gated. - src/config-loader.cts forwarded workflow.post_planning_gaps from ~/.gsd/defaults.json but silently dropped this key — same file, same nesting, one resolved and one didn't. Now forwarded with the same flat + nested-alias fallback shape, added to the resolution-keys lockstep canary and the #3532 shadowed-warning set (nested alias reporting generalized over both keys). Emitted-Drift-Ack-Growth: quick.md — #3894: +Step 4 ordering rule (the research-before-questions check the discuss-phase and new-project paths already make); a real behavioral gate, not incidental bloat. * fix(#3894): review fold-ins — gate the CONTEXT.md reference, colon slash-forms - quick/steps/research-phase.md directed the researcher subagent to read <quick_id>-CONTEXT.md under DISCUSS_MODE with no existence hedge — but under the new ordering (research BEFORE discussion) the file cannot exist yet when the researcher is dispatched. The reference now says read-only-if-present with the #3894 reason; the alignment purpose still applies on the default ordering. - quick.md's new rule used the hyphen slash forms (/gsd-discuss-phase, /gsd-new-project); source artifacts under gsd-core/workflows must author the colon form the install-time converters key on — the same file already uses /gsd:new-project and /gsd:quick elsewhere. * docs(#3894): planning-config row names the flat CONFIG_DEFAULTS alias config-field-docs requires every CONFIG_DEFAULTS key to appear in the doc; the row documented the canonical namespaced form only. Adds the same alias sentence post_planning_gaps's row carries, plus the #3894 quick-path note. * chore(#3894): changeset fragment (pr number backfilled after PR creation) * chore(#3894): backfill changeset PR number (4047) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
192eb1dfbd |
fix(#3886): git commit timeout reported as commit_timeout; stale lock surfaced; 30s band (#4046)
* test(#3886): a timed-out git commit reports commit_timeout, not commit_failed (failing first) * fix(#3886): git commit timeout reported as commit_timeout; 30s band; stale-lock surfaced cmdCommit's git commit invocation did not distinguish a spawnSync timeout from a real non-zero exit (#2608 fixed this for the staging loop only): a slow pre-commit hook crossing the 10s cap was SIGTERM'd mid-hook and reported as reason commit_failed with whatever partial stderr git had flushed (in the reporter's case an incidental CRLF warning), while the kill left a stale .git/index.lock blocking the next attempt. All three commit sites now check isSpawnTimeout before the nothing-to-commit/ordinary-failure branches: cmdCommit reports reason commit_timeout + timed_out:true and names the stale lock's path (surfaced, not auto-deleted — deleting a lock a live git holds is destructive; the caller recovers deliberately); the subrepo counterparts do the same within their per-repo result / rollback error. The commit calls also move to the 30s band the push call already uses — husky+lint-staged alone idles ~4s on Windows before any task runs. * fix(#3886): review fold-ins — git-path lock resolution, shared band constant, executor contract row, precedence pin - The stale-lock path is resolved via git rev-parse --git-path index.lock, never a literal .git/index.lock join (#3588 row 8's class: a linked worktree's .git is a FILE, so the literal path cannot exist there while the real lock — under <gitdir>/worktrees/<name>/ — blocks the next commit; this repo leans on linked worktrees). - COMMIT_TIMEOUT_MS hoisted; all three sites and their messages build from it (the subrepo variant also regains the stdout fallback the primary site had). - agents/gsd-executor.md's commit-result contract gains the commit_timeout row with the OPPOSITE retry advice from staging_timeout (remove the stale lock, then retry once) — an executor matching the doc previously had no handling for the new reason. - Precedence pin: a timeout whose partial output contains 'nothing to commit' must still read as a timeout (branch-reorder mutant). Emitted-Drift-Ack-Growth: gsd-executor.md — #3886: +commit_timeout row to the commit-result contract with the retry guidance OPPOSITE staging_timeout's (remove the stale lock, then retry once); the executor previously had no handling for the new reason. * chore(#3886): changeset fragment (pr number backfilled after PR creation) * chore(#3886): backfill changeset PR number (4046) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
0bf778c352 |
fix(#3849): phase allocation counts numbers held by sibling git worktrees (#4042)
* test(#3849): phase allocation must skip numbers held by sibling worktrees (failing first) * fix(#3849): phase allocation counts numbers held by sibling git worktrees Both allocators (cmdPhaseAdd, cmdPhaseAddBatch) chose max+1 over numbers gathered from ONE checkout — headers, bullets (add only), on-disk dirs. Every sibling git worktree carries its own .planning/ on its own branch, so a phase minted there was invisible and the same number was allocated twice (the reported incident: two Phase 441s, one with six written plans, surfaced a day late by human memory). New shared horizon collectSiblingWorktreePhaseNums: one git worktree list --porcelain, then per sibling — phase-dir names (the cheap scan that would have caught the incident) and the WHOLE sibling ROADMAP.md headers (a row can predate its dir; milestone-scoping would be wrong — a number used under any milestone on another branch is taken). Widen, never refuse: unreadable sibling / no .planning / not a git repo / git unavailable each contribute nothing and allocation is unchanged. Reuses isSentinelPhaseId and the allocators' own patterns. Secondary (#1229 never reached batch): cmdPhaseAddBatch now also scans roadmap bullets — a bullet-only 'Phase N' row was invisible to batch allocation, exactly the condition #1229 was filed for. Also fixes the two new tests' result-key access (output.phases, not output.results). * fix(#3849): review fold-ins — subprocess band, bounded test git, linked-worktree fixture - execFileSync options now match the repo's git band (10s window, windowsHide, 4MiB maxBuffer) — a spurious 4s timeout silently reverted to the pre-fix collision. - test git helper bounded (15s) per local/no-unbounded-spawn. - new fixture: allocation FROM a linked worktree counts the main checkout — the incident's actual topology direction. - fixture-setup rmSync carries the sanctioned lint-disable (setup, not teardown; cleanup() still owns directory removal). * fix(#3849): exempt the sibling-worktree scan from the enumeration-drift guard collectSiblingWorktreePhaseNums reads a SIBLING checkout's phases dir — a different question from the cwd-scoped listMilestonePhaseDirs the guard routes everything to (which cannot see another worktree's .planning at all). Function-scoped, per ADR-3180 Decision 4(a): any other re-derivation in phase.cts is still caught. The GREEN bench caught the omission. * chore(#3849): changeset fragment (pr number backfilled after PR creation) * chore(#3849): backfill changeset PR number (4042) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
519ac23ebb |
fix(#3839): hook tables say PreToolUse (validate-commit) and SessionStart (session-state) (#4041)
* test(#3839): docs hook tables must match surface registrations (failing first) * docs(#3839): hook tables say PreToolUse for validate-commit, SessionStart for session-state gsd-validate-commit.sh is registered PreToolUse (src/runtime-hooks-surface.cts; its exit-2 block IS the contract — a post-tool hook cannot prevent a commit) and gsd-session-state.sh is registered SessionStart (session orientation, not post-tool tracking). Both rows said PostToolUse in ARCHITECTURE.md and the three INVENTORY locales; the issue asked for a neighbouring-row scan, which is how the session-state row was found. All other rows in the four tables verify against the surface. * fix(#3839): review fold-ins — 10 more wrong rows in ko-KR/pt-BR/zh-CN, parser authority + drift pins Adversarial review found the same two wrong rows shipped in five more files the issue's table missed (ko-KR ARCHITECTURE+INVENTORY, pt-BR ARCHITECTURE+INVENTORY, zh-CN ARCHITECTURE) — all fixed; DOC_TABLES now covers all ten shipped tables. The parity parser unioned only the Kimi mirror list, silently exempting agent-isolation-guard (registered via the dynamic preToolEvent push): probes are now parsed too, with bare hook names resolved against hooks/ ground truth and dynamic event variables resolved to their canonical (non-Gemini) events; an exact-set pin replaces the loose size guard. allow-test-rule marker carries the issue ref; unverified-ceiling 280→281 (audited: the new marker is legitimate — the suite reads product docs whose text is the contract). * fix(#3839): register the hook-table parity suite in the docs-guard lane The new suite reads ten docs/ paths, so lint-docs-guard-registration requires it in the docs-guard registry — the first GREEN bench run caught the omission (the RED run's docs-guard failures were the same signal, previously misread as marker fallout). * chore(#3839): changeset fragment (pr number backfilled after PR creation) * chore(#3839): backfill changeset PR number (4041) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
3c08315a5e |
fix(#3827): new-mode routing gate — classify approval no longer authorizes scaffold writes (#4037)
* test(#3827): new-mode routing approval gate before roadmapper (failing first) * fix(#3827): new-mode routing gate — classify approval no longer authorizes scaffold writes The route_new_mode step delegated to gsd-roadmapper (PROJECT.md, REQUIREMENTS.md, ROADMAP.md, STATE.md creation + commit) with no gate: the discovery gate approved classification only, and the zero-conflict branch said 'proceed to routing silently'. Merge mode previews its diff and gates via approve-revise-abort; new mode now shows the exact destinations and requires Create planning setup | Keep synthesized intel only | Abort, per the skill contract's routing-gate requirement. The keep-intel-only choice is the analysis-only path the issue asks for: no roadmapper, no destination writes, intel preserved; finalize labels it 'new (intel only)' and points at /gsd:new-project instead of plan-phase. Ambiguous gate answers re-ask once, then treat as Abort — never infer Create. Zero-conflict wording is mode-aware: silence about conflicts is not authorization to write. Emitted-Drift-Ack-Growth: ingest-docs.md — #3827: +routing gate display block, AskUserQuestion contract, three disposition branches (incl. the intel-only no-write path), ambiguous-answer rule, intel-only finalize line, mode-aware zero-conflict pointer; a real behavioral gate, not incidental bloat. * chore(#3827): changeset fragment (pr number backfilled after PR creation) * chore(#3827): backfill changeset PR number (4037) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
331747ea99 |
fix(#3817): count the truncation remainder — display truncates, counting must not (#4034)
* test(#3817): audit-open counts must include the truncation remainder * fix(#3817): count the truncation remainder — display truncates, counting must not * chore(#3817): changeset fragment (pr number backfilled after PR creation) * chore(#3817): backfill changeset PR number (4034) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ac0eed1267 | Merge pull request #4015 from open-gsd/fix/3889-instrument-chunk-timeout | ||
|
Tom Boucher
|
213a2fff63 |
chore(#3813): delete the caller-less listMilestoneArchiveDirs seam; #1883 contract now pins the live path (#4029)
* test(#3813): pin the #1883 unreadable-milestones contract on the live planning-snapshot path * fix(#3813): delete the caller-less listMilestoneArchiveDirs seam; #1883 contract now pins the live path * chore(#3813): changeset fragment (pr number backfilled after PR creation) * chore(#3813): backfill changeset PR number (4029) * chore(#3813): docs-exempt marker — internal dead-code removal --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
b811ea16fc |
fix(#3807): advance-plan refuses an ambiguous multi-Phase Current Position (#4028)
* test(#3807): advance-plan must refuse an ambiguous multi-entry Current Position * fix(#3807): refuse an ambiguous multi-Phase Current Position before advancing * chore(#3807): changeset fragment (pr number backfilled after PR creation) * chore(#3807): backfill changeset PR number (4028) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
3a4c3cb83e |
fix(#3805): audit-uat honours the audit_acknowledged marker via the shared predicate (#4025)
* test(#3805): audit-uat must honour the audit_acknowledged marker * fix(#3805): route audit-uat's UAT and VERIFICATION scans through the shared acknowledged predicate * chore(#3805): changeset fragment (pr number backfilled after PR creation) * chore(#3805): backfill changeset PR number (4025) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
f4fefb0bef |
fix(#3804): audit-uat enumerates all three phase-archive layouts (#4022)
* test(#3804): audit-uat must see all three phase-archive layouts * fix(#3804): enumerate all three phase-archive layouts (flat, workstream-archived, workstream-active) * chore(#3804): changeset fragment (pr number backfilled after PR creation) * chore(#3804): backfill changeset PR number (4022) --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
c1a2c33886 |
perf(#4012): two subprocess spawns, not five
My instrumentation suite tipped ubuntu shard 1/3 over its 15-minute job cap.
Measured, not guessed: baseline on next (
|
||
|
Tom Boucher
|
80de48c319 |
enhance(#3914): every phase records a truthful guard ledger (#4018)
* fix(#3914): retire n/no-process-exit where its successor governs
Epic #3889 criterion 5 — no phase closes with a guard added and its
predecessor left standing — is violated in the tree by the epic that wrote it.
local/require-registered-exit was registered on gsd-core/bin/**/*.cjs and
scripts/**/*.cjs, while n/no-process-exit stayed 'error' over a nine-glob block
covering those same two. Only the hooks 'off' exemption ever came down; the
predecessor's registration never did. Both rules have been enforcing the same
property on the same surfaces since P6.
Narrowed, not deleted. Seven of those nine globs have NO successor —
eslint-rules/, bin/lib/, pi/, examples/, vscode/, .kilo/, .opencode/ — so
deleting the rule outright would silently drop enforcement on all seven. That
is the inversion this epic has already hit three times: removing a coarse guard
because a narrower one exists somewhere it does not reach. Flat config is
last-match-wins and both successor blocks come after the nine-glob block, so
'n/no-process-exit': 'off' in exactly those two retires the predecessor
precisely where the successor governs and nowhere else.
The successor is strictly more precise: it permits process.exit only inside
terminateNow in cli-exit.cts, the single sanctioned terminator (ADR-3889 §3),
where n/no-process-exit permits none and would flag terminateNow's own
generated copy.
Asserted at the consumer's altitude via ESLint.calculateConfigForFile on real
paths, with the positive control that matters: n/no-process-exit is still
'error' on six of the seven successor-less globs, so a future edit that turns
this into a blanket disable goes red. bin/lib/ has no file in this checkout and
is reported as untested rather than given an invented path. Severity is
normalized across the string/numeric/array forms the API can return, and the
normalized value asserted — not truthiness.
Verified by running calculateConfigForFile myself on both superseded globs and
four controls before trusting the test.
Found and fixed inline: the change made an eslint-disable directive at
gsd-tools.cjs:257 partially unused, which --max-warnings 0 rejects; narrowed to
the one rule still in force.
Verification runs on the remote runner.
Refs #3914
* docs(#3914): the epic added three guards, it did not remove one
The audit reconciled the epic ledger against what actually landed. The net is
+3, not -1: four lint:generated-sync --check arms (gen-scripts-cli-exit,
gen-hooks-cli-exit, gen-exit-code-registry, gen-exit-code-docs) plus one rule,
against two retirements.
An epic whose thesis was consolidation ended with a larger guard surface than
it started with. The additions are each defensible; the claim that the total
fell was never true.
Two of the three prior errors in this amendment are mine. It said "Net -1 by
count" above terms reading -1 -1 +1 +1 +1, which sums to +1 — an arithmetic
error in the paragraph directly below the sentence arguing that an ADR about
honest accounting must not pad its own ledger. And the term list omitted two of
the four --check arms, which is what turns that +1 into the real +3.
Recorded rather than quietly rewritten. This ledger has now been wrong three
times — the original -2, the -1 that replaced it, and #3914's own table, which
states -1 above terms summing to 0 — and a written claim nobody checked against
the thing it describes is the exact failure this epic exists to close.
Refs #3914
* fix(#3914): make the successor actually supersede before retiring the predecessor
An isolated security review found that the previous commit turned off a guard
that was still doing work. Reproduced by executing both rules against a
fixture, not inferred:
const exit = 'exit';
process[exit](1);
n/no-process-exit flags it; local/require-registered-exit did not, because it
early-returned on callee.computed. So retiring the predecessor on
gsd-core/bin/**/*.cjs and scripts/**/*.cjs un-guarded that shape on precisely
the two globs this epic's exit contract cares most about.
This is the third time in this epic I have removed a coarse guard on the claim
that a narrower one covered it, without checking construct-level parity — after
the allowlist key-to-prefix-to-exact-membership sequence and the band
ranges-to-categories one. The rule is the same every time: a narrower guard
supersedes a coarser one only where it demonstrably reaches at least as far,
and "demonstrably" means executing both against the constructs, not reading
either.
The successor now resolves computed property access for the statically
determinable cases — a string Literal, and an Identifier bound once to a string
Literal, resolved through scope — and leaves genuinely dynamic properties
alone so the rule does not over-fire. Measured after the fix: plain
process.exit flagged, process['exit']() flagged, process[exit]() flagged,
process[globalThis.k]() not flagged. That makes it a strict superset of the
predecessor on these globs, since process['exit']() was caught by NEITHER rule
before.
The second finding is worse than the first, because it was reasoning rather
than oversight. My justification comment claimed n/no-process-exit "would flag
terminateNow's own generated copy here". It would not — that file is in the
global ignore list, so neither rule ever lints it. There was no conflict to
resolve; I wrote a rationale I had not checked, in a change whose entire
subject is written claims nobody verified. Both comment blocks now state the
real basis.
The tests that should have caught this asserted only rule SEVERITY per glob and
never construct REACH, which is exactly how a coverage hole passed. A parity
matrix now pins all five shapes, including a RED/GREEN regression pin against
an inlined reproduction of the pre-fix rule — inlined rather than loaded from
HEAD, because HEAD resolves to the fixed commit under the remote runner and
would silently stop testing anything.
Verification runs on the remote runner.
Refs #3914
* fix(#3914): the two exit rules are complementary — keep both
Reverts this branch's retirement of n/no-process-exit. The premise was wrong
twice, and the second review proved the change itself was wrong.
I claimed local/require-registered-exit was a strict superset on
gsd-core/bin/**/*.cjs and scripts/**/*.cjs. Measured, successor vs predecessor:
function f(exit) { process[exit](1); } 0 vs 1
let exit='exit'; exit='exit'; process[exit]() 0 vs 1
const { exit } = ...; process[exit](1) 0 vs 1
plus for-of bindings, let-then-assign, var redeclaration, catch params, and an
undeclared global named exit. The predecessor matches any identifier NAMED
exit however it is bound; the successor resolves only a string literal or a
single-write const. It never was a superset — I asserted the relationship after
fixing one construct and did not re-check the rest.
The justification was independently false: all three generated cli-exit copies
are in the global ignore list, so n/no-process-exit was never flagging
terminateNow. There was no conflict to resolve. I wrote a rationale I had not
verified, in the phase whose subject is written claims nobody checked.
So criterion 5 does not apply to this pair. They are not predecessor and
successor — they are complementary, each catching constructs the other misses.
The epic's criterion assumed a replacement relationship that does not exist
here, and retiring either rule loses real coverage. The ADR ledger now says so
with the measured shapes.
What survives is the genuine improvement: the computed-property strengthening.
local/require-registered-exit now catches process['exit'](1) and optional-chain
terminators like process?.[k]?.(1), which NEITHER rule caught before, while
correctly ignoring a genuinely dynamic property so it does not over-fire.
The parity tests are rewritten to assert what is true rather than what I wanted
to be true: a bidirectional matrix where each rule is shown catching shapes the
other misses. The previous matrix tested only the four shapes where the
successor wins, which is precisely why the regression shipped — a test set
selected to confirm the thesis.
Also corrected: a stale ADR sentence claiming a third wrong ledger version that
does not exist (the table it described now reads +3 over terms summing to +3),
and a changeset whose stated motivation was the false generated-copy conflict.
Verification runs on the remote runner.
Refs #3914
* fix(#3914): the exemption term was a no-op — the net is +4
Fourth correction to this ledger, and a fourth error of the same kind.
Every version counted removing the n/no-process-exit 'off' entry from the hooks
block as -1. Measured: calculateConfigForFile returns undefined for that rule on
hooks/**. It was never registered there, and no broader block sets it globally,
so the 'off' entry overrode nothing and removing it changed no enforcement at
all. A no-op removal, not a guard removal — the same category error as counting
baseline acknowledgement entries: a thing that is not a guard, in guard units.
It is misattributed too; that block came down in
|
||
|
Tom Boucher
|
51ca9f39ba |
fix(#3801): register inline_plan_threshold in the defaults manifest and correct the docs (#4019)
* fix(#3801): register inline_plan_threshold in the defaults manifest (default 2) and correct settings-advanced * chore(#3801): changeset fragment (pr number backfilled after PR creation) * chore(#3801): backfill changeset PR number (4019) * test(#3801): parse the defaults table with the shared markdown-table parser --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ac7587287b |
fix(#3812): document how Current Position actually resolves a duplicate field (#4017)
* docs(#3812): say that Current Position is single-valued, and pin the behavior that makes it true #3812 shipped CLOSED with half its acceptance unmet. #3873 delivered cardinality for FRONTMATTER keys - current_phase/current_plan render as optional at docs/reference/state-md.md:89,91, covered by tests/gen-state-md-docs.test.cjs:374. The issue's actual ask was the ## Current Position BODY section, and that never landed. Surfaced by an /adr-phase-coverage audit of epic #3473; the issue was reopened rather than noted. The section now states three things: every field is single-valued, the section is overwritten rather than appended to, and a duplicate resolves to the FIRST occurrence with no warning - so a line appended in good faith is silently ignored rather than winning. Progress history belongs in ## Performance Metrics, two headings down, and the text now points there. The third claim is a behavioral promise about the reader, so it was VERIFIED BY EXECUTION before being written rather than inferred from the issue title: stateExtractField(<"Phase: 1 of 5 (First)" ... "Phase: 9 of 9 (Appended later)">, "Phase") -> "1 of 5 (First)" The mechanism is state-document.cjs:405 - the plain-line pattern ^<field>:[ \t]*(.+) carries flags im with NO g, so String.match returns the first hit. Writing "first wins" without running it would have repeated the exact error I had to retract twice in this epic already. A test pins the reader, not the prose. Three rows in tests/state.test.cjs: T1 (load-bearing) asserts the duplicated case resolves first; T2 asserts the ordinary single-field case still works, so a fix that only functions when duplicated cannot pass; T3 puts a Plan: line BETWEEN the two Phase: lines and asserts it resolves independently - negative space, because a reader returning the first line of the SECTION rather than the first matching FIELD would satisfy T1 alone. Proven to discriminate: a last-match variant returns "9 of 9 (Appended later)" and T1 reds. No assertion checks that the document contains a sentence. That is what local/no-source-grep exists to stop, and it would pin wording that is allowed to improve. The point of the test is that if that regex ever gains g and a last-match walk, the test fails - instead of the documentation quietly becoming a lie with nothing to notice. Prose only, no new heading. docs-state-md-locale-parity compares heading-level sequences by LCS rather than text, so added paragraphs cannot fail it while an added HEADING would fail all four locales. The constraint is structural, not stylistic - confirmed by running that comparison after the edit. The four locale copies are translated rather than left stale. They are not gate-enforced for prose, so "nothing fails" was available and is not the same as correct: leaving four documents asserting something the English one now contradicts is a correctness problem. Code spans and the anchor link stay untranslated - they name real tokens. The whole approach rests on one fact, checked first: ## Current Position at :196-208 sits OUTSIDE every generated marker region (:81-104, :138-151), so a hand edit survives --write. Re-confirmed after all five edits - gen-state-md-docs --check reports all 6 targets up to date. Had that been false the fix would have belonged in the generator, and a hand edit would have been silently reverted. One real gate failure fixed inline rather than reported: the new test's comments referenced docs/reference/state-md.md, which was not in that file's registered exempt-docs paths, and lint-docs-guard-registration failed lint:ci correctly. Registered. Known limit, named rather than folded in: gsd-tools validate/health still do NOT warn on a duplicated Phase:. #3812 records that as a "consider", not a requirement, and confirms none of the nine rules in src/health-diagnostic-rules/{state-consistency,phase-structure}.cts counts occurrences. Documenting the silent first-match is the delivered scope; making it loud is new scope and stays unclaimed. Closes #3812 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3812): the rule I documented was false — replace it with the measured one An isolated review returned two blockers. Both mine, and the first is the worse kind: I wrote a falsifiable rule into a reference page and got it wrong. 1. "A duplicate resolves to the FIRST occurrence" is FALSE. stateExtractField (src/state-document.cts:401-419) tries BOLD `**F:**` across the whole input, THEN plain `^F:`, THEN a pipe-table row. Form precedence beats document order. Measured against the built reader, all intra-section: Phase: A (plain) / **Phase:** B (bold, later) -> B LATER WINS Phase: A (indented) / Phase: B (plain, later) -> B LATER WINS Phase: A (plain) / | Phase | T (table) | -> A first wins My original verification tested plain-versus-plain, saw first-wins, and generalized to all forms. Measuring one case and claiming the general rule is the same error I have had to retract twice already in this epic. It is also worse than silence. The sentence told authors an appended line is safely ignored; a bold line appended "for emphasis" silently overrides the original. Someone trusting the doc would have corrupted their own state file. And #3812 never asked for a resolution rule - it asked for single-valued, overwrite-not-append, and where history goes. The rule was my unrequested addition. Replaced with the measured truth: resolution is by FORM (bold anywhere, then plain at line-start, then table row), and only WITHIN the winning form does the first occurrence win. Both consequences stated plainly - a higher-ranked form wins regardless of position, and an indented `Phase:` is invisible to the plain form. All five claims in the new paragraph verified by execution before being written, including the two I had wrong. 2. The tests tested the wrong case and passed for the wrong reason. T1/T3 put the second `Phase:` under `## Somewhere else` - the INTER-section case, which #2956 already fixed by scoping. #3812 says verbatim that #2956 "fixed the inter-section case and never addressed intra-section duplication", so the case the new prose describes was untested, and the fixtures passed because of section scoping rather than field resolution. They also called bare stateExtractField rather than the production chain, T2 could not discriminate first from last at all, and no fixture mixed forms - which is precisely why the false claim survived to review. Rewritten as four rows, all intra-section, all through the real stateCurrentPositionSlice -> stateExtractField path: plain-then-plain (first wins within a form), plain-then-bold (the bold LATER value wins - the row whose absence let the false claim ship), indented-then-plain (indented invisible), and sibling-field independence. Each proven to fail against a reader that disagrees. 3. Two dead anchors. pt-BR and zh-CN linked `#performance-metrics` while their own headings are `### Métricas de Desempenho` and `### 性能指标`. Both fixed to the anchor their own heading generates. ja-JP/ko-KR kept the English heading, so theirs already resolved. 4. A ja/ko sentence inverted its own meaning. Both rendered "which is the section designed to grow" with a bare demonstrative whose nearest referent read as Current Position - saying the opposite of the point. Rewritten so the clause attaches unambiguously to `## Performance Metrics`. 5. Cross-locale drift, flagged by the implementing agent rather than by me: after fixing EN, the four locales still stated the OLD false rule. Four documents asserting something measured to be wrong is worse than four saying nothing. All four now carry a faithful translation of the corrected paragraph, with code spans, each file's own anchor, and the ja/ko referent fix preserved. Verified: all five claims executed against the built reader; every rewritten test row proven to discriminate; gen-state-md-docs --check reports all 6 targets up to date, so the edits stay outside the generated marker regions; locale heading parity unaffected (prose only, no headings added); build:lib, lint and lint:ci all exit 0. Refs #3812 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3812): second false rule on the same page — scope the ranking to the section A second isolated review found a second false falsifiable claim, and the failure mode is the same one twice in a row: attempt 1: verified plain-vs-plain, wrote a claim about ALL FORMS attempt 2: verified bare stateExtractField, wrote a claim about THE DOCUMENT Both times the claim covered a wider surface than what was actually executed. The fix each time was not a better sentence, it was executing the surface the sentence describes. BLOCKER — "bold `**Phase:**` anywhere in the DOCUMENT wins" is false. ## Current Position / Phase: 1 of 5 + ## Archive / **Phase:** 88 bare stateExtractField(whole doc) -> "88 (other section)" PRODUCTION (slice then extract) -> "1 of 5 (in section)" #2956's section slice means production never hands another section to the matcher; a bold line in `## Archive`, or in the YAML frontmatter, is simply not seen. The ranking is real but scoped: it applies WITHIN `## Current Position`. I verified against the bare function and wrote a claim about the system. Every existing test placed its bold line inside the section, which is exactly why nothing contradicted the claim. T5 now puts a bold `**Phase:**` in `## Archive` and asserts production returns the in-section plain value, with the unscoped reader asserted to DISAGREE so the row proves the scoping rather than assuming it. BLOCKER — the changeset still shipped the ORIGINAL retracted claim. I corrected the page and left the release note saying "resolves to the first occurrence ... a second entry added in good faith is silently ignored". The note contradicted the page it announces, and the release note is what most people actually read. Rewritten to the corrected rule. MEDIUM — the concession was inverted. It read "wins even if it comes FIRST in the file", which is the vacuous direction; the surprising case, and the one the very next clause illustrates with an APPENDED bold line, is "even if it comes LAST". All four locales reproduced the inversion faithfully, so it was an EN-source defect rather than translation drift. Two sharp edges now named, both measured: a bold `**Phase:**` followed only by trailing spaces resolves to an EMPTY STRING and does not fall through to a valid plain line below (T6 pins it); and `| **Phase:** | 3 of 4 |` short-circuits to the bold form and returns the literal `"| 3 of 4 |"`. A page that teaches form ranking has to say where the ranking bites. Also fixed: all five files labelled the link `## Performance Metrics` while the heading is `### Performance Metrics`. Anchors resolved correctly everywhere; only the label's level was wrong. Every clause in the final paragraph re-verified through the PRODUCTION chain (stateCurrentPositionSlice -> stateExtractField), clause by clause, before being written: bold in another section does not win; bold in frontmatter does not win; bold appended last does win; first wins within one form; trailing-space bold yields empty. All four locales carry the same corrected rule. gen-state-md-docs --check reports all 6 targets up to date; heading counts unchanged at 20/20 across all five files, so locale heading-parity is untouched; build:lib, lint and lint:ci all exit 0. Refs #3812 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3812): backfill changeset pr number Refs #3812 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
|
1051c6d8d4 |
fix(#3799): scope legacy cleanup to the install's resolved config dir and add --no-legacy-cleanup (#4013)
* fix(#3795): read the interrupted agent id before clearing the stale marker (#4006) * test(#3795): the interrupted-agent read must precede the stale-id clear * fix(#3795): read the interrupted agent id before clearing the stale marker execute-plan's init_agent_tracking step ran `rm -f .planning/current-agent-id.txt` BEFORE the existence check that read it, so the interrupted-agent branch and the Task resume prompt it exists to offer were unreachable (#3795) — a kill -9 mid-executor left the file, and the next run deleted it before looking. The read now precedes the clear; fresh-run semantics (no stale id leaking into the new spawn) are preserved. A structural guard pins the order. Emitted-Drift-Ack-Growth: execute-plan.md — #3795: +bytes from reordering the interrupted-agent read before the rm plus the explaining comment * chore(#3795): changeset fragment (pr number backfilled after PR creation) * chore(#3795): backfill changeset PR number (4006) --------- Co-authored-by: sim <sim@local> * fix(#3799): scope legacy cleanup to the install's resolved config dir and add --no-legacy-cleanup * chore(#3799): changeset fragment (pr number backfilled after PR creation) * chore(#3799): backfill changeset PR number (4013) * test(#3799): mark the legacy-name fixtures with gsd-allow-legacy-name --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
bf8905fcc9 |
fix(#4012): a hang never emits the events I was listening for
The init marker settled it. The diagnostic reported "THE REPORTER LOADED BUT RECORDED NO TEST EVENTS — the events file contains only the reporter's own reporter:init marker", which refutes the reporter-never-loaded hypothesis and leaves exactly one explanation. The runner spawns a child process per test file and surfaces a subtest's test:start / test:pass / test:fail to the parent's reporter only once the child REPORTS that test — which happens when it completes. The fixture hangs forever, so it never completes, so it never reports. I had recorded exactly those three event types: the precise set that a hang guarantees you will never see. The feature could not have worked for the case it was built for. test:enqueue and test:dequeue are emitted by the runner as it queues and begins each file, independent of anything inside finishing. test:dequeue is what actually means "in flight", and it is now the primary signal, with test:start kept as a secondary one. A file is in flight when it has been dequeued and has no terminal event. The four branches now describe states that are all real: the events file absent (reporter never loaded); the init marker alone (the runner dequeued nothing at all — genuinely surprising now rather than the expected outcome); everything dequeued and terminated (the files finished and the process hung afterwards, a handle leak); and one or more dequeued-but-unterminated files, named, which is the case this whole feature exists to report. Verified against the exact shape the real hang produces, by executing analyzeChunkEvents on a synthetic events file: init + enqueue + dequeue with no terminal event reports hangs.test.cjs as in flight, and appending a test:pass clears it. Four more unit tests cover the ordering and multi-file cases with no subprocess, so this logic is now checkable without a runner round-trip — which matters, because every defect in this feature so far was visible only remotely. T1 is untouched and should now pass for the right reason. Verification runs on the remote runner. Refs #4012 |
||
|
sim
|
444137e63a |
fix(#4012): make the artifact say whether the reporter ever loaded
Down to 3 remote failures, all one chain. The explicit no-events reporting is working — the diagnostic now states the events file does not exist, instead of silently printing the generic message. But it then ASSERTED a cause: "the child was killed before the reporter wrote even one event (process/spawn startup stall, not a test hang)". That was a guess dressed as a finding, and the fixture contradicts it: it starts a real test, so test:start should fire in milliseconds against a 2000ms budget. Two hypotheses remained and I could not separate them locally, because the local test runner is hook-blocked here: either the custom reporter never LOADS in the child, or it loads and no event reaches it before the SIGKILL. Rather than guess a third time, the artifact now answers it. The reporter appends a reporter:init line as its first action, before consuming anything, so the file's contents discriminate: absent means the reporter never loaded; init-only means it loaded and saw no test events; init plus events means it works. The diagnostic has a branch for each and, where the cause is genuinely unresolved, names both possibilities instead of picking one. Also passes the reporter as a file:// URL via pathToFileURL. Node documents the --test-reporter value as an import()-style specifier, and a bare absolute path is not a portable one — notably on Windows. That is a correctness fix whichever hypothesis holds, and it is a live candidate for the first. FIXED_OVERHEAD is derived by reducing over the actual argv strings, so the longer URL is accounted automatically. Verified by execution, not assumption: composing the reporter against an EMPTY event stream writes exactly one line, the init marker. That is the whole point of the marker, so it is pinned by a test rather than left to inspection. T1 stays red and untouched. Verification runs on the remote runner. Refs #4012 |
||
|
sim
|
5504724d7a |
fix(#4012): the reporter body must return nully, not an iterable
Fourth failure on this feature, and this one was caused by the previous fix's lint workaround. TypeError [ERR_INVALID_RETURN_VALUE]: Expected nully to be returned from the "body" function but got an instance of Array. When stream.compose is given an async FUNCTION as the body, that function must return nully. The reporter ended with 'return []' under a comment asserting Node "still requires the exported function to return an iterable" — exactly backwards, and the direct cause of 41 failures across every run-tests.cjs invocation. That return existed only to dodge ESLint's require-yield after the previous commit converted async function* to async function. A lint workaround became a runtime crash, and the comment written to justify it stated the opposite of the contract. Both are now corrected to what the runtime actually does. Verified by EXECUTION rather than by reading: composing the real reporter against a fake event stream completes with no error and leaves both handled events durably on disk, with the ignored event type skipped. The failing form was reproduced the same way first, so the diagnosis is not inferred from the stack trace alone. The new unit test requires the reporter directly and asserts the returned value is nully — the assertion that would have caught this before it reached the runner — plus the exact NDJSON written. No subprocess, so this half of the feature is verifiable without a full runner pass, which matters because every defect in this feature so far has only been observable remotely. T1 remains untouched and red. Verification runs on the remote runner. Refs #4012 |
||
|
sim
|
7f2af28639 |
fix(#4012): the reporter sink must be a regular file, not devNull
Third failure on this feature, and this one broke everything rather than just the diagnostic: 43 failures across every run-tests.cjs invocation. Error: EINVAL: invalid argument, fsync Emitted 'error' event on WriteStream instance Node opens a WriteStream for a --test-reporter-destination and FSYNCS it on close. fsync on /dev/null is EINVAL — it is a character device, not a regular file. So os.devNull is not a usable reporter destination at all, and every chunk crashed on exit. The sink only ever needed to be a regular file that stays empty, since the reporter writes its real output through appendFileSync to the path in GSD_RUN_TESTS_EVENTS_FILE. It is now one fixed file inside the existing events dir, pre-created rather than relying on the stream's create-on-open, and left empty by design. One sink for the whole run, not per chunk, so its path length stays constant — reporterOverhead feeds FIXED_OVERHEAD, which is computed once before chunking, and a variable-length path would silently mis-account the Windows argv ceiling. The comment that named devNull now says why the destination must be a regular file, so this does not get re-optimized back into the same crash. Reaching for devNull was the mistake: it looks like the obviously correct way to discard output, and it is, for a pipe or an fd — but not for something Node is going to fsync. Each of the three failures on this feature was a different edge of the same assumption, that a reporter destination behaves like ordinary output. The regression test asserts the closest externally observable consequence — a normal run must not surface the EINVAL/fsync text. The argv construction lives inside main() with no exported seam, and the sink is swept before a test could stat it; adding a seam purely to assert that is left out rather than reshaping production code for the test. Stated plainly rather than implied. The failing T1 is untouched and still red. Verification runs on the remote runner. Refs #4012 |
||
|
sim
|
0abd137ec7 |
fix(#4012): the events reporter has to survive SIGKILL
The remote run proved the instrumentation did not work. Timing landed — "chunk 1/1 was killed after 2006ms" — but the in-flight-file naming produced nothing and fell through to the pre-existing generic message. The feature I wrote to diagnose a kill was itself destroyed by the kill. Root cause, confirmed rather than assumed. The reporter yielded strings, which node pipes into the --test-reporter-destination WriteStream. That stream BUFFERS. execFileSync's timeout sends SIGKILL, which is uncatchable and gives nothing a chance to flush, so the events sat in a buffer that died with the child. The parent's own timer reported correctly because it lives in the parent — which is exactly why half the feature looked fine. The reporter now writes each event with fs.appendFileSync, unbuffered and durable at the moment it happens, to a path passed through GSD_RUN_TESTS_EVENTS_FILE. Env vars do not count toward the Windows 32,767-char argv ceiling, so moving the path out of argv also REDUCES FIXED_OVERHEAD; the accounting moved with it rather than being left stale. The destination is now a fixed devNull sink that stays empty by design. Silence was the reason this was invisible for a whole run. Failing to read the events file now says so explicitly, and distinguishes a file that could not be read at all from one that exists but is empty — the generic fallback firing quietly is what let a broken feature look like a working one. A write is unbuffered but not atomic, so a kill can still interleave a partial line; the reader tolerates exactly one unparsable trailing line and reports the complete ones before it. The failing T1 was left red and untouched rather than weakened to pass. Three new unit tests cover the reader directly, with no subprocess, so the parsing half is verifiable without a full runner pass: missing file, existing-but-empty file, and a truncated final line. Also adds ndjson-reporter.cjs to GSD_SCRIPTS_LIB_FILES in bin/install.js — scripts/lib/ ships, and omitting it meant the file would install everywhere and orphan on uninstall. That single omission caused 4 of the 7 remote failures. Verification runs on the remote runner. Refs #4012 |
||
|
sim
|
11c24ce973 |
chore(#4012): regenerate the 19 install-tree goldens
scripts/ ships, so a new file under scripts/lib/ moves every install-tree golden. The remote runner caught this as 26 failures across all 19 runtimes. My fault in the dispatch: I limited the implementation's verification to build:lib and lint:ci and left regen:derived out. Both of those passed, which is precisely why a green local gate is not a substitute for the runner — the goldens are only exercised there. One line per golden: scripts/lib/ndjson-reporter.cjs joining the shipped tree. Refs #4012 |
||
|
sim
|
8497833a15 |
fix(#4012): a killed chunk now names the file that was hanging
The per-chunk timeout fired correctly but reported almost nothing, so every diagnosis cost a CI round-trip. run-tests.cjs logged chunk START only — no timestamp, no duration, no end line — then on a kill printed all ~55 basenames and asked the operator to work out whether output kept flowing (slow) or stopped early (hang). It could not name the in-flight file because the child is spawned with stdio inherit, deliberately, per #3597/#1051. Three additions. Per-chunk elapsed timing on every path, not just failures, so drift toward the cap is visible before it becomes a kill. Every timing number in the investigation behind this had to be reconstructed by hand from GitHub log timestamps. A second, machine-readable reporter running ALONGSIDE the human one, writing NDJSON to its own file. On a kill that file is read back and the files with a test:start and no matching completion are named, with the staleness of the last event, so "stopped 480s ago at X" reads differently from "still emitting at kill". stdio stays inherit and nothing is piped or tee'd — the maxBuffer and live-output risks that shaped the original design are untouched. Ranking of the killed chunk's files by known weight, flagging any absent from tests/test-timings.json, since an unweighted file is an unknown quantity. Two details that are correct rather than lucky. Passing --test-reporter at all replaces node's implicit default, so the human reporter is now named explicitly and reproduces node's own selection (spec on a TTY, tap otherwise) — visible output is unchanged. And the destination path's chunk index is zero-padded to a fixed width because FIXED_OVERHEAD is computed ONCE before chunking; a variable-length path would have silently mis-accounted the Windows 32,767-char argv ceiling and reintroduced #3597. The reporter flags are added to FIXED_OVERHEAD exactly as --test-force-exit is. The multi-reporter pairing and the stream.compose reporter contract were confirmed against Node's v24 documentation, not recalled — the first draft carried them as an unverified assumption and said so. Also corrects a stale comment claiming the 600s cap sits "below the 20m job cap". The lane is sharded 3x at timeout-minutes: 45; the windows shards were at 19m when chunk 1/5 was killed on |