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>
`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>
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>
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>
Adds a new `phase_status` field to the `init.plan-phase` SDK + CJS query
output and a §1.5 "Closed-Phase Gate" in workflows/plan-phase.md that
short-circuits on closed phases instead of silently replanning over
shipped code.
## What was broken
`gsd-sdk query init.plan-phase <N>` returned the same "ready to plan"
payload for a closed phase (REQUIREMENTS Met, VERIFICATION.md status:
passed, ROADMAP flipped) as for an open one. No field signaled closure,
so `/gsd:plan-phase --reviews` happily replanned over closed phases —
risking documentation drift on already-shipped code.
## Fix
- Export `determinePhaseStatus` from `commands.cjs` (already present, was
module-private).
- Both `cmdInitPlanPhase` (CJS) and `initPlanPhase` (TS SDK) now compute
`phase_status` from plan/summary counts + VERIFICATION.md status using
the existing `determinePhaseStatus` helper — the project-wide phase
lifecycle vocabulary (Pending | Planned | In Progress | Executed |
Complete | Needs Review). No directory yet → Pending.
- Workflow `plan-phase.md` adds §1.5 "Closed-Phase Gate":
- `phase_status == "Complete"` with `--reviews` → hard-stop, no
override (replanning a closed phase via review feedback is never
legitimate; concerns belong in a follow-up phase or new issue).
- `phase_status == "Complete"` without `--force` → exit with a clear
notice pointing at VERIFICATION.md.
- `phase_status == "Complete"` with `--force` → continue with a
transcript banner so the deliberate replan is visible.
`Executed` and `Needs Review` are intentionally not gated — those mean
planning finished but verification did not pass, and replanning is the
correct next step.
## Tests
- SDK: 4 new `phase_status` cases in init.test.ts covering Pending /
Planned / Executed / Complete transitions.
- Existing init.plan-phase golden parity test continues to pass (the
`researcher_model: '' vs sonnet` drift in that test predates this
change and is unrelated).
- Full Mac+Docker suite: 9323 / 9323 passed (Mac), 9318 / 9323 passed
(Docker, 5 skipped).
Fixes#3569
Phase 5.1 of the CJS↔SDK hard-seam migration (parent #3524). Migrates
the bin/lib/state-command-router.cjs handlers map to delegate every
canonical state subcommand through the executeForCjs synchronous
primitive (shipped in Phase 5.0, PR #3558).
## Bundled fix for Phase 5.0 worker defect
Discovered during Phase 5.1 implementation that the Phase 5.0
worker drops projectDir and workstream from
RuntimeBridgeExecuteInput. The dispatch closure at
sdk/src/runtime-bridge-sync/worker.ts:41-42 hardcoded projectDir
to '', so registry handlers that read .planning/ from projectDir
(every state.* handler) saw an empty path and failed. Phase 5.0's
pinning tests passed because they exercised commands that don't
depend on projectDir (generate-slug takes its arg directly;
unknown_command doesn't dispatch). Maintainer authorized bundling
the fix into this PR.
Fix: moved QueryNativeDirectAdapter construction inside the
dispatchNative lambda so request.projectDir and request.workstream
close over the per-request values. Per-request adapter construction
adds <1ms overhead; correctness wins. Regression test at
sdk/src/runtime-bridge-sync/projectdir-regression.test.ts demonstrates
RED before fix → GREEN after.
Phase 5.0's index.test.ts native_failure fixture was passing
because of the bug — it relied on projectDir = '' producing a
specific error path. Updated to use a /nonexistent-... path that
triggers ENOENT under realpath, producing native_failure as intended.
## What landed for Phase 5.1
- bin/lib/state-command-router.cjs migrated. Every subcommand
entry in the handlers map dispatches via executeForCjs when SDK
is available, with transparent fallback to the existing CJS
handlers in state.cjs if (a) SDK is not built / not present, or
(b) GSD_WORKSTREAM is set (the sync-bridge worker cannot serve
workstream-scoped commands per the SDK transport architecture).
- Special cases preserved:
- load --raw: SDK data formatted into key=value lines matching
cmdStateLoad's exact format.
- complete-phase: CJS-only (no SDK counterpart yet).
- add-roadmap-evolution: stays on the unsupported list (SDK-only).
- Golden parity tests added for 12 previously-uncovered state
subcommands: advance-plan, record-metric, update-progress,
add-decision, add-blocker, resolve-blocker, record-session,
signal-waiting, signal-resume, planned-phase, milestone-switch,
prune.
## Design decisions worth reviewer visibility
1. Lazy SDK loading with CJS fallback. The migration routes via
executeForCjs only when the SDK is loadable; otherwise falls
back to the existing CJS handlers. Conservative for rollback —
if the SDK build is broken on a deploy, state commands keep
working via the CJS path. Trade-off: drift surface is not
structurally eliminated yet — the CJS handlers remain reachable.
2. Workstream → CJS fallback. The SDK transport forces subprocess
for workstream commands, but subprocess is disabled in the sync
bridge. When GSD_WORKSTREAM is set, the entire state command
falls back to CJS rather than failing. Workstream users continue
running the CJS handlers; the SDK path is exercised only in the
default (no workstream) case.
3. Two documented parity divergences. state.record-metric: CJS
auto-creates ## Performance Metrics section when absent; SDK
returns {recorded: false, reason}. Test requires fixture with
the section present. state.prune: CJS counts phases from disk;
SDK reads from frontmatter fields. Test asserts structural shape
rather than exact equality.
## Numbers
- Full CJS suite: 9323/9323 pass (baseline 9323; +0 net because
the 12 new parity tests are SDK-side vitest, not CJS-side).
- SDK vitest sync-bridge: 10/10 pass.
- Regression test: 3/3 pass (proved RED before fix, GREEN after).
- tests/state.test.cjs (the safety net): 104/104 pass unchanged.
## Performance
gsd-tools state load via the SDK path: 49ms first call (Worker
startup), 43-44ms steady-state median. Slower than the
Phase 5.0-measured 0.1ms because state.load does fs reads on top
of the bridge overhead. Still well within the budget for CJS
dispatcher overhead.
Closes#3567.
Phase 5.0 of the CJS↔SDK hard-seam migration (parent #3524).
Foundational PR. Ships ONLY the synchronous primitive on the SDK
runtime bridge plus pinning tests. Per-family CJS router migrations
(state.*, verify.*, init.*, phase.*, phases.*, validate.*,
roadmap.*, frontmatter.*, config.*) become follow-up enhancements
that each reuse this primitive.
## What landed
- sdk/src/runtime-bridge-sync/index.ts (155 lines) — public API.
Exports executeForCjs(input: RuntimeBridgeExecuteInput):
RuntimeBridgeSyncResult. Synchronous; lazily creates the
synckit sync function on first call.
- sdk/src/runtime-bridge-sync/worker.ts (167 lines) — synckit
worker. Constructs a native-only QueryRuntimeBridge (with
allowFallbackToSubprocess: false), awaits its async execute,
catches GSDToolsError / GSDError, maps classification to the
six ADR-0001 canonical error kinds plus exit code.
- sdk/src/runtime-bridge-sync/index.test.ts (197 lines, 8 vitest
pinning fixtures) — success path; unknown_command;
native_failure (shape); validation_error (shape); shape
invariants; idempotency.
- tests/runtime-bridge-sync-smoke.test.cjs (99 lines, 4
node:test cases) — proves the primitive works from CJS
callers via require().
## Decisions
1. Synchronous-bridging mechanism: synckit. Disqualified:
- deasync: stagnant (68 open issues, single maintainer,
last release Nov 2025), private Node API (process.binding('uv')),
untested on Node 22, documented deadlocks with modern
Promise chains.
- Sync-native SDK refactor: technically infeasible —
acquireStateLock in state-mutation.ts uses await setTimeout
for retry backoff; making that fully sync requires either
Atomics.wait (which IS synckit), busy-loop (degrades
responsiveness), or breaking 100+ SDK consumers.
Synckit (v0.11.12) is pure JS, actively maintained (last
push today), stable public APIs (Atomics.wait +
SharedArrayBuffer), Node 22 compatible, no native compile.
2. Native-only transport inside the worker. The sync bridge
uses allowFallbackToSubprocess: false. Unknown commands
surface as unknown_command instead of spawning gsd-sdk.
Keeps the worker self-contained and predictable.
3. Worker path resolution. resolveWorkerPath() navigates ../..
from the loaded module URL to land at
dist/runtime-bridge-sync/worker.js — works under both
vitest (loads src) and CJS consumers (load dist).
4. GSDError.Blocked → validation_error. ADR-0001's 6-kind
taxonomy has no `blocked` kind; Blocked classification is
mapped onto validation_error since the operational shape
matches (prerequisite missing).
## Numbers
- 8 SDK vitest pinning tests pass.
- 4 CJS smoke tests pass.
- Full suite: 9286/9286 pass (baseline 9282; +4 from the
new smoke test cases).
- SDK vitest unit: 1860/1860 pass.
- Performance: 80ms first-call cold latency (Worker startup +
bridge construction); 0.1ms steady-state per-call latency
(10-call average after warmup). Well within budget for CJS
dispatcher overhead.
## Canonical error kind coverage
- unknown_command: covered with pinning fixture
- native_failure: shape coverage (handler that throws)
- validation_error: shape coverage (GSDError.Validation +
GSDError.Blocked)
- internal_error: shape coverage only (eliciting TypeError
reliably from a registered handler requires elaborate fixture)
- native_timeout: NOT pinned (no registered handler genuinely
times out; classification logic present in worker)
- fallback_failure: NOT pinned (subprocess fallback disabled
by design in sync bridge)
The classification logic is in the worker regardless; per-family
migration PRs will exercise the unpinned kinds incidentally.
## Wiring
- sdk/package.json: synckit ^0.11.12 added as runtime
dependency (not devDependency — it's required at runtime
whenever a CJS caller invokes executeForCjs).
- sdk/package-lock.json: regenerated.
- CONTEXT.md: new "Sync Runtime Bridge Module" entry added
after Dispatch Policy Module. Existing "CJS Command Router
Adapter Module" entry amended with one sentence pointing at
the primitive and the per-family migration roadmap.
No generator/freshness check needed for this phase — the
primitive IS the SDK (not a generated CJS mirror).
Closes#3555.
Phase 4 of the CJS↔SDK hard-seam migration (parent #3524).
Eliminates the `findProjectRoot` duplication that lived at
bin/lib/core.cjs:74-140 and sdk/src/query/helpers.ts:497-590,
the drift carrier behind historical bugs #1362 and #2561.
- sdk/src/project-root/index.ts — source of truth (120 lines,
pure-with-sync-fs). Exports findProjectRoot(startDir: string)
and FIND_PROJECT_ROOT_MAX_DEPTH constant.
- sdk/src/project-root/index.test.ts — 13 vitest pinning fixtures
covering all four heuristics, the #1362 guard, malformed
config fallback, empty sub_repos, deep nesting, and depth-limit
enforcement.
- sdk/scripts/gen-project-root.mjs — generator. Captures
function body via Function.prototype.toString() from compiled
sdk/dist/. Emits CJS preamble for destructured node:fs /
node:path / node:os imports.
- sdk/scripts/check-project-root-fresh.mjs — freshness check.
Imports the generator function directly (Phase 3's cleaner
pattern).
- get-shit-done/bin/lib/project-root.generated.cjs — generator-
emitted CJS mirror.
- tests/project-root-generator.test.cjs — 11 parity assertions
comparing SDK source and generated CJS for every fixture.
- sdk/src/query/helpers.ts: -127 lines. The 94-line inline
findProjectRoot plus the FIND_PROJECT_ROOT_MAX_DEPTH constant
(originally at line 471) replaced by a single re-export:
`export { findProjectRoot } from '../project-root/index.js';`
Removed unused `parse as parsePath` import.
- get-shit-done/bin/lib/core.cjs: -83 lines net. The 67-line
inline findProjectRoot replaced by a single
`require('./project-root.generated.cjs')`. The detectSubRepos
helper at lines 40-56 stays (used by loadConfig migration).
- sdk/package.json: gen:project-root + check:project-root-fresh
scripts.
- package.json: proxy for the freshness check.
- .githooks/pre-commit: drift block.
- .github/workflows/test.yml: drift check step after the
state-document drift step.
- CONTEXT.md: Project-Root Resolution Module entry.
- docs/INVENTORY.md, docs/INVENTORY-MANIFEST.json:
+1 module count, +1 row.
- Full suite: 9226/9226 pass (baseline 9215 + 11 new parity
fixtures).
- SDK vitest: 1804/1804 pass.
- Reader shrink: -127 SDK + -83 CJS = 210 lines of duplication
deleted across the two Readers. New shared Module is 120 lines.
1. Depth limit canonicalization. CJS findProjectRoot previously
had no explicit walk-up bound (walked until dir === root or
homedir). The new Module uses FIND_PROJECT_ROOT_MAX_DEPTH = 10,
matching the SDK's pre-existing value. Only affects paths
nested more than 10 levels deep from a .planning/ root — a
pathological case in practice. None of the existing 22 CJS
findProjectRoot tests covered this; the new parity test does.
2. platformReadSync → readFileSync. The old CJS findProjectRoot
used the platformReadSync wrapper from
shell-command-projection.cjs for reading .planning/config.json,
which returns null on read failure. The Module uses raw
readFileSync, which throws — caught by the surrounding
try/catch that already swallowed errors. Functionally
equivalent for the existing code path; no test exercises the
null-return semantic.
Closes#3553.
Phase 3 of the CJS↔SDK hard-seam migration (parent #3524).
Introduces the Builder/Reader pattern for paired Modules with
mixed pure-and-I/O concerns — the template for Phase 4 and
follow-up enhancements that migrate other paired Modules.
Phase 1 and Phase 2 migrated Modules where both sides used
character-equivalent logic. Phase 3 introduces the case where
the pure logic is shareable but the I/O is legitimately per-side.
The Builder/Reader split resolves this:
- The Builder is pure — accepts pre-collected data
(BuilderInputs struct), returns the typed projection. One
source of truth; one generator-emitted CJS mirror. Drift
is structurally impossible.
- The Readers are per-side hand-authored Adapters that do the
fs reads in their native idiom (currently both sync; either
side can go async later without touching the Builder), then
delegate to the Builder.
- sdk/src/workstream-inventory/builder.ts — Builder source.
170 lines. Pure. Exports buildWorkstreamInventory(inputs),
isCompletedInventory(status), plus the three typed inventory
interfaces (WorkstreamPhaseInventory, WorkstreamInventory,
WorkstreamInventoryList).
- sdk/src/workstream-inventory/builder.test.ts — 18 vitest
pinning fixtures across all status branches, progress-percent
clamping, active-marker projection, and isCompletedInventory
classifier.
- sdk/scripts/gen-workstream-inventory-builder.mjs — generator.
Captures function bodies via Function.prototype.toString();
emits with the standard GENERATED FILE banner. Includes a
small `const relative = path.relative;` preamble in the
output to handle ESM destructured imports in the compiled
source.
- sdk/scripts/check-workstream-inventory-builder-fresh.mjs —
freshness check. Imports the generator function directly
(rather than duplicating logic) — a cleaner pattern than
Phase 1/2's approach.
- get-shit-done/bin/lib/workstream-inventory-builder.generated.cjs —
generator-emitted CJS mirror.
- tests/workstream-inventory-builder-generator.test.cjs — 16
parity assertions confirming CJS-generated output ==
SDK source output for every fixture.
- bin/lib/workstream-inventory.cjs: 159 → 132 lines.
Projection logic gone. `inspectWorkstream` and
`listWorkstreamInventories` collect BuilderInputs via the
existing sync fs functions and delegate to the Builder.
`isCompletedInventory` re-exported from the Builder (its
signature changed from object→string, but no external
callers exist so the change is safe).
- sdk/src/query/workstream-inventory.ts: 196 → 143 lines.
Same shape, sync fs (the SDK was already sync — surprise from
recon). Types re-exported from the Builder.
- sdk/package.json: gen:workstream-inventory-builder and
check:workstream-inventory-builder-fresh scripts.
- package.json: proxy for the freshness check.
- .githooks/pre-commit: drift block.
- .github/workflows/test.yml: drift check step.
- CONTEXT.md: amended "Workstream Inventory Module" entry
to document the Builder/Reader split.
- docs/INVENTORY.md, docs/INVENTORY-MANIFEST.json:
+1 module count, +1 row for the generated builder.
- Full suite: 9229/9229 pass (baseline 9215 + 14 net new from
the parity assertions).
- Vitest: 18 Builder fixtures pass.
- Reader shrink: -27 lines on CJS, -53 lines on SDK.
- Net diff (modified files only): +68 / -133 = 65-line
reduction. New files (Builder, generator, freshness check,
parity test) add ~600 lines of new structured code.
1. `isCompletedInventory` signature changed from
isCompletedInventory(inventory: object) to
isCompletedInventory(status: string). Original CJS exported
the object form but no external caller passed an object —
they all passed inventory.status. Verified by grep before
committing.
2. Generator preamble. The compiled ESM uses
`import { relative } from 'node:path'`, making `relative`
a free variable in `buildWorkstreamInventory`. The generator
emits `const relative = path.relative;` so the captured
function body works in CJS.
3. Freshness check imports the generator. The freshness check
imports the generator's buildWorkstreamInventoryBuilderCjs()
function directly rather than duplicating generation logic.
Cleaner than Phase 1/2; future generators should follow this.
Shareable via the Builder/Reader pattern in future enhancements:
- frontmatter (pure YAML/markdown parsing)
- plan-scan (pure PLAN.md structure parsing)
- decisions (pure decision-record parsing)
- secrets (regex-based detection in text)
- uat (UAT-criteria parsing)
Structural divergence — different approach needed:
- state — sync vs async file ops; mutation paths differ.
- workstream — lifecycle ops; per-side API surface differs.
- phase, roadmap, init, profile-output, template — large
surfaces; each its own potential enhancement.
None of these is in scope for Phase 3.
Closes#3544.
Phase 2 of the CJS↔SDK hard-seam migration (parent #3524).
Eliminates the structural drift surface that produced bug class
After this phase, neither bin/lib/ nor sdk/src/ defines
CONFIG_DEFAULTS, VALID_CONFIG_KEYS, DYNAMIC_KEY_PATTERNS, or the
four legacy-key normalizations inline. All come from one canonical
source: the Configuration Module (sdk/src/configuration/index.ts)
+ two JSON manifests (sdk/shared/config-{defaults,schema}.manifest.json).
The CJS mirror is generator-emitted (get-shit-done/bin/lib/configuration.generated.cjs)
with a CI freshness check (sdk/scripts/check-configuration-fresh.mjs).
- sdk/shared/config-defaults.manifest.json — canonical nested defaults,
union of CJS + SDK keys (includes security_*, post_planning_gaps,
agent_skills, mode, every git/workflow/hooks sub-section).
- sdk/shared/config-schema.manifest.json — VALID_CONFIG_KEYS array,
RUNTIME_STATE_KEYS array, DYNAMIC_KEY_PATTERNS array with source
strings (regex reconstructed at runtime).
- sdk/src/configuration/index.ts — source of truth. Exports
loadConfig (pure read), normalizeLegacyKeys (pure, idempotent,
returns Normalization[]), mergeDefaults (deep-merge), migrateOnDisk
(explicit opt-in disk writeback), plus CONFIG_DEFAULTS,
VALID_CONFIG_KEYS, RUNTIME_STATE_KEYS, DYNAMIC_KEY_PATTERNS.
- sdk/src/configuration/index.test.ts — 29 vitest pinning tests.
- sdk/scripts/gen-configuration.mjs — generator (Function.prototype.toString()
inspection of compiled SDK dist, plus brace-balanced text scan for
internal helpers, matching the Phase 1 pattern).
- sdk/scripts/check-configuration-fresh.mjs — CI freshness gate.
- tests/configuration-generator.test.cjs — 27 parity assertions
(CJS-generated == SDK source).
- tests/configuration-migrate-config.test.cjs — 3 cases for the new
gsd-tools migrate-config subcommand.
- bin/lib/core.cjs: CONFIG_DEFAULTS literal now sources values from
CANONICAL_CONFIG_DEFAULTS (the manifest), with a thin flat
projection at the load boundary to preserve the existing
flat-shape return contract for the ~21 CJS test files and 100+
consumers. All four legacy-key migration blocks (branching_strategy,
sub_repos, multiRepo, depth — historically lines 351-358, 388-397,
401-408, 416-423) collapse to a single normalizeLegacyKeys call
in each code path. The inline platformWriteSync writeback stays
for now to preserve sync loadConfig semantics; the new async
migrateOnDisk is reachable via gsd-tools migrate-config.
- bin/lib/config-schema.cjs: 135 → 31 lines. Re-exports from the
generated Module.
- bin/lib/config.cjs: adds cmdMigrateConfig handler (calls
migrateOnDisk on the explicit user-driven path).
- bin/gsd-tools.cjs: wires migrate-config into command dispatch.
- sdk/src/config.ts: re-exports CONFIG_DEFAULTS and mergeDefaults
from the Module. loadConfig now calls normalizeLegacyKeys before
mergeDefaults (replaces the inline branching_strategy graft).
- sdk/src/query/config-schema.ts: 160 → 36 lines. Re-exports from
the Module.
- tests/config-schema-sdk-parity.test.cjs: refactored from
"CJS Set equals SDK Set" (trivially true post-migration) to
"both sides source from the manifest" — structural plus runtime
invariant.
- Four other tests that text-grepped source files for valid keys
(plan-review-convergence, bug-3212, bug-2492, feat-3210) are
updated to use runtime VALID_CONFIG_KEYS.has() or manifest JSON
lookups.
- CONTEXT.md: new Configuration Module entry with full Interface
contract.
- Root package.json: check:configuration-fresh proxy script.
- sdk/package.json: gen:configuration + check:configuration-fresh.
- .githooks/pre-commit: configuration drift block.
- .github/workflows/test.yml: configuration drift step after the
alias drift check.
- 9201 CJS tests pass (baseline pre-cycle: 9195; +6 net new tests
across migrate-config + parity refactor)
- 1872 SDK vitest tests pass
- 29 Configuration Module vitest fixtures
- 27 CJS/SDK parity fixtures
- Net diff: +388 / −519 = 131-line reduction across the seven cycles,
despite adding the new Module, manifests, generator, freshness
check, and two new test files.
1. SDK CONFIG_DEFAULTS now includes manifest-canonical keys
(resolve_model_ids: false, context_window: 200000, phase_naming,
claude_md_path, git.create_tag, workflow.security_*,
workflow.code_review_*, planning.*, hooks.workflow_guard, ship.*).
Consumers accessing via [key: string]: unknown index get
the manifest default instead of undefined.
2. SDK mergeDefaults is now proper recursive deep-merge instead of
spread-per-section. Overlay { workflow: { research: false } }
now preserves sibling workflow keys; previously it replaced
the entire workflow section with only research + the section's
defaults. Semantically identical for the common case;
strictly better for partial nested overrides.
3. New gsd-tools migrate-config CLI subcommand for the explicit,
opt-in on-disk migration path.
Closes#3536.
* feat(3530): STATE.md Document Module via generator (Phase 1 of #3524)
Phase 1 of the CJS↔SDK hard-seam migration (parent #3524).
Converts the hand-synced state-document.cjs/state-document.ts pair
into a generator-driven seam, modeled on the existing
command-aliases.generated.* precedent.
What landed:
- sdk/src/query/state-document.ts is the source of truth.
- sdk/scripts/gen-state-document.ts emits
get-shit-done/bin/lib/state-document.generated.cjs from the
compiled SDK dist via Function.prototype.toString() inspection
for the 7 public exports and 3 internal helpers.
- sdk/scripts/check-state-document-fresh.mjs is the CI freshness
gate; pre-commit hook also runs it when relevant files change.
- get-shit-done/bin/lib/state-document.cjs is reduced to a one-line
re-export from state-document.generated.cjs so existing callers
(state.cjs, workstream-inventory.cjs, init.cjs) need no changes.
- New CI step in .github/workflows/test.yml after the existing alias
drift check.
- sdk/package.json: gen:state-document, check:state-document-fresh
scripts. tsx added as devDep.
- Root package.json: proxy script for the freshness check.
- CONTEXT.md: one-sentence amendment on STATE.md Document Module
recording the source-of-truth file path.
Tests:
- sdk/src/query/state-document.test.ts: 34 vitest fixtures across
the 7 public exports (TDD pinning safety net).
- tests/state-document-generator.test.cjs: 31 node:test parity
assertions comparing SDK source vs generated CJS for every
fixture.
- Full suite: 9177/9177 pass (baseline was 9146; +31 new tests).
One subtle behavior change worth flagging: the old hand-written
state-document.cjs used String(str) coercion inside escapeRegex,
which the SDK source does not. The generator faithfully matches
the SDK (the source of truth per ADR-3524), so the new CJS no
longer coerces non-string input to string before regex-escaping.
No current caller passes non-string input, so no observable
regression in the test suite. Flagged in the PR body for
reviewers.
Closes#3530.
* fix(3530): address state-document review findings
Closes#3522. When --respect-staged is passed the git add loop is skipped
entirely so per-hunk staging from git add -p is preserved. Default behavior
(full re-stage) is unchanged. The #3061 pathspec invariant holds under both
modes. Nothing-staged within scope returns { committed: false, reason:
'nothing staged' } without error.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes two root causes behind bug #3517:
1. Idempotency: completed_phases was blindly incremented (parseInt + 1),
causing phase.complete N run twice to double-count (4 → 5 → 6).
Now derives from ROADMAP progress table Complete-row count, making
the operation idempotent.
2. Field coverage: eight STATE.md fields were left stale after phase
completion. Now updates in the same atomic lock section:
- frontmatter: stopped_at, last_updated, total_plans, completed_plans
- body: Current focus, Status line, By Phase table row
completed_plans = count of *-SUMMARY.md files across all phase dirs
total_plans = sum of M/N plan counts from ROADMAP progress table
percent = recomputed from fresh derived counts
Closes#3517
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extract composeStatusline() helper from duplicated inline template logic in
runStatusline() and renderStatusline(). Both call sites now route through the
helper, which accepts a position param ('end' | 'front', default 'end').
- 'end' (default) preserves byte-identical output to v1.38.x and earlier
- 'front' renders ctx immediately after model name, before the first │
- Invalid values silently coerce to 'end' at runtime (belt-and-suspenders;
config-set rejects invalid values upfront via enum validator)
Adds statusline.context_position to VALID_CONFIG_KEYS in both CJS and TS
schemas, enum validator in config.cjs, docs row in CONFIGURATION.md,
and a changeset. Closes#2937.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds boolean config key `git.create_tag` (default: true, fully backcompat)
so projects with their own release flow can disable GSD's automatic
`git tag -a v[X.Y]` on milestone completion. Also adds tag-collision
pre-check to prevent silent failure on re-run. Closes#3086
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The `has_git` boolean returned by `init new-project` and `init ingest-docs`
was derived from a shallow `pathExists(cwd, '.git')` check, so a subdirectory
of an existing repo reported `has_git: false`. The workflow then ran
`git init`, creating a nested `.git` inside the outer worktree and silently
diverting subsequent `gsd-sdk commit` calls into the nested repo.
Replace the shallow check with `git rev-parse --is-inside-work-tree`
semantics in both CJS (`get-shit-done/bin/lib/init.cjs`) and TS
(`sdk/src/query/init.ts`, `sdk/src/query/init-complex.ts`) handlers via a new
shared `gitWorktreeInfoInternal` helper, and expose `git_worktree_root` +
`in_nested_subdir` so the workflows can refuse `git init` inside an existing
worktree and warn that planning files will track to the outer repo.
Regression test: `tests/bug-3491-nested-git-worktree.test.cjs`.
Fixes#3491
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The DAG resolver in phase-plan-index only matched full-stem
('03-01-auth-hardening') and canonical-prefix ('03-01') forms when
resolving `depends_on` references, so plans in decimal phases
(e.g. `99.9-test`, `02.2-cross-repo`) declaring short-form
`depends_on: [01]` had their edges silently dropped. Dependents
collapsed into wave 1 and the SDK emitted a misleading
"declared wave: N but depends_on DAG places it in wave 1" warning
that pointed at the wave declaration rather than the broken reference.
Add a tertiary short-form index keyed on the trailing `-NN` of each
plan's canonical ID — derived per plan via `lastIndexOf('-')` so it
handles integer, letter-suffixed, and decimal phase IDs uniformly.
Emit a dedicated `Plan X: unresolved depends_on reference 'NN' — no
matching plan in phase` warning whenever a dep fails all three lookup
forms, so a dropped edge can no longer hide behind the wave-mismatch
warning.
Regression coverage added in `sdk/src/query/phase.test.ts` for the
decimal-phase short-form case, the integer-phase short-form case, and
the unresolved-reference warning.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When a roadmap places shared phase-detail bodies under a non-version-prefixed
`## Phase Details` heading AFTER a `### 📋 vX.Y+ (Planned)` sibling in document
order, the entire Phase Details section fell outside the slice returned by
`extractCurrentMilestone`. `gsd-sdk query phase.insert N` then reported
"Phase N not found in ROADMAP.md" even though `### Phase N:` was unambiguously
present.
PR #2455 (closing #2422) added a same-version `continue` branch that already
handles the version-prefixed variant (e.g. `## v2.0 Phase Details`). This fix
extends the same intent to the generic-label variant: after the initial
boundary scan, look for a literal `^#{1,3}\s+Phase\s+Details\b` heading past
`sectionEnd` and, if found, append the Phase Details block (up to the next
real milestone boundary — version-bearing or milestone-emoji-bearing heading —
or EOF) to the returned slice. The intervening planned-milestone content is
skipped so it does not leak into the active-milestone view.
Bounded to a single append so a malformed roadmap can't loop. Only matches the
literal `Phase Details` label (canonical per GSD ROADMAP template); anything
else continues to terminate the slice. Does not regress the v2.0/v2.1+
version-prefixed handling shipped by #2455 (existing `bug-2422` test still
passes).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(execute-phase): classify quota/rate-limit failures across runtimes (#3095)
Dispatched executor subagents that die from provider quota or rate-limit
errors currently look identical to a crashed agent to the orchestrator —
so step 7's recovery prompt offers "retry now" when the right action is
"wait for reset and resume". This adds a runtime-agnostic classifier and
wires execute-phase step 7 to it.
- `agent.classify-failure` SDK query returns
`{class: 'quota-exceeded' | 'classify-handoff-bug' | 'unknown-failure',
sentinel?, retryAfterSeconds?}`. Sentinels cover Claude Code
(`usage limit`, `429`), Copilot CLI (`rate_limit`,
`user_weekly_rate_limited`), Codex (`usage_limit_reached`,
`too many requests`), and Gemini (`RESOURCE_EXHAUSTED`,
`exceeded your`).
- `execute-phase.md` step 7 now branches on the class. Quota-exceeded
presents a wait-for-reset prompt and points at the safe-resume gate
landing in #3212 instead of re-dispatching a fresh executor.
- `docs/research/provider-rate-limit-signals.md` records the proactive
(header / SDK event) signals each provider exposes and the upstream
Claude Code / Copilot / Codex issues blocking hook-side detection —
the forward path once host runtimes surface them.
Resume-from-partial-worktree and context-load metrics from the original
report are deliberately out of scope; they overlap #3212's
`state.verify-against-disk` work already in flight.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(execute): render quota retry hint and refresh alias artifacts
* fix(workflow): restore slash namespace and execute-phase size budget
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
