9 Commits

Author	SHA1	Message	Date
sim	21c46ecf52	fix: scope safe.directory ownership bypass to the real-repo-root git ls-files call ... Found while running gsd-test for #3308: tests/commit-files-pathspec.test.cjs's repo-wide `--files` scan runs `git ls-files -z -- *.md` directly against the checked-out repo root (not a createTempGitProject() fixture, unlike every other gitOrThrow call in this file). Inside a container-provisioned test runner the checkout's on-disk owner can legitimately differ from the running UID, tripping git's CVE-2022-24765 dubious-ownership guard and failing the scan closed (exitCode 128) rather than reporting a real file-list result — reproduced on gsd-test's linux-node22 and linux-node24 lanes. Adds `-c safe.directory=*` to that ONE invocation only, so the bypass is scoped to this call rather than a global `git config` write that would leak into every other git call in the process. No source behavior changed; test-infrastructure resilience only.	2026-08-12 22:05:05 -04:00
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: <!-- WRONG: gsd_run query commit "docs: message" (missing --files!) --> 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
Tom Boucher	9341d8b8d3	test(#3334): fold the workflow-dispatch & review-lane fix-* cluster — Wave 2 (#3342) ... Folds 15 tests/fix-*.test.cjs regression files (191 test() blocks) into their module's main suite, per the wave decomposition of #3315 (H3 of epic #3053). 187 blocks land in 8 existing suites (4 exact-duplicate cases dropped, documented inline); 4 blocks move via git mv into 2 new suite files with no prior coverage to merge into. Zero production behavior change. Also tightens two H1 (#3313) ratchets that the fold's own file-count reduction moved past their grace window, per the ratchets' documented dual failure mode (a stale/too-loose baseline fails exactly like a novel violation): - lint-test-file-count.allowlist.json: removes the stale "audit" entry (folding fix-2766 into tests/uat.test.cjs drops that module back to its 2-file cap). - lint-allow-test-rule-refs.ceiling.json: lowers maxFiles 314 -> 309, the real post-fold high-water mark (gsd-test's own repo-baseline test caught this — CI, not a human, found it). Two orthogonal review passes (Standards+Spec code-review, isolated security-review) found and this commit fixes two issues before push: a genuinely-distinct #2287 test case (file-absent vs. file-present- resolved) that a prior fold pass had wrongly dropped as a duplicate — restored verbatim into tests/uat.test.cjs; and a missing same-line allow-test-rule citation on the #2196 block in tests/debug-session-management.test.cjs, added for consistency with its sibling #2257 block. lint-removed-but-needed also caught two stale doc references to the now-folded-away fix-2285-claude-orchestration-wiring.test.cjs filename (docs/adr/1143-claude-orchestration-capability.md, gsd-core/references/execute-phase-response-language.md) — updated both to point at tests/claude-orchestration.test.cjs, its new home. Co-authored-by: sim <sim@local>	2026-08-10 20:51:40 -04:00
Tom Boucher	3fac6e629f	test(#3145): bound the installer/runtime cluster onto the process seam (#3176) ... * test(#3145): bound the installer/runtime cluster onto the process seam Migrates 156 unbounded sync spawn sites across 47 files. Allowlist 120 to 73. Timeouts are sized from evidence already in the tree rather than a house default, because this wave spawns installers rather than git plumbing and an undersized bound does not catch a hang -- it manufactures CI flake, which is worse, since a flake gets re-run instead of investigated. install.test.cjs records a real spawnSync ETIMEDOUT at a 60000ms cap on a loaded bench while another lane passed the same commit in 12.7s, so full installs are bound at 120000ms against that recorded incident. Also adds an auditable escape to the guard's timeout ceiling. The 600000ms cap was set in #3143 from partial evidence, but fragment-single-edit- propagation carries a documented, load-tested 900000ms bound on a run that chains a full build plus eight generators -- the guard would have rejected a correct timeout the moment that file left the allowlist. A value above the ceiling is now permitted only with an inline allow-spawn-timeout-ceiling marker carrying a non-empty reason. It raises the ceiling; it never waives the requirement for a bound, which is asserted directly. install-shared.cjs keeps its hand-rolled assert rather than routing through throwIfFailed: its message embeds both streams, and throwIfFailed carries only a trimmed stderr. The message now also names the outcome, so a bounded timeout reads as such across its 38 importers instead of as expected null to equal 0. * test(#3145): extract class-norm timeouts and correct the build-hooks sizing A pre-PR review found 52 copies of four class-norm timeout constants across this wave. These are not per-suite fixture bindings -- they are shared facts about how long a class of subprocess takes, derived from a recorded bench incident. That norm already moved once (60000 to 120000 after a real ETIMEDOUT), and 52 copies would have drifted the next time it moved. Extracts tests/helpers/timeouts.cjs, where each norm is justified once, and converts the copies. A site that genuinely differs -- a real tsc compile, or regen:derived -- keeps its own local constant with its own justification. Also corrects a misclassification: scripts/build-hooks.js was sized as a build at 120000 in twelve places and 60000 in another, but it compiles and bundles nothing. Its own header says no bundling needed; it copies pre-built files and syntax-checks them with vm. Three different values bounded one script; now there is one. * test(#3145): fix red CI — lint self-match and a Windows chunk overrun Two failures on PR 3176. lint-allow-test-rule-refs read a RuleTester fixture as a real exemption. The fixture exists to prove an unrelated marker does NOT suppress the rule, so it carries that marker's literal text as test data. Split via concatenation, the same idiom no-unbounded-spawn-allowlist.test.cjs already uses for its own self-match problem. The explanatory comment needed the same treatment. The Windows shard 3/3 chunk was killed at its 600000ms budget. Output stopped seven minutes before the kill, so this was an overrun rather than a slow chunk: regenDerivedPropagatesSingleFragmentEditWithNoSecondSourceSurface runs regen:derived bounded at 900000ms, which is larger than the whole chunk budget, so the chunk killer always fires first and it can never complete there. Both the test and that bound predate this change; modifying the file pulled it into the Windows targeted set and exposed it. Skipped on Windows with the reason recorded; the Linux lanes cover it. The 900000 bound and its ceiling marker are unchanged -- they are correct. * test(#3145): refresh the stale test-timings cost table The Windows shard was killed at its 600000ms per-chunk budget. run-tests.cjs packs chunks by measured duration from tests/test-timings.json, and an unknown file falls back to the table's median weight -- advisory by design, but it silently underweights exactly the files that matter. Four of the failing chunk's 22 files were absent from the table, including the two heaviest: fragment-single-edit-propagation.install.test.cjs at 230s (it runs regen:derived) and agent-fragments-emission.install.test.cjs at 79s. Both were weighted as average, so the chunk's total weight read 53.68 against a budget of 60 and the packer produced a single chunk. Regenerated from a passing full-suite run, per the remedy the script itself documents. 700 to 770 entries, 70 added, 0 dropped -- verified, since gen-test-timings.cjs replaces the table wholesale rather than merging. Proven against the real packer: the same 22 files now weigh 103.91 and split into two chunks. No logic, budget, or timeout was changed; raising a budget to make a red gate pass is not a fix. --------- Co-authored-by: sim <sim@local>	2026-08-07 15:18:18 -04:00
Tom Boucher	1d208e5af6	test(#3144): bound the git/worktree cluster onto the process seam (#3152) ... * test(#3144): bound the git/worktree cluster onto the process seam Migrates 180 unbounded sync spawn sites across 19 files. Every previously unbounded call now carries an explicit timeout with a comment giving the number and why. The migration is not a callee swap. execSync and execFileSync throw on a non-zero exit and the seam never does, so each site was classified first: sites that rely on the throw route to gitOrThrow, and sites that already read .status to detect an EXPECTED non-zero -- an intended cherry-pick conflict, a rev-parse outside a repo driving a skip -- route to the never-throwing runGit instead, which would otherwise throw on exactly the exit being probed for. Two same-named git() helpers in worktree-cleanup.test.cjs have different return contracts, one trimmed and one raw; both are preserved rather than unified. Collapses five hand-rolled throw wrappers onto one throwIfFailed in git-fixture.cjs, which gitOrThrow now also uses so the shape cannot drift. Allowlist drops 139 to 120; BASELINE lowered to match. * test(#3144): fix pre-PR review findings Documents throwIfFailed in the CONTEXT.md glossary and CONTRIBUTING.md -- it became the shared throw mechanism without either doc naming it. Routes the sixth and seventh hand-rolled copies of the throw shape through throwIfFailed (worktree-baseref-install, worktree-safety-reap); the first consolidation missed both. Converts ci-rebase-check's 8 fixture-setup calls from unchecked runGit to gitOrThrow so a failed setup step aborts where it fails rather than surfacing later as a confusing failure against the wrong subject. Adds 12 direct unit tests for throwIfFailed, which until now was only exercised transitively. Splits verify.test.cjs's non-git grep/sed bound off GIT_TIMEOUT_MS. --------- Co-authored-by: sim <sim@local>	2026-08-07 10:58:34 -04:00
Tom Boucher	28e486faf7	fix(#2608): fail closed when git add fails during commit staging (#2693) ... * fix(#2608): fail closed when `git add` fails during commit staging `cmdCommit` ignored `git add` failures. #2523 had already stopped a failed path entering the commit pathspec, but skipping it silently left two bad outcomes, both reproduced against the pre-fix build: - SOME paths fail -> `{"committed":true}`. `git commit` still ran and PARTIALLY committed the subset that happened to stage, under a message describing the full requested scope. - EVERY path fails -> `{"reason":"nothing_to_commit"}`, which is not what happened and points the operator nowhere. In both cases git's original `add` stderr was discarded, so the user saw a downstream `commit_failed` / pathspec error naming an innocent file — the symptom reported in the issue from a linked worktree whose git directory was outside the managed writable root. Staging failures are now collected and the command fails closed BEFORE `git commit` runs, returning the issue's specified shape: { committed: false, hash: null, reason: "staging_failed", file: "<first failing path>", error: "<original git add stderr>", failures: [ { file, error, timed_out }, ... ] } A timeout is distinguished as `staging_timeout` (issue AC5) using the projection's SIGTERM+ETIMEDOUT signal — the same idiom worktree-safety.cts uses. The check is placed ahead of the `nothing_to_commit` branch so an all-paths-failed run reports the staging cause rather than an empty changeset. Unchanged: successful staging still commits exactly the declared scope and leaves unrelated staged files alone; an explicitly-named file that does not exist is still skipped rather than staged as a deletion (#2014/#2523), and a request where every named file is missing still reports `nothing_to_commit` — no `git add` ran, so there is no staging failure to report. Regression tests inject the failure by monkeypatching `execGit` on the projection module (per CLAUDE.md, over `chmod 0o000`, which does not fault under root and would make the tests vacuous), driven in a `node -e` child because `output()` writes via `fs.writeSync(1, …)` and cannot be captured in-process. Pre-fix, 6 of the 10 assertions fail; post-fix all pass. Closes #2608 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * fix(#2608): roll back the index, guard the sibling surfaces, document the new reasons Six findings from the orthogonal review of the first commit, all fixed here. 1. A `staging_failed` return left the index PARTIALLY STAGED. The paths that did stage stayed in the index with no commit made and no cleanup, so the next bare `git commit` would sweep them up — the same silent partial commit this fix exists to prevent, deferred one step. (Pre-fix the partial state at least got consumed by the incorrect commit.) The staging failure path now resets the paths it staged, matching cmdPrSubrepo's established rollback-then-error convention. The reset is scoped to what THIS call staged — paths the caller had already staged are captured up front and excluded, so a caller's own work is never destroyed — and is best-effort, since an unwritable index (the very failure being reported) cannot be reset either. 2. `cmdCommitToSubrepo` still had the identical defect: a failed `git add` was dropped silently and the function committed the subset that happened to stage, discarding git's stderr. It now fails closed per sub-repo with the same staging_failed/staging_timeout reasons and the same scoped rollback. 3. The `git rm --cached --ignore-unmatch` branch (default mode, for a planning file that no longer exists on disk) still discarded its result. It mutates the index exactly like `git add`, and `--ignore-unmatch` already makes "no such path" a success, so a non-zero exit there is a real I/O failure — now routed through the same staging-failure path. 4. `agents/gsd-executor.md` documented the commit envelope as an exhaustive three-shape enum and pattern-matched only `nothing_to_commit | commit_failed`. It is the sole consumer doc for this surface, so the new reasons are added with explicit guidance not to retry (a retry hits the same unwritable index), and the "one of three shapes" framing is corrected. 5. The default (non---files) staging path and `--amend` are now covered by tests. Both were already guarded by the first commit but unexercised. 6. The changeset framed the fix as `--files`-only; it applies to default and sub-repo commits too, and now mentions the rollback. Regenerated the agent size baseline and the 18 golden install-parity fixtures for the gsd-executor.md edit. 16 assertions across both surfaces verified against the built lib. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * test(#2608): update the #2523 out-of-repo contract to the new staging_failed reason The remote test run surfaced this: `#2523: out-of-repo --files path is rejected by git` asserted `reason: 'nothing_to_commit'`, and now gets `staging_failed`. This is a deliberate contract improvement, not a papered-over failure. The old reason existed only because a failed `git add` was skipped and the resulting empty `stagedPaths` fell through to the empty-changeset branch. But "nothing to commit" is not what happened — the caller named a file and git refused it — and that misreport is exactly the class of defect #2608 closes. The result now carries the offending path and git's own message ("… is outside repository at …"), which is strictly more actionable for the same condition. #2523's two substantive invariants are untouched and still asserted: no commit is created, and the index is left clean. Two assertions are ADDED (the path is named, git's message is preserved) so the richer contract is pinned rather than merely allowed. Per CONTRIBUTING, a stale-test correction rides its own commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * fix(#2608): compact the executor doc addition to stay under the agent LARGE cap The remote test run failed: `gsd-executor.md is 49217 bytes — exceeds the LARGE hard cap of 49152`. The file was already at 48596 (556 bytes of headroom) and the new commit-envelope documentation pushed it 65 bytes over. The cap is a red line, not a budget to raise, so the addition is compacted rather than the cap moved: four lines instead of eight, keeping the load-bearing facts — the two new reasons, that nothing was committed and the index was rolled back, that `file` + `error` should be surfaced, and that retrying is wrong because a retry hits the same cause. Dropped only the restatement of the linked-worktree example (already in the changeset and PR) and the `failures[]` field (a superset of `file`/`error`, discoverable from the payload). Net addition is now 276 bytes; the file sits at 48872 with 280 bytes of headroom. Extracting the agent's shared boilerplate to references/ would buy much more, but that is a restructuring of the executor agent and does not belong in a commit-staging bugfix. Agent size baseline and the golden install-parity fixtures regenerated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * chore(#2608): backfill changeset PR number (#2693) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 02:45:35 -04:00
Tom Boucher	6ee4349272	fix(#2537): extract offer_next step to references/ (~3.3KB headroom restored) (#2642) ... * fix(#2537): extract offer_next step to references/ (~3.3KB headroom restored) * chore(#2537): backfill changeset pr to 2642	2026-07-25 05:30:20 -04:00
Tom Boucher	e4dd0cbdd5	fix(#2523): normalize --files to repo-relative; reject out-of-repo; gate push on git-add exit (#2638) ... * test(#2523): absolute + mixed + out-of-repo --files paths * fix(#2523): normalize --files to repo-relative; reject out-of-repo; gate push on git-add exit * chore(#2523): backfill changeset pr to 2638	2026-07-25 01:50:36 -04:00
Tom Boucher	4bb846b67a	fix(#2112): scope commit to --files pathspec, not entire index (#2148) ... * fix(2112): scope commit to --files pathspec, not entire index cmdCommit/cmdCommitToSubrepo/cmdPrSubrepo staged exactly the files named in --files but then ran a bare 'git commit' with no pathspec, absorbing anything else in the index into a commit whose message described only the named files (#2112). Fix: append '-- ...stagedPaths' to the commit args when the caller declared a scope. Three guards are load-bearing: - stagedPaths (not filesToStage) excludes skipped missing files (#2014) - explicitFiles gate keeps the default .planning/ path byte-identical - MERGE_HEAD check via 'git rev-parse' falls back to bare commit during merge - --amend is left without pathspec (different operation) cmdPrSubrepo pathspec uses changedFiles (old+new for renames) so the full rename is captured atomically. Also fixes workflow markdown in spec-phase.md and add-tests.md. All-files-missing now short-circuits to nothing_to_commit instead of absorbing the entire index under a message describing files that were not committed. * docs(changeset): backfill PR number (#2148) * test: update golden-install-parity fixtures for workflow markdown changes (#2112) * test: update golden fixtures + workflow baselines for #2112 changes - claude-local.json golden fixture (now generated via gen script) - workflow-size-baseline.json (add-tests.md +16, spec-phase.md +42 bytes) - Extended gen-golden-install-parity-zcode.cjs to also regenerate the claude local-layout fixture	2026-07-13 00:21:46 -04:00