Three parallel diagnostic agents (G: sdk/install path-sep, H: hook scripts,
I: worktree/workspace) characterised the remaining windows-22 failures on
23b52f1a. Patches by class:
CRLF in test parsers / file-content reads
- bug-2136-sh-hook-version: shebang check split('\n') → split(/\r?\n/)
- bug-3542-executor-git-stash-prohibition: strip \r from read content
(git rewrites stashed text with CRLF on win autocrlf=true checkout)
- workspace: BOM-strip + explicit \r strip in parseCommandFile (BOM at
byte 0 defeats /^---/ anchor → fmMatch null → fm.name undefined)
Windows path-separator / 8.3-shortname normalization
- bug-3491-nested-git-worktree: use fs.realpathSync.native to expand
%TEMP% RUNNER~1 → runneradmin; normalize sep before path-equality
- prune-orphaned-worktrees: normalize \\→/ before substring includes
(git emits forward-slash in --porcelain on Windows even when
path.join produced backslashes)
- bug-3017-codex-hook-absolute-node: accept POSIX path OR path with
drive-letter prefix in hookPath equality assertion
- bug-3126-global-skills-base-runtime-path: use path.join for expected
/xdg/<runtime> values (production calls path.join → \xdg\… on win32)
Test under-specified platform / forgot win32 env
- bug-2979-hook-absolute-node: pass {platform:'linux'} to
buildHookCommand + rewriteLegacyManagedNodeHookCommands so the
POSIX-branch tests don't pick up the #3393 GitBash code path
- bug-3288-model-catalog + bug-3571-config-manifest: also set
USERPROFILE alongside HOME so os.homedir() on win32 redirects to
the test fixture instead of the runner's real ~
External-cmd resolution
- bug-2647-outer-tarball-sdk-dist: use npm.cmd + {shell:true} on win32
so execFileSync resolves PATHEXT (literal `npm` is ENOENT)
ESM loader: tests/runtime-bridge-sync-smoke.test.cjs already migrated to
pathToFileURL in 23b52f1a (cluster E).
Explicit per-test/describe skip on win32 (with required string reasons
to satisfy no-unconditional-win32-skip guard)
- feat-3347-graphify-auto-update-hook: 3 describes — harness spawns
bash/kill/sleep + the hook itself is bash
- feat-3595-fs-fault-injection: move \t and \n filenames into the
POSIX-only branch (NTFS forbids 0x00–0x1F in filenames)
- bug-2775/2829/3033/3231/3359: POSIX shim under ~/.local/bin with
chmod 0o755 — not how Windows install works
- bug-3211: single subtest where cp.execSync reassignment isn't
picked up on win32 (POSIX coverage via the mock; live windows
behavior covered by 3211-D which keeps running)
- install-path-detection: parses sh-style export PATH= rc files;
Windows has no rc files (registry Path)
- worktree-safety-policy: single test using POSIX /repo/wt fixture
paths that can't be expressed under win32 path.resolve
Validated: plex2 (ubuntu docker) 11224/0 pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six independent windows-only failure clusters identified from the
windows-22 CI log on 1be0e4e2:
- scripts/command-contract-helpers.cjs: parseFrontmatter split on `\n`
→ on Windows checkout (autocrlf=true) every line carries trailing \r,
lines.indexOf('---', 1) returns -1, all fields read as missing. Drove
the bulk of three "67 subtests failed" suite-level errors covering
workstreams.md, workspace.md, verify-work.md, add-tests.md, etc.
Fix: split(/\r?\n/).
- tests/enh-3271-sdk-adr-structure.test.cjs: same CRLF pattern — H2
captures pulled '\r' into headings like "decision\r", breaking
equality checks on "## Decision" / "## Consequences". Fix: same.
- tests/runtime-bridge-sync-smoke.test.cjs: await import(BRIDGE_PATH)
passed a Windows absolute path to Node's ESM loader, which rejects
with "Only URLs with a scheme in: file, data, and node are
supported." Fix: wrap once at module scope with pathToFileURL.
pathToFileURL is a no-op for POSIX absolute paths.
- tests/bug-2957-claude-global-postinstall-message.test.cjs: hardcoded
'/tmp/gsd-test-settings.json' resolved to D:\tmp\... on Windows
where the parent dir doesn't exist → ENOENT on fs.writeFileSync
inside finishInstall. Fix: os.tmpdir() + pid suffix.
- tests/bug-2774-worktree-cleanup-workspace-safety.test.cjs: the
"while/read loop" subtest and the "end-to-end against real git
worktrees" describe both assert POSIX shell behavior (process
substitution `< <(...)`, RUNNER~1 8.3-shortname mismatch). Skip on
win32 with explicit reasons (satisfies the
no-unconditional-win32-skip guard).
- tests/bug-2838-summary-rescue-gitignored-planning.test.cjs: entire
describe extracts bash rescue blocks from workflow .md files and
runs them; the shell contract itself is the test's point. Skip on
win32 with reason.
- tests/helpers.cjs cleanup(): bumped rmSync retry budget from
10×100ms to 20×250ms — 1s wasn't enough for Windows Defender's
deferred handle release; bumping to 5s should absorb the residual
EBUSY failures observed in bug-1736 / bug-2248 / bug-2698 after the
first retry bump landed in 1be0e4e2.
Validated: holodeck (ubuntu docker) 11224/0 pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bug-2801, bug-3072, bug-3321 each read .md workflow files from disk and
parse fenced code blocks with /\`\`\`bash\n(...)\`\`\`/ — on a Windows
checkout with autocrlf=true the file content is CRLF, the regex never
matches, code blocks come back empty, and the assertion fails with
"expected bash code blocks in workflow" (and similar).
Fix: accept optional \r before each literal \n in the fence delimiter
and split lines on /\r?\n/. Same pattern bug-2839 had with its local
parseFrontmatter copy (already migrated to the CRLF-aware shared helper
in 1be0e4e2). bug-2995 + tests/security-scan.test.cjs also match this
grep but use \n in literal-string fixtures (not file-content regexes);
not affected.
Validated: holodeck (ubuntu docker) 11224/0 pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two windows-only failure clusters surfaced when the chunking fix in
52f23ac0 made Windows tests actually run:
1) Cleanup EBUSY race. On Windows, fs.rmSync without {maxRetries,retryDelay}
races against AV scanners / file-indexers / just-exited child processes
that still hold handles when teardown fires. Surfaces as
EBUSY: resource busy or locked, rmdir 'C:\Users\...\AppData\Local\Temp\gsd-...'
in bug-1736, bug-2248, bug-2698, bug-2838, and every test routing through
the shared tests/helpers.cjs cleanup() (≈170 consumers — bug-2774 et al).
Fix: add {maxRetries: 10, retryDelay: 100} to the shared helper plus the
four inline cleanup sites. On POSIX the retry loop is dead code (first
try succeeds), so no impact on ubuntu/macos.
2) bug-2839 had a local parseFrontmatter that anchored on /^---\n/ — on a
Windows checkout with autocrlf=true the file content is CRLF, the regex
never matches, the function returns null, and the before() hook asserts
"agent must have YAML frontmatter" → entire suite cancels. The shared
tests/helpers.cjs parseFrontmatter is already CRLF-aware (split /\r?\n/);
switch to it.
Validated: plex2 (ubuntu docker) 11224/0 pass post-patch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The hook double-forked the rebuild subprocess and returned before the
subprocess wrote .planning/graphs/.rebuild.lock. Callers (notably the
feat-3347 test cleanup) waited for the lock to disappear before
rm -rf'ing the tmpdir, but an absent lock was ambiguous: it could mean
"subprocess finished and trapped lock removal" OR "subprocess hasn't
started yet." Under ubuntu CI load the second case won, cleanup raced
ahead, and rmSync walked into .planning/graphs while the subprocess
was still creating files — surfacing as
ENOTEMPTY: directory not empty, rmdir '/tmp/gsd-3347-*/.planning/graphs'
on the "dispatches on: git commit -m fix" test.
Spawn the rebuild as a regular backgrounded job, capture $!, and write
the lock file synchronously in the parent before exit. Lock-presence
is now a reliable in-flight signal; the rebuild script's existing
trap-on-EXIT rm still owns cleanup.
Validated: holodeck (ubuntu docker) 11224 pass / 0 fail.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two CodeRabbit threads from PR #3649 review:
- docs/TESTING-SUITES.md:75 — removed the "stable message substring"
fallback from the error-assertion guidance. Project rule (per the
no-source-grep lint and lint-no-source-grep.cjs) is structured/typed
checks only — err.code, JSON fields, enums. Substring matching
re-introduces the exact prose-coupling we banned.
- scripts/run-tests.cjs:106 — the "no test files found" error now
reports the resolved testDir variable instead of the hardcoded
'tests/' string, so when GSD_TEST_DIR points elsewhere the message
names the actual directory the harness searched.
Local: docker gsd-test-summary 11224/0 on holodeck.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Windows CreateProcess caps lpCommandLine at 32,767 chars. The original
`execFileSync(node, ['--test', ...546 paths])` exceeded that on every
Windows runner and exited within ~70ms with no test output. Linux/macOS
allow ~2 MB ARG_MAX so the same call worked there.
`scripts/run-tests.cjs` now splits selected files into chunks that keep
each spawn's argv under 28,000 chars (operator-overridable via
RUN_TESTS_MAX_CMDLINE_CHARS), runs them sequentially, and reports the
first non-zero exit. Cross-platform regression test forces chunking with
a low ceiling and asserts the `run-tests: chunk N/M …` stderr marker.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
scripts/run-tests.cjs gains `--suite <name>` filtering using a filename
suffix convention (`*.security.test.cjs`, `*.integration.test.cjs`, …).
Files with no marker are `unit` (the default fast lane); files with a
marker land in the matching suite. No `--suite` flag preserves the prior
behavior of running every test (backcompat for `npm test` and
`npm run test:coverage`).
New package scripts wire the suites to stable entrypoints:
test:unit, test:integration, test:install, test:security, test:slow,
test:coverage:unit, test:coverage:all. Unknown suite → exit 2 with the
list of valid suites; empty suite → exit 0 with a stderr notice so empty
lanes (e.g. `security` before adversarial tests land) don't gate CI.
CI matrix grows from `ubuntu × {22,24}` + a single macOS lane to
`{ubuntu, macos, windows} × {22, 24, 26}`. `fail-fast: false` so one
lane failure doesn't cancel siblings. Node 26 is `continue-on-error`
until actions/setup-node stabilises that image. PR CI runs unit +
integration + security on every cell; `install` and `slow` only on
`main` push. A dedicated `coverage` job runs `test:coverage:unit` on
ubuntu/Node 24 and uploads the report.
Grouping policy lives in docs/TESTING-SUITES.md with a pointer from
CONTRIBUTING.md. New harness test covers arg parsing, filter selection,
empty-suite behavior, and failure propagation.
Closes#3597.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(3595): filesystem fault-injection for platformWriteSync atomic-write seam
Per CONTRIBUTING.md §"QA Matrix Requirements / Filesystem writes and
installers", adds adversarial coverage against the canonical write
seam every CJS config/state/generated-artifact writer routes through —
`platformWriteSync` in get-shit-done/bin/lib/shell-command-projection.cjs.
Fault matrix exercised via node:test mock.method() on real fs seams,
with t.after() restoring mocks so failures don't leak between tests:
- happy path baseline (atomicity + no orphan tmp file)
- renameSync EXDEV → fallback path writes directly, tmp cleaned up
- tmp writeFileSync ENOSPC → fallback writes directly
- both tmp and fallback fail → fallback error propagates (PINNED:
original cause is swallowed; open follow-up for .cause chaining)
- mkdirSync EACCES → escapes unhandled (PINNED current behavior)
- target path is an existing directory → typed errno code surfaces
AND the directory is preserved
- paths with spaces / Unicode / tabs / newlines (POSIX only for \n)
- 25 sequential writes leave 0 tmp orphans (cleanup invariant)
- platformEnsureDir is idempotent (no EEXIST throw)
- platformEnsureDir EACCES propagates
Symlink-safety invariants (security-critical):
- REPLACES a symlink with a regular file rather than following it —
a planted symlink in .planning/ pointing at ~/.ssh/authorized_keys
is NOT clobbered. Test pins this so a future refactor to
fs.writeFileSync (which follows symlinks) is a visible regression.
- Broken symlinks are replaced with the intended regular file.
Concurrent-write collision: a renameSync EBUSY on the in-flight write
must still produce a parseable, complete final file via the fallback —
never a half-written corruption.
13 new tests; total 192/192 pass when bundled with the pre-existing
state/config/worktree-safety suites (179 of theirs). Zero production
code changes — test-only PR.
Two known-open gaps deliberately NOT fixed in this PR (warrant separate
focused issues):
- platformWriteSync fallback path swallows the original tmp-write
error. Operator only sees the fallback's error when both fail.
- platformWriteSync mkdirSync(dirname, recursive:true) error
escapes without context — no message-wrapping or typed reason.
Closes#3595
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(3595): use t.skip() for Win32 symlink gates + fix rename mock delegation
Two Codex review findings addressed:
1. Symlink tests previously used `if (process.platform === 'win32') return`
which CI reports as PASS even though the test ran zero assertions.
Replaced with `t.skip('symlinks on Win32 need admin'); return;` so
CI correctly reports SKIPPED on Windows lanes. Applied to both the
symlink-replace and broken-symlink tests.
2. The concurrent-collision test's rename mock referenced a
non-existent `fs.renameSync.wrapped` property in its fallback
branch — that path would silently no-op instead of delegating to
the real renameSync. Capture the real `fs.renameSync` BEFORE
installing the mock and call `originalRename.call(fs, src, dest)`
in the fallback branch, matching the ENOSPC test's pattern.
Codex review on PR #3634.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(3594): adversarial parser fixtures + frontmatter/roadmap matrix + property-style suite
Lands the adversarial parser-input corpus that CONTRIBUTING.md
§"QA Matrix Requirements / Parser and project-file inputs" and
TEST-EXAMPLES.md §"Parser Adversarial Fixtures" describe.
New tests/fixtures/adversarial/ layout:
frontmatter/
duplicate-keys.md — same key twice (collapses last-wins)
crlf-mixed.md — CRLF endings throughout
unclosed-block.md — `---` open with no close
unicode-keys-and-values.md — non-ASCII + emoji + Greek
null-byte-value.md — U+0000 in a value
huge-bounded.md — 2000-item array, ~30KB
roadmap/
phase-heading-inside-fenced-code.md — #2787 fence shadowing
nested-fenced-code.md — outer + inner ``` blocks
unicode-phase-titles.md — JP / Greek / emoji titles
repeated-phase-ids.md — phase 1 declared twice
decimal-phase-mixed.md — 2 vs 2.1 vs 2.10 vs 21
markdown-headings-inside-html-comment.md — comment shadowing
Test files (all node:test, no try/finally in test bodies, no source-grep,
no raw-text matching on stdout/file content):
tests/feat-3594-parser-adversarial-frontmatter.test.cjs (12 tests)
Loads each fixture, pins parser invariants on extractFrontmatter()
return shape. Cross-corpus "does not throw on any fixture" sweep.
tests/feat-3594-parser-adversarial-roadmap.test.cjs (18 tests)
Loads each fixture into a temp project's .planning/ROADMAP.md and
drives `gsd-tools roadmap get-phase <N>` via the runCli harness
introduced by #3593. Asserts on the typed JSON payload.
tests/feat-3594-parser-property-style.test.cjs (2 tests)
Deterministic mulberry32 PRNG generates 500 malformed-ish
frontmatter inputs per test. Pins (a) extractFrontmatter is total
over the corpus (no null-deref TypeError, always returns a plain
object on success), (b) the suite completes well under 2 seconds
(quadratic-regression guard).
Known-open bugs surfaced and pinned (intentionally NOT fixed in this
PR — separate issues warranted):
- CJS roadmap parser matches `## Phase N:` headings inside fenced
code blocks (the SDK parser tracks fences per the #2787 comment in
sdk/src/query/roadmap.ts but the CJS path has not caught up).
- CJS roadmap parser matches `## Phase N:` headings inside HTML
comments.
Both are documented in-test with the "currently STILL matches it
(open: needs <fix>)" naming pattern so the day the production fix
lands, flipping the assertion from `found: true` to `found: false` is
the regression guard.
Test totals:
- 32 new feat-3594-* tests (12 frontmatter + 18 roadmap + 2 property)
- 108/108 pass when running together with the pre-existing
frontmatter.test.cjs + roadmap.test.cjs suites (76 of theirs).
Closes#3594
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(3594): use Fisher-Yates shuffle for deterministic seeded inputs
Replaces `arr.sort(() => rng() - 0.5)` with a Fisher-Yates shuffle
driven by the supplied PRNG. The sort-based shuffle is non-transitive:
V8's TimSort behavior on non-transitive comparators is engine-defined,
so the same seed produced different orderings across Node versions —
undermining the test's stated reproducibility guarantee.
Fisher-Yates is O(n), transitive (no comparator at all), and consumes
exactly n-1 RNG values in a fixed order. The mulberry32 seed now
determines the input sequence end-to-end.
Codex review on PR #3633.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#3610 added a `bundled-gsd-hook` classifier to classifyPromptUserAction
that auto-removes blocked hook files on first-time-baseline scan. The
match shape was a regex `/^hooks\/gsd-[^/]+\.(?:js|sh|cjs|mjs)$/` that
matched ANY file under hooks/ named gsd-<name>.{js,sh,cjs,mjs}, not only
the 13 hooks the npm package actually ships. As a result the classifier
silently auto-classified — and the resolver auto-removed — user-authored
custom hooks (hooks/gsd-personal-experiment.js) and retired bundled hooks
from prior versions (hooks/gsd-old-statusline.js).
Evidence the maintainer was already working around this: 0862df15 (the
#3610 follow-up) renamed the integration-test fixture
hooks/gsd-retired-hook.js -> hooks/gsd-retired-hook.txt specifically to
dodge the classifier so the test could exercise the "explicit block" path
it was written for.
Fix: replace the shape regex with an explicit Set of the 13 shipped hook
filenames (BUNDLED_GSD_HOOK_FILES). Files outside the whitelist fall
through to the existing block-or-prompt flow so users retain control.
A regression guard (tests/bug-3628-bundled-hook-classifier-whitelist.test.cjs)
fails CI if the whitelist drifts from the on-disk hooks/ directory in
either direction: whitelisted-but-missing OR shipped-but-not-whitelisted.
The latter check uses the SAME shape regex the buggy classifier used,
re-purposed as a lint that ensures every gsd-*-shaped file shipped in the
distribution IS in the whitelist.
Behavioural tests cover: every entry in BUNDLED_GSD_HOOK_FILES classifies
as bundled-gsd-hook -> remove; six user-owned / retired filenames return
null (proves the regression is fixed); nested hooks/gsd-*/ directories
still return null (the #3610 nested-directory boundary stays intact);
non-gsd hooks still return null (the user-hook-preservation boundary
stays intact).
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the shared adversarial-input harness described in TEST-EXAMPLES.md
§"CLI Negative Matrix" and applies it across two layers:
1. tests/helpers/cli-negative.cjs — runCli() wraps spawnSync of
get-shit-done/bin/gsd-tools.cjs, prepends --json-errors by default,
and returns a typed IR { status, ok, reason, message,
hasStackTrace, ... } so adversarial-case tests assert on
reason codes — never on stderr prose.
2. tests/feat-3593-cli-negative-config.test.cjs — full 12-category
matrix for the config command family (the highest-risk read/write
surface): missing/empty/whitespace args, duplicate --cwd,
unknown subcommand, value-looks-like-a-flag, corrupt config.json,
50KB key, Unicode/emoji keys and values, and 9 distinct shell-
metacharacter payloads asserted as NOT-executed via per-test
sentinel-file probes.
3. tests/feat-3593-cli-negative-universal.test.cjs — narrower
cross-family sweep (phase, roadmap, state, config, workstream,
init, validate). Pins the three universal invariants every
family must satisfy: bare invocation does not crash with a V8
stack trace, unknown subcommand emits a typed reason, and shell
payloads as argv values are not executed.
4. tests/feat-3593-cli-negative-harness.test.cjs — meta-test that
pins the harness IR contract so a future regression in the
parser (stack-trace detection, JSON shape extraction, hostile
stderr handling) surfaces before it cascades through every
matrix file.
Bug fix surfaced by the new tests:
get-shit-done/bin/lib/config.cjs cmdConfigSet — invoking
`config-set <key>` with no value silently returned exit 0 and
emitted { updated: true } even though the value parameter was
undefined. JSON.stringify dropped the key during the write or
persisted a corrupt entry. Now rejected with typed ERROR_REASON.USAGE
before any write. Matching guard added to SDK configSet for parity.
Harness coverage delivered: 58 new tests (9 meta + 26 config + 23
universal sweep). Pre-existing config suites (101 tests) all pass.
lint-no-source-grep clean.
Refs #3593
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a Pull Request Guidelines bullet making explicit what v1.42.3
hotfix taught us the hard way: when a production change makes an
existing test assertion stale, the test correction must be its own
test: (or fix:) commit, not bundled into a docs: commit that also
explains the change.
The release-sdk hotfix cherry-pick filter routes by commit-subject
prefix (fix:, chore:, test: — see release-sdk.yml). A docs: commit
that hides a test fix is invisible to the picker. The result is a
half-shipped state on the hotfix branch: production code changed,
test assertion stale, CI red.
This is the upstream contributor-side mitigation. The picker-side
fix landed in PR #3623 (broadens the prefix filter and the
classifier's CI-gating-path detection); this PR documents the
upstream discipline that keeps the picker from getting fooled in
the first place.
Refs #3621
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`relPlanningPath(workstream)` previously called `posix.join('.planning',
'workstreams', workstream)` without validating the workstream argument.
Direct SDK callers — and `planningPaths` / `ContextEngine` which both
forward through `relPlanningPath` — could pass values like
`'../../../outside'`, `'foo/bar'`, or `'foo\\bar'` and route planning
operations outside the intended `.planning/workstreams/<name>` subtree.
The env-sourced workstream code path in `planningPaths` already validated
via `validateWorkstreamName` (line 444-445, pre-filtering to `null` on
failure per the #2791 silent-fallback contract). Explicit SDK arguments
had no equivalent gate.
Fix: validate inside `relPlanningPath` using the same shared
`validateWorkstreamName` policy. Every caller — direct SDK use,
`planningPaths`, `ContextEngine` — fails closed at the same seam.
Empty/undefined workstream still returns `.planning` for back-compat
(treated as "no workstream provided"); non-empty invalid names throw a
synchronous Error with the offending value in the message.
Env-sourced behaviour is unchanged: `planningPaths` continues to filter
invalid env values to `null` before they reach `relPlanningPath`, so the
silent-fallback path for malformed `GSD_WORKSTREAM` env still works.
Regression test
(sdk/src/bug-3589-planning-paths-validation.test.ts):
- 9 traversal/invalid cases (.., /, \\, spaces, .hidden, /abs,
-leading-hyphen) all throw with a `/workstream/i`-matching message.
- Valid names (`frontend`, `api_v2`, `alpha.beta-1`) continue to
produce the expected `.planning/workstreams/<name>` path.
- `planningPaths('/tmp', '../../../outside')` rejects before path
construction (proven via try/catch — resultPath stays null).
- Valid workstream + `planningPaths` produces the expected subtree
(`.planning/workstreams/frontend/STATE.md` etc.).
- Omitted workstream still returns root `.planning` with no `workstreams`
segment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(3621): cherry-pick test-fixture commits in hotfix runs
The release-sdk hotfix loop excluded test-fixture updates that align CI
with a cherry-picked production fix, leaving the hotfix branch with new
production behavior and stale test assertions. Broke v1.42.3 CI (run
25949422676) when fix(3562) was cherry-picked but its bundled test
correction in docs(3562) commit 08848df8 was POLICY_SKIPPED by the
prefix filter.
Two-part fix:
1. release-sdk.yml prefix regex now accepts test: alongside fix:/chore:.
feat:, docs:, refactor: still POLICY_SKIPPED as before.
2. scripts/diff-touches-shipped-paths.cjs treats tests/-rooted paths and
sdk/src vitest specs as CI-gating-equivalent. A test: commit touching
only those paths now passes the shipped-paths gate.
The #2980 push-blocking guard is preserved as a separate first-priority
check: any commit touching .github/workflows/<file> still skips
regardless of test paths in the same bundle, because the default
GITHUB_TOKEN lacks the workflow scope and the push step would fail.
New regression coverage in tests/bug-3621-cherry-pick-test-fixtures.test.cjs:
- workflow prefix regex includes test:
- isCiGating accepts tests/ and sdk/src vitest specs, rejects
non-spec sdk/src paths and incidental "test" name occurrences
- classifier exits 0 for test-only, mixed test+docs, and the original
shipped paths
- classifier exits 1 for pure docs-only and workflow-only diffs
- new explicit assertion that #2980 push-blocking wins over #3621:
workflow + test + changelog bundle still skips
Adjusted one pre-existing bug-2980 test fixture to use a non-
push-blocking non-shipped path (planning/notes.md) instead of
.github/workflows/release-sdk.yml. The original assertion was
documenting "mixed diff includes shipped path → include" but its
fixture happened to also trigger the push-blocking guard now made
explicit by this PR.
Fixes#3621
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(release-sdk): align hotfix summary labels with test matcher
* fix(3621): align operator-facing strings with the fix/chore/test matcher
The candidate-loop regex was updated to accept test: but several
human-facing strings in the same job still read fix/chore. Update every
description/comment/summary line for consistency so operators reading
the run summary or workflow_dispatch inputs see the same set of accepted
prefixes the matcher actually applies.
Also corrected the NON_SHIPPED_SKIPPED summary text — it claimed test
changes belong on main, not in a hotfix. That assumption is what #3621
fixes; tests under tests/ and sdk/src vitest specs are now CI-gating
candidates and may be picked. The summary now scopes the never-pick
guidance to CI / docs / planning paths only.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
createGSDToolsRuntime accepted opts.workstream and forwarded it to the
QuerySubprocessAdapter (line 38) but the QueryNativeDirectAdapter's
dispatch closure dropped it:
dispatch: (registryCommand, registryArgs) =>
registry.dispatch(registryCommand, registryArgs, opts.projectDir)
`registry.dispatch(command, args, projectDir, workstream?)` accepts a
4th workstream argument and forwards it to handlers. When a GSDTools
instance was created with a workstream, the native fast-path silently
routed planning-path queries to the root `.planning/` tree instead of
`.planning/workstreams/<name>/`. Subprocess dispatch correctly carried
the workstream; native dispatch did not — runtime-bridge mode parity
broke for any workstream-aware GSDTools consumer using the native path.
One-line fix: pass opts.workstream as the 4th arg to registry.dispatch.
Regression test exercises three paths:
1. Constructor-seam unit test: spy on QueryNativeDirectAdapter,
capture the dispatch closure, verify it reaches a registry that
reports the unknown-command error message.
2. Back-compat: same with workstream omitted — closure still reaches
the registry.
3. End-to-end: spy on createRegistry to inject a probe registry with a
registered handler that records its args. Invoke through
runtime.bridge.dispatchHotpath(). Assert the handler observed
workstream='frontend-ws' as its 3rd arg.
RED verified: end-to-end probe fails on pre-fix tree with
`expected undefined to be 'frontend-ws'`. GREEN after the fix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`init.new-milestone` reported `phase_dir_count: 0` for projects whose
phase directories carry a project_code prefix (`.planning/phases/CK-01-name`)
when the ROADMAP used numeric `### Phase N:` headings. Verified via a
temp-project repro that mirrors the reporter's setup.
Root cause: `getMilestonePhaseFilter` builds an `isDirInMilestone(dirName)`
predicate that tries two paths:
1) Numeric — requires the dir name to START with a digit. `CK-01-name`
starts with `C`, so this skips.
2) Custom-ID — captures the leading kebab token (`CK-01-name` as a
whole) and compares it to the normalised milestone phase IDs
(`{"1"}`). No match.
There was no path that stripped the project_code prefix before retrying
the numeric match. Added a third path that strips the same shape
`normalizePhaseName` already recognises (`^[A-Z]{1,6}-(?=\d)`) and retries
the numeric match. This runs AFTER the custom-ID path so a ROADMAP that
uses `### Phase PROJ-42:` continues to win via the custom-ID match for
a `PROJ-42` directory; the new branch only fires when the milestone is
keyed on the bare numeric form.
The fix lands in both:
- get-shit-done/bin/lib/core.cjs:isDirInMilestone (active CJS runtime)
- sdk/src/query/state.ts:isDirInMilestone (SDK twin)
`getMilestonePhaseFilter` is shared by multiple callers — init.new-milestone,
phase complete, verify-work, validate-health — so the fix benefits every
caller that walks `.planning/phases/` against a numeric ROADMAP.
Regression test
(tests/bug-3600-milestone-phase-filter-project-code-prefix.test.cjs):
1. Reporter's case: CK-01-name + CK-02-build dirs against Phase 1 / 2
headings → phase_dir_count === 2.
2. Existing contract: 01-first dir against Phase 1 heading still counts.
3. Custom-ID contract: PROJ-42 dir against `### Phase PROJ-42:` still
counts via the existing custom-ID match (no regression).
4. Counter-test: CK-99-backlog and CK-100-future dirs MUST NOT count
against a milestone with only Phase 1 — the strip-and-retry must
still respect the milestone's actual phase set.
All assertions go through `init new-milestone --json` (typed payload —
`phase_dir_count`). No raw text matching.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
phase.cjs:updateRoadmapAfterPhaseRemoval renumbers plan references in
ROADMAP.md via a regex that captured `NN-NN` followed by an optional
suffix:
/(?<![0-9-])(\d{2})-(\d{2})(?=(?:-(?:PLAN|SUMMARY)\.md)?(?![0-9-]))/g
The suffix branch was strict: it only accepted `-(PLAN|SUMMARY).md`
directly after the plan number. A slug like
`07-01-cherry-pick-foundation-PLAN.md` placed `-cherry-…` between the
number and the canonical suffix, so both the suffix branch AND the
"bare token" branch (`(?![0-9-])` — fails because the next char is `-`)
failed. Result: the on-disk file got renamed to
`06-01-cherry-pick-foundation-PLAN.md` by the directory-rename pass,
but the ROADMAP entry kept pointing at the stale `07-01-…` prefix —
disk/ROADMAP inconsistency.
Fix: extend the suffix branch to allow an optional kebab-case slug
between the plan number and the PLAN/SUMMARY suffix:
(?:(?:-[A-Za-z][A-Za-z0-9-]*)*-(?:PLAN|SUMMARY)\.md)|(?![0-9-])
Each slug token must start with a letter so `07-01-02-PLAN.md` is not
silently consumed as one slugged token (the `-02` is correctly
unreachable from the slug branch because it starts with a digit).
Regression test exercises three cases via the typed `roadmap get-phase
--json` query (no raw text matching on ROADMAP.md content):
1. Slugged PLAN + SUMMARY filenames get renumbered (#3602 fix).
2. Compact `NN-NN-PLAN.md` filenames still renumber correctly
(#3601 / earlier contracts preserved).
3. Counter-test: ISO dates (`2026-01-01`) and version tags (`v1-2-3`)
in ROADMAP prose are NOT modified — the existing `(?<![0-9-])` /
`(?![0-9-])` boundaries hold against false positives.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`phase remove N` for an integer phase silently deleted the adjacent
`### Phase N.1:` decimal section when the decimal was a peer-depth
heading. The bug was in the section-removal regex inside
get-shit-done/bin/lib/phase.cjs:updateRoadmapAfterPhaseRemoval:
(?=\n#{2,4}\s+Phase\s+\d+\s*:|$)
The lookahead required the next header's digits to be followed by
`\s*:` — true for `### Phase 3:` but false for `### Phase 2.1:` because
the `.1` breaks the match. The non-greedy `[\s\S]*?` body then consumed
`Phase 2.1` along with `Phase 2` until it found the next integer
header. The on-disk phase directory `.planning/phases/02.1-*` survived
but its ROADMAP entry was gone — disk/ROADMAP inconsistency.
The fix uses a depth-aware lookahead: capture the hash count of the
header being removed with a named group `(?<h>#{2,4})` and require the
end-of-section lookahead to match the SAME depth via `\k<h>(?!#)`. The
`(?!#)` guards against `###` accidentally matching a deeper `####`
header by anchoring on the captured hash count.
This preserves two contracts simultaneously:
- #3601: removing `### Phase 2:` (depth 3) stops at the next depth-3
header, including `### Phase 2.1:` — the peer-level decimal is
preserved.
- #3355: removing `### Phase 27:` (depth 3) continues past
`#### Phase 27.1:` (depth 4, a child of the integer phase) until it
reaches the next depth-3 header. The child decimal is part of the
integer phase being removed.
The regression test exercises the public CLI via runGsdTools and
asserts on typed JSON output from `roadmap get-phase --json` — no raw
text matching on ROADMAP.md content (per CONTRIBUTING.md
"Prohibited: Raw Text Matching on Test Outputs").
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`npx get-shit-done-cc@latest --codex` aborted with
"installer migration blocked pending user choice" listing 12 hooks/gsd-*
files. Those files are part of the GSD npm distribution
(hooks/gsd-prompt-guard.js, hooks/gsd-context-monitor.js, etc.), not
user-owned content, so asking the user to choose between keep/remove for
them was a UX bug, not a real choice. The installer is about to write
the fresh bundled versions in their place.
Root cause: `classifyPromptUserAction` in
get-shit-done/bin/lib/installer-migration-report.cjs knew two
unambiguous categories (`stale-sdk-build-artifact`, `user-facing-skill`)
but had no rule for the bundled GSD hooks. The first-time-baseline scan
classified them as `stale-gsd-looking` prompt-user blockers, and
`assertInstallerMigrationsUnblocked` threw.
A second gate compounded the bug: the safe-default resolver in
bin/install.js was wrapped in `if (!_migrationIsTty)`, so even with a
correct classification rule, TTY runs (every `npx get-shit-done-cc`
invocation) skipped the resolver and went straight to the hard throw.
Fix:
1) Add `hooks/gsd-<name>.(js|sh|cjs|mjs)` to `classifyPromptUserAction`
as `bundled-gsd-hook` → `remove`. The regex is anchored at the
top-level `hooks/` directory so nested paths like
`hooks/gsd-helpers/index.js` (or any user-owned helper directory) do
NOT auto-classify.
2) Remove the `!_migrationIsTty` gate from the resolver call in
bin/install.js. The classifier-based path is unambiguous and must
apply regardless of TTY; the env-override branch
(GSD_INSTALLER_MIGRATION_RESOLVE) still applies only when isTty=false
inside the resolver, preserving the #3541 semantic.
Regression test added
(tests/bug-3610-installer-migration-bundled-hooks-classification.test.cjs):
- Positive: hooks/gsd-*.{js,sh} → category=bundled-gsd-hook, choice=remove.
- Counter-test: hooks/my-custom-hook.js → classifier returns null
(user files are preserved).
- Boundary: hooks/gsd-helpers/index.js → classifier returns null
(nested directories don't auto-classify).
- End-to-end: 12 reporter-exact bundled hooks + empty manifest →
resolver clears every blocker, assertInstallerMigrationsUnblocked
does not throw.
Test exercises the real installer-migration code path
(`runInstallerMigrations` + `resolveInstallerMigrationPromptsForNonTty`
+ `assertInstallerMigrationsUnblocked`) — no source-grep, no raw text
matching on outputs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`roadmap get-phase PROJ-42` returned `{found: false}` because
phaseMarkdownRegexSource() unconditionally strips the project-code prefix
(`^[A-Z]{1,6}-(?=\d)`) before matching, building the regex `0*42` which
matches `### Phase 42:` but never `### Phase PROJ-42:`. The function's
own docstring promised a fallback to escapeRegex(phaseNum) for custom
IDs, but the line-680 regex match consumes the stripped-numeric form
before that branch is reachable.
Fix: add phaseMarkdownRegexSourceExact() that returns the exact-escaped
source for project-code-prefixed inputs (or null for un-prefixed). Update
cmdRoadmapGetPhase to do a two-pass search — try the exact-prefixed form
first, only fall back to the existing padding-tolerant numeric form if
the exact heading is not present.
Two-pass at the call site (rather than alternation inside the regex
source) is required: a roadmap containing both `### Phase 42:` and
`### Phase PROJ-42:` cannot be disambiguated by a single alternation
because regex match-position is leftmost-wins, so the bare numeric
heading at line N would always intercept the match intended for the
prefixed sibling at line M.
The #3537 contract is preserved: `roadmap get-phase CK-01` against a
roadmap that uses `### Phase 1:` prose still resolves correctly via the
numeric fallback, because the exact-prefixed pass returns null and the
existing padded-numeric pass runs unchanged.
Tests added (tests/bug-3599-roadmap-get-phase-project-code-prefix.test.cjs):
1. PROJ-42 query against `### Phase PROJ-42:` heading — found
2. Counter-test: bare `42` query against `### Phase PROJ-42:` — NOT found
3. #3537 contract preserved: CK-01 query → `### Phase 1:` heading
4. Disambiguation: both `### Phase 42:` and `### Phase PROJ-42:` in
one roadmap; each query resolves to its specific match
All assertions go through runGsdTools + JSON parse — typed payload,
no raw text matching on stdout.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the single 747-line /gsd-help reference with a progressive-disclosure
dispatcher (#2551 pattern). Newcomers get a one-page tour; returning users get
a 10-line refresher with --brief; the complete reference stays available behind
--full; /gsd-help <topic> emits one section; and /gsd-help --brief <topic>
is a compact scoped lookup (signature + one-line summary).
- workflows/help.md becomes a small dispatcher routing on $ARGUMENTS
- workflows/help/modes/{brief,default,full,topic}.md hold the tier bodies
- commands/gsd/help.md passes $ARGUMENTS through, advertises composable form
- docs/COMMANDS.md documents the new flags and topic form
- existing tests that read help.md repointed at help/modes/full.md
(bug-2836, bug-2950, bug-2954, cursor-reviewer, execute-phase-wave)
- new feat-3039-help-tiered test enforces structure, size budgets,
dispatcher routing, shim arg passthrough, topic→section coverage,
orphan-heading detection, conflict-resolution rules, routing preamble,
and compact-scope rule
Trek-e review fixes (PR #3040):
- topic.md output rules split into 5a/5b/5c — explicit handling for single
sections, multi-section "plus" joins, and bold-line sub-block anchors;
each rule also takes scope (full vs compact) into account
- explicit resolved-routing preamble line emitted by topic.md before content
("**Topic:** `<alias>` → `<heading>` *(scope: full | compact)*") so the
user sees which alias matched at which scope (review finding #3)
- composable `--brief <topic>` invokes topic.md in compact scope: heading
+ first `**/gsd:*`** signature line + one-line summary. Dispatcher and
topic.md cooperate via $ARGUMENTS pass-through (review finding #4)
- full.md capped at LARGE-tier budget (FULL_BUDGET = 1500); the non-recursive
workflow-size-budget test does not reach modes/ subdirs
- structural <progressive_disclosure> table parse (5-row assertion) replaces
substring-soup regex matching — 5 rows = 4 base tiers + composable scope
- forward /gsd:* sub-block token coverage + reverse orphan-heading allowlist
catch alias-table drift in both directions
- four conflict-resolution tests guard dispatcher promises (--brief+--full
without topic → --full; --brief <topic> → compact; --full <topic> or bare
→ full; dispatcher retains --brief when delegating to topic.md)
- hardcoded topic lists removed from docs/COMMANDS.md and full.md (drift
surfaces reduced from 5 to 2)
- topic.md alias bloat trimmed (~75 → ~25 rows); cleanup/update split into
distinct sub-block rows under ### Utility Commands
- comment-rot ("~750 lines") removed from default.md and full.md
- dispatcher size guard tightened from < 100 to <= 40 lines
- commands/gsd/help.md <process> block trimmed to one line
- MD040 fence languages added to all plain code blocks across mode files
Main-merge conflict resolution:
- workflows/help.md kept as dispatcher (body lives in help/modes/full.md)
- /gsd-<cmd> → /gsd:<cmd> rename from #3452 reapplied to the mode files
(full.md, default.md, brief.md, topic.md) — the six namespace routers
(/gsd-context, /gsd-ideate, /gsd-manage, /gsd-project, /gsd-quality,
/gsd-workflow) and wildcards (/gsd-*) preserved in hyphen form per
main's convention
- bug-2950 test combines branch's path repointing with main's namespaced
replacement strings
bin/install.js and the SDK already treat Antigravity as a distinct runtime
with config dir ~/.gemini/antigravity, env var ANTIGRAVITY_CONFIG_DIR, and
CLI flag --antigravity. get-shit-done/workflows/update.md did not — so
/gsd-update invoked from an Antigravity install classified the runtime as
base Gemini, because:
- RUNTIME_DIRS listed "gemini:.gemini" with no antigravity entry, so the
scan matched ~/.gemini before ever looking for ~/.gemini/antigravity.
- The PREFERRED_RUNTIME env-var ladder checked GEMINI_CONFIG_DIR but not
ANTIGRAVITY_CONFIG_DIR.
- The local-scope scan loops at lines 101 and 590 listed .gemini with no
.gemini/antigravity sibling.
- The ENV_RUNTIME_DIRS append block ignored ANTIGRAVITY_CONFIG_DIR.
- The path-to-runtime classification bullets only mapped /.gemini/ ->
gemini, with no /.gemini/antigravity/ -> antigravity branch.
Every list is now updated so the more-specific antigravity entry precedes
the base gemini entry, matching the installer at bin/install.js (lines
396-404, 1745-1749, 6175, 6475).
tests/bug-3608-antigravity-update-runtime-classification.test.cjs is the
structural regression guard. It parses the RUNTIME_DIRS bash array out of
update.md and asserts the antigravity entry is present and ordered before
gemini, asserts the env-var ladder checks ANTIGRAVITY_CONFIG_DIR before
GEMINI_CONFIG_DIR, asserts every `for dir in ...` scan loop that mentions
.gemini also lists .gemini/antigravity ordered before it, and asserts the
path-classification prose bullet lists antigravity before gemini.
Per CONTRIBUTING.md: the test uses readFileSync on a .md file annotated
`// allow-test-rule: source-text-is-the-product` because the bash blocks
inside update.md ARE the deployed program — the agent loads update.md and
runs them as-written.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six surviving references to /gsd-research-phase (deleted in #3042) and
/gsd-insert-phase (consolidated into /gsd:phase insert in v1.40.0)
remained in five agent contracts because every prior scrub pass (#3029,
#3044, #3131) limited its SEARCH_DIRS to workflows/, references/,
templates/, contexts/, commands/, and hooks/ — agents/ was outside scope.
agents/gsd-executor.md:195 is user-facing: the executor surfaces it
during a package-install failure recovery checkpoint, so a real user
hits "Unknown command" while trying to recover from a stalled phase.
Replacements:
- /gsd-research-phase -> /gsd:plan-phase --research-phase <N>
(agents/gsd-executor.md:195, agents/gsd-phase-researcher.md:17,
agents/gsd-planner.md:186, agents/gsd-planner.md:991,
agents/gsd-research-synthesizer.md:115)
- /gsd-insert-phase -> /gsd:phase insert
(agents/gsd-roadmapper.md:205)
Adds tests/bug-3605-stale-research-insert-phase-agent-refs.test.cjs as
the regression guard. It scans agents/*.md for any retired command name
(/gsd-research-phase, /gsd-insert-phase, /gsd-add-phase,
/gsd-remove-phase, /gsd-analyze-dependencies) with proper word-boundary
matching so a future consolidation that misses agents/ fails CI.
The guard mirrors tests/bug-2950-stale-command-refs.test.cjs which
covers the same anti-pattern for workflows/.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex review surfaced 2 MED + 2 LOW in-scope findings (3 LOW were
pre-existing or out-of-scope, see below); all in-scope items addressed:
1. (MED, test sensitivity) The gh probe test only checked the boolean
return shape, so a future change that re-introduces shell-string
execSync for the gh path would pass. Added an
`architectural-invariant` structural test that reads the production
source file and asserts:
- no `execSync(` call appears anywhere in code,
- no `spawnSync` with `shell: true`,
- `execFileSync` is the only imported child_process primitive,
- every options object explicitly pins `shell: false`.
This is the canonical pattern from CONTRIBUTING.md for invariants
that behavioral tests can't observe — the defect is the *presence*
of the shell parsing primitive, not its output.
2. (MED, cross-platform) The execFileSync options didn't explicitly pin
`shell: false`. Default is already false, but spelling it out (a)
documents the architectural invariant at the call site, (b) prevents
a future options-spread refactor from silently flipping it, and
(c) hardens against a Windows `git.cmd` shim path that could
otherwise route through cmd.exe.
3. (LOW, test visibility) The exploit-blocked test silently `return`ed
when git rejected the payload branch name on a stricter platform,
turning a coverage loss into a stealth pass. Replaced with vitest's
`ctx.skip()` so a lane that loses coverage now shows up in the skip
count.
Out of scope, intentionally not changed:
- The `try/finally` at sdk/src/query/check-ship-ready.test.ts:79 is
pre-existing test code from before this PR. One-concern-per-PR rule
says no drive-by cleanup.
- The "use createTempGitProject helper" suggestion: that helper lives
in tests/helpers.cjs (root, node:test world). SDK tests use vitest
with their own ad-hoc tmpdir pattern; matching the SDK convention.
- The afterEach cleanup uses `rm` directly, matching the surrounding
SDK test convention; not changing without broader SDK-side refactor.
Validation:
- SDK unit suite via vitest: 1,870/1,870 pass (+1 invariant test).
- Full root suite via gsd-test-both: 10,676/10,676 Mac AND Linux Docker,
zero cross-platform diff.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`gsd-sdk query check.ship-ready <phase>` built a git command as a shell
string with the current branch name interpolated. Git branch names can
legally contain shell metacharacters, so a repo checked out on a
malicious branch like `foo;touch${IFS}INJ;bar` executed arbitrary shell
commands.
Vulnerability site (pre-fix):
sdk/src/query/check-ship-ready.ts:50
runSyncSafe(`git config --get branch.${current_branch}.merge`, cwd)
→ execSync('git config --get branch.foo;touch${IFS}INJ;bar.merge')
→ /bin/sh -c parses three commands; the middle one runs `touch INJ`
in the project dir and creates the sentinel file.
Manually reproduced on git 2.53.0:
- refname `foo;touch${IFS}INJ;bar` is accepted by `git check-ref-format`
and by `git checkout -b`.
- `current_branch` returned from `git rev-parse --abbrev-ref HEAD`
contains the metacharacters verbatim.
- Interpolation into the buggy execSync call creates the sentinel.
Fix:
- Replace `runSyncSafe(cmd: string, cwd)` (execSync, shell-string) with
`runArgvSafe(file, args: readonly string[], cwd)` (execFileSync,
argv-based, no shell).
- Same shape for the boolean wrapper: `boolArgvSafe`.
- Convert all 7 subprocess sites in the module to argv form:
- `git status --porcelain`
- `git rev-parse --abbrev-ref HEAD`
- `git config --get branch.<name>.merge` ← the interpolation site
- `git rev-parse --verify main`
- `git remote`
- `gh --version`
- `which gh`
- Shell is never invoked. Branch names — even ones with `;`, `$IFS`,
backticks, `$()` — are passed as a single argv element and treated
as opaque data.
Regression test (`sdk/src/query/check-ship-ready.test.ts`):
- `#3587: branch name with shell-injection payload does not execute
injected command` — creates a real git repo, checks out the proven
exploit branch `foo;touch${IFS}INJECTED_BY_3587;bar`, runs
checkShipReady, and asserts the sentinel file does NOT exist. This
test FAILS on the unfixed code (verified pre-implementation) and
PASSES on the fixed code — true red→green TDD.
- `#3587: round-trips a metacharacter branch name verbatim in
current_branch` — positive proof the branch name survives argv as
data (would fail if a future change re-introduces shell quoting).
- `#3587: gh probe does not invoke a shell` — locks the gh path
against a future regression that might add an interpolation site.
Validation:
- SDK unit suite via vitest: 1,869/1,869 pass.
- Full root suite via gsd-test-both (per CLAUDE.md): 10,676/10,676
on Mac AND 10,676/10,676 on Linux Docker, zero cross-platform diff.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex review surfaced 1 HIGH, 2 MED, 2 LOW; all addressed:
1. (HIGH, CONTRIBUTING.md violation) Adapter body assertion was raw
text matching `content.includes('<codex_skill_adapter>')` on the
rendered SKILL.md. Replaced with a structural assertion against
the exported builder: `content.includes(getCodexSkillAdapterHeader(name))`.
The expected value is now the full closed adapter block produced
by the production IR — open tag, body, and `</codex_skill_adapter>`
closing tag — so a truncated, empty, or missing-closing-tag adapter
cannot satisfy the assertion. Pre-flight sanity-checks the builder
itself emits the expected open/close shape.
2. (MED, sensitivity) Count-only check on installed skills was
replaceable by a same-count partial install that swapped real
commands for bogus `gsd-*` dirs. Replaced with `deepStrictEqual`
on the sorted full set, computed from `commands/gsd/**/*.md` via
a local `expectedSkillNames()` walker that mirrors the installer's
naming rule (nested dirs collapse to `gsd-<dir>-<file>`).
3. (LOW, flakiness) `runCodexInstallCaptured()` could throw after
creating the temp `codexHome` but before returning, so the
describe-level `afterEach` couldn't see the path and the dir would
leak. Added a try/catch around `install()` that cleans up the
temp dir before rethrowing.
4. (LOW, flakiness) Harness mutates process-global `console.*`,
`process.env`, `process.cwd()` — added `{ concurrency: false }` to
the describe block, matching the existing convention in
`tests/bug-3562-codex-install-skill-surface.test.cjs:45`.
5. (LOW, CONTRIBUTING.md helpers) Local `parseFrontmatter()` was
duplicating the shared helper. Switched to the canonical
`parseFrontmatter` exported by `tests/helpers.cjs`; also adopted
`createTempDir` and `cleanup` from the same module for consistency.
Validation:
- New test still 5/5 pass.
- Full suite via gsd-test-both: 10,682/10,682 on Mac AND Linux Docker,
zero cross-platform diff.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
