Files

0xdhx 6950ae3679 test(#2269 ): repo-wide regression guard for query commit --files scoping (#2290 )

* test(#2269): repo-wide regression guard for query commit --files scoping

regression protection that fix does not carry: a repo-wide scan, edge-case
pins, property tests, and a behavioral test.

- Repo-wide scan across all five directories that carry live invocations
(gsd-core/workflows, gsd-core/references, agents, commands, skills), so a
future unscoped `commit` / `query commit` site fails CI wherever it lands.
Backslash-continued lines are joined first, and a quote-parity walk
distinguishes a real `--files` flag from one mentioned inside the quoted
commit message.
- Edge-case pins for the shapes the live content does not exercise: the
`query`-less spelling, a flag preceding the command (`--cwd`), a prose
mid-sentence mention, and a `commit_docs` JSON-key false positive.
- Three `fast-check` properties over the quote-parity logic, following the
tests/adr-parser.property.test.cjs precedent.
- A behavioral test that stands up a real temp git repo, leaves an unstaged
`.planning/` stray plus an unrelated staged file, runs the real
`gsd-tools commit --files`, and asserts via git status/diff that only the
intended artifact landed. The scope is derived from secure-phase.md's own
commit line, so reverting that line's `--files` fails this test too.

Census over the five roots at this base: 87 invocations, 0 unscoped.

* test(#2269): scan mid-prose argument-bearing invocations too

The repo-wide guard's INVOCATION_RE is line-start-anchored, which keeps
bare backtick mentions out of scope but also blinded the scan to fully
argument-bearing `query commit` invocations embedded mid-sentence in
instructional prose. Three such live invocations sit inside the scan's
own roots today (new-milestone.md, new-project.md, plan-phase.md) — all
scoped, but never entering the candidate set, so trimming their
`--files` clause would reintroduce #2269 behind a green suite.

Add a second tier: MIDLINE_INVOCATION_RE matches the invocation token
anywhere a quoted commit message follows (`commit "` — the executable
shape a bare mention never carries), and invocationCandidates() extracts
the backtick-bounded invocation substring so hasScopedFiles's
quote-parity walk is not skewed by surrounding prose quotes.

Census after widening: 87 anchored + 3 mid-line = 90 invocations,
0 unscoped; the mid-line tier picks up exactly the three cited sites
and nothing else across all five scan roots.

* test(#2269): align the --files value predicate with the runtime's flag filter

The scan's --files value test was /--files\s+\S/, which scores
`--files --amend` as scoped because `-` is \S. routeCommit disagrees:

args.slice(filesIndex + 1).filter(a => !a.startsWith('--'))

drops every `--`-prefixed token, so `--files --amend` yields files=[] and
lands on the same unscoped default branch as a trailing bare `--files` —
#2269 verbatim. Two live sites sit one token-deletion from the shape:
gsd-core/references/git-planning-commit.md and
gsd-core/workflows/execute-plan.md both run
`... commit "" --files .planning/codebase/*.md --amend`.

The predicate now mirrors the runtime's own rule. Single-dash tokens stay
values, because the runtime filters on '--', not '-'.

Pinned: `--files --amend` and `--files --no-verify` as unscoped, plus the
two live `--files <glob> --amend` shapes and `--files -weird-name.md` as
scoped negative controls, so the fix cannot over-correct into "any
--files near a flag is unscoped" with nothing failing.

Also tightens the fast-check path generator, which is not cosmetic. It
excluded only ["\s], so it could draw a `--`-prefixed token and assert it
scoped: measured 26 hits in 200,000 draws (~1 in 7,700), i.e. ~1 CI run
in 77 at the default 100 runs would have failed as a mystery flake once
this fix landed. The complement is now its own property, and it fails
against the pre-fix predicate (counterexample ["","--#"]).

The behavioral test's --files presence check moves to the same predicate,
and its two failure modes are now separate messages: a genuinely missing
--files (the #2269 regression) versus an unquoted value that only breaks
this test's own scope derivation.

* test(#2269): score each invocation on a line separately, not the whole line

invocationCandidates returned [line] for any line-start match, so scoring
was satisfied by one --files anywhere on the line and a later scoped
invocation vouched for an earlier unscoped one:

gsd_run query commit "a" && gsd_run query commit "b" --files x.md
gsd_run query commit "a" ; gsd-tools query phase-list --files y.md

Same "one hit satisfies the whole candidate" class as the --files value
bug in the previous commit. No live line has the shape today, so this is
pinned rather than left to be rediscovered.

A line-start invocation is now split at shell command separators before
scoring. The separator must be followed by a binary token, so a binary
inside a command substitution — `commit "$(gsd-tools query x)" --files
a.md` — is not treated as a second invocation; that negative control is
asserted, since the obvious "split at every binary token" implementation
turns it into a false offender.

Census on the current tree is unchanged: 89 anchored + 3 mid-line = 92
invocations, 0 unscoped, and the split produces zero extra segments on
live content.

* test(#2269): drop the unnecessary file-level allow-test-rule exemption

no-source-grep only fires on a readFileSync whose path expression carries
BOTH a .cjs/.js/.ts extension and a quoted bin|lib|gsd-core|src literal
(looksLikeSourcePath, eslint-rules/no-source-grep.cjs). This scanner reads
.md only, so the rule never triggered and the exemption bought nothing.

It was not inert, though: the escape is file-level — getAllComments() sees
the header and the rule returns {} for the whole file — so it silently
disabled no-source-grep for the pre-existing #2112/#2523 tests here and
for anything added later.

Verified by removing it and running eslint on the file: clean.

* test(#2269): mirror routeCommit by tokenizing, not by scanning line text

The scan's predicate searched the whole line for `--files` with double-quote
parity, while the runtime does an exact-token argv lookup scoped to a single
command. Those semantics disagreed, and the disagreement was exploitable in
both directions:

guard=true runtime=false | ... commit "docs: update ROADMAP.md" && echo done --files unused.md
guard=true runtime=false | ... commit 'docs: explain --files usage'
guard=false runtime=true | ... commit 'prints a " sometimes' --files .planning/PLAN.md

The first is #2269 verbatim: `--files` is echo's argument and never reaches
gsd-tools argv, so cmdCommit takes the blanket-`.planning/` default while the
guard stays silent. Injecting that line into secure-phase.md produced 0
offenders.

Each earlier round fixed one of these by widening the approximation, which
only moved the disagreement. So stop approximating: tokenize the line the way
a shell would — honouring BOTH quote characters, backslash escapes and unquoted
control operators — then run routeCommit's own predicate over the tokens.
Two previously hand-encoded special cases now fall out for free: `--files=x`
is unscoped (indexOf needs the exact token) and `--files -weird.md` is scoped
(the runtime filters on '--', not '-').

Operators are marked structurally rather than re-identified by comparing a
token's text against an operator set, because `commit '|' --files a.md`
produces a token whose VALUE is `|` and which is ordinary message text.

The property tests are part of this commit, not a follow-up: all three built
their line from a hardcoded double-quoted template, so no number of runs could
generate the single-quoted shape. They pinned one quoting dialect while reading
as though they pinned the predicate, which is what let the above through. The
delimiter is now drawn, and a fourth property asserts directly against a
re-implementation of routeCommit's own predicate.

Also removes a second copy of the old heuristic from the behavioral test, and
widens the mid-prose tier to `'` — it keyed on `commit "` only, so a
single-quoted mid-prose invocation was invisible to the scan entirely.

* test(#2269): scan docs/, which carries live invocations the claim excluded

The comment asserted the five roots were "every directory that carries live
invocations". That was false against the current tree, not hypothetically:
docs/zh-CN/references/ carries 7 live `query commit` invocations — the Chinese
mirrors of three gsd-core/references/ files that ARE scanned.

They are all scoped today, which is exactly why this needed its own assertion:
adding or dropping the root does not move the offender count, so the coverage
loss was silent in both directions. The scan now records which roots actually
contributed invocations and asserts docs/ is among them. Stated as a reach
property rather than a census figure, since a hardcoded count is the drift this
file has already been bitten by.

The locales are the right place to care about: ja-JP/ko-KR/pt-BR have no
references/ subtree at all, so the translations already drift per-locale — the
condition under which an unscoped example is reintroduced in one copy
unnoticed. New files under an existing root were always picked up by the
recursive walk; the gap was only ever at the root level.

* test(#2269): don't flag invocations inside HTML comments; drop a stale census

Two smaller findings from the same review.

The scan treated any `gsd_run … commit "` occurrence as executable, so an HTML
comment documenting the historical defect was reported as an offender:

Nothing in the scanned roots trips this today, but it made the guard hostile to
documenting the very bug it protects against — a plausible thing to add
precisely BECAUSE this issue exists. Comment spans are now stripped before
scanning, preserving newlines so a multi-line comment cannot fuse the text on
either side of it into one logical line. The strip lives beside the other scan
primitives rather than inside the test, so the assertion and the scan cannot
drift apart.

And the comment claiming "the total match count stays at 86" was stale (89).
No assertion read it. Rather than correct the figure, state the invariant it
was standing in for — dropping `.*` swaps onboard.md out of coverage WITHOUT
changing the offender count, since that site is scoped either way. The number
had already drifted three times; the property will not.

* test(#2269): consume comments, single-&, and redirections before scoring argv

Round-10 Major (davesienkowski): tokenize modelled &&/||/;/| and nothing
else, so `--files >/dev/null 2>&1`, `--files > out.md`, `# --files`, and
`& echo --files` all scored as scoped while routeCommit sees files=[] and
takes the blanket-.planning/ default. The live tail in
execute-phase-requirement-revert.md is one token-deletion from the silent
false negative.

The tokenizer now ends the command at a word-start unquoted `#`, treats a
single `&` as a control operator, and consumes redirections (with glued
IO numbers, `2>&1` included) as redir-marked tokens that hasScopedFiles
excludes from argv. Quoted and mid-word `#` stay literal — ship.md's
`PR #${PR_NUMBER}` message is pinned scoped. `$VAR` tokens deliberately
stay values: statically undecidable, and the three original #2269 fix
sites all pass `--files "${PHASE_DIR}/…"`.

* test(#2269): a foreign command's `commit` no longer reads as a gsd offender

Round-10 Minor (davesienkowski): segmentInvocations' no-hit fallback
returned the whole line, so `gsd_run query state && git commit -m "x"`
and `gsd_run query state | grep commit` — matched by the anchor's
deliberately loose `.*` — were scored as unscoped gsd commits and
false-flagged with the wrong failure message.

The fallback now applies only to a segment that actually carries the gsd
binary followed by a commit token (env-prefixed or wrapped invocations,
where dropping the line would be silent coverage loss); a line whose
`commit` belongs to another command contributes no candidates.

* test(#2269): drop the commands/ exemption from the per-root reach assertion

Round-10 Nit (davesienkowski): the exemption documented a gap that no
longer exists — commands/gsd/review-backlog.md contributes a candidate,
so commands/ is held to the same dead-weight test as every other root.

* test(#2269): put the tokenizer's escape branches inside the tested domain

Round-10 Minor 1 (trek-e): both backslash-escape branches in tokenize
were unreachable by any assertion — every generator stripped every
backslash, and no hand-pinned case carried one.

Double-quoted property messages may now contain `"` and `\`: embedFor
escapes them into the LINE while the property keeps the raw string as
the argv the shell would deliver, so the escape branch sits inside the
differential oracle's domain. Single-quoted messages still exclude
backslashes — the shell has no escape inside '…', so there is no escaped
spelling to generate. Four hand-pinned cases cover both branches in both
directions (escaped quote before a real --files, escaped quote hiding a
message-internal --files, escaped space in the message, escaped space in
the value).

* test(#2269): normalize path separators in the scan's diagnostic strings

Round-10 Minor 2 (trek-e): scanned/offenders entries were built from raw
readdirSync (recursive) names, so a failure message on Windows would
render backslashed paths, against the repo's normalize-unconditionally
convention. Join with the raw entry; report with the normalized one.

* test(#2269): pin the unquoted escape branch in the unscoped direction too

Claim-audit catch on the round's own response draft: the unquoted
backslash-escape branch was pinned only in the scoped direction, and the
unquoted property generator strips backslashes, so the unscoped
direction was outside every assertion. One pin closes it.

* test(#2269): decide an invocation by its command shape, not by its markup

Both scan tiers answered "is this text executable" with a syntactic proxy —
one anchored the command at line start, the other required a quoted commit
message — and both were wrong, in opposite directions. The anchor flagged a
fenced block that deliberately SHOWS the unscoped form. The quoted-message
tier could not see an unquoted invocation at all: `gsd_run query commit
fixup` reaches the identical cmdCommit, entered no candidate set, and
trimming its --files clause reintroduced #2269 with nothing to fail.

Markdown context was the obvious replacement and is refused, on measurement.
Keying on fences means parsing them — fence character, opening run length,
nesting, tilde fences, four-space indented blocks, unclosed markers — and
every bug in that parser is a silent false negative. I built that version
first and it lost four executable shapes before I stopped: `cd "$ROOT" &&
gsd_run query commit …`, `if …; then gsd_run …; fi`, four-space indented
blocks, and a four-backtick fence containing three-backtick runs. Each loss
was invisible; the census stayed at 99.

So the discriminator is the command shape:

<binary> [ query | -flag [value] ]* <commit-token> <at least one arg>

The middle clause separates a command from a SENTENCE containing the same two
words — `Update STATE.md using gsd-tools.cjs query (or legacy gsd-tools)
commit mutations:` fails it at `(or`, with no markup parsed. The trailing
clause separates an invocation from a mention. Each line is scanned whole and
its inline code spans are scanned too, unioned: the whole-line pass reaches
every executable shape regardless of markup, and the span pass reaches the one
case it cannot, where a backtick glues to the binary token. The union is
additive, so a mis-parsed span can only add a visible false positive, never
hide an invocation.

This closes the unquoted case in BOTH the code-span and bare-prose forms, so
no part of it is left declined. Named residual, pinned as a test rather than
left to be discovered: an undelimited prose mention running straight into its
sentence (`see gsd_run query commit for the scoping rules`) is flagged, because
nothing distinguishes it from an invocation with arguments without guessing at
English. Bounded and measured — the roots carry 93 bare-prose mentions of the
binary and 0 with a commit token, the repo's convention is to backtick a
command reference, and the failure is a visible red.

Two things fell out. An interpreter prefix (`node gsd-tools.cjs commit …`,
live in docs/CLI-TOOLS.md) was invisible to both old tiers and is now in
scope; that pulled in the CLI usage synopses, so a bracketed optional FLAG now
marks synopsis notation — the discrimination is the bracketed flag, never
"contains a bracket", since 24 live invocations carry brackets inside their
quoted message (one as its --files value) and none brackets a flag.

tokenize and hasScopedFiles are untouched — byte-identical. Only the question
"is this text an invocation at all" changed. Census re-derived with the file's
own primitives: 99 candidates across 62 files, 0 unscoped, workflows 67 /
references 11 / agents 12 / commands 1 / skills 1 / docs 7 — identical to the
previous tier logic. Against the pre-fix tree (200daa456^) it still flags
exactly the 3 sites #2269 was filed about.

* test(#2269): let a wrong-example declare itself, in comment position only

A block showing the unscoped form on purpose scored as an offender. That is a
false positive against content correct as written, and the worst kind: the fix
a contributor reaches for is to mangle a teaching example until the linter
stops complaining. The HTML-comment escape only covered examples written as
comments.

Exempting fenced blocks was the prescribed remedy and is refused on
measurement: 96 of the 99 live invocations sit inside fences. Against the
pre-fix tree (200daa456^) the scan flags exactly the 3 sites #2269 was filed
about, and 0 with fences exempted. That does not narrow the guard, it disables
it — and no property of the surrounding markup can stand in for the author's
intent anyway, which is the general form of the same point.

So intent is declared: `# gsd-scan-ignore: <reason>` exempts the line, and a
reason is required because a bare token is not a declaration. Undeclared, the
identical line stays an offender — it is byte-for-byte what a real regression
looks like.

The marker is honoured ONLY in comment position, via the same unquoted-`#`
rule tokenize() already applies, so the two cannot disagree about where the
command ends. Matching the token anywhere on the line would let the commit
MESSAGE carry it — `commit "docs: explain gsd-scan-ignore: semantics"` — and
silently exempt a real offender. A guard that can be talked out of firing by
its own documentation is strictly worse than the false positive the marker
exists to fix, so both directions are pinned.

The marker is a new convention here and I would rather be redirected than
assume: happy to rename it, key it to an HTML comment, or drop it for whatever
shape you prefer.

* test(#2269): pin the escaped-backtick case in both directions

The span pass skips a backslash-escaped backtick, because an escaped backtick
is literal text and pairing it invents a code span the rendered document does
not have — the invented span then reads as an unscoped invocation, a false
offender against prose that merely displays a backtick.

That guard had no test: reverting it broke nothing, and a property with no
test is one the next edit removes for free. Pinned in both directions, so the
assertion covers the escape rather than the absence of span handling.

* test(#2269): close a bypass in the ignore marker's comment-position test

The marker's comment-position check keyed on "preceded by whitespace", which
is not the rule the shell applies and not the rule tokenize() applies. In
`commit docs:\ # gsd-scan-ignore: reason` the backslash escapes the space, so
the shell keeps `docs: #` as ONE word: the `#` is literal, the command runs,
and it runs UNSCOPED. The raw-text check saw a comment and exempted the line.

That is the guard being disarmed by text the author controls, which is worse
than the false positive the marker was added to fix — a guard that can be
talked out of firing is not a guard. Found by an adversarial audit of this
round's own claims, not by the tests, which is why both halves below are now
pinned.

Two independent conditions, deliberately:

- commentPortion tracks WORD START the way tokenize does, rather than looking
at the preceding character. That closes the escaped-separator case directly.
- A marker that survives tokenization as an ARGUMENT disqualifies the line
outright. tokenize() drops everything from a real comment onward, so a
genuine declaration leaves no token carrying the marker; anything that does
reached argv, which means the runtime executed it.

The second is what makes the closure structural rather than a matter of
getting commentPortion's edges exactly right — and it has edges. A
REDIRECTION swallows `#` and its text into a redir token, so the shell passes
it to the redirect target and never treats it as a comment, while raw-text
reading still sees one. Pinned as its own case, since it is the shape only the
cross-check separates.

Reverting either half fails the marker test independently.

* test(#2269): require a tracking reference on every scan-ignore declaration

The gsd-scan-ignore: marker accepted any non-space reason text, giving the
exemption no expiry and no ledger — the permanent-allow-test-rule shape
RULESET.TESTS.delete-bad-tests names. ADR-456 already settled this for the
sibling allow-test-rule: convention, so the marker now requires the same
#NNN issue reference (or an https:// URL).

scripts/lint-allow-test-rule-refs.cjs walks tests/ only and keys on the
ESLint comment form, so it cannot see a marker living in a .md file. Rather
than teach a second token to a script whose whole contract is that form,
the scan enforces the rule over its own roots — it already runs in CI on
every shard.

An attempted declaration that carries no reference is reported AS one,
asserted before the offender list. It is also an offender (it does not
exempt), and letting the generic assertion win would tell an author who had
already explained the line that their commit is unscoped — sending them to
re-read a flag that was never the problem.

Live declarations in the six scan roots: 0, so nothing is grandfathered.

* test(#2269): name every remedy in the failure, and document the convention

The offender assertion named only --files, but the guard has three distinct
causes and only one of them is the bug. An ordinary English sentence that
runs `gsd_run query commit` straight into its prose is flagged — by design,
since nothing separates it from an invocation with arguments without
guessing at English — and a contributor told only that their commit is
unscoped will mangle the sentence until the guard shuts up. That is exactly
the outcome the declaration marker was invented to prevent.

The message now names all three remedies (scope it / backtick the mention /
declare the wrong-example) and points at CONTRIBUTING.md. This is the
standard the repo already states one section over for its sibling gate:
"The failure output names its own remedy".

The help text is hoisted out of the assertion so it can be pinned. A failure
message is unreachable on the passing path, so nothing would have noticed a
remedy being edited back out of it.

The marker lives in .md files across six roots, so documenting it in this
test's comments reaches nobody who hits it. CONTRIBUTING.md now carries the
convention beside the sibling allow-test-rule: exception, and its example is
a live instance of itself — removing the declaration makes the example an
offender (verified).

CONTRIBUTING.md is outside changeset lint's USER_FACING_PREFIXES, so the
diff still returns ok_no_user_facing_changes (verified, not assumed).

* test(#2269): decide synopsis notation by the first argument, not by the line

SYNOPSIS_TOKEN_RE disqualified the whole segment, so notation anywhere on a
line silently disqualified a real invocation. Wrong in both directions, and
both reachable:

See [--files](#anchor) then run gsd_run query commit "docs: x"
^ an ordinary markdown link, and docs/ is a scan root

gsd_run query commit "docs: x" [--amend]
^ a real, executable, unscoped call

Both scored 0 candidates. The second is the dangerous one: `[--amend]` is a
literal word to the shell, so the line runs, reaches routeCommit with
files=[], and sweeps the index — #2269 verbatim, with the guard silent on
exactly the defect it exists to catch.

Positionally there is no ambiguity. A synopsis documents a call it does not
make, so its first argument is a placeholder; a real call's first argument
is its commit message. The test moved to that position.

`<message>` reaches the predicate as a REDIRECTION — `<` is a redirection
character, so tokenize() reads it the way the shell would. That mangling is
the identifying feature rather than an obstacle, and keying on it avoids the
false negative a raw-text match would have introduced: inside a quoted
message `<Widget>` is ordinary text, one token, no redirection. Pinned.

All five live synopsis lines (docs/CLI-TOOLS.md and its four localized
mirrors) remain excluded — verified by running the primitives over each.
Reversion control: restoring the whole-line rule fails 'usage-synopsis
notation documents the CLI and is not a call to it', and only that test.

* test(#2269): reach invocations behind a subshell, a shell -c, and an escaped backtick

Three shapes that were executable and invisible.

(1) `(gsd_run query commit "docs: x")`. Subshell grouping changes no argv,
so `(` was never a tokenizer metacharacter — which left `(gsd_run` as one
token the binary anchor could not match. The anchor now strips a leading
run of `(`.

Backticks are deliberately NOT stripped with it, and the first attempt that
did strip them is why this is stated rather than assumed: to a shell a
backtick is never part of a binary name, so a backticked invocation belongs
to the code-span pass, which extracts the command from inside the
delimiters. Stripping them in the anchor made the whole-line pass find the
same invocation a second time, absorbing the surrounding sentence as
arguments — the census went 99 -> 102 with no verdict changing. Measured,
reverted, and pinned as a comment so the next reader does not re-try it.

(2) `bash -c "gsd_run query commit fixup"`. A shell invoked with -c runs its
next argument AS a command, so the invocation sits inside a quoted token
that no markup rule can reach. The recursion is keyed on the INVOKER, never
on "a quoted token that parses as a command" — the wider rule would flag a
commit MESSAGE that quotes an invocation, a false positive against ordinary
documentation. Pinned in both directions.

(3) codeSpans skipped an entire backtick run on an odd backslash prefix,
which is stricter than the rule its own comment states: an escaped backtick
consumes exactly ONE, and the rest of the run is still a delimiter. Fixed,
and pinned with a case that now yields a span where it previously yielded
none. The reviewer's own example stays at zero spans — correctly, because a
1-run opener does not pair with a 2-run closer — and that is pinned too, so
the distinction is not re-litigated as a regression.

Census re-derived at this head over the six roots with the file's own
primitives: 99 candidates across 65 files, 0 unscoped (workflows 67 /
references 11 / agents 12 / commands 1 / skills 1 / docs 7) — identical to
the previous head, so the widening is coverage-neutral by measurement.

Reversion controls: 3/3 fire against a named test.

* test(#2269): assert scan coverage over the repo, not over each root

The per-root reach assertion was wrong in both directions at once.

Too strong: commands/ contributes exactly one invocation and skills/ one, so
an unrelated PR retiring review-backlog or re-syncing the Chinese mirrors
turned this red with a failure that had nothing to do with #2269. A root is
now allowed to legitimately go to zero.

Too weak, and this is the part worth having: it could only ever re-confirm
the roots already listed. Removing a root from scanRoots was SILENT under it
— the assertion iterates the roots that remain — and a directory that
ACQUIRES invocations without being a root was invisible to it. That is the
gap this file has actually been bitten by twice: agents/ in one round,
docs/zh-CN/ in the next, each found by a reviewer rather than by the suite.

Replaced with the property those checks were approximating: over every
tracked .md in the repo, a file carrying a live invocation must be covered
by a scan root. Dropping any root now fails (verified for docs/ and
agents/), and so does a new directory acquiring one.

This also closes the residual @davesienkowski raised in round 10 —
completeness was asserted only for docs/, never as a general property — and
the one I answered then by saying a git ls-files walk inside the test was
something I would rather propose explicitly than smuggle in. Proposing it:
git ls-files is already used against the repo by seven test files here, one
of which fails closed on a non-zero exit exactly as this does. An
unenumerable file list is an UNKNOWN coverage set, not an empty one.

Measured: 1523 tracked .md, 99 candidates inside the roots, and exactly one
candidate-bearing file outside them — CHANGELOG.md, whose invocation is
scoped. It is excluded with its reason rather than silently: it is
regenerated from .changeset/ fragments, so a marker added to it would not
survive the next release, and it records commands that shipped rather than
instructing anyone to run one.

Suite duration 6.5s -> 9.3s for the wider walk.

* test(#2269): put the recognition half inside the property domain

All four existing properties aim at hasScopedFiles — the half backed by a
runtime oracle, and the half that has been stable for rounds. Every defect
found since lives in the other half: whether a line is an invocation at all.
That asymmetry is the problem, because a miss there is a silent false
negative, where a miss in the scope predicate has an oracle watching it.

So the new properties generate the CONTEXT rather than the arguments. The
recognition rule is "an invocation is found by its command shape, whatever
markup surrounds it", and that is a claim about a domain a generator can
cover: subshells, interpreters, env prefixes, shell keywords, prompts, list
and blockquote markers, indentation, chaining, and `sh -c` wrapping. Each of
those was previously a hand-pinned example, several added only after a
reviewer found the gap.

It paid immediately. Three defects, none of which exists on today's tree:

- A markdown BLOCKQUOTE marker is the one markup form the tokenizer cannot
ignore, because `>` is also a redirection: `> gsd_run query commit "docs:
x"` reads as a redirection whose target is the binary. Found on the
property's first run. 34 blockquoted lines in the six roots invoke this
binary today; none carries a commit token yet.
- `> sh -c "gsd_run commit a"` — the blockquote strip fed only one of the
three passes. The passes now apply to every VIEW of the line.
- `(sh -c "gsd_run commit a"` — the subshell strip was applied to the gsd
binary test but not the shell-invoker test. One helper now, not two copies.

The last two are COMPOSITIONS of shapes that each pass alone, which is the
class hand-written examples are worst at and the specific reason this
finding was worth taking as stated rather than as more examples.

The wrapper axis is drawn only with an unquoted body: a -c payload is itself
quoted, so a quoted message inside it needs a nested-quoting domain, and a
wrong generator domain is this file's most repeated own-goal (two
seed-dependent reds). Constraining the body is what makes the axis safe.

Verified rather than claimed:
- 20,000 generated cases, 0 counterexamples; suite green on three
independent seeds.
- Reverting each of the four recognition fixes in isolation is now caught by
the property, not only by the hand-pinned case. Before the wrapper axis it
caught 2 of 4 — measured, which is why the axis was added.
- Census unchanged: 99 candidates / 65 files / 0 unscoped, same per-root
split, over 1523 tracked .md.
- Discrimination against the pre-fix tree (200daa456^): 97 candidates,
exactly 3 offenders — next.md, secure-phase.md, validate-phase.md.

* test(#2269): cover the scan's own assembly, found by three silent controls

Running a reversion control over every fix in this round left three silent,
and all three were real gaps rather than control artefacts.

Two were the same shape: the WIRING between the walkers and the scan's
result lists was covered by nothing. Every test drove documentCandidates /
documentUntrackedDeclarations directly, so replacing the untracked-
declaration source with an empty list — and emptying the uncovered-file
list — both left the suite green. The real corpus cannot catch either: it is
clean, so those assertions can only ever observe an empty result. That is
the structural reason a synthetic corpus is needed and not a nicety.

Factored the per-document classification into scanDocument() and the
uncovered-file walk into uncoveredFiles(), both beside the other primitives
for the reason already stated there — an assertion must not pass against a
private copy while the scan does something else — and drove each with a
synthetic input. Both controls now fire.

The third was worse, because it looked like a check and was not one:
`assert.match(OFFENDER_HELP, /backtick/i)` is satisfied by the word
"backticked" in the clause explaining why a backticked mention is skipped,
so deleting the backtick REMEDY left the assertion green. Matched on the
instruction instead.

Reversion-control matrix over the whole round: 12 controls, 12 fire against
a named test. Suite 29/29, 0 skipped.

* test(#2269): keep this file's own prose out of the exemption lint

Self-found by running scripts/lint-allow-test-rule-refs.cjs, which the round
had a specific reason to run: the lint extracts everything after the token
on a line and requires a #NNN or URL in it, so the comment explaining that
requirement was itself read as a new untracked exemption --

tests/commit-files-pathspec.test.cjs :: ` reason, and scripts/...

-- and lint-tests would have gone red on a prose line. Reworded so no line
carries the bare token; the lint now reports no novel offenders.

Fitting rather than embarrassing: this is the exact class Major 1 is about,
and the lint caught it on the file arguing for the same discipline.

* test(#2269): fix four defects an adversarial review of this round found

Ran a cross-AI adversarial review over the round's own claims before pushing.
It confirmed 10 of 12 and returned four MISSED findings; all four were real.

1. `shellDashCPayloads` searched the whole segment for a shell name, so
`echo bash -c "gsd_run query commit fixup"` became a candidate. echo
PRINTS the string; it does not run it. The invoker must be at the command
position — only an env assignment, a shell keyword, or a list/prompt
marker may precede it. `echo` and `printf` are commands and no longer
qualify; `then` / `$` / `FOO=1` / a subshell still do. Both directions
pinned.

2. SHELL_INVOKER_RE was a guess at four spellings. `ash`, `csh`, `tcsh`,
`fish` and `yash` all take -c and all run what follows, and missing them
is a silent false negative in the one function whose job is reaching a
command the tokenizer cannot see. All nine spellings pinned.

3. A marker with NO reason at all fell through to the generic offender
diagnosis, because the loose detector required `\S` after the colon. That
is the likeliest way to get the marker wrong, so it is the case that most
needs the specific message. The reason is now EXTRACTED rather than
matched in one shot, so an empty reason is still an ATTEMPT.

4. The predicate now MIRRORS scripts/lint-allow-test-rule-refs' own
ISSUE_REF_RE (`/#\d+|https?:\/\//`) instead of approximating it. The
review refuted my claim of an HTTPS-only rule: the regex accepted
`http://` and the prose describing it did not. The regex was right and
the sentence was wrong — so the sentence is gone and the constant is
cited. A contributor who satisfies one marker and not the other would
otherwise have been handed two conventions wearing one name.

Also a generator-domain correction, not a narrowing: `node bash -c "..."`
is not an executable line — node takes a script path and `bash` is not one —
so the interpreter context is no longer drawn with a shell wrapper. Leaving
it in had the property demanding recognition of a non-command.

The review's other refutation is NOT actioned, and deliberately: it measured
9 failures, every one a fixture hook dying on `spawnSync /bin/sh EPERM`
inside its own sandbox, with the scanner and property tests passing there.
That is the documented reviewer-sandbox artefact class, not a defect in the
claims. Locally the suite is 29/29, 0 skipped, on three independent seeds.

Census re-derived after the rework: unchanged at 99 candidates / 65 files /
0 unscoped, same per-root split, 1523 tracked .md walked.

* test(#2269): pin the invoker set exactly, and let command modifiers through

Two follow-ons from the review's invoker findings, both verified rather than
reasoned about.

The invoker regex was hand-written and I did not trust it, so I enumerated
it instead of sampling: over every <=2-letter prefix it admits exactly
{sh, ash, bash, csh, dash, fish, ksh, tcsh, yash, zsh} and nothing else.
`ssh` is the near-miss that mattered — admitting it would treat a REMOTE
command as a local shell running the payload — and it is correctly rejected.
Pinned along with cash / josh / publish / wish / rsh.

The mirror of that gap is the prefix side: `time bash -c "..."` really does
run the payload, and the command-position rule introduced one commit ago
stopped at `time`. Command modifiers that pass straight through (time, exec,
nohup, env, command) now skip like the shell keywords do.

Named residual rather than a half-fix: a modifier carrying its OWN flags
(`sudo -u alice bash -c ...`) still stops the search, because skipping
arbitrary flag/value pairs means modelling each modifier's option grammar.
No live instance in the six roots, and the failure direction is a missed
candidate.

Census unchanged: 99 / 65 files / 0 unscoped. Suite 29/29, 0 skipped.

* test(#2269): say http(s):// where the predicate accepts it

An audit of this round's own response comment caught that the fix for the
HTTPS-only overclaim went only into the test's internal comment. The three
strings a contributor actually READS — the offender help, the malformed-
declaration message, and CONTRIBUTING.md — still said `https://` while the
shared predicate is `/#\d+|https?:\/\//` and accepts `http://`.

So the implementation mirrored the sibling lint while the documented
convention stayed narrower than it: exactly the two-conventions-wearing-one-
name problem the mirroring was adopted to avoid, reintroduced in the half a
contributor sees. Both directions were already pinned as tests; only the
prose was wrong.

* test(#2269): reach inside command substitutions, whatever precedes them

`$(gsd_run query commit "docs: x")` scored ZERO candidates in both
directions. `$(` glues to the binary exactly as `(` does, leaving
`$(gsd_run` as one token no command-name test could match — the same
silent false negative the subshell opener produced, on the invocation
idiom the tree uses most.

The strip is widened rather than duplicated, so it covers the shell
invoker test too: one helper, per the rule already stated beside it.

WHAT PRECEDES THE OPENER IS NOT ENUMERATED, and that is the whole of
the design. Keying on an assignment prefix covers most of the live
substitution sites and misses the rest — measured with the file's own
binary predicate rather than a line regex, because two regexes gave two
different totals: 432 tokens whose last `$(` is followed by a real gsd
binary, of which 423 are plain assignments and 9 are not. Those 9 span
three distinct non-assignment prefixes, all live:

for REVIEW_FLAG in $(gsd_run review-lane flags) (x3)
${PLAN_PRE_HOOKS_JSON:-$(gsd_run loop render-hooks plan:pre)} (x3)
`ROADMAP=$(gsd_run ...) -- backtick-glued assignment (x3)

Ordinary text (`pre$(...)`) and an indexed assignment (`A[0]=$(...)`)
glue just as hard. Every such enumeration is one idiom behind the shell,
and every miss is silent, so the rule is positional: strip to the LAST
substitution opener in the token. Whatever preceded it was, by
construction, not the command.

Arithmetic falls out of the same mechanism rather than a special case:
`$((x))` leaves a `(` in front of the name, which no binary carries, so
it is refused — mirroring the shell, which itself needs `$( (` spaced
before it reads a nested subshell there. Pinned as a zero-candidate
case, as is the array literal `arr=(...)`, which contains no `$(` at all.

Scan census re-derived with the file's own primitives: 99 candidates
across 65 files, 0 unscoped — workflows 67 / references 11 / agents 12 /
commands 1 / skills 1 / docs 7. Unchanged, so the widening is
coverage-neutral by measurement: no live `$( ... )` site carries a
commit token.

Discrimination re-run: a planted unscoped substitution in a scan root is
caught and named; its `--files` twin is not flagged.

* test(#2269): stop the -c pass skipping a substitution-captured invoker

Self-found by sweeping the defect class the round's finding names — a
command context glues to the binary token — across every command-name
test rather than only the one that was reported.

`shellDashCPayloads` treats any `VAR=` token as a skippable prefix, on
the reasoning that `FOO=1 bash -c "…"` prefixes the command. But
`V=$(bash -c "…")` IS the command: skipping it walks the invoker search
past the shell onto `-c`, which is not an invoker, so the payload is
never reached and an unscoped commit inside it stays invisible.

An assignment is now skippable only when it carries no substitution, and
the prefix pattern admits the indexed form (`A[0]=`) for the same reason
the strip above does. `FOO=1 bash -c "…"` and the full invoker matrix
are unchanged and still pass, which is what makes this a narrowing of
the skip rather than a removal of it.

No live instance in the six roots — the failure direction is a missed
candidate, which is the direction this file treats as the one it cannot
afford.

* test(#2269): quoting decides the opener, per character not per token

Found by an adversarial pass over this round's own claims, then fixed
again after that pass refuted the first fix.

THE DEFECT. `printf %s '$(gsd_run' query commit fixup` runs no gsd
command: the opener is single-quoted literal text. But tokenization
removes quotes, so the token's value is `$(gsd_run` and the strip read
it as a binary, fabricating an invocation out of a string argument.

THE FIRST FIX WAS WRONG, IN THE UNAFFORDABLE DIRECTION. Recording "did
this token consume a quote" and refusing the strip on it closes the case
above and opens a worse one: a quote need not cover the opener.
`echo "pre"$(gsd_run query commit fixup)` executes, and so does
`$(gsd_""run query commit fixup)`, and a token-wide flag silences the
guard on both. That trades a visible false positive for a silent miss,
which is the trade this file refuses everywhere else.

So the mask is PER CHARACTER: tokenize carries a '0'/'1' string parallel
to each token's value, marking characters that were quoted or
backslash-escaped, and the command-name helpers strip only an opener
whose own characters were bare. Both directions are pinned — the three
quoted-opener shapes score zero, the two mixed-quoting shapes score one.

This also closes the same shape for the subshell opener (`'(gsd_run'`),
which predates this round, and it retires BINARY_LEAD_MARKUP_RE: the
walk it replaced could not consult the mask, and a regex plus a mask
would have been two descriptions of one rule.

NAMED RESIDUAL, unchanged by this and out of scope. `printf %s 'gsd_run'
query commit fixup` still reads as an invocation, because no markup is
stripped there — the token's value simply IS the binary name. Closing it
needs quote provenance carried through the command-shape test, not just
the strip. The direction is a visible false positive with a declared
remedy, not a silent miss.

Scan census unchanged: 99 candidates / 65 files / 0 unscoped.

2026-08-12 20:45:11 -04:00

71 KiB

Raw Blame History

Contributing to GSD Core

Getting Started

# Clone the repo
git clone https://github.com/open-gsd/gsd-core.git
cd gsd-core

# Activate the pinned Node version from .nvmrc
nvm use

# Validate your environment
npm run check:env

# Install dependencies (reproducible, lockfile-driven)
npm ci

# Run tests
npm test

npm ci is required over npm install. It installs exactly what package-lock.json specifies and fails fast if the lockfile is out of sync — this is intentional.

docs/contributing/bootstrap.md is the source of truth for setup. See it for Node version managers other than nvm (fnm, asdf, mise), the environment validator, daily commands, and troubleshooting.

Types of Contributions

GSD accepts three types of contributions. Each type has a different process and a different bar for acceptance. Read this section before opening anything.

🐛 Fix (Bug Report)

A fix corrects something that is broken, crashes, produces wrong output, or behaves contrary to documented behavior.

Process:

Open a Bug Report issue — fill it out completely.
Wait for a maintainer to confirm it is a bug (label: confirmed-bug). For obvious, reproducible bugs this is typically fast.
Fix it. Write a test that would have caught the bug.
Open a PR using the Fix PR template — link the confirmed issue.

Rejection reasons: Not reproducible, works-as-designed, duplicate of an existing issue.

⚡ Enhancement

An enhancement improves an existing feature — better output, faster execution, cleaner UX, expanded edge-case handling. It does not add new commands, new workflows, or new concepts.

The bar: Enhancements must have a scoped written proposal approved by a maintainer before any code is written. A PR for an enhancement will be closed without review if the linked issue does not carry the approved-enhancement label.

Process:

Open an Enhancement issue with the full proposal. The issue template requires: the problem being solved, the concrete benefit, the scope of changes, and alternatives considered.
Wait for maintainer approval. A maintainer must label the issue approved-enhancement before you write a single line of code. Do not open a PR against an unapproved enhancement issue — it will be closed.
Write the code. Keep the scope exactly as approved. If scope creep occurs, comment on the issue and get re-approval before continuing.
Open a PR using the Enhancement PR template — link the approved issue.

Rejection reasons: Issue not labeled approved-enhancement, scope exceeds what was approved, no written proposal, duplicate of existing behavior.

✨ Feature

A feature adds something new — a new command, a new workflow, a new concept, a new integration. Features have the highest bar because they add permanent maintenance burden to a solo-developer tool maintained by a small team.

The bar: Features require a complete written specification approved by a maintainer before any code is written. A PR for a feature will be closed without review if the linked issue does not carry the approved-feature label. Incomplete specs are closed, not revised by maintainers.

Process:

Discuss first — check Discussions to see if the idea has been raised. If it has and was declined, don't open a new issue.
Open a Feature Request issue with the complete spec. The template requires: the solo-developer problem being solved, what is being added, full scope of affected files and systems, user stories, acceptance criteria, and assessment of maintenance burden.
Wait for maintainer approval. A maintainer must label the issue approved-feature before you write a single line of code. Approval is not guaranteed — GSD is intentionally lean and many valid ideas are declined because they conflict with the project's design philosophy.
Write the code. Implement exactly the approved spec. Changes to scope require re-approval.
Open a PR using the Feature PR template — link the approved issue.

Rejection reasons: Issue not labeled approved-feature, spec is incomplete, scope exceeds what was approved, feature conflicts with GSD's solo-developer focus, maintenance burden too high.

📐 Proposing an ADR or PRD

An ADR (Architecture Decision Record) documents a significant architectural decision. A PRD (Product Requirements Document) captures the what and why of a feature before implementation. Both are governed by the same issue-first rule as everything else.

Process:

Open an issue of the appropriate type (enhancement for an ADR revisiting an existing area, feature for a new architectural surface, chore for policy/docs decisions). Fill it out completely.
Wait for maintainer approval. A maintainer must label the issue approved-enhancement, approved-feature, or confirm the chore before any file is created.
The GitHub-assigned issue number becomes your filename prefix. Create the file on a branch named after the issue:
- docs/adr/<issue#>-<slug>.md for ADRs
- docs/prd/<issue#>-<slug>.md for PRDs
- Branch: docs/<issue#>-<slug>
Open a PR using the appropriate template and close the issue with Closes #<issue#> in the PR body.

One issue = one ADR-or-PRD = one PR. Do not batch multiple decisions into one file or one PR.

Do not compute a "next number" locally. Any PR that uses the legacy NNNN-* sequential pattern for a new ADR or PRD will be asked to rename the file to the <issue#>-<slug>.md format before merge.

Example: Issue #2264 was opened, approved, and its number became the prefix: docs/adr/2264-golden-parity-redesign.md.

Rejection reasons: Issue not approved before file was created, filename uses local-compute sequential number instead of issue#, multiple decisions bundled in one PR, file placed in wrong directory (docs/adr/ vs docs/prd/).

The Issue-First Rule — No Exceptions

No code before approval.

For fixes: open the issue, confirm it's a bug, then fix it. For enhancements: open the issue, get approved-enhancement, then code. For features: open the issue, get approved-feature, then code.

PRs that arrive without a properly-labeled linked issue are closed automatically. This is not a bureaucratic hurdle — it protects you from spending time on work that will be rejected, and it protects maintainers from reviewing code for changes that were never agreed to.

Where Do I Open My PR? (Branching Model)

GSD uses two long-lived branches: main (production, what's on npm @latest) and next (integration for the upcoming release). Almost every PR targets next. Full guide: docs/branching.md.

Your branch	PR target	Notes
`feat/NNN-slug`	`next`	Default for all new features
`fix/NNN-slug`	`next`	Default for all bug fixes; ships in next minor or via hotfix cherry-pick
`chore/`, `docs/`, `refactor/`, `test/`, `perf/`, `ci/`, `revert/`	`next`	All routine work
`fix/critical-NNN-slug`	`main`	Production-down emergencies only; auto-back-merges to `next`
`release/X.Y.0`	`main`	Created by `release.yml` — don't make these by hand
`hotfix/X.Y.Z`	`main`	Created by `release.yml` (dispatch with a patch version X.Y.Z) — don't make these by hand
Stabilization PR for an in-flight release	`release/X.Y.0`	Fix a regression found during the RC cycle

Day-to-day commands:

git fetch origin
git checkout next
git pull --ff-only origin next
git checkout -b fix/3187-config-corruption
# ... commit, push
gh pr create --base next --repo open-gsd/gsd-core

If you target the wrong branch by accident, the PR Target Validator workflow will post a comment with the one-line fix (click "Edit" by the PR title and change the base branch — no need to recreate the PR).

Why this matters: Under the old single-branch model, every PR rebased onto main, which moved on every merge. next moves far less often — only when another PR to next lands — so in practice you rebase much less.

But next does still require "up-to-date before merging". Branch protection has required_status_checks.strict = true; check it yourself with gh api repos/open-gsd/gsd-core/branches/next/protection --jq '.required_status_checks.strict'. If another PR lands while yours is open, yours goes BEHIND and must be rebased before it can merge.

Budget for that, because the rebase is not free here: it changes your HEAD sha, which invalidates the sha-bound pass marker the push gate reads, so a rebase means re-running the full remote verification and another CI cycle before the gate clears again. Rebase last — immediately before you push for review — rather than paying for a verification you are about to discard.

Pull Request Guidelines

Architecture & Domain Standards (Maintainer-Defined)

The following files are maintainer-owned coding standards and must be treated as canonical when contributing:

CONTEXT.md — domain language and module naming standards
docs/adr/ — Architecture Decision Records (ADRs) for accepted architectural decisions

Full contributor requirements — including CONTEXT.md format, ADR governance, and AI-agent-assisted work standards — are in docs/contributor-standards.md.

Contributor requirements (summary):

Read CONTEXT.md before naming or refactoring modules/interfaces/seams.
Use CONTEXT.md vocabulary consistently in code comments, tests, issue/PR text, and docs for the touched area.
Check relevant ADRs in docs/adr/ before proposing or implementing architectural changes.
If a change intentionally revisits an ADR decision, call it out explicitly in the linked issue and PR rationale.
Do not rewrite maintainer intent in CONTEXT.md/ADRs as part of drive-by cleanup; propose focused updates tied to approved scope.
If using an AI assistant, prompt it to read CONTEXT.md and the relevant ADRs before writing any code or docs, and verify it used the correct vocabulary before opening the PR.

Every PR must link to an approved issue. PRs without a linked issue are closed without review, no exceptions.

No draft PRs — draft PRs are automatically closed. Only open a PR when it is complete, tested, and ready for review. If your work is not finished, keep it on your local branch until it is.
Use the correct PR template — there are separate templates for Fix, Enhancement, and Feature. Using the wrong template or using the default template for a feature is a rejection reason.
Link with a closing keyword — use Closes #123, Fixes #123, or Resolves #123 in the PR body. The CI check will fail and the PR will be auto-closed if no valid issue reference is found.
- Test-only and docs-only follow-up PRs may reference without closing. If your PR is documentation or regression coverage only — say, a repo-wide guard for a fix that already shipped — and there is no open issue for it to close, use a non-closing reference instead: Refs #123. Ref, Refs, References, Relates to, Related to, and Follow-up to are all accepted in that position. Do not write a closing keyword against an already-closed issue to satisfy the check; on merge it closes nothing, and it trains readers to treat closing keywords as decorative.
- Qualifying diff shape: every changed file must be under tests/, under docs/, or a root-level *.md (README.md, CONTRIBUTING.md, …). This mirrors the doc-only classification the push gate already uses, and it is deliberately root-only — markdown under a subdirectory (gsd-core/workflows/*.md, agents/*.md, commands/**/*.md) is runtime-loaded text, not documentation, so it still requires a closing keyword. CHANGELOG.md is excluded too: edit it through a .changeset/ fragment, never directly.
- This weaker form is accepted only for that diff shape. A PR touching anything else still needs a closing keyword, and a PR with no issue reference at all still fails. On a very large PR (more than 100 changed files) the check cannot confirm the diff shape and falls back to requiring a closing keyword.
One concern per PR — bug fixes, enhancements, and features must be separate PRs
No drive-by formatting — don't reformat code unrelated to your change
Don't bundle test-fixture updates into docs: or unrelated commits — when a production change makes an existing test assertion stale, the test correction MUST land as its own test: (or fix:) commit, not bundled into a docs: commit that also updates the explanation. The release-sdk hotfix cherry-pick filter routes by commit-subject prefix (fix:, chore:, test:); a test-fixture correction packed under a docs: prefix is invisible to the picker and ships a half-state to the hotfix branch — production code changed, test assertion stale. v1.42.3 hit this exact mode (#3621). The fix is upstream: keep the test-fixture commit separate.
CI must pass — all configured matrix jobs must be green. Node 22 remains the compatibility floor; Node 24 is the primary target; Node 26 compatibility must be preserved for code and tests even when a Node 26 CI lane is not yet available.
Scope matches the approved issue — if your PR does more than what the issue describes, the extra changes will be asked to be removed or moved to a new issue

CHANGELOG Entries — Drop a Fragment

Do not edit CHANGELOG.md directly. Two PRs that both append to a ### Fixed block always conflict on merge — git can't pick a serialization order without a human. Instead, every PR with user-facing changes drops a fragment file in .changeset/.

npm run changeset -- --type Fixed --pr <YOUR_PR_NUMBER> \
  --body "**\`/gsd-foo\` no longer drops trailing slashes** — explain the user-visible change."

This writes .changeset/<adjective>-<noun>-<noun>.md. Three random words → concurrent PRs never collide. Allowed type: values follow Keep a Changelog: Added, Changed, Deprecated, Removed, Fixed, Security.

Fragments are consolidated into CHANGELOG.md at release time by the release workflow. See .changeset/README.md for the format spec and #2975 for the rationale.

CI enforcement: the Changeset Required workflow (scripts/changeset/lint.cjs) fails any PR that touches bin/, gsd-core/, src/, agents/, commands/, hooks/, or sdk/src/ without a .changeset/*.md fragment. (src/ is the TypeScript source of truth compiled into gsd-core/bin/lib/*.cjs, so editing it is a user-facing change even though the generated .cjs is gitignored and never appears in the diff.)

Running it locally. The lint derives its changed-file set from GITHUB_BASE_REF, which only CI sets. node scripts/changeset/lint.cjs on a developer machine therefore does not evaluate your branch and can report success on a PR that CI will fail. Pass the base explicitly to reproduce the CI result:
GITHUB_BASE_REF=next node scripts/changeset/lint.cjs
``` The gate also **validates the content** of every changed fragment: a fragment whose frontmatter does not parse (e.g. a `pr: 0` placeholder that was never backfilled to the real PR number) fails the gate with `fail_invalid_fragment`, naming the offending file. This stops a malformed fragment from merging to `next` and only detonating later in the release job's CHANGELOG render.

Opt-out: PRs with no user-facing impact (test refactors, lint config changes, CI tweaks, formatting-only changes) can add the no-changelog label. The lint honors it. When unsure whether a change is user-facing, add the fragment.

Release notes formatting

GitHub release notes are generated automatically. The release and hotfix workflows first create the release with gh release create --generate-notes, then run scripts/release-notes/format-github-release-notes.cjs --apply to rewrite the body into the project's curated format: an Install block, followed by What's Changed grouped into Feature / Enhancement / Fix sections (classified by each PR's conventional-commit title prefix — feat → Feature, fix → Fix, non-user-facing types test/chore/ci/docs/refactor/perf/revert → omitted from the user-facing notes, everything else → Enhancement), then New Contributors and the Full Changelog link.

To re-format an existing release by hand (e.g. backfilling an older release):

node scripts/release-notes/format-github-release-notes.cjs \
  --tag vX.Y.Z --repo open-gsd/gsd-core --apply

Omit --apply to print the reformatted body to stdout for review without publishing.

PR title convention (enforced at open time)

Because the changelog is built from PR titles, your PR title must follow:

type(#<issue>): short summary

Start with the type — feat, fix, or any other conventional type (chore, docs, refactor, …). No leading tags or prefixes: a title like [security] fix(config): … defeats the ^fix bucket anchor and silently files the entry under the wrong changelog section.
Put the linked issue ref in the scope — (#<digits>). This is what renders as a link to the issue in the changelog line. fix(core): … buckets correctly but produces a changelog entry with no issue link.
A breaking-change marker is fine: feat(#42)!: ….

Examples: fix(#1542): roadmap rollback, feat(#39): milestone-prefixed phase IDs, enhance(#1549): add PR-title validator.

CI enforcement: pr-title-validator.yml checks the title on open/edit and fails with the required format if it doesn't conform. It reuses the same matcher the changelog classifier uses (scripts/release-notes/conventional-title.cjs), so a title that passes the check is guaranteed to bucket and link correctly. Fix a flagged title by editing it in place — the check re-runs on edit, no need to recreate the PR.

Documentation Updates — Update the Relevant Docs

If your PR adds, changes, deprecates, or removes user-visible behavior, you must update the relevant documentation in docs/. CI will fail any PR whose changeset fragment is typed Added, Changed, Deprecated, or Removed without also modifying at least one file under docs/ (#3213).

Fixed and Security fragments do not trigger this lint — bug fixes restore documented behavior, they do not introduce new behavior to document. (Edit the docs anyway if a fix corrects something the docs got wrong.)

Which docs to update

Change type	Required doc updates
New command or flag	`docs/COMMANDS.md`, `docs/FEATURES.md`
Changed command behavior or output	`docs/USER-GUIDE.md`, `docs/COMMANDS.md`
Configuration / schema change	`docs/CONFIGURATION.md`
Architectural change	`docs/ARCHITECTURE.md`, `docs/adr/`
Agent or skill change	`docs/AGENTS.md`
Removed command, flag, or workflow	All docs that referenced it

Language policy

All content in docs/ and the root README.md must be written in English. English is the canonical source. The translated READMEs (README.pt-BR.md, README.zh-CN.md, README.ja-JP.md, README.ko-KR.md) are community-maintained translations and do not need to be updated by every PR.

CI enforcement

The Docs Required workflow (scripts/lint-docs-required.cjs) reads the changeset fragments touched in the PR diff. If any has type Added / Changed / Deprecated / Removed, it requires at least one file under docs/ to also appear in the diff.

Opt-outs (with paper trail)

When a change genuinely has no user-facing documentation impact (infrastructure rewrite, internal refactor, test-only addition, CI fix), use one of:

Label: add the no-docs label to the PR. Leave a comment explaining why no docs update was needed.
Per-fragment marker: add  on its own line inside the body of each triggering changeset fragment (typically at the end). The reason is required and must be non-empty — a bare  or  is rejected (no audit trail = no exemption). The marker is extracted at parse time by scripts/changeset/parse.cjs and stripped from the body before the CHANGELOG.md and GitHub release-notes serializers see it — it leaves a paper trail in the source fragment without leaking into published release notes. Inline mentions of the marker syntax (e.g. inside backticks) are intentionally ignored; the parser only acts on a marker that occupies its own line. Both routes leave a paper trail; the label is global, the marker is per-fragment for mixed PRs.

When unsure whether a change is user-facing, update the docs.

Testing Standards

All tests use Node.js built-in test runner (node:test) and assertion library (node:assert). Do not use Jest, Mocha, Chai, or any external test framework.

Suite grouping. Tests live in named suites (unit, integration, install, security, slow) selected by filename suffix: a file named foo.security.test.cjs belongs to the security suite; a file with no suffix (foo.test.cjs) belongs to unit. See docs/TESTING-SUITES.md for the full policy, CI matrix, and per-suite scripts (npm run test:unit, npm run test:security, npm run test:coverage:unit, …). Default npm test still runs every test — backwards compatible.

Required Imports

const { describe, it, test, beforeEach, afterEach, before, after, mock } = require('node:test');
const assert = require('node:assert/strict');

Setup and Cleanup

There are two approved cleanup patterns. Choose the one that fits the situation.

Pattern 1 — Shared fixtures (beforeEach/afterEach): Use when all tests in a describe block share identical setup and teardown. This is the most common case.

// GOOD — shared setup/teardown with hooks
describe('my feature', () => {
  let tmpDir;

  beforeEach(() => {
    tmpDir = createTempProject();
  });

  afterEach(() => {
    cleanup(tmpDir);
  });

  test('does the thing', () => {
    assert.strictEqual(result, expected);
  });
});

Pattern 2 — Per-test cleanup (t.after()): Use when individual tests require unique teardown that differs from other tests in the same block.

// GOOD — per-test cleanup when each test needs different teardown
test('does the thing with a custom setup', (t) => {
  const tmpDir = createTempProject('custom-prefix');
  t.after(() => cleanup(tmpDir));

  assert.strictEqual(result, expected);
});

Never use try/finally inside test bodies. It is verbose, masks test failures, and is not an approved pattern in this project.

// BAD — try/finally inside a test body
test('does the thing', () => {
  const tmpDir = createTempProject();
  try {
    assert.strictEqual(result, expected);
  } finally {
    cleanup(tmpDir); // masks failures — don't do this
  }
});

try/finally is only permitted inside standalone utility or helper functions that have no access to test context.

Use Centralized Test Helpers

Import helpers from tests/helpers.cjs instead of inlining temp directory creation:

const { createTempProject, createTempGitProject, createTempDir, cleanup, runGsdTools } = require('./helpers.cjs');

Helper	Creates	Use When
`createTempProject(prefix?)`	tmpDir with `.planning/phases/`	Testing GSD tools that need planning structure
`createTempGitProject(prefix?)`	Same + git init + initial commit	Testing git-dependent features
`createTempDir(prefix?)`	Bare temp directory	Testing features that don't need `.planning/`
`cleanup(tmpDir)`	Removes directory recursively	Always use in `afterEach`
`runGsdTools(args, cwd, env?)`	Executes gsd-tools.cjs	Testing CLI commands

Spawning a subprocess: use the process seam

Anything that shells out goes through tests/helpers/process-seam.cjs — never a hand-rolled spawnSync/execFileSync in your suite.

const { runNode, runGit, runHook, OUTCOME } = require('./helpers/process-seam.cjs');

const r = runHook(HOOK_PATH, [], { input: JSON.stringify(payload), timeoutMs: 5000 });
assert.equal(r.outcome, OUTCOME.EXITED);
assert.equal(r.exitCode, 0);

Primitive	Spawns
`runNode(argv, opts)`	`process.execPath`
`runGit(argv, opts)`	`git`
`runHook(scriptPath, argv, opts)`	`opts.interpreter` (default `process.execPath`; pass `'bash'` for a shell script)

opts: { cwd, env, input, timeoutMs, killSignal, interpreter }.

Every call returns the same discriminated union — { outcome, exitCode, stdout, stderr, timedOut, signal, killed, code } — and never throws for a child's exit code, a timeout, a buffer overflow, or a spawn failure. All four are data, so you assert on them:

assert.equal(r.outcome, OUTCOME.TIMED_OUT);
assert.equal(r.timedOut, true);

Two rules the seam enforces for you:

Every call is timeout-bounded. timeoutMs defaults to 60s; there is no unbounded path. An unbounded subprocess is an indefinite hang, and it is how macOS CI silently stops reporting.
outcome distinguishes cases that look identical. A timeout and a maxBuffer overflow both report exitCode: null and signal: 'SIGTERM', differing only in code (ETIMEDOUT vs ENOBUFS). Branch on outcome, never on signal.

The seam is not a fault-injection surface — it cannot tell an injected timeout from a genuine bench OOM. Inject faults in-process through a module's deps parameter instead.

Per-suite wrappers are still expected and encouraged: bind your fixture (cwd, env, payload) in a local helper and delegate the spawn to the seam.

Class-norm timeouts live in tests/helpers/timeouts.cjs — PROBE_TIMEOUT_MS, GIT_TIMEOUT_MS, BUILD_TIMEOUT_MS, INSTALL_TIMEOUT_MS. These describe how long a whole CLASS of subprocess call takes (a CLI probe, git plumbing on a fixture repo, a hooks build, a full bin/install.js run), not a single suite's preference, so import them rather than re-declaring the same literal with the same comment in yet another file. Only write a local constant when a site genuinely differs from its class (a real tsc compile, a regen:derived run, ...) — and give that local constant its own justifying comment explaining why it departs from the norm.

When you want git to throw: `gitOrThrow`

runGit never throws — that is the whole point of it. But execSync and execFileSync do throw on a non-zero exit, and a lot of fixture setup relies on that: git commit failing should stop the test right there, not hand back an empty string that produces a baffling assertion failure twenty lines later.

For that case use tests/helpers/git-fixture.cjs:

const { gitOrThrow } = require('./helpers/git-fixture.cjs');

gitOrThrow(['init', '-b', 'main'], { cwd: dir });
gitOrThrow(['commit', '-m', 'seed'], { cwd: dir });   // throws if git exits non-zero
const branch = gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: dir }).trim();

It returns stdout as a string on success. On any non-EXITED outcome, or a non-zero exit, it throws an Error carrying status, exitCode, stdout, stderr, signal, timedOut and outcome as own properties. status and exitCode are deliberate aliases: status is what the legacy execSync idiom reads (catch (err) { assert.equal(err.status, 1) }), so a migrated call site keeps working.

You want	Use
Every outcome as data; you branch on `outcome`	`runGit`
Fixture setup that must abort loudly on failure	`gitOrThrow`

process-seam.cjs itself is untouched by this — it still never throws.

If your per-suite wrapper spawns something that is not git — a node CLI via runNode, a bash snippet via runHook — and its callers depend on a throw, call throwIfFailed(result, displayName) directly instead of hand-rolling the same outcome !== EXITED || exitCode !== 0 check. gitOrThrow is itself just throwIfFailed bound to runGit, so every thrown error — git or not — carries the same status/exitCode/stdout/stderr/signal/timedOut/outcome shape:

const { throwIfFailed } = require('./helpers/git-fixture.cjs');

const r = runNode([BUILD_SCRIPT], { timeoutMs: BUILD_TIMEOUT_MS });
throwIfFailed(r, 'build-hooks.js (before install tests)');

When you want the legacy shape without a throw: `toLegacyResult`

Some call sites never wanted a throw in the first place — they already branch on exit status as data, reading .status/.stdout/.stderr off the result themselves. Those still need the seam's exitCode renamed to the legacy status field their assertions expect. Use toLegacyResult instead of hand-rolling the three-line mapping — ~8 test files did exactly that independently before this export existed (#3147):

const { toLegacyResult } = require('./helpers/git-fixture.cjs');

function runLint(args = []) {
  const r = runNode([LINT_SCRIPT, ...args], { timeoutMs: PROBE_TIMEOUT_MS });
  return toLegacyResult(r); // { status, stdout, stderr }
}

It is a bare mapping and nothing more. If your call site needs an extra field beyond that shape (a parsed-JSON body, a fixture-specific path alongside the result), compose it rather than extending the helper: { ...toLegacyResult(result), extra }. And if your site's return shape genuinely diverges from { status, stdout, stderr } — e.g. it substitutes a parsed report object for raw stdout — leave it as its own local mapping; forcing every result-reshaping helper onto one shared function is the same drift toLegacyResult exists to prevent, just in the other direction.

The lint rule that enforces it

local/no-unbounded-spawn (eslint-rules/no-unbounded-spawn.cjs) fails any spawnSync, execFileSync or execSync under tests/ that is not timeout-bounded. It resolves renamed destructures (const { execSync: exec } = require('node:child_process')) and chained requires (require('node:child_process').execSync(...)), so renaming your way around it does not work.

Two things it deliberately rejects, because both look bounded and are not:

timeout: 0 — Node reads zero as no timeout.
timeout: 999999999 — anything above the 600000 ms ceiling is effectively unbounded. Size the number to what the command actually runs and say why in a comment.

A non-literal value (timeout: GIT_TIMEOUT_MS) is trusted — that is the shape you should be writing.

When a call genuinely needs more than the 600000 ms ceiling — a full installer run, a build plus generators — the escape is an inline marker comment, exactly the // allow-test-rule: <reason> idiom above:

// allow-spawn-timeout-ceiling: regen:derived chains a full build plus eight generators
timeout: 900000,

The reason is required and must be non-empty; a bare // allow-spawn-timeout-ceiling: (or one with only whitespace after the colon) is not an audit trail and still reports timeoutTooLarge. The marker binds only to the call it decorates — either the line immediately above it, or anywhere inside that call's own source range — never to the rest of the file. Critically, the escape only ever raises the ceiling for a call that already resolves to a numeric timeout: it never waives the requirement for a bound. A marked call with no timeout at all still reports unboundedSpawn.

There is no allowlist. eslint-rules/no-unbounded-spawn.allowlist.json grandfathered files that predated the rule; the epic that introduced it (#3064) migrated every site across four waves and deleted the file in its terminal wave (#3148), so local/no-unbounded-spawn now runs with no exemption surface across tests/**. There is no file to add an entry to — fix the timeout at the call site instead. The only sanctioned escapes are an explicit timeout on a raw spawn (for a call shape the process seam cannot express, e.g. a shell: true invocation for npm.cmd on Windows, or stdio redirection to a real fd) and the // allow-spawn-timeout-ceiling: <reason> marker above for a bound over the 600000 ms ceiling. Never reach for eslint-disable on this rule — with the allowlist gone, that is the only remaining way to silence it, and a test asserts that no such comment exists anywhere under tests/.

Test Structure

describe('featureName', () => {
  let tmpDir;

  beforeEach(() => {
    tmpDir = createTempProject();
    // Additional setup specific to this suite
  });

  afterEach(() => {
    cleanup(tmpDir);
  });

  test('handles normal case', () => {
    // Arrange
    // Act
    // Assert
  });

  test('handles edge case', () => {
    // ...
  });

  describe('sub-feature', () => {
    // Nested describes can have their own hooks
    beforeEach(() => {
      // Additional setup for sub-feature
    });

    test('sub-feature works', () => {
      // ...
    });
  });
});

Fixture Data Formatting

Template literals inside test blocks inherit indentation from the surrounding code. This can introduce unexpected leading whitespace that breaks regex anchors and string matching. Construct multi-line fixture strings using array join() instead:

// GOOD — no indentation bleed
const content = [
  'line one',
  'line two',
  'line three',
].join('\n');

// BAD — template literal inherits surrounding indentation
const content = `
  line one
  line two
  line three
`;

QA Matrix Requirements

Happy-path tests are not enough for code that accepts user input, reads project files, writes to disk, shells out, generates artifacts, or builds prompts. New tests for those areas must include adversarial inputs and negative proof that unsafe behavior did not happen.

See TEST-EXAMPLES.md for concrete demo tests that show these requirements in practice.

Standing rule for error/fallback branches: feeding an adversarial input is not sufficient on its own — if the code degrades permissively instead of throwing, the test must assert the specific degraded verdict, not just that the call survived. See TESTING-STANDARDS.md — "Standing rule: assert the degraded verdict".

Use this matrix when it applies to the changed surface:

Happy path
Missing input
Empty input
Whitespace-only input
Malformed input
Out-of-range input
Duplicate or conflicting input
Hostile input
Filesystem failure
Concurrency or retry
Cross-platform path/newline behavior
Regression fixture from the linked issue

You do not need all twelve cases for every PR. You do need to cover the cases that match the risk of the touched code. If a case is not applicable, the PR should make that obvious from the issue scope or test rationale.

CLI and command routing

Changes to CLI parsing, command dispatch, query dispatch, command routers, gsd-tools, or gsd-sdk must include a negative input matrix for the affected command family.

Required cases where relevant:

Missing required arguments
Empty strings, for example --phase ""
Whitespace-only values
Duplicate flags, for example --phase 1 --phase 2
Conflicting flags, for example --json --raw
Malformed assignments, for example --phase= and --phase==1
Unknown subcommands at the touched command depth
Values that look like flags, for example --name --weird
Very long values and Unicode values
Shell metacharacters in values, for example ;, &&, $(), backticks, and quotes

CLI tests must assert on the full command contract:

Exit status
Structured --json result when the command supports JSON
Filesystem mutation or absence of mutation
No stack trace in non-debug failure output
No shell interpolation of attacker-controlled values

Prefer spawnSync(process.execPath, [scriptPath, ...args], { cwd, encoding: 'utf8' }) or execFileSync() with argv arrays. Do not use shell strings for tests that contain hostile values.

Parser and project-file inputs

Changes to markdown, TOML, frontmatter, roadmap, phase, state, config, or schema parsing must include adversarial fixtures. Put reusable fixtures under tests/fixtures/adversarial/ with a directory that names the input type, such as roadmap/, frontmatter/, config/, toml/, or planning-state/.

Required cases where relevant:

Malformed frontmatter
Duplicate keys
Mixed CRLF/LF newlines
Unclosed or nested fenced code blocks
Headings inside fenced code blocks
Unicode headings
Repeated or decimal phase IDs
Path traversal-like names such as ../../x
Null bytes or replacement characters
Huge but bounded files
TOML duplicate tables or trailing garbage
Empty arrays vs missing arrays
Scalars where arrays are expected, and objects where strings are expected

Property-style parser tests are encouraged for high-risk parsers. They must be deterministic: pin the seed, bound the iteration count, and print replay data on failure.

Fixture provenance (#2371)

A gate's fixtures may not be derived from the gate's own writer, grammar, or docstring examples. A negative fixture must come from a source that does not know the gate exists.

This is stricter than the adversarial-input rule above and exists because of it: tests/fixtures/adversarial/ covers hostile input, but a fixture written by the parser's own author — even a deliberately "realistic" one — is still drawn from the author's mental model of the format. It can only ever confirm what the author already believed, never surface what they didn't anticipate. A property-test generator has the same failure mode one level up: seeding the generator from the writer/render function that produces the same format makes the document shape a constant, so the property can never explore a document the writer wouldn't produce (see the document-shaped vs. writer-seeded property tests in tests/api-coverage.test.cjs for a worked example — the writer-seeded one cannot fail against a decoy table; the document-shaped one can).

For a gate whose fixtures come from real user reports, put them under tests/fixtures/representative/<gate>/ with a MANIFEST.json labeling each fixture's source issue and expected gate verdict, and drive them through the gate's real CLI entrypoint (gate-verdict altitude), not the parser function in isolation — see tests/fixtures/representative/README.md and tests/representative-corpus.test.cjs. If the gate is not yet fixed, do not mark the assertion { todo: true } and do not skip it: this repo's test-runner (gsd-test / gsd-test-runner) has no concept of node:test's todo option — its JSONL result parser only recognizes kind: "pass" | "fail", so a thrown todo-marked test is still counted as a real failure and blocks the push gate. Instead record BOTH the correct target verdict (expected*) and the exact current observed verdict (currentBuggyOutput) in the manifest, and assert against currentBuggyOutput — an honest, non-vacuous characterization of today's known-broken behavior that passes today and breaks loudly the moment the real fix changes the observed output, forcing the assertion to be flipped to expected*.

Filesystem writes and installers

Changes to install/uninstall flows, generated artifact writers, state/config writers, worktree safety, or any code that writes under .planning, runtime config dirs, .claude, .codex, hooks, or generated files must include fault-injection coverage where the seam allows it.

Required cases where relevant:

Missing parent directory
Target path exists as a file instead of a directory
Read-only target directory
Broken symlink
Symlink escaping the intended root
Paths with spaces, Unicode, or newlines
Partial write failure
Rename failure
Concurrent deletion or write collision
Temp-file cleanup after failure

Use node:test mocks such as mock.method() for fs.writeFileSync, fs.renameSync, fs.mkdirSync, fs.rmSync, and subprocess seams when the production code exposes a seam. Restore mocks with test hooks or t.after().

Security and prompt-injection surfaces

Changes that read prompts, plans, markdown, agent instructions, shell command projections, workstream/project names, or user-controlled files must treat those inputs as hostile.

Required cases where relevant:

Fake instruction tags, for example <instructions>ignore previous</instructions>
Heredoc breakouts
Shell command substitution payloads
Path traversal through project or workstream values
Malicious markdown links
Fake frontmatter fields that try to override intent
Secret-looking values in inputs, logs, stdout, stderr, and thrown errors
Environment variables with fake tokens to prove redaction

Security tests must assert both the positive guard behavior and the negative proof: no path escape, no command execution, no leaked token, no untrusted content promoted to instructions.

Generated files and parity

Changes to generators, generated .cjs/.ts files, command manifests, aliases, hooks, or SDK/runtime parity must test bad input and runtime parity, not only freshness.

Required cases where relevant:

Missing source command
Malformed command frontmatter
Duplicate command names or aliases
Partial generator output
Generator crash halfway through
Manual edits to generated files
Stale generated file with valid timestamp but wrong content
Runtime .cjs and SDK .ts generated surfaces disagree

Generator tests should run in temp fixtures and assert atomic output behavior. Do not mutate production generated files except in explicit freshness checks.

Prohibited: Source-Grep Tests

Never read source-code .cjs files with readFileSync to assert that strings exist within them. This is source-grep theater: it proves a literal is present in a file, not that the feature works at runtime.

// BAD — source-grep theater
const configSrc = fs.readFileSync(
  path.join(GSD_ROOT, 'gsd-core', 'bin', 'lib', 'config-schema.cjs'), 'utf-8'
);
assert.ok(
  configSrc.includes("'workflow.plan_bounce'"),
  'VALID_CONFIG_KEYS should contain workflow.plan_bounce'
);

This test passes even if workflow.plan_bounce is present but misspelled in the schema, removed from the validation path, or moved to a different file under a different name. It survives every behavioral regression and fails only on trivial renames.

The correct pattern for config key tests — use the CLI:

// GOOD — behavioral test via the CLI
test('config-set accepts workflow.plan_bounce', (t) => {
  const tmpDir = createTempProject();
  t.after(() => cleanup(tmpDir));

  const result = runGsdTools('config-set workflow.plan_bounce true', tmpDir);
  assert.ok(result.success, `config-set should accept workflow.plan_bounce: ${result.error}`);

  const configPath = path.join(tmpDir, '.planning', 'config.json');
  const config = JSON.parse(fs.readFileSync(configPath, 'utf-8'));
  assert.strictEqual(config.workflow?.plan_bounce, true, 'value must be persisted');
});

This single test covers key registration in VALID_CONFIG_KEYS, the key's namespace resolution in KNOWN_TOP_LEVEL, and value persistence — all behaviors that the source-grep test could not touch.

Why this pattern broke at scale: Commit 990c3e64 in this repo updated 5 source-grep tests in one pass when VALID_CONFIG_KEYS moved between files. Zero of those tests were testing behavior. If they had been behavioral tests, the migration would have been invisible.

CI enforcement: The local/no-source-grep ESLint rule (eslint-rules/no-source-grep.cjs, wired in eslint.config.mjs) detects violations. Any test file that calls readFileSync on a .cjs path in a source directory without the exemption annotation below is flagged by npx eslint . (the Lint — ESLint CI step).

Exception: `allow-test-rule: <reason>`

Some tests legitimately read source files. There are six recognized categories:

Reason	When to use
`source-text-is-the-product`	Agent `.md`, workflow `.md`, command `.md` files — their text IS what the runtime loads. Testing text content tests the deployed contract.
`architectural-invariant`	Implementation must use a specific primitive (e.g., `Atomics.wait`, atomic file writes) that cannot be tested by observing outputs.
`structural-regression-guard`	A specific code pattern must (or must not) exist to prevent a class of bug (e.g., regex global-state misuse). Behavioral tests cannot distinguish which pattern was used.
`docs-parity`	A reference doc must stay in sync with source-defined constants (e.g., `CONFIG_DEFAULTS`). The source is the canonical list; there is no runtime API to enumerate it.
`integration-test-input`	A source file is used as a real fixture input to a transformation function under test — the file is not inspected for strings but passed as data.
`structural-implementation-guard`	A feature's interception or wiring point is not reachable end-to-end via `runGsdTools`. Used temporarily until a behavioral path exists.
`pending-migration-to-typed-ir`	Tracked for correction, not exempted. Test was identified by the lint as carrying a raw-text-matching pattern that contradicts the rule above. Each annotated file MUST cite the open migration issue (e.g. `// allow-test-rule: pending-migration-to-typed-ir [#NNNN]`) so the tracking is auditable. New tests cannot use this category — they must refactor production to expose typed IR. The annotation is removed when the test is corrected.

Annotate with a standalone // comment before the file's opening block comment:

// allow-test-rule: architectural-invariant
// state.cjs locking must use Atomics.wait(), not a spin-loop. Behavioral tests
// cannot observe which sleep primitive was chosen — only source inspection can.

/**
 * Regression tests for locking bugs #1909...
 */

The annotation must be a standalone // allow-test-rule: line, not inside a /** */ block comment — the CI linter scans for the pattern // allow-test-rule:.

Prohibited: Raw Text Matching on Test Outputs (file content, stdout, stderr)

Source-grep is not just readFileSync of a .cjs file. The same anti-pattern shows up wherever a test pattern-matches against text that a system-under-test produced, regardless of whether that text came from a source file, a rendered shim, a child process's stdout, or a free-form reason string. All forms are forbidden.

The following are all violations of the same rule:

// BAD — substring match on text written by the code under test
const cmdContent = fs.readFileSync(path.join(tmpDir, 'gsd-sdk.cmd'), 'utf8');
assert.ok(cmdContent.includes(`@node ${jsonQuoted} %*`), '.cmd embeds shim path');

// BAD — regex match on a child process's human-readable stdout formatter
const r = cp.spawnSync(SCRIPT, ['--patches-dir', dir]);
assert.match(r.stdout, /Failures: 1/);
assert.match(r.stdout, /not a regular file/);

// BAD — "structured parser" that hides string ops behind a function wrapper
function parseCmdShim(content) {
  const lines = content.split('\r\n').filter((l) => l.length > 0);
  return { header: lines[0], usesCRLF: content.includes('\r\n') };
}

// BAD — assert.match on a free-form `reason` string from a JSON report
assert.ok(/not a regular file/.test(report.results[0].reason));

Each of these passes on accidental near-matches (a comment containing @node somewhere, a stack trace that happens to say Failures: 1, a mis-typed reason that still contains the substring you're matching) and fails on harmless reformatting (changing Failures: 1 to 1 failure, swapping CRLF rendering style, rewording the error prose).

The rule

Tests assert on typed structured values. If the code under test produces text, the code under test must also expose a structured intermediate representation, and the test must assert on that IR — never on the rendered text.

Concretely: for any system-under-test that produces text output (a file renderer, a CLI formatter, an error-message builder), the production code MUST expose a typed alternative that the test consumes:

Output kind	Required structured surface	What the test asserts on
Rendered file (shim, template, generated code)	A pure builder function returning the IR (`{ invocation, eol, fileNames, render }`)	`triple.invocation.target === expected`, `triple.eol.cmd === '\r\n'`
CLI human-formatter output	A `--json` mode that emits the same data structurally	`report.results[0].reason === REASON.FAIL_INSTALLED_NOT_REGULAR_FILE`
Error / status / reason	A frozen enum (`Object.freeze({ FAIL_X: 'fail_x', ... })`)	`assert.equal(result.reason, REASON.FAIL_X)`
File presence after a write	`fs.statSync().isFile()`, `.size > 0`, `.mtimeMs` advances	Filesystem facts; never read the file content back

Concrete example from this repo

gsd-core/bin/verify-reapply-patches.cjs exposes a frozen REASON enum and emits it through --json. Tests assert report.results[0].reason === REASON.FAIL_USER_LINES_MISSING rather than regex-matching the human-readable prose. The human formatter exists for operator console output only — tests must not depend on it. Adding a new reason code requires updating the REASON enum, the --json output, AND the test that locks Object.keys(REASON).sort() — three coordinated changes that keep the code surface from drifting from the test surface. A pure builder that returns the IR (no I/O) and a writer that consumes it — fs.statSync(target).size === Buffer.byteLength(render()) to prove the writer writes what the renderer produces, without comparing content — is the same pattern applied to rendered files.

Hiding grep behind a function is still grep

parseCmdShim, parsePs1Invocation, etc. that internally do content.split(...), lines[1].trim(), content.includes(...) are still string manipulation. The fact that the entry point looks like a parser doesn't change what's happening underneath — the test is still asserting on the lexical shape of rendered text. The fix is not "wrap the grep in a function with a typed-looking return value." The fix is to eliminate the rendered text from the test path entirely by surfacing the IR.

When you cannot eliminate text matching

There are exactly two cases where text content is the legitimate object of a test, both already covered by the existing exemption matrix:

source-text-is-the-product — workflow .md / agent .md / command .md files where the deployed text IS what the runtime loads.
docs-parity — a reference doc must mirror source-defined constants and there is no runtime enumeration API.

For everything else, if a test reaches for .includes() / .startsWith() / assert.match(text, /…/), the production code is missing a typed surface. Add the typed surface; do not work around it.

CI enforcement: the local/no-source-grep ESLint rule (eslint-rules/no-source-grep.cjs) is being extended (see issue tracker for the latest scope) to flag String#includes/String#startsWith/String#endsWith/assert.match on readFileSync results and on cp.spawnSync stdout/stderr in test files, with the same // allow-test-rule: exemption mechanism.

Node.js Version Compatibility

Node 22 is the minimum supported version. Node 24 is the primary CI target. Node 26 is the forward-compatibility target: do not add tests or production code that depend on deprecated behavior likely to fail there.

Version	Status
Node 22	Minimum required — Active LTS until October 2026, Maintenance LTS until April 2027
Node 24	Primary CI target — current Active LTS, all tests must pass
Node 26	Forward-compatible target — avoid deprecated APIs and exact runtime-error prose

Do not use:

Deprecated APIs
APIs not available in Node 22

Safe to use:

node:test — stable since Node 18, fully featured in 24
describe/it/test — all supported
beforeEach/afterEach/before/after — all supported
t.after() — per-test cleanup
mock.method() — approved for scoped filesystem/subprocess fault injection
t.plan() — fully supported
Snapshot testing — fully supported

Assertions

Use node:assert/strict for strict equality by default:

const assert = require('node:assert/strict');

assert.strictEqual(actual, expected);      // ===
assert.deepStrictEqual(actual, expected);  // deep ===
assert.ok(value);                          // truthy
assert.throws(() => { ... }, /pattern/);   // throws
assert.rejects(async () => { ... });       // async throws

Running Tests

# Run all tests
npm test

# Run a single test file
node --test tests/core.test.cjs

# Run with coverage
npm run test:coverage

For examples of required negative matrices, parser fixtures, filesystem fault injection, security abuse tests, generated-file checks, and runtime/SDK parity tests, see TEST-EXAMPLES.md.

Preferred local benchmark runner (before PR)

When you can, run the local test bench harness before opening a PR — especially for Windows-sensitive changes.

Setup guide: gsd-test-runner getting started
Preferred PR evidence: include the bench results summary (or artifact link) in your PR body.

This gives maintainers a faster, higher-confidence signal than CI-only validation.

Pre-PR Seam Checks (Manifest/Alias Routing)

If you touched src/command-aliases.cts or any of the eight src/*-command-router.cts sources it feeds, run:

npm run check:alias-drift

This verifies the built alias artifacts under gsd-core/bin/lib/ agree with their source of truth — each family's *_SUBCOMMANDS list must match the subcommand values derived from its *_COMMAND_ALIASES table, in order, and each router must reference its own list. The surface is enumerated once in scripts/lib/alias-drift-families.cjs.

Editing shipped content (gsd-core/workflows, references, templates, contexts, agents/, commands/gsd/)

Editing the content of a copied shipped file — a gsd-core/workflows/*.md, an agent, a command definition — requires zero manual fixture regeneration. There is no committed path→hash manifest or per-file size baseline to update by hand; the differential attribution check (tests/emitted-attribution.test.cjs, ADR-2719) computes what your PR changed against next and requires every emitted-artifact hash that moved to be attributable to your diff. If it is not, the check fails and names the paths.

Legitimate cases where emitted bytes move for a reason your diff cannot show directly — a converter change, for example — go through a per-PR fragment under tests/emitted-drift-acks/ (#2914; name the path, say why); see CONTEXT.md's ### Emitted Artifact Provenance entry for the full model. Growth in a gsd-core/workflows/*.md or agents/gsd-*.md file is reported with its exact byte delta and needs the same acknowledgment; the outer tier hard caps in tests/workflow-size-budget.test.cjs / tests/agent-size-budget.test.cjs are unaffected and still apply. The legacy single tests/emitted-drift-ack.json is still read and unioned in for any branch that still carries it, but new acknowledgments go in a NEW fragment, never that file.

You do not need to memorize any of this. The failure output names its own remedy — it tells you to create a new fragment under tests/emitted-drift-acks/ (with a name nobody else is using — include your issue or PR number), which key to use, and prints a minimal valid document you can paste. Note the two key spaces, because the message says which one applies: an unattributable hash ripple is keyed on the emitted path (skills/gsd-add-tests/SKILL.md), while growth is keyed on the bare filename as it appears under gsd-core/workflows/ or agents/ (explore.md). When you remove the last entry from your fragment, delete the fragment file too — its presence is the alarm, so an empty one signals nothing. Nothing here is regenerated: if you find yourself looking for a baseline file to re-run a generator over, that file was deleted by #2724 and is not coming back.

Why fragments, not one file (#2914): every PR needing an acknowledgment used to rewrite tests/emitted-drift-ack.json's paths map wholesale — a single shared mutable file every such PR touches guarantees a merge conflict between any two of them (5 of 6 conflicting PRs in one open queue collided on this file and nothing else), and it means spent, already-merged entries pile up on next. A fragment per PR — the same shape .changeset/ already uses for the identical problem — means two PRs can never conflict on this seam again, and a fragment left on next after merge is inert rather than a shared cell. Two ack sources (two fragments, or a fragment and the legacy file) may never name the same path; that is a hard, loudly-reported error, not a silent last-wins.

tests/emitted-drift-ack.json (the legacy single file, specifically — NOT the fragment directory) must never persist on next (#2914): every entry is scoped to the diff that introduced it, so once merged it is, by definition, already at the base — spent and inert, regardless of shape, and its persistence is what makes it a shared merge-conflict cell. A fragment persisting on next is harmless, since fragments are independently named and cannot conflict with anything, so this guard is deliberately scoped to the legacy file alone. This is enforced only on next itself, by the guard-no-ack-on-next job in .github/workflows/test.yml (push-to-next trigger, scripts/lint-emitted-drift-ack.cjs --guard-next), never as a PR-lane check — a PR-lane "base ack must be absent" check would red every open PR the moment one landed (the #2768 shape #2789 exists to prevent). If you ever see the legacy file present on next, delete it; do not try to make it well-formed.

npm run regen:derived still exists for the artifacts that ARE committed and derived — sync-manifest-versions, the ADR index, the capability matrix, the inventory manifest, the registry, and tests/fixtures/install-tree/*.json (npm run gen:install-tree, the one fixture family ADR-2719 §7 keeps committed, because it conflicts on 0 of 7 and its diffs are readable). Run it after a change to any of those, before committing:

npm run regen:derived

Optional local pre-commit hook entry (Git-native):

.githooks/pre-commit is committed — you do not write it, you only point git at it. It runs check:alias-drift when you stage one of the tracked sources that check reads, and stays silent otherwise.

# one-time setup
git config core.hooksPath .githooks

This is opt-in and stays that way: nothing in npm install sets core.hooksPath for you, so a fresh clone acquires no hooks. To stop using them, git config --unset core.hooksPath.

Do not paste a copy of the hook body into your own .githooks/pre-commit. Bash cannot require() a CommonJS module, so the hook does carry the watched paths as literals — but tests/precommit-alias-drift-hook.test.cjs runs the real hook against every source derived from scripts/lib/alias-drift-families.cjs and fails in both directions: if the hook stops watching a source the checker reads, and if it keeps watching a router the checker dropped. A copy in your own tree has no such test behind it, and a hand-maintained copy is exactly what silently rotted the previous version of this recipe (#2725) — every path in it named the retired sdk/ tree or a gitignored build output, so the guard matched nothing for months.

Optional local pre-push hook to block a private author-email pattern:

.githooks/pre-push is committed too, and is covered by the same core.hooksPath opt-in above. It is a no-op until you set the regex, so enabling hooks does not enable this check:

# set locally in your shell profile (example)
export GSD_BLOCKED_AUTHOR_REGEX='@example-corp\.com$'

With that exported, a push carrying a commit whose author email matches is blocked, and the hook names the offending commits. Unset the variable to disable it.

Every `commit` invocation in shipped content must declare `--files`

tests/commit-files-pathspec.test.cjs scans every .md under gsd-core/workflows/, gsd-core/references/, agents/, commands/, skills/ and docs/ for invocations of the commit seam, and fails if any of them reaches the runtime without a --files scope. An unscoped invocation lands on the blanket-stage default and sweeps the whole .planning/ index into a commit whose message names one artifact — that is #2269, and cmdCommit is a CRITICAL-blast-radius seam, so the guard is repo-wide rather than keyed to the three sites that were reported.

The scan decides what is an invocation by command shape, not by the markup around it — a fenced block, an indented block, a cd … && prefix and a bare line are all scanned alike, because 96 of the live invocations sit inside fences and exempting them would blind the guard to every site the issue was filed about. Two consequences you may hit while editing shipped content, and the failure output names both:

A prose mention that runs into its sentence is flagged. Nothing distinguishes gsd_run query commit followed by ordinary words from an invocation with arguments without guessing at English, so the scan does not try. Write the command reference in backticks — the repo's own convention — and it is correctly read as a mention.

A deliberate wrong-example must declare itself. An example that shows the unscoped form is byte-identical to a regression, so no property of the surrounding markup can stand in for your intent. Declare it on the invocation's own line, in shell-comment position:

gsd_run query commit "docs: message"   # gsd-scan-ignore: #2269 counter-example for the docs

That block is a live example of itself: the invocation above really is unscoped, and it is the declaration — not the fence around it — that keeps the scan quiet.

The reason must name a tracking issue (#NNN) or an http(s):// URL, exactly as ADR-456 requires of the sibling allow-test-rule: marker — an exemption with no ledger never gets revisited. A marker with a free-text reason is reported as a malformed declaration rather than as an unscoped commit, so you are told which of the two problems you actually have. A marker that survives shell tokenization as an argument declares nothing: it reached argv, which means the runtime executed the line.

CI Test Quality Checks

The following checks run on every PR in addition to the test suite:

Job	What it checks	How to pass
`Lint — ESLint`	No source-grep tests (see above), via the `local/no-source-grep` rule	Replace with `runGsdTools()` behavioral tests, or add `// allow-test-rule: <reason>`
`Lint — cross-platform portability`	Windows-portability defects in tests, via `local/no-path-literal-in-assert` (more rules land per ADR-1703) — e.g. a path-returning call asserted against a hardcoded `/`-literal	Normalize the actual: `String(pathFn(...)).replace(/\\/g, '/')`, or structure platform-specific code behind a `process.platform !== 'win32'` guard. No `eslint-disable` — see cross-platform-portability-rules.md

Run locally before pushing: npm run lint (or npx eslint .)

Architecture-Aware Testing Requirements

When work touches architecture, routing, policy, registry assembly, or command semantics:

Write tests against module interfaces and seam behavior, not implementation trivia.
Prefer invariant/contract tests that protect ADR-backed behavior and CONTEXT.md terminology.
Ensure tests validate canonical behavior through the defined seam (for example: structured result contracts, canonical command metadata, and adapter parity), not source-text coupling.
If ADRs define expected behavior, tests should assert those expectations directly.

Test Requirements by Contribution Type

The required tests differ depending on what you are contributing:

Bug Fix: A regression test is required. Write the test first — it must demonstrate the original failure before your fix is applied, then pass after the fix. A PR that fixes a bug without a regression test will be asked to add one. If the bug involves CLI input, parsers, filesystem writes, security/prompt surfaces, generated files, or SDK/runtime parity, the regression test must use the relevant QA matrix above and include negative proof that the bad behavior no longer happens. "Tests pass" does not prove correctness; it proves the bug isn't present in the tests that exist.

Enhancement: Tests covering the enhanced behavior are required. Update any existing tests that test the area you changed. If the enhancement expands accepted input, changes command routing, broadens parser behavior, changes generated output, or touches installer/write paths, add the relevant adversarial cases from the QA matrix above. Do not leave tests that pass but no longer accurately describe the behavior.

Feature: Tests are required for the primary success path and enough failure scenarios to cover the relevant QA matrix above. At minimum, every feature must cover one failure scenario; features that expose CLI input, parse user files, write files, generate artifacts, call subprocesses, or build prompts must cover the relevant negative/hostile cases. Leaving gaps in test coverage for a new feature is a rejection reason.

Behavior Change: If your change modifies existing behavior, the existing tests covering that behavior must be updated or replaced. For high-risk surfaces, update the adversarial tests as well as the happy path. Leaving passing-but-incorrect tests in the suite is not acceptable — a test that passes but asserts the old (now wrong) behavior makes the suite less useful than no test at all.

Reviewer Standards

Reviewers do not rely solely on CI to verify correctness. Before approving a PR, reviewers:

Build locally (npm run build if applicable)
Run the full test suite locally (npm test)
Confirm regression tests exist for bug fixes and that they would fail without the fix
Validate that the implementation matches what the linked issue described — green CI on the wrong implementation is not an approval signal

"Tests pass in CI" is not sufficient for merge. The implementation must correctly solve the problem described in the linked issue.

Code Review Lessons

Input validation: check shape, not just type

Defensive normalization at trust boundaries must validate both the value's type and its semantic shape. A typeof === 'string' check is necessary but insufficient when the field's contract requires a specific format (UUID v4, semver, file path, etc.). See ADR 227 for the architectural standard and concrete cases.

Code Style

CommonJS (.cjs) — the project uses require(), not ESM import
No external dependencies in core — gsd-tools.cjs and all lib files use only Node.js built-ins
Conventional commits — feat:, fix:, docs:, refactor:, test:, ci:. The full grammar is <type>(<scope>): <subject> (enforced by hooks/gsd-validate-commit.sh; subject ≤72 chars, lowercase, imperative mood, no trailing period). When the work resolves a tracked issue, put the issue number in the scope: fix(#1520): randomize mktemp temp paths on BSD/macOS. The same convention applies to PR titles — release notes are grouped by the title's type prefix (feat → Feature, fix → Fix, non-user-facing types omitted, everything else → Enhancement).

File Structure

bin/install.js          — Installer (multi-runtime)
gsd-core/
  bin/lib/              — Core library modules (.cjs)
  workflows/            — Workflow definitions (.md)
                          Large workflows split per progressive-disclosure
                          pattern: workflows/<name>/modes/*.md +
                          workflows/<name>/templates/*. Parent dispatches
                          to mode files. See workflows/discuss-phase/ as
                          the canonical example (the discuss-phase/modes split, #717). New modes for
                          discuss-phase land in
                          workflows/discuss-phase/modes/<mode>.md.
                          Per-file growth is caught by the differential
                          attribution check (tests/emitted-attribution.test.cjs,
                          ADR-2719) — it reports the exact byte delta and
                          requires a per-PR fragment in
                          tests/emitted-drift-acks/ (#2914), no committed
                          snapshot to regenerate. Loose tier
                          hard caps remain in tests/workflow-size-budget.test.cjs.
                          The same applies to agent files (agents/gsd-*.md,
                          tests/agent-size-budget.test.cjs). Full how-to +
                          reference in docs/TESTING-SUITES.md (Workflow &
                          agent size budget); see issue #1074.
  references/           — Reference documentation (.md)
  templates/            — File templates
agents/                 — Agent definitions (.md) — CANONICAL SOURCE
commands/gsd/           — Slash command definitions (.md)
tests/                  — Test files (.test.cjs)
  helpers.cjs           — Shared test utilities
docs/                   — User-facing documentation

Source of truth for agents

Only agents/ at the repo root is tracked by git. The following directories may exist on a developer machine with GSD installed and must not be edited — they are install-sync outputs and will be overwritten:

Path	Gitignored	What it is
`.claude/agents/`	Yes (`.gitignore:9`)	Local Claude Code runtime sync
`.cursor/agents/`	Yes (`.gitignore:12`)	Local Cursor IDE bundle
`.github/agents/gsd-*`	Yes (`.gitignore:37`)	Local CI-surface bundle

If you find that .claude/agents/ has drifted from agents/ (e.g., after a branch change), re-run bin/install.js to re-sync from the canonical source. Always edit agents/ — never the derivative directories.

Security

Path validation — use validatePath() from security.cjs for any user-provided paths
No shell injection — use execFileSync (array args) over execSync (string interpolation)
No ${{ }} in GitHub Actions run: blocks — bind to env: mappings first

71 KiB Raw Blame History

Contributing to GSD Core

Getting Started

Types of Contributions

🐛 Fix (Bug Report)

⚡ Enhancement

✨ Feature

📐 Proposing an ADR or PRD

The Issue-First Rule — No Exceptions

Where Do I Open My PR? (Branching Model)

Pull Request Guidelines

Architecture & Domain Standards (Maintainer-Defined)

CHANGELOG Entries — Drop a Fragment

Release notes formatting

PR title convention (enforced at open time)

Documentation Updates — Update the Relevant Docs

Which docs to update

Language policy

CI enforcement

Opt-outs (with paper trail)

Testing Standards

Required Imports

Setup and Cleanup

Use Centralized Test Helpers

Spawning a subprocess: use the process seam

When you want git to throw: gitOrThrow

When you want the legacy shape without a throw: toLegacyResult

The lint rule that enforces it

Test Structure

Fixture Data Formatting

QA Matrix Requirements

CLI and command routing

Parser and project-file inputs

Fixture provenance (#2371)

Filesystem writes and installers

Security and prompt-injection surfaces

Generated files and parity

Prohibited: Source-Grep Tests

Exception: allow-test-rule: <reason>

Prohibited: Raw Text Matching on Test Outputs (file content, stdout, stderr)

The rule

Concrete example from this repo

Hiding grep behind a function is still grep

When you cannot eliminate text matching

Node.js Version Compatibility

Assertions

Running Tests

Preferred local benchmark runner (before PR)

Pre-PR Seam Checks (Manifest/Alias Routing)

Editing shipped content (gsd-core/workflows, references, templates, contexts, agents/, commands/gsd/)

Every commit invocation in shipped content must declare --files

CI Test Quality Checks

Architecture-Aware Testing Requirements

Test Requirements by Contribution Type

Reviewer Standards

Code Review Lessons

Input validation: check shape, not just type

Code Style

File Structure

Source of truth for agents

Security

71 KiB

Raw Blame History

When you want git to throw: `gitOrThrow`

When you want the legacy shape without a throw: `toLegacyResult`

Exception: `allow-test-rule: <reason>`

Every `commit` invocation in shipped content must declare `--files`