* Enforce documentation updates via lint:docs + PR templates (#3213)
New scripts/lint-docs-required.cjs + Docs Required CI workflow fail any
PR whose changeset fragment is typed Added / Changed / Deprecated / Removed
without modifying at least one file under docs/.
Mirrors scripts/changeset/lint.cjs: pure evaluateLint({ changedFiles,
fragments, labels }) returning { ok, reason, triggering } over a frozen
LINT_REASON enum; CLI wrapper reads the PR diff and parses each touched
changeset fragment via the existing parseFragment helper.
Escape hatches:
- no-docs PR label (global)
- per-fragment <!-- docs-exempt: <reason> --> marker, all triggering
fragments must carry it for the PR to pass
Fixed and Security fragments do not trigger the lint — bug fixes restore
documented behavior, they do not introduce new behavior to document.
PR templates (enhancement.md, feature.md) gain a Documentation checklist
section pointing at the which-doc-to-update matrix. CONTRIBUTING.md adds
a Documentation Updates section codifying that matrix, the English-canonical
language policy for docs/ and the root README, and the two opt-out routes.
Closes#3213
* Address Codex review: fail-closed on malformed fragments and strip docs-exempt marker from rendered release notes (#3213)
Two P2 issues caught by `codex review --base main`:
1) Malformed fragments could silently bypass docs enforcement. parseFragment
would return ok:false on a triggering Added fragment with bad frontmatter
and readFragmentsFromDisk dropped it, so evaluateLint saw no triggering
fragments and passed. The changeset-required lint only checks fragment
_presence_ not _validity_, so the assumed fallback did not catch it.
Fix: readFragmentsFromDisk now returns { fragments, malformed }; evaluateLint
accepts a malformed param and emits a new FAIL_MALFORMED_FRAGMENT verdict
that outranks every OK path (including the no-docs label) — a parse failure
must be fixed before docs lint can decide anything else.
2) The per-fragment <!-- docs-exempt: reason --> marker lived in the fragment
body, so the existing changelog (serializeChangelog) and GitHub release-notes
(formatBullet) serializers published it verbatim. Worse, both serializers
append `(#NNNN)` to the body's last line — with the marker as the trailing
line, the PR suffix attached to the hidden comment instead of the visible
bullet.
Fix: parseFragment now extracts the marker into a typed `docsExempt` field
and strips it from `body`, so all downstream renderers produce clean output
without remembering to strip. The regex is anchored to its own line (^...$
with m flag) so inline mentions of the marker syntax in documentation
(e.g. inside backticks) cannot accidentally exempt a fragment. Bounded
character class [^\n>] keeps the regex linear-time.
Test additions:
- tests/lint-docs-required.test.cjs: FAIL_MALFORMED_FRAGMENT coverage,
end-to-end "Added fragment with bad pr → malformed → fail-closed" regression
test, updated readFragmentsFromDisk return-shape assertions, isExemptFragment
now checks the typed docsExempt field rather than body content.
- tests/changeset-parse.test.cjs: extractDocsExempt extraction cases (with/
without reason, case-insensitive, EMPTY_BODY when body is only a marker),
inline-mention false-positive guard, real-marker-wins-when-also-inline test.
- tests/changeset-new.test.cjs: fragment shape now includes docsExempt: null.
CONTRIBUTING.md updated to clarify the "on its own line" requirement and the
parse-time stripping behavior. The bootstrap fragment cleaned up so its body
no longer contains a literal marker example that would have triggered the
false-positive case.
Full suite: 9696/9696 pass.
* CRLF-safe docs-exempt marker stripping (Codex review pass 2, #3213)
Second `codex review --commit` pass caught a CRLF regression in the
docs-exempt extraction added in the previous commit.
Repro: a Windows-authored fragment
---\r\ntype: Added\r\npr: 1\r\n---\r\nFeature.\r\n\r\n<!-- docs-exempt: x -->\r\n
would parse to body `Feature.\r\n\r\n\r` because:
- The previous trailing-newline slice trimmed only `\n`, leaving `\r`.
- DOCS_EXEMPT_RE was anchored with `$` only — in multiline mode `$`
matches before `\n` but does not consume `\r`, so the marker line's
trailing `\r` was left behind after replace.
- The cleanup regex stripped trailing `\n` but not `\r`.
Net effect: serializeChangelog emitted
- Feature.\r
\r
\r (#1)
— the `(#1)` PR suffix landed on a blank line instead of attached to
the visible bullet. Same bug surfaces in github-release-notes formatBullet.
Fix:
- DOCS_EXEMPT_RE: add `\r?` before `$` so the regex consumes the CR of a
CRLF terminator. Switch reason character class from `[^\n>]` to
`[^\r\n>]` so CRLF-authored reasons don't carry a trailing `\r`.
- extractDocsExempt cleanup: `[ \t\r]+$/gm` strips trailing `\r` on each
line; `(?:\r?\n){3,}` collapses CRLF triple-blank-lines; `[\r\n]+$`
strips every trailing line terminator (LF or CR).
- parseFragment trailing-newline slice: CRLF-aware — strips `\r\n` (2
chars) before falling through to single `\n`.
Tests: two CRLF regression cases in tests/changeset-parse.test.cjs —
Codex's exact repro (end-to-end through serializeChangelog) plus the
no-marker CRLF passthrough case. Full suite: 9698/9698 pass.
* CRLF regression test asserts on parseChangelog IR not rendered text (Codex review pass 3, #3213)
Third `codex review` pass caught that the CRLF regression test added in
the previous commit asserted on serializeChangelog's rendered Markdown
via `out.split('\n')` + `assert.match`. That violates CONTRIBUTING.md's
"Prohibited: Raw Text Matching on Test Outputs" rule and the documented
serializer contract in `serialize.cjs`:
> tests assert via round-trip (parse(serialize(ir)))
> rather than by inspecting serialized text
Replace the regex check with the established `parseChangelog(out)`
round-trip and assert on the structured `{ body: 'Feature.', pr: 1 }`
bullet. This is also a stronger regression check than the substring
match: Codex's own probe in the review session confirmed the pre-fix
buggy body shape (`Feature.\r\n\r\n\r`) breaks parseChangelog's bullet
regex entirely (returns `bullets: []`), so the round-trip catches the
exact failure mode end-to-end.
Full suite: 9698/9698 pass.
* Address CodeRabbit findings: anchor link + require non-empty docs-exempt reason (#3213)
CodeRabbit's review on the PR caught two actionable issues, both quick wins.
Anchor link in PR templates pointed to a heading that does not exist. The
CONTRIBUTING.md heading "Documentation Updates — Update the Relevant Docs"
contains an em-dash, which GitHub strips entirely when generating anchor
slugs (it does NOT collapse to a hyphen). The actual anchor is
#documentation-updates-update-the-relevant-docs (single hyphen between every
word), not #documentation-updates--update-the-relevant-docs (double hyphen
where the em-dash was). Both feature.md and enhancement.md fixed.
The docs-exempt marker matched a bare `<!-- docs-exempt -->` with no reason,
which defeats the entire purpose of the escape hatch — the marker exists to
leave an audit trail explaining WHY a PR is exempt. Without a reason it is
a silent bypass.
Fix: DOCS_EXEMPT_RE now requires both the colon AND a non-whitespace first
reason character. Bare `<!-- docs-exempt -->`, empty `<!-- docs-exempt: -->`,
and whitespace-only `<!-- docs-exempt: -->` are all rejected as if the
marker were not present (`docsExempt: null`). The lint then falls through
to its normal docs-required / no-docs-label checks.
`isExemptFragment` in the lint module tightened too — defense-in-depth: even
if a caller constructs a fragment with `docsExempt: ''` directly, it does
not count as exempt. The predicate now requires `typeof === 'string'` and
non-empty after trim.
Tests:
- changeset-parse.test.cjs: three new explicit-rejection cases (bare marker,
empty reason, whitespace-only reason). Existing DOCS_EXEMPT_RE shape test
extended with negative assertions for the same three forms.
- lint-docs-required.test.cjs: prior "empty reason still exempt" test
inverted — empty/whitespace docsExempt now produces FAIL_DOCS_MISSING.
isExemptFragment helper test extended with the same negative cases.
- CONTRIBUTING.md: clarified that the reason is required and non-empty.
Skipped CodeRabbit's third finding ("use `npm run lint:docs` in CI workflow
instead of `node scripts/lint-docs-required.cjs`") — the existing
changeset-required.yml uses the direct-node form for the equivalent
changeset lint, so the new docs-required.yml is convention-consistent.
Switching one without the other would create drift, and switching both is
out of scope for #3213.
Bootstrap fragment continues to extract cleanly under the stricter regex
(verified — `docsExempt` field still contains the full bootstrap reason).
Full suite: 9701/9701 pass.
* 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>
GSD 1.42.2 reported a successful Codex install but printed
`Skipped Codex skill-copy generation (Codex discovers official skills
directly)` and left users with no routable `$gsd-*` entrypoints in
Codex CLI 0.130.0+. Confirmed reproducible on Windows 11 and macOS by
three independent reporters.
Root cause: Codex CLI 0.130.0 does NOT auto-discover commands from
`~/.codex/get-shit-done/workflows/*.md` or `agents/*.md`. It only
registers slash commands derived from `~/.codex/skills/<name>/SKILL.md`.
The "Codex discovers official skills directly" assumption was wrong.
The fix already shipped in #3562 — `bin/install.js` now calls
`copyCommandsAsCodexSkills()` for the Codex install path, materializing
one `SKILL.md` per `commands/gsd/*.md` with Codex-flavored frontmatter
and the `<codex_skill_adapter>` body. Verified locally: a Codex global
install into a temp `CODEX_HOME` produces 67 `gsd-*/SKILL.md` files
including `gsd-map-codebase` (the literal command from the bug report).
This change adds the regression test the triage diagnose pass called
for. It pins five facets of the contract so the 1.42.2 failure mode
cannot silently come back:
1. `<CODEX_HOME>/skills/gsd-*/SKILL.md` exists for every shipped
command (count matches `commands/gsd/**/*.md` exactly).
2. Each generated SKILL.md has YAML frontmatter with `name:` matching
the directory and a non-empty `description:`.
3. Every SKILL.md body contains the `<codex_skill_adapter>` block —
without it Codex can't route `$gsd-<cmd>` invocations.
4. The five representative commands named in the report and triage
(`gsd-map-codebase`, `gsd-execute-phase`, `gsd-plan-phase`,
`gsd-new-project`, `gsd-health`) are all present.
5. Installer output never prints "Skipped Codex skill-copy generation"
or "Codex discovers official skills directly", and DOES print
"Installed N skills to skills/" — locking the success-while-skipping
anti-pattern out.
Validation:
- New test: 5/5 pass on Mac and Linux Docker.
- Full suite (`gsd-test-both` per CLAUDE.md): 10,682/10,682 on Mac,
10,682/10,682 on Linux Docker, zero cross-platform diff.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit surfaced one outstanding inline finding plus an outside-diff
finding and a finer point on two already-remediated sites; all addressed:
1. (inline, Minor) runtime-slash.cjs:31 — a degenerate input like `/gsd:`,
`gsd:`, or `gsd-` normalizes to empty and the previous fallback returned
the original colon-form input, reintroducing the deprecated shape the
module exists to suppress. Now returns `''` (empty string) so callers
see "no command" instead of an unroutable string. Also catches
whitespace-only inputs the same way. New unit tests pin the contract.
2. (inline, Major) drift.cjs library purity — the earlier remediation
passed `projectDir` into `detectDrift` and re-resolved runtime inside the
library. CodeRabbit (correctly) flagged this as breaking the module's
pure-library contract. detectDrift now accepts `input.runtime` directly;
verify.cmdVerifyCodebaseDrift resolves the runtime once and passes the
literal name in. drift.cjs no longer reads env or config at all.
3. (duplicate inline, Minor) gsd2-import.cjs:475 — when
`gsd2-import --path <dir>` targets a project that isn't the process
cwd, the preview command was formatted for the wrong runtime.
buildPreview now receives the resolved `projectDir` (the same one
used to find the .gsd/ root) instead of the raw `cwd`.
4. (outside-diff, Minor) tests/copilot-install.test.cjs:1-5 — the
`allow-test-rule: integration-test-input` rationale block specifically
named verify.cjs as the fixture, but #3584 changed that test to use a
synthetic input. Comment now describes the real shape of the file's
readFileSync usage (commands/, agents/, install.js source inputs to
the installer/converter functions under test) and notes the synthetic
substitution for the bin/lib path.
Full suite: 9366/9366 pass (+1 new test). Lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex review surfaced five issues in the initial slash-formatter PR — three MED
and two LOW. All five are addressed:
1. (MED) formatter corrupted argument tails under codex runtime. Splitting on
the first whitespace and lowercasing only the command token preserves
path-like arguments (`Map-Codebase --paths C:\\Users\\Me\\Project`) and
case-sensitive flag values. (`runtime-slash.cjs`)
2. (MED) profile-output `cmdGenerateDevPreferences` emitted a hardcoded
`command_name: '/gsd-dev-preferences'`. Replaced with
`formatGsdSlash('dev-preferences', resolveRuntime(cwd))` so the structured
result honors codex/skills runtime distinction. (`profile-output.cjs`)
3. (MED) `drift.detectDrift` → `buildMessage` called `resolveRuntime(null)`,
ignoring a project's `.planning/config.json` `runtime` setting when
`GSD_RUNTIME` env var was absent. Threaded `projectDir` through
`detectDrift({projectDir})` → `buildMessage(..., projectDir)` →
`resolveRuntime(projectDir)`. `verify.cmdVerifyCodebaseDrift` now passes
`cwd` into the detect call. (`drift.cjs`, `verify.cjs`)
4. (LOW) `gsd2-import.buildPreview` had the same env-vs-config issue. Threaded
`cwd` through `buildPreview(..., projectDir)` for parity. (`gsd2-import.cjs`)
5. (LOW) The codex emitter test only asserted absence of `/gsd:` — a regression
to `/gsd-` (skills) form in codex output would have passed undetected.
Added positive assertions that every gsd-referencing fix string under
`GSD_RUNTIME=codex` contains `$gsd-` and contains neither `/gsd-` nor
`/gsd:`. Also added formatter unit tests pinning the argument-tail
preservation contract for both hyphen and codex runtimes.
Full suite: 9365/9365 pass (+2 new). Lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
