golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity
2,268 Commits 1 Branch 0 Tags
1e6737cd8ee114bf0ea2a23619bc582534ad7d2d
Commit Graph

2268 Commits

This Branch
This Branch
All Branches
Author SHA1 Message Date
Tom Boucher
1e6737cd8e feat(plan-phase): --research-phase flag + scrub stale slash-command refs (#3042, #3044) (#3045) 
* feat(plan-phase): --research-phase flag absorbs deleted /gsd-research-phase + scrub stale refs (#3042, #3044)

#3042 (orphaned research-phase): /gsd-research-phase had a workflow file
but no slash-command stub. Rather than restore the orphan, the research-
only capability is now a flag on /gsd-plan-phase:

  /gsd-plan-phase --research-phase <N>

When set, the workflow scopes to phase N, runs the research step (Section
5 of the existing plan-phase workflow), then early-exits before the
planner/plan-checker/verifier chain.

Per RCA against the deleted standalone, the flag adds two modifiers to
fully cover the original surface (Option B from the RCA discussion):

- --view : print existing RESEARCH.md to stdout, no spawn. Cheapest mode
  for the correction-without-replanning loop the issue reporter
  explicitly called out. Errors with a clear hint if RESEARCH.md is
  missing.
- --research : reuse the existing "force re-research" semantics. In
  research-only mode this skips the existing-RESEARCH.md prompt and
  re-spawns unconditionally.
- Neither flag, RESEARCH.md exists : prompt update/view/skip. Mirrors
  the deleted standalone's existing-artifact menu (#3042 RCA).

#3044 (stale slash-command refs): scrubbed five deleted commands from
all user-facing surfaces, including English docs, 4 localized doc sets
(ja-JP, ko-KR, zh-CN, pt-BR), workflows, templates, and references.

  /gsd-check-todos          → /gsd-capture --list
  /gsd-new-workspace        → /gsd-workspace --new
  /gsd-status               → /gsd-progress
  /gsd-plan-milestone-gaps  → table rows / orphan sections removed
                              (PR #3038 only scrubbed workflows/agent;
                              missed the docs surfaces this PR covers)
  /gsd-research-phase       → /gsd-plan-phase --research-phase

Includes a fix to docs/issue-driven-orchestration.md (PR #3036)
which itself referenced /gsd-new-workspace 4 times — self-correction.

Removed:
- get-shit-done/workflows/research-phase.md (orphan, capability
  absorbed into --research-phase flag)

Tests:
- tests/bug-3042-3044-research-flag-and-stale-refs.test.cjs — 46
  structural-IR tests across both bugs:
  - argument-hint advertises --research-phase + --view
  - workflow parses --research-phase, sets RESEARCH_ONLY,
    early-exits before planner
  - --view prints RESEARCH.md without spawning
  - --research forces refresh in research-only mode
  - existing-RESEARCH.md prompt path with update/view/skip
  - workflows/research-phase.md is removed
  - 5 deleted slash-commands absent from 17 English user-facing
    surfaces + 16 localized doc surfaces (4 locales × 4 docs each)
  - replacement command tokens present where deleted ones lived

6950/6950 full suite pass. Lints clean.

Closes #3042
Closes #3044

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix: address all 8 CR findings on PR #3045

Major (3):
- get-shit-done/workflows/plan-phase.md:344 — added explicit early-exit
  guard at Section 5.1: "Skip if RESEARCH_ONLY=true". Without it, an LLM
  could fall through "use existing, skip to step 6" → planner spawn,
  violating the research-only contract. The guard makes the early-exit
  unreachable from any non-research-only branch.
- get-shit-done/references/continuation-format.md (3 examples) +
  zh-CN/.../continuation-format.md (3 examples) — pointed to
  `/gsd-plan-phase --research-phase` but docs/COMMANDS.md didn't
  document the flag. Added a full --research-phase + --view + --research
  modifier section to the /gsd-plan-phase flag table in COMMANDS.md so
  the canonical reference matches the continuation examples.

Minor (5):
- docs/FEATURES.md:1632 — `/gsd-plan-phase --research-phase` →
  `/gsd-plan-phase --research-phase <N>` (include required arg).
- get-shit-done/templates/README.md:46 — NN-VALIDATION.md producer
  reverted from `/gsd-plan-phase --research-phase` (Nyquist) to plain
  `/gsd-plan-phase` (Nyquist). VALIDATION.md is created during normal
  Nyquist flow, not research-only mode — the bulk replacement was
  wrong for that line.
- get-shit-done/workflows/help.md:89 — signature line was missing
  `--research`; added it alongside `--research-phase` and `--view`.
- tests/bug-3042-3044-...:197 — promptHasView/promptHasSkip were
  tautological (matched anywhere in 1700-line workflow). Tightened
  to a proximity check anchored on "RESEARCH.md already exists" prompt
  header within a 600-char window. Updated workflow to emit that
  literal phrase.
- tests/feat-2840-...:95 — workspace assertion used `/gsd-workspace`
  but the documented replacement is `/gsd-workspace --new`. Tightened
  to require both tokens (in 3 places: requiredCommands list, regex
  in conceptPairs, error message).

6950/6950 full suite pass. Lint clean.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-02 23:12:50 -04:00
Tom Boucher
dca12242b5 fix(install): skip Gemini local commands/gsd when global GSD present (#3037) (#3041) 
* fix(install): skip Gemini local commands/gsd when global GSD present (#3037)

Reporter showed that running `npx get-shit-done-cc --gemini --global`
followed by `--gemini --local` in a project creates the same 65 GSD
command files in both Gemini scopes:
  - ~/.gemini/commands/gsd/ (user scope)
  - <project>/.gemini/commands/gsd/ (workspace scope)

Gemini conflict-detects by command name across scopes and renames every
overlapping /gsd:* command to /workspace.gsd:* and /user.gsd:*, breaking
the documented /gsd:* namespace.

Fix: in bin/install.js, when handling --gemini --local, detect whether
~/.gemini/commands/gsd/ already exists with managed-shape content. If
so, skip the local copy and print a clear three-line warning explaining
the conflict avoidance. The user-scope install already provides the
same /gsd:* commands in this project; the local copy adds zero value.

Sibling fixes (test isolation):
- tests/install-minimal-all-runtimes.test.cjs: pass HOME/USERPROFILE
  through the spawned installer's env so the developer's real
  ~/.gemini/commands/gsd/ doesn't trigger the new skip path during
  test runs that want to assert the local-install populates
  commands/gsd/.
- tests/gemini-namespacing.test.cjs: the "Gemini Install (Behavioral)"
  describe block now creates an isolated tmpHome and points
  process.env.HOME at it before calling install(false, 'gemini'),
  with proper restore in afterEach.

Test:
- tests/bug-3037-gemini-duplicate-commands.test.cjs — 4 structural
  tests:
    1. global install populates HOME/.gemini/commands/gsd
    2. local install AFTER global skips the local copy
    3. local install with NO existing global still populates locally
       (no-regression)
    4. local install when HOME has .gemini/ but no GSD-managed
       commands/gsd/ still populates locally (non-GSD-Gemini-user
       no-regression)

6909/6909 full suite pass. Lints clean.

Closes #3037

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix: address CR feedback on PR #3041 — narrower detection + USERPROFILE restore

CR findings:

1. **bin/install.js (Major)** — userScopeHasGsd used
   `fs.readdirSync(homeGeminiGsd).length > 0` which would skip the
   local install for any non-empty directory, including a user who
   hand-dropped a single override at ~/.gemini/commands/gsd/<thing>
   .toml without ever running --gemini --global. Narrowed the
   detection to require at least 3 canonical GSD command files
   (help.toml, progress.toml, new-project.toml) — a marker that
   ships in every GSD Gemini install (minimal mode included) and is
   structurally impossible to produce by accident.

2. **tests/bug-3037-...:59 (Minor)** — beforeEach overwrites
   process.env.USERPROFILE but afterEach only restores HOME, leaking
   the temp home into later tests on Windows or any code path that
   reads USERPROFILE. Added save/restore symmetric with HOME.

Plus added a 5th regression test covering the narrowed detection:
"local install when HOME has hand-dropped overrides UNDER commands/gsd/
(but no full GSD) still populates locally" — directly exercises the
edge case CR identified.

5/5 targeted tests pass. 6910/6910 full suite pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-02 17:44:52 -04:00
Tom Boucher
7714b5244b fix(workflows,docs): scrub stale /gsd-code-review-fix and /gsd-plan-milestone-gaps refs (#3029, #3034) (#3038) 
* fix(workflows,docs): scrub stale /gsd-code-review-fix and /gsd-plan-milestone-gaps refs (#3029, #3034)

#2790 consolidated /gsd-code-review-fix into /gsd-code-review --fix and
deleted /gsd-plan-milestone-gaps in favor of inline gap planning as part
of /gsd-audit-milestone's output. The deletion was propagated through
some surfaces (#2950 covered help/do/settings/discuss-phase/etc.) but
several user-facing surfaces still emitted the old forms:

#3029 — /gsd-code-review-fix references in:
- agents/gsd-code-fixer.md (description, "Spawned by", recovery prose)
- get-shit-done/workflows/code-review.md (offer text)
- get-shit-done/workflows/execute-phase.md (offer text)
- get-shit-done/workflows/code-review-fix.md (internal retry hints)
- docs/INVENTORY.md (agent + workflow rows)
- docs/CONFIGURATION.md (workflow.code_review row)
- docs/USER-GUIDE.md (3 occurrences in walkthrough)
- docs/AGENTS.md (gsd-code-fixer agent stub)
- docs/FEATURES.md (commands list + REQ-REVIEW-04)

All replaced with /gsd-code-review --fix. Internal retry hints in the
workflow file itself updated to point at the new form. Release notes
(docs/RELEASE-*.md) and gsd-ns-review's "absorbed by" deletion note
left unchanged — historical/explanatory content.

#3034 — /gsd-plan-milestone-gaps references in:
- get-shit-done/workflows/audit-milestone.md (<offer_next> blocks for
  gaps_found and tech_debt: lines 281, 323)
- commands/gsd/complete-milestone.md (gaps_found pre-flight: lines 46, 57)

Replaced with inline closure path:
  /gsd-phase --insert <N> "Close gap: <REQ-ID> ..."
  /gsd-discuss-phase <N>
  /gsd-plan-phase <N>
  /gsd-execute-phase <N>

Plus a Nyquist-coverage hint pointing at /gsd-validate-phase /
/gsd-secure-phase for retroactive audit-chain hygiene gaps. The
gsd-ns-project SKILL.md "deleted by #2790" note is preserved
(it's the canonical pointer for future readers asking what
happened to the command).

Tests:
- tests/bug-3029-3034-stale-command-routes.test.cjs — parser-based
  assertions per fixed surface, plus a structural cross-check that
  gsd-ns-project keeps the deletion note. 15 tests, all green.
- 6905/6905 full suite passes.

Closes #3029
Closes #3034

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix: address CR feedback on PR #3038 — argument order, structural tests, agent count

CR findings on PR #3038:

1. **docs/USER-GUIDE.md (Major)** — `--fix` examples used flag-first form
   (`/gsd-code-review --fix 3`), but the supported CLI grammar is
   phase-first (`/gsd-code-review 3 --fix`). The original sed-based
   replacement preserved the position of the `gsd-code-review-fix`
   token, producing the wrong order. Fixed in USER-GUIDE.md (3
   occurrences) and the same drift in the workflow surfaces:
   - get-shit-done/workflows/code-review-fix.md (2 retry hints)
   - get-shit-done/workflows/code-review.md (offer text)
   - get-shit-done/workflows/execute-phase.md (offer text)

2. **docs/AGENTS.md (Minor)** — internal count drift: line 483 said
   "Ten additional agents" but line 725 said "12 advanced/specialized".
   Filesystem reality: 33 agents total, 21 primary, 12 specialized
   (count of `### ` stubs in the Advanced and Specialized section).
   Updated lines 3, 13, 483 to use 12/33 and added the two missing
   names (doc-classifier, doc-synthesizer) to the inline list at
   line 13.

3. **tests:94 (Major refactor suggestion)** — `.includes()` token checks
   were source-grep style. Refactored to a typed-IR pattern: extract
   the SET of slash-command tokens via regex, assert membership on the
   parsed Set instead of substring scanning the raw file text. Added
   the `allow-test-rule` comment explaining the IR-build vs
   IR-assertion split per scripts/lint-no-source-grep.cjs convention.

4. **tests:130 (Major)** — replacement-path assertion was file-wide and
   could false-pass on generic mentions of "inline" elsewhere in the
   file. Refactored: `extractOfferBlocks(content)` returns the typed
   list of `<offer_next>` and "Pre-flight" blocks where the deleted
   command previously lived, and the assertion runs against those
   blocks specifically. Now requires `/gsd-phase --insert` or
   inline-audit prose to appear in the same offer block, not just
   somewhere in the file.

15/15 targeted tests pass. 6905/6905 full suite pass. Lints clean.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-02 17:23:44 -04:00
Tom Boucher
117b3ec009 docs: add issue-driven orchestration guide (#2840) (#3036) 
* docs: add issue-driven orchestration guide (#2840)

Adds docs/issue-driven-orchestration.md — a recipe for driving GSD from a
GitHub / Linear / Jira issue using existing primitives. Maps Symphony-style
orchestration concepts onto GSD commands without vendoring code, adding a
daemon, or introducing tracker integration.

Concept mapping covers:
- WORKFLOW.md → ROADMAP.md / STATE.md / phase CONTEXT.md / phase PLAN.md
- isolated agent workspace → /gsd-new-workspace --strategy worktree
- agent dispatch → /gsd-manager (interactive), /gsd-autonomous (unattended)
- per-phase steps → /gsd-discuss-phase → /gsd-plan-phase → /gsd-execute-phase
- proof-of-work → /gsd-verify-work (UAT.md persists across /clear)
- adversarial review → /gsd-review (cross-AI peer review)
- human merge gate → /gsd-ship
- follow-up capture → /gsd-note, /gsd-plant-seed, /gsd-new-milestone

End-to-end flow walks through 7 numbered steps from picking the tracker
issue to capturing follow-ups. Safety boundaries (isolated worktrees,
explicit human review, no automatic public posting, verification before
ship) and non-goals (no vendoring, no daemon, no mandatory tracker, no
gate bypass, no command-surface expansion) are spelled out explicitly so
the doc cannot drift into "let's just add one more flag".

Cross-linked from docs/README.md (Documentation Index) and
docs/USER-GUIDE.md (Table of Contents preamble).

Tests: tests/feat-2840-issue-driven-orchestration-guide.test.cjs — 9
structural-IR tests parse the guide into a typed record and assert on
flags (commandsPresent, conceptPairs, nonGoalFlags, safetyFlags,
numberedSteps). Fence-language MD040 check enforced. Cross-link
presence enforced. No raw-text assertions on prose.

6890/6890 tests pass. Lint:tests clean (allow-test-rule comment justifies
the doc-shape parser per scripts/lint-no-source-grep.cjs escape hatch).
Lint:changeset clean.

Closes #2840

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(test): guard USER-GUIDE.md existsSync before read (CR #3036)

CR Minor: cross-linked-from-USER-GUIDE.md test called fs.readFileSync
directly without first asserting fs.existsSync, asymmetric with the
README.md test above. A missing USER-GUIDE.md would throw ENOENT instead
of producing a meaningful assertion message. Mirror the null-guard
pattern.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-02 16:57:42 -04:00
Tom Boucher
95d2bc20f8 feat(hooks): opt-in SessionStart update banner for non-statusline users (#2795) (#3035) 
* feat(hooks): opt-in SessionStart update banner for non-statusline users (#2795)

When a user declines (or keeps a non-GSD) statusline at install time, the
installer now offers an opt-in SessionStart banner that surfaces GSD update
availability. The banner reads the existing
~/.cache/gsd/gsd-update-check.json cache (written by
gsd-check-update-worker.js) and emits a single systemMessage line only when
update_available is true:

    GSD update available: <installed> → <latest>. Run /gsd-update.

It is silent when up-to-date and rate-limits "check failed" diagnostics to
once per 24h via a sentinel file so a corrupt cache doesn't nag every
session. Removed cleanly by `npx get-shit-done-cc --uninstall` which strips
both the script and the SessionStart entry. The banner is never offered when
GSD's statusline is being installed (statusline already surfaces update
info, so re-prompting would be noise).

Implementation:
- hooks/gsd-update-banner.js — pure functions buildBannerOutput,
  shouldSuppressFailureWarning, readCache; thin main() wires them.
- bin/install.js — handleUpdateBanner() prompt, parseUpdateBannerInput(),
  buildUpdateBannerHookEntry(), buildUpdateBannerPromptText(); chained into
  installAllRuntimes() so finalize() receives both flags. updateBannerCommand
  computed alongside the other JS-hook commands; finishInstall() registers
  the SessionStart entry only when shouldInstallBanner === true and the
  hook file is present at the target.
- Hook ships in scripts/build-hooks.js HOOKS_TO_COPY, listed in
  MANAGED_HOOKS for stale-detection in gsd-check-update-worker.js, in the
  uninstall hook-removal lists in install.js, and in the
  rewriteLegacyManagedNodeHookCommands allowlist.

Tests:
- tests/feat-2795-update-banner.test.cjs — 22 tests, structural-IR
  assertions on parsed JSON envelopes (no raw-text matching). Covers
  pure-function branches (cache present/absent, parseError, rate-limit
  suppression, missing version fields), end-to-end hook invocation against
  fixture cache states, and install.js wiring (prompt text, input parsing,
  hook entry shape).
- tests/trae-install.test.cjs — updated install() return-shape assertion to
  include updateBannerCommand: null for the no-settings runtime.
- 6881/6881 tests pass.

Docs (bundled in same commit per the bundle-docs-with-code skill):
- docs/USER-GUIDE.md — new "Surface GSD Update Notifications Without GSD's
  Statusline" task section with opt-in/opt-out instructions.
- docs/FEATURES.md — REQ-HOOK-08 added; "Update Banner" subsection under
  the Hook System feature with cache flow + removal path.
- docs/INVENTORY.md — hook count 11 → 12, new row for gsd-update-banner.js.
- docs/INVENTORY-MANIFEST.json — regenerated.

Closes #2795

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(install): gate banner prompt on actual installability (CR #3035)

CodeRabbit findings on PR #3035:

- bin/install.js (Major): continueAfterStatusline gated banner prompt on
  the raw `shouldInstallStatusline` flag from handleStatusline. But
  finishInstall later silently skips the statusline write on local
  installs unless --force-statusline is set (#2248). Two consequences:
    1. Interactive local Claude/Gemini installs got neither a statusline
       nor a banner offer.
    2. Codex/Cursor/Copilot/Windsurf/Trae/Cline-only installs (where
       every result.updateBannerCommand is null) still got prompted even
       though the choice was silently ignored.
  Fix: derive willInstallStatusline = shouldInstallStatusline &&
  (isGlobal || forceStatusline), and gate the banner prompt on a
  canInstallBanner precondition computed from results[].updateBannerCommand.
  Pass the raw shouldInstallStatusline through to finalize unchanged so
  per-runtime statusline gating in finishInstall is unaffected.

- tests/feat-2795-update-banner.test.cjs (Minor): rate-limit suppression
  test parsed r1.stdout without first asserting r1.status === 0. Other
  e2e tests in this file (lines 210, 241) do this. A non-zero exit would
  surface as a cryptic SyntaxError instead of a status assertion failure.
  Fix applied verbatim.

6881/6881 tests pass.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-02 16:33:16 -04:00
Tom Boucher
35fffe7f31 docs(out-of-scope): record #2758 agent-template-rendering decision 
Closed on the technical merits: the determinism claim is theoretical (no
observed misinterpretation), token waste is small and unmeasured, and PR
#2279's orchestrator-embedding path already serves the deterministic-gating
need without a parallel templating subsystem.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-02 15:56:24 -04:00
Tom Boucher
d137ce86ec docs(out-of-scope): record #2756 temporal-context decision 
Reporter did not return to clarify the actual ask after the narrowing-then-
retraction in the comment thread. Closing as wontfix per .out-of-scope/
temporal-context.md with re-open criteria spelled out.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-02 15:53:08 -04:00
Tom Boucher
8c43ba7301 docs(#3025): MCP tool schema as a context-budget concern (#3032) 
* docs(#3025): MCP tool schema as a context-budget concern

Adds documentation covering the largest GSD cost lever that GSD
itself does not own: MCP tool schema injection. Every enabled MCP
server adds its schema to every turn (often 20k+ tokens for
heavyweight servers like browser/playwright, mac-tools, etc.),
which can dwarf whatever `model_profile` tuning saves.

Two doc surfaces (per the bundle-docs-with-code skill depth gradient):

1. get-shit-done/references/context-budget.md
   - New "MCP Tool Schema Cost (Harness Concern)" section.
   - Explains schemas-per-turn cost framing.
   - Names enabledMcpjsonServers / disabledMcpjsonServers and
     .claude/settings.json explicitly.
   - Pre-phase audit checklist: browser/playwright, platform-specific,
     cross-project/stale, duplicate/shadow.
   - Explicit "GSD does not manage MCP enablement — harness concern"
     statement so users don't hunt for a GSD setting.
   - Links to Anthropic Claude Code MCP docs as canonical reference.
   - Notes compounding interaction with model_profile (additive levers).

2. docs/USER-GUIDE.md
   - New task-oriented "Trim MCP servers to reduce per-turn cost"
     section above "Using Non-Claude Runtimes".
   - Same checklist condensed.
   - Cross-link to context-budget.md for the full reference.

Tests:
- tests/feat-3025-mcp-token-budget-docs.test.cjs (12 cases) parses
  both docs into typed semantic-flag records and asserts behavioral
  invariants (mentions key, includes audit, names harness, etc.)
  rather than substring-matching prose. Adheres to CONTRIBUTING.md
  no-source-grep — section can be reworded freely as long as the
  required semantics survive.
- Markdownlint pre-flight tests (MD040 fence language, MD056 table
  column count) per the bundle-docs-with-code skill so CR can't
  ratchet on prose nitpicks across multiple review rounds.

Verification:
- 12/12 pass on regression test
- 6857/6857 full suite (12 net new)
- lint-no-source-grep clean (377 test files)

Companion to #3023 (per-phase-type model map) and #3024 (dynamic
routing). Together they cover the three biggest cost levers users
ask about; this issue covers the one GSD does not own.

Closes #3025

* docs(#3025): batch 3 CR fixes — pr id, relative link, named flag

CodeRabbit on PR #3032 (3 minor — 2 inline + 1 nitpick), all in one
push per the bundle-docs-with-code skill (avoid per-round nitpick
ratchet):

1. Inline (Minor) — .changeset/mcp-token-budget-docs.md:3
   `pr: TBD` → `pr: 3032` so changeset tooling can link the entry.

2. Inline (Minor) — docs/USER-GUIDE.md:1101
   Used a hardcoded `https://github.com/.../blob/main/...` URL for the
   cross-link to `context-budget.md`. Rest of USER-GUIDE.md uses
   relative links. Switched to `../get-shit-done/references/context-
   budget.md#mcp-tool-schema-cost-harness-concern` so feature-branch
   work shows the right content and rename-resilience is preserved.

3. Nitpick — tests/feat-3025-mcp-token-budget-docs.test.cjs:234
   The cross-link assertion used an inline `/context-budget/i.test(...)`
   while every other invariant in the file lived as a named flag in
   `parseMcpBudgetSection`. Per CONTRIBUTING.md no-source-grep, added
   `crossLinksContextBudget` to the parser and asserted on
   `parsed.crossLinksContextBudget` so the cross-link rule sits next
   to its siblings.

Verification:
- 12/12 pass on regression test (no count change; refactor only)
- No source code changes, only docs + tests

* test(#3025): strip inline markdown before phrase-match (CR nitpick)

CodeRabbit caught that the `explainsHarnessNotGsd` primary regex
branch couldn't match "GSD does **not** manage" in
context-budget.md because the markdown bold markers (`**`) sit
between contiguous words. The test passed today only via the
fallback `harness (concern|setting|controlled)` branch — the
primary branch was effectively dead code.

Fix: strip inline markdown emphasis (`**`, `*`, `~~`) and inline-
code backticks before any phrase-matching in `parseMcpBudgetSection`.
All seven flag computations now run against the stripped text so
markdown formatting can't silently invalidate any invariant.

Underscores are intentionally NOT stripped — `model_profile` and
other snake_case identifiers must survive intact for the
mentionsModelProfileInteraction check to find them.

Verification: 12/12 still pass; primary branches now fire on
real markdown content rather than relying on fallbacks.

* test(#3025): guard markdownlint tests against null section (CR nitpick)

CodeRabbit caught that the MD040 and MD056 markdownlint pre-flight
tests called `section.match(...)` and `section.split('\n')`
directly on the value returned by `extractSection`, which returns
null when no matching header is found. If the MCP section is ever
removed (regression), both tests would throw `TypeError: Cannot
read properties of null` instead of producing a clean assertion
failure naming the actual problem.

The semantic tests above are protected because parseMcpBudgetSection
short-circuits to a typed-falsy record on null input. The
markdownlint tests bypassed that guard since they need raw section
text, not parsed flags. Added `assert.ok(section, ...)` preconditions
to both so a missing section produces a meaningful failure message.

No content changes; defensive programming only.

Verification: 12/12 still pass.
 2026-05-02 15:24:26 -04:00
Tom Boucher
e1d661ece0 feat(#3024): dynamic routing with failure-tier escalation (#3031) 
* feat(#3024): dynamic routing with failure-tier escalation

Adds a `dynamic_routing` block to .planning/config.json that lets
the resolver start agents on a cheap tier and escalate one tier up
when the orchestrator detects a soft failure (verification
inconclusive, plan-check FLAG, etc.). Solves the "pay Opus rates as
insurance" anti-pattern by making escalation observed-quality-driven.

Architecture:
- AGENT_DEFAULT_TIERS map (light/standard/heavy) — every agent in
  MODEL_PROFILES declares a default tier; tests assert coverage
  so adding a new agent without updating the map fails CI.
- nextTier(currentTier) helper — light → standard → heavy → heavy
  (heavy stays at heavy; can't go further).
- resolveModelForTier(cwd, agentType, attempt) — new resolver. The
  orchestrator tracks the attempt counter and passes 0 for the
  first spawn, 1+ on escalation. The resolver caps internally at
  max_escalations so the orchestrator can blindly bump the counter.
- Schema validation: dynamic_routing.enabled / escalate_on_failure /
  max_escalations / tier_models.<light|standard|heavy>. Unknown
  tiers and unknown sub-keys rejected at config-set time.
- SDK schema mirror updated to keep CJS/SDK in lockstep (#2653).

Resolution precedence (highest → lowest):
  1. model_overrides[<agent>]              (full IDs accepted)
  2. dynamic_routing.tier_models[<tier>]   (NEW; escalation-aware)
  3. models[<phase_type>]                  (#3023 phase-type map)
  4. model_profile                         (per-agent column)
  5. Runtime default

Backward compatibility: dynamic_routing is disabled by default
(enabled: false or block omitted). resolveModelForTier short-
circuits to resolveModelInternal in that case, so callers can
adopt unconditionally without breaking existing behavior.

This PR delivers the JS-layer infrastructure: schema + tier map +
resolver. Orchestrator adoption (workflow markdown updates that
detect soft failures and call resolveModelForTier with attempt+1)
is incremental follow-up — verifier / plan-checker / integration-
checker each adopt the protocol when ready.

Tests (23 cases, all structural-IR — no stdout grep):
- Schema invariants: AGENT_DEFAULT_TIERS coverage, VALID_AGENT_TIERS
  exact match, every assignment uses a valid tier
- nextTier helper: light→standard→heavy→heavy, null on invalid input
- Disabled mode: no block + enabled:false both no-op (back-compat)
- Enabled mode: attempt=0 returns default tier model, attempt=1
  escalates, beyond max_escalations caps, heavy agents stay heavy,
  default max_escalations=1 when omitted
- Precedence: per-agent override beats dynamic_routing,
  dynamic_routing beats phase-type models
- Validation: every settings key accepted, unknown tiers/sub-keys
  rejected, bare `dynamic_routing` rejected as config-set target

Documentation:
- get-shit-done/references/model-profiles.md — full reference section
- docs/CONFIGURATION.md — full settings table + escalation flow
- docs/USER-GUIDE.md — task-oriented "Cheap-by-default" section
- docs/FEATURES.md — config row cross-link

Verification:
- 23/23 pass on regression test
- 6843/6843 full suite (23 net new from 6820)
- lint-no-source-grep clean (376 test files)
- SDK schema mirror keeps CJS/SDK in sync per #2653 parity test

Closes #3024

* fix(#3024): honor escalate_on_failure:false + 3 CR follow-ups

CodeRabbit on PR #3031 (4 findings — 1 Major + 2 Minor + 1 Nitpick):

1. **Major (inline)** — get-shit-done/bin/lib/core.cjs:1668
   resolveModelForTier ignored dynamic_routing.escalate_on_failure.
   When the user set it to false, escalation should be disabled, but
   the resolver only checked attempt/max_escalations. An orchestrator
   that always passes attempt+1 on retry would silently escalate
   despite the user opting out.
   Fix: gate effectiveAttempt on `dr.escalate_on_failure !== false`
   so false short-circuits every attempt back to the default tier.

2. **Minor (inline)** — docs/CONFIGURATION.md:123-126
   The dynamic_routing rows in the Core Settings table had 4 cells
   instead of 5 (missing the Options column), breaking the table
   structure. Added explicit Options values for enabled / escalate_on_failure
   / max_escalations rows.

3. **Minor (outside-diff)** — references/model-profiles.md:179-195
   "Resolution Logic" sketch was pre-#3024 and didn't include
   dynamic_routing in the precedence ladder. Updated to a 6-step
   block with dynamic_routing at step 3 (between override and
   phase-type).

4. **Nitpick** — tests/feat-3024-dynamic-routing.test.cjs:189+
   Tests used `if (lightAgent) { ... }` guards that silent-pass
   when AGENT_DEFAULT_TIERS drifts. Replaced all 5 conditional
   skips with `assert.ok(lightAgent, '...')` preconditions so a
   tier-mapping change surfaces as a test failure.

Plus: 2 new regression tests for the Major fix:
- escalate_on_failure:false caps every attempt at default tier
- escalate_on_failure:true (explicit) still escalates normally

Verification:
- 25/25 pass on regression test (23 prior + 2 escalate_on_failure)
- 6845/6845 full suite (2 net new)
- lint-no-source-grep clean

* docs(#3024): align precedence + add fence language tags (CR follow-up)

CodeRabbit (3 minor):

1. docs/CONFIGURATION.md:691 — "Per-Phase-Type Models → Resolution
   precedence" was a 4-step block written pre-#3024; readers got
   contradictory rules between the per-phase-type section and the
   later dynamic_routing section. Updated to the same 5-step ladder
   with dynamic_routing at step 2, and noted that dynamic_routing
   is disabled by default so this section's behavior is unchanged
   when the kill-switch is off.

2. docs/CONFIGURATION.md:770 — escalation-flow code fence missing
   language tag (MD040). Added `text`.

3. references/model-profiles.md:184 — resolution-ladder code fence
   missing language tag (MD040). Added `text`.

No code changes; docs only. Verification: regression test still 25/25.

* docs(#3024): clarify precedence prose — five layers, not four (CR nitpick)

CodeRabbit nitpick: the "Per-Phase-Type Models → Resolution
precedence" prose said "The four layers compose..." but the ladder
above lists five (including Runtime default). Also "dynamic_routing
escalates per-attempt above all of them" misreads as suggesting
dynamic_routing wins over model_overrides — actually overrides still
win at step 1.

Reworded top-down so the precedence direction is unambiguous:
  - model_profile = base
  - models = phase-level override
  - dynamic_routing = per-attempt escalation
  - model_overrides = per-agent exception (top)
  - runtime default = fallback

No code changes; docs only.

* docs(#3024): note escalate_on_failure:false in escalation-flow diagram (CR)

CodeRabbit nitpick: the escalation-flow diagram in
docs/CONFIGURATION.md described the soft-failure → respawn →
tier_models[next_tier_up] path, but didn't surface the
`dynamic_routing.escalate_on_failure: false` kill-switch right next
to it. Users reading the flow diagram (which is the canonical place
to understand attempt behavior) wouldn't see that the kill-switch
overrides the soft-failure branch.

Added a one-paragraph note immediately after the flow listing,
before the tier-sequence example, so the kill-switch is visible
exactly where users decide whether escalation will happen.

No code changes; docs only.
 2026-05-02 14:26:35 -04:00
Tom Boucher
d812c66020 feat(#3023): per-phase-type model map in .planning/config.json (#3030) 
* feat(#3023): per-phase-type model map in .planning/config.json

Adds a new `models` block to .planning/config.json with six phase-type
slots (planning / discuss / research / execution / verification /
completion). Lets users express coarse tuning ("Opus for planning,
Sonnet for the rest") without learning the agent taxonomy.

Resolution precedence (highest → lowest):
  1. Per-agent `model_overrides[agent]`     (full IDs; targeted exception)
  2. Phase-type `models[phase_type]`         (NEW; tier alias)
  3. Profile table (`model_profile`)          (per-agent column)
  4. Runtime default

The three layers compose: `models` defaults a phase, `model_overrides`
carves an exception. Phase-type values are tier aliases (opus/sonnet/
haiku/inherit) so the runtime-resolution chain (#2517) stays correct
end-to-end without further branching.

Implementation:
- model-profiles.cjs: new AGENT_TO_PHASE_TYPE map + VALID_PHASE_TYPES
  set. Each agent in MODEL_PROFILES gets one phase-type assignment;
  tests assert coverage so adding a new agent without updating the
  table fails CI.
- core.cjs (resolveModelInternal): inserts phase-type tier lookup
  between per-agent override and profile-derived tier. Skips runtime
  resolution when the resolved tier is 'inherit' (was previously gated
  only on profile === 'inherit'; phase-type can now produce inherit
  independently).
- core.cjs (loadConfig): pass `parsed.models` through both code paths
  so resolveModelInternal can read it.
- config-schema.cjs + sdk/src/query/config-schema.ts: dynamic-pattern
  validator accepts only the six known phase-types; unknown slots
  rejected at config-set time.

Backward compat: configs without `models` behave exactly as today.

Tests (15 cases, all structural-IR — no stdout grep):
- Schema: AGENT_TO_PHASE_TYPE coverage, VALID_PHASE_TYPES exact match
- Resolver: phase-type alone; per-agent override beats phase-type;
  phase-type beats profile; issue's full example; "inherit"; empty
  block is no-op; no block is no-op
- Validation: each of the 6 slots accepted; unknown slot rejected;
  bare `models` (no slot) rejected

Verification:
- 15/15 pass on new regression test
- 6808/6808 full suite (5 net new), 0 fail
- lint-no-source-grep clean across 375 test files

Closes #3023

* docs(#3023): document `models` per-phase-type config in user-facing docs

Adds `models` block coverage to the three user-facing docs that ship
with each release:

1. docs/CONFIGURATION.md
   - New "Per-Phase-Type Models" section between "Per-Agent Overrides"
     and "Non-Claude Runtimes" with:
       * full example mixing models + model_overrides
       * phase-type → agent mapping table
       * resolution-precedence pseudocode
       * accepted values (tier alias only)
       * "When to use which" decision matrix
       * validation behavior + example error
   - Added `"models": {}` to the Full Schema snippet
   - Added a row for `models.<phase_type>` to the config keys table
     (next to model_profile_overrides for adjacency)

2. docs/FEATURES.md
   - Added a row for models.<phase_type> in the Configurable Settings
     table (right under model_profile)
   - Cross-link to CONFIGURATION.md for the full surface

3. docs/USER-GUIDE.md
   - New task-oriented "Tuning model cost by phase" section above
     "Using Non-Claude Runtimes" — leads with the concrete config
     and shows the override pattern (one-shot phase + targeted exception)
   - Cross-link to CONFIGURATION.md

Verification:
- 29/29 pass on config-schema-docs-parity + docs-update + new feature
  test (parity-check passes, so the config-schema entry I added in the
  feature commit is now matched by a docs row)
- 6808/6808 full suite pass
- lint-no-source-grep clean

Doc style follows the same pattern used by the existing model_profile,
model_overrides, and model_profile_overrides sections — example-led,
table-backed, cross-referenced. Each doc surfaces the feature at the
right depth (reference / settings table / task guide).

* fix(#3023): mirror phase-type tier in resolveReasoningEffortInternal (CR Major)

CodeRabbit caught a real Codex correctness bug + 3 minor docs/test issues:

1. **Major (outside-diff)** — resolveReasoningEffortInternal in core.cjs
   derived its tier exclusively from the profile table, ignoring the
   models.<phase_type> override added in #3023. Failure mode on Codex:

     Config: model_profile=balanced, models.execution=opus, agent=gsd-executor
     resolveModelInternal:           tier=opus    → gpt-5.4
     resolveReasoningEffortInternal: tier=sonnet  → reasoning_effort=medium
                                                     ↑
                                                  WRONG — should be xhigh
                                                  (opus tier on Codex)

   The runtime received a mismatched (model, effort) pair. Mirrored the
   phase-type lookup from resolveModelInternal so both functions derive
   from the same tier source. 'inherit' phase-type returns null effort
   (no runtime entry maps to 'inherit'; let runtime decide).

2. Minor — .changeset/per-phase-type-models.md `pr: TBD` → `pr: 3030`.

3. Minor (outside-diff) — model-profiles.md "Resolution Logic" section
   omitted the new phase-type tier. Updated the 4-step block to a 5-step
   block including `models[phase_type]` between override and profile,
   plus a paragraph noting that `model` and `reasoning_effort` derive
   from the same tier source.

4. Nitpick — added 2 typo-safety tests:
   - models.research = "haiku3" (typo) → falls through to profile
   - models.research = "openai/gpt-5" (full ID) → falls through to profile
   Plus 5 new reasoning_effort tests covering the Major fix:
   - exported correctly
   - phase-type override flips both model AND effort to same tier
   - inherit phase-type returns null effort
   - per-agent override still bypasses phase-type for effort
   - claude runtime ignores models.* (no effort propagation)

Verification:
- 24/24 pass on regression test (15 original + 2 typo-safety + 5 effort + 2 outside-diff related)
- 6815/6815 full suite (7 net new from 6808)
- lint-no-source-grep clean

The reasoning_effort tests are written semantically (phase-type override
must produce the SAME effort as a profile-only opus config) rather than
hard-coding tier-specific effort strings, so changes to the runtime tier
map don't break them.

* fix(#3023): phase-type override beats profile=inherit (CR Major round 2)

CodeRabbit caught another precedence inversion: when
  { model_profile: 'inherit', models: { execution: 'opus' } }
both resolvers short-circuited on `profile === 'inherit'` BEFORE the
phase-type override could be honored. Result: model returned 'inherit'
and reasoning_effort returned null — both contradicting the documented
precedence where models[phase_type] wins over model_profile.

Fix in resolveModelInternal:
- Compute tier from phase-type FIRST. If phase-type is a valid alias,
  it wins. Otherwise, fall back to profile-derived tier OR 'inherit'
  (when profile === 'inherit').
- Gate the runtime-resolution branch on `tier !== 'inherit'` (was
  `profile !== 'inherit'`) so phase-type=opus can flip runtime mapping
  on even when profile=inherit.
- Gate the inherit-return on `tier === 'inherit'` (was
  `profile === 'inherit'`).

Fix in resolveReasoningEffortInternal:
- Remove the `if (profile === 'inherit') return null;` early-return.
- Compute tier from phase-type first, fall back to profile. If
  phase-type is explicitly 'inherit' OR the resolved tier is 'inherit',
  return null (no runtime entry maps to inherit).

Tests added (5 new):
- model: phase-type wins over profile=inherit (with explicit opus, with
  haiku for one phase + planner-without-slot still inheriting)
- model: profile=inherit + no models block → all agents inherit (no
  regression on existing inherit semantics)
- model: profile=inherit + models block but agent has no slot → that
  agent inherits, agents with slots get phase-type tier
- effort: phase-type opus + profile=inherit → produces opus-tier
  effort, NOT null (the original bug)

Verification:
- 27/27 pass on regression test (24 prior + 3 model + 1 effort)
- 6820/6820 full suite (5 net new)
- lint-no-source-grep clean

The effort test reads the expected value by running a profile-only opus
config and comparing — semantic check, not hard-coded effort string. So
runtime tier map changes don't break the test.
 2026-05-02 13:19:15 -04:00
Tom Boucher
c9f5b7daac fix(#3020): probe user shell PATH at install-time, not just process.env.PATH (#3028) 
* fix(#3020): probe user shell PATH at install-time, not just process.env.PATH

The installer's "✓ GSD SDK ready" message was a false positive whenever
the install subprocess's process.env.PATH contained the gsd-sdk shim
but the user's later interactive shells did not. Three known sources
of mismatch on POSIX:

- ~/.local/bin: install subprocess inherits npm/npx-injected PATH;
  user's login shell may not add ~/.local/bin if .profile/.bashrc/
  .zshrc don't.
- nvm/fnm/volta: node version managers shim PATH per-shell, so
  `npm prefix -g` from inside the install subprocess can resolve to
  a different bin dir than the user's interactive shell sees.
- npm-prefix tooling: some installers inject extra PATH entries that
  vanish in fresh sessions.

Result reported on #3011 by @x0rk and @stefanoginella: install prints
✓, but every workflow invocation later fails with
"bash: gsd-sdk: command not found".

Fix:

- isGsdSdkOnPath(pathString?) — now accepts an explicit PATH string.
  Zero-arg form preserves existing behavior (reads process.env.PATH).
  Pure walk, no spawn. Lets callers verify against any PATH source.

- getUserShellPath() — new helper. Probes the user's login shell via
  `$SHELL -lc 'printf %s "$PATH"'` (POSIX). 2-second timeout so a
  misconfigured rc file can't hang the install. Returns null on
  Windows (cross-shell PATH probing requires a different strategy
  per Git Bash / PowerShell / cmd.exe — tracked separately) or when
  the probe fails; callers fall back to process.env.PATH in that case.

- installSdkIfNeeded() — after the existing isGsdSdkOnPath() check
  passes, also verify the shim is reachable from getUserShellPath()
  on POSIX. If install-PATH and user-shell-PATH disagree, downgrade
  to the actionable ⚠ diagnostic from PR #3014 (which has the shim
  location, shell-specific PATH-update commands, and an npx fallback
  note). Routing affected users into PR #3014's diagnostic is the
  point — not silently green-then-red.

Tests:
- bug-3020-install-shell-path-probe.test.cjs (10 tests, structural):
  - isGsdSdkOnPath accepts an explicit PATH (true/false on fixture
    PATH dirs with/without an executable shim)
  - zero-arg form returns a boolean
  - empty string PATH → false
  - getUserShellPath returns string-or-null
  - returns null on Windows
  - returns null when $SHELL unset on POSIX
  - cross-shell mismatch detection: install-PATH and user-PATH that
    differ produce different isGsdSdkOnPath results — the invariant
    the install-time check now exploits
- All assertions on structural records, not console output. Adheres
  to typed-IR / CONTRIBUTING.md "Prohibited: Raw Text Matching".

Verification:
- 10/10 pass on new regression test
- 6768/6768 pass on full suite (5 net-new tests)
- lint-no-source-grep clean

Windows cross-shell coverage (gsd-sdk.cmd resolves under PowerShell
but not Git Bash without a no-extension sibling) is tracked separately
— this PR is the POSIX-side fix and the Windows scaffolding (the
optional pathString arg on isGsdSdkOnPath) that a Windows fix can
build on.

Closes #3020

* fix(#3020): type-guard pathString, last-line PATH parse (CR)

CodeRabbit on PR #3028 (4 findings — 3 actionable + 1 nitpick):

1. .changeset/install-shell-path-probe.md (2 findings):
   - `pr: TBD` → `pr: 3028`
   - Doc said `echo $PATH` but impl uses `printf %s "$PATH"` (chosen
     to avoid shell-dependent echo behavior, e.g. interpreting `-n`).
     Aligned changeset prose with implementation.

2. bin/install.js:9176 — isGsdSdkOnPath(pathString) used
   `pathString !== undefined` to gate the explicit-PATH branch, but
   getUserShellPath() can return null and `null.split()` throws.
   Tightened to `typeof pathString === 'string'` so null / number /
   object inputs fall back to process.env.PATH. Added 2 regression
   tests covering the null and non-string cases.

3. bin/install.js:9232 — getUserShellPath trimmed entire stdout. A
   misconfigured rc file that prints a banner / motd / log line
   BEFORE the printf would pollute the result and incorrectly flip
   the cross-shell check to false. Take the LAST non-empty line
   (PATH itself is single-line) so noise can't hijack the probe.

4. Nitpick: the changeset PR placeholder — covered by (1).

Verification: 12/12 pass on regression test (10 original + 2 new
type-guard tests), 6770/6770 full suite, lint clean.

* docs(#3020): JSDoc references printf %s "$PATH", not echo $PATH (CR)

CodeRabbit caught two stale JSDoc references that still said
`$SHELL -lc 'echo $PATH'` while the implementation uses
`$SHELL -lc 'printf %s "$PATH"'`. echo is undesirable here because:

- POSIX echo's behavior with `-n` / backslash escapes varies across
  shells (bash builtin vs /bin/echo vs zsh) and can introduce
  trailing-newline pollution that the per-line trim now papers over.
- printf is portable and emits exactly the bytes given.

Synced both stale doc strings:
  - bin/install.js:9211 (getUserShellPath JSDoc)
  - tests/bug-3020-install-shell-path-probe.test.cjs:27 (header)

No behavior change — implementation already uses printf.
 2026-05-02 11:45:39 -04:00
Tom Boucher
6df9b44297 fix(#3018): codex adapter must stop and ask, not silently default decisions (#3027) 
* fix(#3018): codex adapter must stop and ask, not silently default decisions

@jon-hendry: running `\$gsd-discuss-phase 81` in Codex Default mode
proceeded toward writing CONTEXT.md / DISCUSSION-LOG.md / checkpoint
artifacts without surfacing the discussion questions to the user. The
generated Codex skill adapter explicitly told it to do that:

  Execute mode fallback:
  - When `request_user_input` is rejected (Execute mode), present a
    plain-text numbered list and pick a reasonable default.

That instruction is wrong for any workflow whose contract is to
discuss with the user (most prominently `$gsd-discuss-phase`). The
fallback now requires the agent to:

1. STOP. Present the questions as a plain-text numbered list, then
   wait for the user's reply.
2. Only proceed without a user answer when one of these is true:
   (a) invocation included --auto / --all,
   (b) user explicitly approved a default for this question, or
   (c) workflow's documented contract permits autonomous defaults.
3. Do NOT write CONTEXT.md, DISCUSSION-LOG.md, PLAN.md, or checkpoint
   files until the user has answered or one of (a)-(c) above applies.

Tests:
- bug-3018-codex-discuss-fallback.test.cjs (5 tests, structural-IR):
  parses the generated header into sections via regex,  asserts on
  the Execute-mode-fallback section's content (must contain stop/
  wait + plain-text directives, must NOT contain "pick a reasonable
  default", must name a permission path, must forbid artifact
  writing). No raw text snapshot — the assertions describe the
  behavioral invariant, so prose can be reworded without test churn.
- codex-config.test.cjs:128 still passes — section still mentions
  "Execute mode" as required.

Verification:
- 5/5 pass on new regression test
- 116/116 pass on bug-3018 + codex-config combined
- 6763/6763 pass on full suite
- lint-no-source-grep clean

Closes #3018

* test(#3018): parse fallback into typed semantic-flag record (CR)

CodeRabbit nitpick on PR #3027: the regression tests grepped the
generated header prose with regex, which is brittle and tests wording
rather than semantics. Per CONTRIBUTING.md "no-source-grep" standard.

Refactored to a structural-IR shape:

- New `parseExecuteModeFallback(section)` walks the section text once
  and returns a typed record:
    {
      ok, sectionLength,
      instructsStop,                          // STOP/HALT/WAIT directive
      presentsPlainTextQuestions,             // plain-text / numbered list
      namesPermissionPath,                    // --auto / --all / explicit approval
      forbidsWritingArtifactsBeforeAnswer,    // write-ban + named artifact class
      silentlyPicksDefaults,                  // anti-pattern guard (must be false)
    }
- Each positive invariant gets its own test asserting on the parsed
  boolean, so a failure points at the exact invariant that broke.
- A final test does a single assert.deepStrictEqual against the full
  expected contract — gives a structured diff when any flag flips.
- The artifact-write ban now requires BOTH a "do not write" intent
  AND a named artifact class (was a single broad regex), so generic
  "do not write" prose elsewhere in the section can't satisfy it.

Verification: 8/8 pass; lint-no-source-grep clean.
 2026-05-02 11:45:36 -04:00
Tom Boucher
e3b64b39f8 fix(#3019): query --help reaches handler instead of short-circuiting (#3026) 
* fix(#3019): query --help reaches handler instead of short-circuiting to top-level usage

The query argv parser in sdk/src/cli.ts harvested -h/--help as a global
flag and main() short-circuited dispatch when args.help was true. Net
effect: every `gsd-sdk query <anything> --help` printed top-level USAGE
instead of contextual subcommand help. There was no path for users to
discover what arguments a query subcommand accepts — they had to trigger
"required" errors by trial and error.

Two-layer fix:

1. sdk/src/cli.ts (parseCliArgsQueryPermissive)
   - Push -h / --help onto queryArgv instead of consuming them silently,
     so the registered handler / gsd-tools.cjs fallback gets to interpret
     the flag and render contextual help.
   - Only honor the global help flag when there is NO real subcommand to
     dispatch to (i.e. queryArgv contains only help flags). Preserves
     `gsd-sdk query --help` → top-level USAGE while letting
     `gsd-sdk query phase add --help` reach the handler.

2. get-shit-done/bin/gsd-tools.cjs
   - Render top-level usage on --help / -h / -? / --usage instead of
     erroring with "Unknown flag". The discovery hint in the usage text
     points users at the working method (run without args → error names
     required arguments) and references #3019 for tracking subcommand-
     level help printers.
   - --version remains rejected (no discovery use-case).

#1818 anti-hallucination invariant preserved: the destructive command
NEVER executes when --help is present. The new shape returns success:true
+ usage on stdout instead of the old success:false + error on stderr —
both satisfy "destructive command did not run", and the new shape also
restores discoverability.

Tests:
- sdk/src/cli.test.ts: 4 new vitest cases covering #3019 — query argv
  parser keeps --help with subcommand, parses -h short flag, preserves
  bare `query --help` top-level behavior, preserves --help position when
  intermixed with other query flags.
- tests/bug-3019-help-passthrough.test.cjs: 5 node:test cases on the
  fallback — bare gsd-tools (no args) errors with usage; --help renders
  usage on stdout exit 0; -h same; subcommand --help renders usage; usage
  hint mentions discovery method (without prose substring matching —
  parses into typed sections).
- tests/bug-1818-unknown-flags.test.cjs: rewritten to assert the new
  invariant ("destructive command did not run" + "usage was rendered")
  instead of the old shape ("--help is rejected with non-zero exit").
  Each destructive test seeds a sentinel artifact (phase dir, slug
  output) and asserts it survives.

Verification:
- 47/47 vitest pass on sdk/src/cli.test.ts
- 5/5 pass on tests/bug-3019-help-passthrough.test.cjs
- 8/8 pass on tests/bug-1818-unknown-flags.test.cjs (rewritten)
- 6763/6763 pass on full node:test suite
- lint-no-source-grep clean (0 violations)

Closes #3019

* fix(#3019): SDK fallback forwards plain-text help, broader usage list (CR)

CodeRabbit on PR #3026 (4 findings — 1 Major outside-diff, 2 inline,
1 nitpick):

1. **Major outside-diff** — sdk/src/cli.ts:442-454. The fallback path
   that delegates to gsd-tools.cjs called parseCliQueryJsonOutput
   (JSON.parse) on stdout. Now that gsd-tools renders plain-text usage
   on --help, JSON.parse threw "Unexpected token 'U'". Wrapped the
   parse in try/catch — on parse failure, forward the plain stdout
   verbatim so subcommand help reaches the user. Regression test:
   tests/bug-3019-help-passthrough.test.cjs spawns the built SDK and
   asserts `gsd-sdk query phase --help` exits 0, stdout contains the
   gsd-tools usage, and stderr does NOT contain a JSON-parse error.

2. .changeset/help-passthrough.md:3 — `pr: TBD` → `pr: 3026`.

3. gsd-tools.cjs:346 (TOP_LEVEL_USAGE):
   - Removed self-referencing `#3019` link (immediately stale after
     this PR merges).
   - Expanded Commands list from 17 → all 47 dispatcher cases:
     agent-skills, audit-open, audit-uat, check-commit, commit, …
     phase, phases, roadmap, milestone, validate, progress, intel,
     graphify, learnings, etc. — the bulk of the surface that was
     previously unreachable via --help discovery.

4. Nitpick: `isUsageOutput` was duplicated in bug-1818 and
   bug-3019-help-passthrough tests. Moved to tests/helpers.cjs with
   structural-comment, removed both duplicates.

Verification: 47/47 vitest pass, 14/14 regression tests pass,
6764/6764 full suite, lint clean.

* test(#3019): use t.skip() instead of bare return when SDK not built (CR)

CodeRabbit follow-up on PR #3026:

The integration test guarded against missing sdk/dist/cli.js with a
bare `return;` — node:test counts that as a passing test (0 assertions
exercised, 0 failures). On a CI checkout that hasn't run the SDK build,
the #3026 regression test silently green-lit and no signal ever
surfaced that the integration check was skipped.

Switched to `t.skip(...)` via the test context parameter so the
omission shows up in the test report. The unit-level fix
(sdk/src/cli.ts) is still covered by vitest, so the skip only affects
the end-to-end spawn-built-SDK check.

Verification: 6/6 pass when SDK is built; 5 pass + 1 skip when not.
 2026-05-02 11:45:33 -04:00
Tom Boucher
8e25eb6546 fix(#3017): codex SessionStart hook uses absolute node, not bare 'node' (#3022) 
* fix(#3017): codex SessionStart hook uses absolute node, not bare 'node'

PR #3002 fixed #2979 for settings.json-based managed JS hooks (Claude
Code, Gemini, Antigravity) by routing through buildHookCommand() →
resolveNodeRunner(), emitting the absolute Node binary path so hooks
resolve under GUI/minimal-PATH runtimes (/usr/bin:/bin:/usr/sbin:/sbin)
where nvm/Homebrew/Volta-installed node is not on PATH.

The Codex install path bypassed both helpers — line 7935 of bin/install.js
wrote `command = "node ${path}"` directly into config.toml. So Codex
SessionStart hook still failed with exit 127 ("node: command not found")
under the same minimal-PATH conditions PR #3002 was meant to close.

Fix:
- Add buildCodexHookBlock(targetDir, { absoluteRunner, eol }) — a pure
  helper that emits the toml hook block with the absolute runner. Returns
  null when absoluteRunner is null so the caller skips registration with
  a warning instead of writing a broken bare-node hook.
- Add rewriteLegacyCodexHookBlock(content, absoluteRunner) — mirror of
  rewriteLegacyManagedNodeHookCommands for the toml surface, so
  reinstall migrates a 1.39.x bare-node config.toml to the absolute form.
  Uses basename equality (CODEX_MANAGED_HOOK_BASENAMES set) so user-
  authored bare-node hooks are left alone.
- Replace the inline string-concat at line 7935 with a call to the new
  helper, threaded with the detected line ending so CRLF files stay CRLF.
- On the codex reinstall path, call rewriteLegacyCodexHookBlock first so
  existing bare-node entries get migrated before the new entry is added.

Tests:
- bug-3017-codex-hook-absolute-node.test.cjs (9 tests, all typed-IR):
  - buildCodexHookBlock emits absolute runner, parses to expected fields
  - returns null on missing runner (caller skips)
  - integrates with resolveNodeRunner() in the live process
  - rewriteLegacyCodexHookBlock migrates managed bare-node entries
  - leaves user-authored bare-node hooks alone (basename allowlist)
  - leaves entries with absolute runner unchanged (idempotent)
  - returns content unchanged when absoluteRunner is null
- codex-config.test.cjs e2e expectation updated to match new shape:
  parsed.hooks.SessionStart[0].hooks[0].command now equals
  '"<process.execPath>" "<hookPath>"' instead of 'node <hookPath>'.

Verification:
- 9/9 pass on the new regression test
- 179/179 pass across all codex-touching test files
- 6767/6767 pass on full suite, lint-no-source-grep clean
- Adheres to typed-IR / CONTRIBUTING.md "Prohibited: Raw Text Matching":
  parseCodexHookBlock returns a typed record; assertions are on
  structured fields (runner, hookPath, type, hasMarker), not stdout regex.

Closes #3017

* test(#3017): tighten runner assertions to exact process.execPath (CR)

CodeRabbit on PR #3022 (3 findings, 2 actionable + 1 nitpick):

1. .changeset/codex-bare-node-fix.md:3 — replace `pr: TBD` with
   `pr: 3022` so changeset metadata is traceable.

2. tests/bug-3017-codex-hook-absolute-node.test.cjs:81-146 — the test
   asserted `parsed.runner !== 'node'` and `parsed.runner.includes('/node')`,
   which would false-positive on any absolute path containing '/node'
   (e.g. /Users/x/notnode/foo). Tightened to compare against the EXACT
   absolute path supplied by the caller (after stripping toml + JSON
   escape layers via a new unescapeRunner() helper). The live-process
   integration test now compares against process.execPath exactly. The
   rewriteLegacyCodexHookBlock test also uses exact-equality.

3. Nitpick (skipped): use repository's TOML parser for parsing instead
   of bespoke regex. The hand-rolled parser is small, scoped, and
   fully tested by these structural assertions; pulling in a TOML lib
   for tests would create a circular dependency on the SUT (the
   installer's own parser). Leaving as-is.

Verification: 9/9 pass on regression test, 6767/6767 full suite, lint clean.
 2026-05-02 11:45:30 -04:00
Tom Boucher
f2decefede fix(#3010): post-install message and docs use /gsd-update --reapply (#3012) 
* fix(#3010): post-install message and docs use /gsd-update --reapply

PR #2824 consolidated 86 skills into ~58, removing the standalone
/gsd-reapply-patches command and folding it into a flag on /gsd-update
(/gsd-update --reapply). The 1.39.1 hotfix (#2954) updated help.md
but missed three other surfaces that still recommended the dead form:

1. bin/install.js reportLocalPatches() — runtime emitter shown after
   every install with backed-up patches. All branches updated:
   - claude/opencode/kilo/copilot: /gsd-update --reapply
   - gemini: /gsd:update --reapply
   - codex: $gsd-update --reapply
   - cursor: gsd-update --reapply (mention the skill name)

2. get-shit-done/workflows/update.md — Step 4 prose and the
   check_local_patches block both referenced /gsd-reapply-patches.
   Replaced with /gsd-update --reapply (with backticks around the
   command per CR feedback for copy/paste UX).

3. Localized docs (en/ja-JP/ko-KR/zh-CN) — 14 files across
   ARCHITECTURE.md / COMMANDS.md / FEATURES.md / INVENTORY.md /
   USER-GUIDE.md / manual-update.md still listed the removed command.

Tests:
- bug-3010-reapply-patches-references.test.cjs (4 tests): scans
  bin/install.js's reportLocalPatches body, every workflow file, and
  every doc (excluding CHANGELOG history and help.md's deprecation
  notice) for the removed command form, and verifies each runtime
  branch emits the consolidated form via captured console output.
- tests/copilot-install.test.cjs:1081-1115 — stale assertions that
  hard-coded the removed string updated to assert /gsd-update --reapply.

Verification: 115/115 pass across both files.

Co-authored-by: Patrick Clery <patrick@patrickclery.com>
Closes #3010

* test(#3010): broaden dead-command scan + tighten runtime exact-match

CodeRabbit follow-up findings on #3012:

1. Workflow + docs scans only matched "/gsd-reapply-patches", missing
   the gemini ("/gsd:reapply-patches") and codex ("$gsd-reapply-patches")
   spellings. A regression that re-introduced either form in localized
   docs would have passed silently. Extracted a DEAD_COMMAND_PATTERNS
   array + findDeadCommands() helper used by both scans, so all three
   removed forms are checked uniformly. Match output also reports which
   spellings hit, for faster diagnosis.

2. reportLocalPatches runtime test asserted output.includes('update --reapply'),
   which is too loose — a malformed prefix like '/gsd:update --reapply' on
   the claude branch would have passed. Replaced with an exact
   {runtime → expected token} map covering all 7 branches:
     claude/opencode/kilo/copilot → /gsd-update --reapply
     gemini → /gsd:update --reapply
     codex → $gsd-update --reapply
     cursor → gsd-update --reapply
   Negative assertion also runs DEAD_COMMAND_PATTERNS against output for
   every runtime, so dead forms can't slip in regardless of branch.

Verification: 4/4 pass on bug-3010-reapply-patches-references.test.cjs.

* test(#3010): add prefix-absence guard for cursor runtime (CR follow-up)

CodeRabbit (Minor): the cursor expected token "gsd-update --reapply" is
a substring of every prefixed form ("/gsd-update --reapply" for claude/
opencode/kilo/copilot, "\$gsd-update --reapply" for codex). The positive
output.includes(expectedToken) check therefore can't distinguish correct
cursor output from a regression where the installer emits a prefixed
form for cursor — both pass the substring check.

Add an explicit prefix-absence assertion for cursor that fails if any
of /, \$, or : appears immediately before "gsd-update --reapply" in
output. The gemini form ("/gsd:update --reapply") doesn't share the
substring (gsd:update vs gsd-update) so it's already caught by the
positive includes failing on cursor's expected bare token.

Verification: 4/4 pass.

---------

Co-authored-by: Patrick Clery <patrick@patrickclery.com>
 2026-05-02 09:38:34 -04:00
Tom Boucher
a4e5cc7c24 fix(#3011): actionable SDK-not-on-PATH diagnostic with shim location and shell-specific commands (#3014) 
* fix(#3011): actionable SDK-not-on-PATH diagnostic with shim location and shell-specific commands

The previous diagnostic was a generic 'GSD SDK files are present but
gsd-sdk is not on your PATH' message with no concrete path or
shell-specific PATH-export command. Windows users reported that they
couldn't find where the shim was written and didn't know how to add
it to PATH for each shell (PowerShell vs cmd.exe vs Git Bash vs WSL
all read PATH from different sources).

New formatSdkPathDiagnostic({ shimDir, platform, runDir }) helper
returns a typed IR:
- shimLocationLine: explicit 'Shim written to: <path>'
- actionLines: platform-specific PATH-export commands
  - Windows: 3 lines (PowerShell, cmd.exe, Git Bash with backslash->/
    translation for bash compatibility)
  - POSIX: 1 line (export PATH=...)
- npxNoteLines: 'you're running via npx ... npm install -g instead'
  when runDir is under an _npx cache segment (where the shim may be
  written to a temp dir that won't persist for the user's interactive
  shell)
- isNpx, isWin32: structured booleans for assertions

Renderer in install.js just emits each line. Tests assert on the
typed IR fields directly (no source-grep, no console-output parsing).

Tests: 12 cases across 5 suites covering Windows shell flavors
(PowerShell preserves backslashes, Git Bash translates to forward),
POSIX exports, null-shimDir fallback to npm install -g advice, npx
detection on both path-separator conventions, and IR shape contract.

Closes #3011

* fix(#3011): cmd.exe guidance uses powershell -Command, not setx

CodeRabbit flagged the cmd.exe action line as a Major Windows
correctness bug:

  setx PATH "${shimDir}; %PATH%"

Two failure modes:
1. setx silently truncates the registry value above 1024 chars,
   permanently storing the truncated PATH and breaking applications
   until restored from the registry backup or fixed manually.
2. %PATH% expands to its current literal value at the moment setx
   runs, and the result is written as REG_SZ instead of REG_EXPAND_SZ.
   Lazy references like %SystemRoot% are baked in as literals, so
   future changes to those variables stop propagating.

Replace with the same SetEnvironmentVariable call already used for
the PowerShell line, invoked through `powershell -Command` so cmd.exe
users get a safe command without us recommending two different APIs.

* fix(#3011): escape shimDir for PowerShell, bash, and POSIX export

CodeRabbit (Minor): a Windows username with a single quote (e.g.
"C:\Users\O'Neil\AppData\Roaming\npm") would interpolate raw into the
suggested commands, producing unparseable shell input the user can't
fix without understanding the bug.

Each shell context needs a different escape:

- PowerShell single-quoted strings: '' is the literal-quote escape.
  Apply to both the PowerShell line and the cmd.exe line (which
  delegates to PowerShell).

- Git Bash, where the path lives inside an outer single-quoted echo:
  '\'' (close-quote, escaped-quote, reopen-quote) embeds a literal
  single quote. The slash-conversion (\\ → /) still applies first.

- POSIX export (Linux/macOS) inside double quotes: escape \, $, ",
  and backtick so the path is copied verbatim. $PATH lives outside
  the escape and still expands at paste time.

Regression test: bug-3011-sdk-path-diagnostic.test.cjs locks in the
expected escape sequence for all three shell flavors.
 2026-05-02 09:30:58 -04:00
Tom Boucher
f55069ecbf test(#2974): migrate 8 test files to typed-IR assertions (#3016) 
* test(#2974): migrate 8 test files to typed-IR assertions

Replaces raw stdout/stderr substring matching with structured-field
assertions per CONTRIBUTING.md "Prohibited: Raw Text Matching on Test
Outputs". Adds shared infrastructure for typed error emission so this
pattern is the easy path going forward.

Shared infrastructure:
- core.cjs: ERROR_REASON frozen enum + setJsonErrorMode/getJsonErrorMode
- gsd-tools.cjs: --json-errors CLI flag, parsed before subcommand dispatch
- config.cjs: typed reasons at all 7 error sites
- graphify.cjs: GRAPHIFY_REASON enum + reason/timeout_ms in execGraphify result
- bin/install.js: pure buildSdkFailFastReport() IR builder + renderer
- hooks/gsd-session-state.sh, gsd-phase-boundary.sh: emit Claude Code
  hookSpecificOutput JSON envelope with typed state_present/config_mode/
  planning_modified/file_path fields (no-op when hooks.community is off)

Test migrations (all pass, 171 tests across the 8 files):
- bug-2649-sdk-fail-fast: assert on ir.reason / ir.context / ir.fix_command
- bug-2687-config-read-warning-parity: assert.equal stderr === ''
- bug-2796-arg-parsing-regression: assert on result.json.updated/.phase
- bug-2838-summary-rescue: parse rescue footer, assert mtime invariant
- bug-2943-config-get-context-window: parse JSON, assert ERROR_REASON.CONFIG_KEY_NOT_FOUND
- graphify: assert reason === GRAPHIFY_REASON.ENOENT/TIMEOUT
- hooks-opt-in: parse hookSpecificOutput, assert typed fields
- security-scan: reclassified as source-text-is-the-product (scan label
  output and CI workflow YAML ARE the deployed contract)

Verification: lint-no-source-grep clean (0 violations), full suite
6741/6741 pass.

Closes #2974

* test(#2974): address CR feedback — typed code field, robust idempotency

Two CodeRabbit findings on #3016 addressed:

1. tests/hooks-opt-in.test.cjs:355 (Minor, inline) —
   parsed.reason.includes('Conventional Commits') was still substring
   matching after the typed-IR migration. Fixed at the source: the
   gsd-validate-commit hook now emits a typed `code` field
   ('CONVENTIONAL_COMMITS_VIOLATION', 'COMMIT_SUBJECT_TOO_LONG')
   alongside the human-readable `reason`. Test asserts strictEqual
   on the code; the prose copy is no longer part of the test contract.

2. tests/bug-2838-summary-rescue-gitignored-planning.test.cjs:224-250
   (Outside-diff) — mtimeMs alone can stay unchanged on coarse-grained
   filesystems (HFS+, FAT) when two rewrites land within the same
   timestamp tick, falsely passing the idempotency assertion.
   Replaced with a full snapshot (mtimeMs, ctimeMs, size, ino, sha256
   of contents) compared via assert.deepStrictEqual — the hash
   catches any rewrite the timestamp would miss.

Verification: 30/30 pass on the two affected files; lint-no-source-grep
clean (0 violations across 368 test files).
 2026-05-02 09:27:23 -04:00
Tom Boucher
de25400b70 fix(#2979): emit absolute node path in managed hooks for GUI/minimal-PATH runtimes (#3002) 
* fix(#2979): emit absolute node path in managed hooks for GUI/minimal-PATH runtimes

Installer-emitted hook commands started with bare 'node' which works
under interactive shells (nvm/Homebrew/Volta on PATH) but fails in
GUI-launched runtimes that start with /usr/bin:/bin:/usr/sbin:/sbin.
Every managed JS hook (gsd-check-update, gsd-statusline, gsd-context-monitor,
gsd-prompt-guard, gsd-read-guard, gsd-read-injection-scanner,
gsd-workflow-guard) failed with /bin/sh: node: command not found —
silently disabling update checks, statusline, and security guards.

Fix: new resolveNodeRunner() helper returns process.execPath (the
absolute path of the Node binary running the installer) forward-slash-
normalized and double-quoted. Used in:
  - buildHookCommand() for global installs (.js runner)
  - local-install code paths for all 7 managed JS hooks

.sh hooks keep bare 'bash' — /bin/bash is in the POSIX standard PATH
and always resolves under minimal-PATH GUI launches.

Tests: bug-2979-hook-absolute-node.test.cjs parses emitted commands
into { runner, hookPath } records and asserts:
  - resolveNodeRunner returns quoted absolute forward-slash node path
  - .js hooks emit absolute runner (default and portableHooks modes)
  - .sh hooks still emit bare 'bash'

Closes #2979

* chore(#2979): add changeset fragment for PR #3002

* chore(#2979): add changeset fragment for PR #3002

* fix(#2979): resolveNodeRunner returns null on missing execPath; rewrite legacy bare-node managed hooks (CR feedback)

CodeRabbit on PR #3002 caught two issues:

1. resolveNodeRunner fell back to bare 'node' when process.execPath was
   empty -- recreating the exact #2979 bug. Now returns null. Callers
   (buildHookCommand and the local-install code paths) check for null
   and skip registration rather than emit a broken command.

2. The original #2979 fix only updated NEWLY registered hooks. Existing
   bare-node managed hook entries from pre-#2979 installs stayed
   broken across reinstalls. New rewriteLegacyManagedNodeHookCommands
   walks settings.hooks and rewrites any managed-hook entry that starts
   with bare 'node ' to use the absolute runner. Filename allowlist
   (gsd-check-update.js, gsd-statusline.js, gsd-context-monitor.js,
   gsd-prompt-guard.js, gsd-read-guard.js, gsd-read-injection-scanner.js,
   gsd-workflow-guard.js) ensures user-authored bare-node hooks are
   left untouched.

Tests: bug-2979-hook-absolute-node.test.cjs grows by 8 cases:
- 5 for the migration walker (rewrites managed entries, leaves quoted-
  runner entries alone, leaves user-authored entries alone, leaves .sh
  entries alone, no-ops on null runner).
- 2 for resolveNodeRunner returning null on empty execPath.
- 1 for buildHookCommand returning null when execPath unavailable.

* chore(#3002): drop direct CHANGELOG.md edit; release entry now lives in .changeset/

The changeset-fragment workflow (#2975) renders fragments into
CHANGELOG.md at release time. Direct edits to [Unreleased] on
each PR caused merge conflicts on every concurrent PR. This commit
restores CHANGELOG.md to match origin/main; the release entry for
this fix is preserved in the .changeset/*.md fragment(s) on this
branch, which the release workflow consolidates.

* fix(#2979): guard hook + statusline pushes against null commands (CR follow-up)

CodeRabbit on PR #3002 found an outside-diff issue: when
resolveNodeRunner() returns null, every dependent *Command becomes
null, but the registration sites still pushed { type: 'command',
command: null } entries onto settings.hooks. The runtime's hook
schema rejects null commands and the failure surfaces as a confusing
parse error.

Fix:
- One unified warning at the top of configureSettings when ANY JS-hook
  command resolves null (operator sees the cause once instead of per-hook).
- Each of the 6 managed JS hook registration if-clauses now guards on
  the *Command variable being truthy: && updateCheckCommand,
  && contextMonitorCommand, && promptGuardCommand, && readGuardCommand,
  && readInjectionScannerCommand, && workflowGuardCommand.
- Statusline registration adds an else-if (!statuslineCommand) clause
  with its own warn before the settings.statusLine write site.

Tests: bug-2979-hook-absolute-node.test.cjs grows by 7 cases
(6 per-hook structural assertions parsing install.js for the
`fs.existsSync(<file>) && <command>` shape, plus 1 statusline
guard-precedes-write test).

* fix(#2979): defense-in-depth validateHookFields before writeSettings (CR)

CodeRabbit on PR #3002 (post-fix-up review): replace source-grep
structural tests with behavioral assertions on the settings object.

The push-site `&& <command>` guards (commit ce696c64) prevent null
commands from being pushed in the first place. As a defense-in-depth
backstop, install.js now runs validateHookFields(settings) right
before writeSettings(); validateHookFields already filters
{type:'command', command: null} entries (line 5884), so even if my
push-site guards ever regress, no null-command entries reach disk.

Tests: replaced the 7 install.js source-grep tests with 8 truly
behavioral tests:
- validateHookFields strips null-command entries for each of the 6
  managed JS hook shapes (parameterized by event + matcher)
- validateHookFields drops the entry entirely when all its hooks are
  null-command
- validateHookFields preserves agent-type hooks while stripping
  null-command sibling hooks in the same entry

These tests exercise the actual function the production code uses,
not its source representation. They survive future refactors of the
registration call sites.

* fix(#2979): tighten managed-hook migration to basename equality (CR)

CodeRabbit on PR #3002 (post-fix-up review): the previous
`trimmed.includes(name)` matcher had a false-positive vector. A
user-authored hook whose path contained a managed filename as a
substring (e.g. /home/me/scripts/wraps-gsd-check-update.js-helper.js)
would be unconditionally rewritten with the GSD runner, replacing
the user's bare `node` with our absolute path -- silently mutating
their hook configuration.

Fix: parse the command into <runner> <script-token> with the
script-token allowed to be quoted (single or double) or bareword.
Extract the path inside quotes, take the basename (handles both
forward and backslash separators on Windows), and match against
MANAGED_HOOK_FILES via Set.has() — exact equality, not substring.

Tests: bug-2979 grows by 4 cases:
- user hook with managed-filename-as-substring is NOT rewritten
- single-quoted path: rewritten correctly
- bareword path: rewritten correctly
- Windows backslash path: basename extraction works
 2026-05-02 00:40:09 -04:00
Tom Boucher
ca78b65de7 fix(#2973): /gsd-profile-user writes dev-preferences.md to skills/, not legacy commands/gsd/ (#3003) 
* fix(#2973): /gsd-profile-user writes dev-preferences.md to skills/ not legacy commands/gsd/

v1.39.0's install summary claimed the legacy ~/.claude/commands/gsd/
directory had been removed in favor of skills-only architecture, but
the cmdGenerateDevPreferences writer at profile-output.cjs:781 still
defaulted to the legacy path. Every /gsd-profile-user --refresh
deterministically re-created the legacy directory.

Missed in PR #1540's migration because dev-preferences is a
runtime-generated user artifact, not a GSD-shipped command file.

Fix:
- Writer default: ~/.claude/skills/gsd-dev-preferences/SKILL.md
- profile-user.md Display message + artifact list reference new path
- New migrateLegacyDevPreferencesToSkill(targetDir, saved) installer
  helper. Called at all 5 skills-aware install branches. Copies
  preserved legacy dev-preferences.md into skills/gsd-dev-preferences/
  SKILL.md, but ONLY if no SKILL.md already exists -- never clobbers
  user-customized skill content.

Tests: bug-2973-profile-user-skills-path.test.cjs runs the writer in
a subprocess (core.cjs:output uses fs.writeSync(1, ...) which bypasses
in-process stubbing), asserts the writer's command_path field is the
skills location, the file is on disk at that path, the legacy path is
NOT created. Tests for migration helper assert it writes when no skill
exists and skips when one does.

Closes #2973

* chore(#2973): add changeset fragment for PR #3003

* fix(#2973): rephrase comment to avoid cline-install leaked-path lint

The new comment at line 780 of profile-output.cjs literally contained
the string '~/.claude/commands/gsd/' which the cline-install
leaked-path regression test (tests/cline-install.test.cjs:175)
correctly flagged.

Cline transforms .claude/skills/ -> .cline/skills/ in installed .cjs
files but does not transform .claude/commands/. The new comment talks
about the legacy 'commands/gsd' subdirectory without the ~/.claude/
prefix, so the lint passes. The path semantics are unchanged -- the
runtime construction at line 787 still uses path.join(os.homedir(),
'.claude', 'skills', ...) which the lint regex does not match.

* test(#2973): add timeout to spawnSync to prevent CI hangs (CR feedback)

CodeRabbit on PR #3003: without a timeout, a regression that hangs the
writer or dispatcher would block CI indefinitely. Added a 30s timeout
(generous for what should complete in <1s) and an explicit signal
assertion so a timeout trip surfaces as a clear test failure with
context rather than a hung worker.

* test(#2973): add allow-test-rule annotation for legitimate product-text parsing

The new var-binding lint from #2982/#2985 caught readFileSync(...).match()
and readFileSync(...).includes() calls in this test. Both are legitimate
structural assertions against the product workflow markdown, not source-grep:

- match() extracts the path from a structured Display: "..." line and
  asserts on the typed path value (same pattern as bug-2470's installer
  scanForLeakedPaths regex test).
- includes() asserts the absence of a legacy path literal.

profile-user.md IS the shipped workflow artifact, and its Display: line
IS what the user sees. Per the existing test-rigor convention, this is
the source-text-is-the-product justification category.

Annotated with allow-test-rule citing that category.

* chore(#3003): drop direct CHANGELOG.md edit; release entry now lives in .changeset/

The changeset-fragment workflow (#2975) renders fragments into
CHANGELOG.md at release time. Direct edits to [Unreleased] on
each PR caused merge conflicts on every concurrent PR. This commit
restores CHANGELOG.md to match origin/main; the release entry for
this fix is preserved in the .changeset/*.md fragment(s) on this
branch, which the release workflow consolidates.

* fix(#2973): preserve user-owned gsd-dev-preferences skill across wipe (CR)

CodeRabbit on PR #3003 caught a real bug: copyCommandsAsClaudeSkills()
wipes ALL gsd-* skill directories at the top of every install, then
reinstalls from the package source. Since gsd-dev-preferences is
user-generated (written by /gsd-profile-user --refresh) and NOT
shipped by the npm package, the wipe deletes the user's customized
SKILL.md with nothing to restore from.

Fix: USER_OWNED_SKILLS allow-list in copyCommandsAsClaudeSkills.
Snapshot files under skills/gsd-dev-preferences/ before the wipe,
restore after. Same preserve/restore pattern as PR #1924.

Tests: bug-2973 grows by 2 cases:
- user-customized SKILL.md survives the wipe
- non-user-owned gsd-* skills are still wiped (preservation is opt-in)
 2026-05-02 00:29:45 -04:00
Tom Boucher
1a51ec5829 fix(#2990): gsd-code-fixer worktree attaches to a new branch, not the user-checked-out one (#3001) 
* fix(#2990): gsd-code-fixer worktree attaches to a new branch, not the user-checked-out one

The agent's setup_worktree step ran 'git worktree add "$wt" "$branch"'
where $branch was the user's currently-checked-out branch in the main
repo. Git refuses to check out the same branch in two worktrees by
default, so the call failed before any review fix could be applied.

This is the next-layer failure after #2686 (foreground/background race)
and #2839 (transactional cleanup): the isolation strategy was correct
in design, blocked only by git's same-branch protection.

Fix:
- Create a new branch 'gsd-reviewfix/${padded_phase}-$$' from the
  current branch tip and attach the worktree to it via
  'git worktree add -b "$reviewfix_branch" "$wt" "$branch"'.
- Cleanup tail is now four steps:
  1. 'git -C "$main_repo" merge --ff-only "$reviewfix_branch"'
     -- captures the agent's commits on the user's branch. --ff-only
     fails loudly on divergence (concurrent commits to $branch); the
     temp branch is preserved for manual merge.
  2. 'git worktree remove "$wt" --force'.
  3. 'git -C "$main_repo" branch -D "$reviewfix_branch"' ONLY if
     ff-only succeeded.
  4. 'rm -f "$sentinel"' last (preserves #2839 transactional ordering).
- Recovery sentinel JSON now records reviewfix_branch alongside
  worktree_path so a re-run after interruption cleans both the orphan
  worktree and the orphan temp branch.

Regression test: tests/bug-2990-code-fixer-worktree-branch.test.cjs
parses the agent .md into structured 'git worktree add' invocation
records (skipping occurrences inside markdown inline-code or bash
comments -- those are citations of the OLD pattern, not executable)
and asserts the structural invariants on the new pattern.

Closes #2990

* chore(#2990): add changeset fragment for PR #3001

* chore(#2990): add changeset fragment for PR #3001

* fix(#2990): correct main_repo parsing and ff_status capture (CR feedback)

CodeRabbit on PR #3001 caught two real bugs in the cleanup tail:

1. `awk '/^worktree / { print $2 }'` truncates paths containing
   spaces. /path/with spaces/repo becomes /path/with. Replaced with
   `sub(/^worktree /, ''); print` which strips the prefix and
   preserves the full path.

2. `if ! git merge ...; then ff_status=$?` captures the exit of the
   `!` operator (always 1 on failure), not the merge command's exit
   code. Restructured to `if cmd; then ff_status=0; else ff_status=$?`
   so the else-branch captures the real merge exit code.

Tests still pass: bug-2990 structural assertions on the agent .md
content unchanged.

* fix(#2990): recovery extracts reviewfix_branch and deletes orphan branch (CR)

CodeRabbit on PR #3001 found two issues:

1. (Major) Recovery code only extracted worktree_path from the sentinel.
   If a prior run died after `git worktree remove` but before
   `git branch -D`, the orphan reviewfix branch survived forever. The
   sentinel records reviewfix_branch (line 272) and the docs claim
   recovery deletes it, but the code didn't.
   Fixed: emit BOTH worktree_path and reviewfix_branch from the parser
   (newline-separated), capture each into shell vars, and call
   `git branch -D "$prior_branch" 2>/dev/null || true` after worktree
   removal but before sentinel deletion.

2. (Quick win) The bug-2990 test used regex .test() against the raw
   markdown, which would have been satisfied by prose mentioning the
   token. Restructured to:
   - parseCleanupGitInvocations() returns ordered records with structured
     fields (verb, targetsReviewfixBranch, isMergeFfOnly, isBranchDelete)
   - assert exactly-one merge --ff-only AND exactly-one branch -D
   - assert merge precedes branch-delete in execution order
   - parse the sentinel JSON.stringify call to extract field names and
     assert reviewfix_branch is among them

   Added 2 new tests for the recovery-block invariant: parses the recovery
   node -e block and asserts it extracts parsed.reviewfix_branch alongside
   parsed.worktree_path; and asserts the recovery shell calls
   `git branch -D "$prior_branch"`.

* test(#2990): add allow-test-rule annotation for product-text parsing (CR follow-up)

The lint-tests CI catch flagged md.match() in the new structural-IR
test suite. The .match() calls extract typed fields (cleanup-tail
git invocation records, sentinel JSON field names, recovery-block
node script content) from agents/gsd-code-fixer.md — which IS the
deployed agent product. Asserting on those typed fields tests the
runtime contract, not source code internals.

source-text-is-the-product is the correct classification per the
existing convention (matches thread-session-management.test.cjs and
the others reclassified in PR #2985's CR follow-up).

* chore(#3001): drop direct CHANGELOG.md edit; release entry now lives in .changeset/

The changeset-fragment workflow (#2975) renders fragments into
CHANGELOG.md at release time. Direct edits to [Unreleased] on
each PR caused merge conflicts on every concurrent PR. This commit
restores CHANGELOG.md to match origin/main; the release entry for
this fix is preserved in the .changeset/*.md fragment(s) on this
branch, which the release workflow consolidates.
 2026-05-02 00:29:43 -04:00
Tom Boucher
4277f7d7e8 fix(#2994): move verify-reapply-patches.cjs to get-shit-done/bin/ so it ships to user installs (#3000) 
* fix(#2994): move verify-reapply-patches.cjs to get-shit-done/bin/ so installer ships it

scripts/verify-reapply-patches.cjs (added in #2972 to close the
verified-yes-without-checking gap from #2969) shipped in the npm tarball
but never reached user installs: bin/install.js copies get-shit-done/
recursively but does not copy the top-level scripts/ directory.

Effect: every fresh install hit `Cannot find module …/scripts/verify-reapply-patches.cjs`
on Step 5 of /gsd-reapply-patches. The whole point of moving
verification out of LLM-driven prose into a deterministic script is
undone if the script does not resolve at runtime.

Fix: move the script to get-shit-done/bin/verify-reapply-patches.cjs
(same pattern as gsd-tools.cjs and other runtime bin scripts that the
installer ships) and update reapply-patches.md Step 5 to invoke
${GSD_HOME}/get-shit-done/bin/verify-reapply-patches.cjs.

Tests:
- bug-2969 SCRIPT path updated to the new location
- New bug-2994-verify-reapply-patches-installed-path.test.cjs parses
  reapply-patches.md into structured invocation records and asserts
  every node ${GSD_HOME}/... reference lives under get-shit-done/
  (the installed tree). Catches future regressions where someone moves
  a runtime-needed script back to scripts/.

Closes #2994

* chore(#2994): add changeset fragment for PR #3000

* chore(#2994): add changeset fragment for PR #3000

* docs(#2994): update verifier-script-location comment to reflect new path (CR)

CodeRabbit on PR #3000: the parenthetical at line 278 still said the
script ships under scripts/, but this PR moved it to get-shit-done/bin/.
Updated the prose to reference the new location and the installer
target path.

* chore(#3000): drop direct CHANGELOG.md edit; release entry now lives in .changeset/

The changeset-fragment workflow (#2975) renders fragments into
CHANGELOG.md at release time. Direct edits to [Unreleased] on
each PR caused merge conflicts on every concurrent PR. This commit
restores CHANGELOG.md to match origin/main; the release entry for
this fix is preserved in the .changeset/*.md fragment(s) on this
branch, which the release workflow consolidates.
 2026-05-02 00:29:34 -04:00
Tom Boucher
cde793f1f0 fix(#2992): deterministic latest-version check — package name is a constant, not LLM choice (#2993) 
* fix(#2992): deterministic latest-version check — package name is a constant, not LLM choice

The /gsd-update workflow's check_latest_version step was prescribed in
LLM-driven prose: "run `npm view get-shit-done-cc version`". The
executing model could and did shortcut the prescription and invent
npm queries against name-shaped guesses — `@get-shit-done/cli`,
`get-shit-done-cli`, `gsd` — all of which 404 or, worse, return an
unrelated typosquat (the 2016 `get-shit-done` timer package). Same
architectural anti-pattern as #2969 (Hunk Verification Gate where
the LLM filled `verified: yes` without checking).

Implementation built TDD per #2992:

  get-shit-done/bin/check-latest-version.cjs
    - PACKAGE_NAME = 'get-shit-done-cc' as a module constant; not
      parameterised, not exposed for override.
    - checkLatestVersion({ spawn? }) returns
      { ok: bool, version?: string, reason: CHECK_REASON.X, detail? }
      via a frozen enum: OK / FAIL_NPM_FAILED / FAIL_INVALID_OUTPUT.
    - --json mode emits the structured record on stdout for the
      workflow to parse via jq.
    - Windows-aware: uses { shell: process.platform === 'win32' }
      since npm is npm.cmd on Windows (same lesson as #2962).
    - Stored under get-shit-done/bin/ (not top-level scripts/) because
      that path IS in the user's installed config dir; top-level
      scripts/ ships in the npm tarball but is not copied into
      ~/.claude/ at install time.

  tests/bug-2992-check-latest-version.test.cjs
    - 7 tests, all assertions on the typed CHECK_REASON enum + the
      structured record. Injectable spawn function so no real npm
      process is invoked. Covers OK, npm-non-zero, invalid-output,
      empty-output, pre-release semver, PACKAGE_NAME constant lock,
      enum-shape lock.

  get-shit-done/workflows/update.md
    - check_latest_version step rewritten to call the script via
      `node "${GSD_HOME}/get-shit-done/bin/check-latest-version.cjs"
      --json` and parse the structured response with jq. Explicit
      "Do NOT run `npm view` or `npm search` directly" guidance
      cites #2992 so future contributors understand why.

Closes #2992

* fix(#2992): trailing slash on GSD_HOME default to satisfy bare-path lint

The bug-2470 regression test scans update.md for bare `$HOME/.claude`
references (no trailing slash). The PR added one in the new
check_latest_version step. Fix: trailing slash on the default value
(`${GSD_HOME:-$HOME/.claude/}`). Bash POSIX collapses the resulting
double slash; the lint pattern's negative lookahead is now satisfied.

* fix(#2992): emit GSD_DIR from get_installed_version, use it in check_latest_version

Addresses CodeRabbit feedback: the previous `${GSD_HOME:-$HOME/.claude/}`
fallback hardcoded the Claude runtime path, which silently breaks for
non-Claude runtimes (gemini, codex, opencode, kilo).

Fix:
- get_installed_version now emits a 4th line with the resolved config
  dir ($LOCAL_DIR or $GLOBAL_DIR), captured by callers as GSD_DIR.
- check_latest_version uses $GSD_DIR/get-shit-done/bin/check-latest-version.cjs.
  Empty GSD_DIR (UNKNOWN scope) skips the version check and falls
  through to fresh-install path.

This keeps the package name deterministic (#2992) AND respects the
detected runtime, instead of assuming Claude.

* chore(#2992): add changeset fragment for PR #2993

* chore(#2992): add changeset fragment for PR #2993

* fix(#2992): consolidate LATEST_RESULT parsing inside the GSD_DIR guard

CodeRabbit on PR #2993: the previous structure separated the GSD_DIR
guard from the jq parsing, so when GSD_DIR was empty the parsing block
ran against an unset LATEST_RESULT and produced misleading 'couldn't
check for updates' diagnostics instead of clean 'no_install_detected'.

Move all field assignments inside the conditional so the skip path
seeds LATEST_OK=false, LATEST_VERSION='', LATEST_REASON='no_install_detected',
and LATEST_STATUS=0 atomically.

* fix(#2992): emit GSD_DIR in early-return; add code-block lang and spawnSync timeout (CR)

CodeRabbit on PR #2993 caught three issues:

1. (Major) The early-return path in get_installed_version (PREFERRED_CONFIG_DIR
   fast path) only echoed 3 lines, but PR #2993 changed the contract to 4
   (GSD_DIR is now line 4). Downstream check_latest_version misread valid
   installs as UNKNOWN. Added `echo "$PREFERRED_CONFIG_DIR"` before exit 0.

2. (Minor) Markdown MD040: fenced code block at line 310 was missing a
   language identifier. Added ```text.

3. (Quick win) spawnSync('npm view ...') had no timeout, so a hung network
   could block /gsd-update indefinitely. Added 15s timeout; on timeout
   spawnSync returns with signal !== null and the existing failure path
   emits FAIL_NPM_FAILED.

* fix(#3008): kill cross-process race in install-minimal:307 mid-copy test

Old shape compared listTmpStageDirs() snapshots before/after the
mid-copy throw. Under scripts/run-tests.cjs --test-concurrency=4,
tests/install-minimal-all-runtimes.test.cjs runs in a parallel
subprocess and also creates gsd-minimal-skills-* dirs in shared
os.tmpdir(). The parallel process's create/remove activity between
this test's two snapshots caused deterministic failure when timing
aligned -- presented as 'flaky' but is a real race.

CI failure data (PR #2993 run 25238555786):
  expected (before): ['gsd-minimal-skills-km1O1O']
  actual   (after):  []

Both processes behaved correctly in isolation. The test was wrong:
it observed a shared filesystem state across processes.

Fix: stub fs.mkdtempSync inside this test to record THIS call's
stage dir path. After the throw, assert fs.existsSync(stagedDir)
=== false. Direct observation of the function's own behavior; no
global tmpdir scan; no parallel-process interference.

Closes #3008

* fix(#2992): distinguish timeout from npm failure; guard empty LATEST_RESULT (CR)

CodeRabbit on PR #2993 (post-fix-up review) caught two improvements:

1. (Low value) check-latest-version.cjs:55-61 — when spawnSync times
   out, r.status is null and r.signal is set (e.g. 'SIGTERM'), but
   r.stderr is empty. Without the signal-first branch, both timeouts
   and genuine npm failures shaped as 'npm exited non-zero' in detail,
   making logs ambiguous. Added explicit signal-first branch:
   'npm timed out (signal: SIGTERM)'.

2. (Quick win) update.md:284-315 — when node is missing or the script
   doesn't exist, LATEST_RESULT is empty. Piping empty to jq parses
   without error but leaves LATEST_OK / LATEST_REASON as empty
   strings, producing the user-visible diagnostic
   'Couldn\'t check for updates (reason: , exit: N)' with a blank
   reason. Added an explicit guard that sets LATEST_REASON to
   'script_not_found_or_node_unavailable' when LATEST_RESULT is empty,
   so operators see a meaningful failure message.

Tests: bug-2992 grows by 2 cases (timeout signal detail + empty
stderr fallback).
 2026-05-02 00:29:31 -04:00
Tom Boucher
ffeeb92c14 fix(#2997): mask SECRET_CONFIG_KEYS in SDK config-set/get and init responses (#2999) 
* fix(#2997): mask SECRET_CONFIG_KEYS in SDK config-set/get and init responses

The CJS→TS port at sdk/src/query/config-mutation.ts:240,243 and
config-query.ts:122,128,132 dropped the masking layer that secrets.cjs
spec defines for brave_search/firecrawl/exa_search. Result: the SDK
echoed plaintext API keys into machine-readable JSON output (stdout,
transcripts, CI logs).

Adjacent leak in init.ts:673-675 / init.cjs:728-730: the init bundle
passed config.brave_search through raw, leaking the API key whenever
the user had stored one.

Fix:
- New sdk/src/query/secrets.ts ports SECRET_CONFIG_KEYS, isSecretKey,
  maskSecret, maskIfSecret. Exact CJS parity (verified by 17 tests
  in secrets.test.ts that import secrets.cjs and compare).
- config-set masks value + previousValue in response; on-disk plaintext
  intact (key stays usable).
- config-get masks read response. --default flows through unmasked
  (user's own input, not stored secret).
- init.ts/init.cjs mask string values only; booleans (availability
  flags) pass through unchanged so the typed contract is preserved.

Tests: 17 in secrets.test.ts (including CJS parity), 5 in
config-mutation.test.ts (#2997 block — covers on-disk-preserved,
previousValue masking, short-value, unset, non-secret pass-through),
4 in config-query.test.ts.

Closes #2997

* chore(#2997): add changeset fragment for PR #2999

* chore(#2997): add changeset fragment for PR #2999

* chore(#2999): drop direct CHANGELOG.md edit; release entry now lives in .changeset/

The changeset-fragment workflow (#2975) renders fragments into
CHANGELOG.md at release time. Direct edits to [Unreleased] on
each PR caused merge conflicts on every concurrent PR. This commit
restores CHANGELOG.md to match origin/main; the release entry for
this fix is preserved in the .changeset/*.md fragment(s) on this
branch, which the release workflow consolidates.
 2026-05-02 00:17:45 -04:00
Tom Boucher
4e378d37d8 fix(#3008): kill cross-process race in install-minimal:307 mid-copy test (#3009) 
Old shape compared listTmpStageDirs() snapshots before/after the
mid-copy throw. Under scripts/run-tests.cjs --test-concurrency=4,
tests/install-minimal-all-runtimes.test.cjs runs in a parallel
subprocess and also creates gsd-minimal-skills-* dirs in shared
os.tmpdir(). The parallel process's create/remove activity between
this test's two snapshots caused deterministic failure when timing
aligned -- presented as 'flaky' but is a real race.

CI failure data (PR #2993 run 25238555786):
  expected (before): ['gsd-minimal-skills-km1O1O']
  actual   (after):  []

Both processes behaved correctly in isolation. The test was wrong:
it observed a shared filesystem state across processes.

Fix: stub fs.mkdtempSync inside this test to record THIS call's
stage dir path. After the throw, assert fs.existsSync(stagedDir)
=== false. Direct observation of the function's own behavior; no
global tmpdir scan; no parallel-process interference.

Closes #3008
 2026-05-01 22:37:48 -04:00
Tom Boucher
9f09246f3b fix(#2998): populate gsd-pristine/ from install transform pipeline so verifier has a real baseline (#3004) 
* fix(#2998): populate gsd-pristine/ from install transform pipeline so verifier has a real baseline

saveLocalPatches declared a pristineDir variable and JSDoc'd 'saves
pristine copies to gsd-pristine/' but no code ever wrote there. Effect:
/gsd-reapply-patches Step 5 verifier (#2972) silently fell back to its
over-broad heuristic ('every significant backup line') -- exactly the
silent-success-on-lost-content failure mode #2969 was designed to
prevent.

Fix: new populatePristineDir({...}) helper runs copyWithPathReplacement
(the install transform pipeline) into a tmp staging dir, then copies out
only the modified-file paths into gsd-pristine/. saveLocalPatches now
accepts a pristineCtx and calls the helper when local patches are
detected. Soft-fails on transform errors (logs warning, continues with
empty pristine -- no worse than pre-fix).

Pristine reflects the about-to-install version's content, which is the
right baseline for 'what would survive without the user's modifications'.

Tests: bug-2998-pristine-dir-populated.test.cjs asserts the helper is
exported, no-ops on empty input, writes one pristine file per source-
existing path, skips ghost paths, and produces deterministic output
(byte-identical across runs -- the property pristine_hashes depends on).

Closes #2998

* chore(#2998): add changeset fragment for PR #3004

* fix(#2998): expand pristine to all manifest install roots; clear stale pristine on populate (CR)

CodeRabbit on PR #3004 caught two issues:

1. populatePristineDir only staged packageSrc/get-shit-done/ but
   manifest.files records edits under several install roots (commands/,
   agents/, hooks/, skills/, root files like .clinerules). Modified
   paths outside get-shit-done/ were silently skipped, leaving the
   verifier with no baseline for those edits. Fixed by computing the
   set of top-level dirs from the modified set and staging each one
   that exists in source. Root-level files (no slash) bypass the
   transform pipeline and are copied directly.

2. populatePristineDir did not wipe pre-existing gsd-pristine/ before
   populating. A previous run's stale pristine could survive into the
   current run's diff baseline. Now wipe before populate AND in the
   catch path so soft-failures don't leave half-populated data on disk.

Tests: bug-2998-pristine-dir-populated.test.cjs grows by 2 cases:
- agents/ paths are staged and copied (was silently skipped pre-fix)
- mixed get-shit-done/ + agents/ in same modified list both stage
 2026-05-01 21:14:14 -04:00
Tom Boucher
c2ada7e799 feat(#2995): post-install path audit for workflow-invoked scripts (#2996) 
* feat(#2995): post-install path audit for workflow-invoked scripts

Catches the gap class surfaced by #2994: a workflow references a script
via ${GSD_HOME}/<path> that ships in the npm tarball but is not copied
to the user's config dir at install time. Unit tests don't catch it
because they resolve the script via path.join(__dirname, '..', 'scripts',
…) — the source layout, not the deployed layout.

Implementation built TDD per #2995, vertical slices with structured-IR
assertions:

  scripts/audit-workflow-script-paths.cjs
    - Pure auditWorkflowScriptPaths({ workflowsDir, repoRoot,
      installedPrefixes }) returns { ok, findings: [{ workflow, path,
      kind }] } via the AUDIT_FINDING enum.
    - Two finding kinds: MISSING_FROM_REPO (typo / file deleted) and
      NOT_INSTALLED (#2994 class — first segment outside installed
      prefixes).
    - Tolerates ${GSD_HOME:-...} default-fallback syntax.

  tests/bug-2995-post-install-script-paths.test.cjs
    - 9 tests across 3 suites:
      • Pure-function pass and per-finding-kind detection (5 tests on
        synthetic fixtures).
      • Real workflow audit (2 tests asserting the actual repo's
        get-shit-done/workflows/ has no NEW gaps and KNOWN_GAPS stays
        consistent with audit findings).
      • Enum shape lock + extractReferences edge cases.
    - All assertions on typed AUDIT_FINDING enum / structured records;
      zero raw text matching.
    - KNOWN_GAPS is a Set keyed on `workflow|path|kind` strings;
      currently contains the #2994 entry. The companion test fails if
      a KNOWN_GAPS entry no longer matches a real finding (forces the
      allow-list to shrink as gaps fix).

The audit immediately catches #2994's gap on `reapply-patches.md`. The
allow-list contains exactly that entry; new gaps fail CI; #2994's fix
will remove the entry as part of the same PR.

Closes #2995
Refs #2994

* chore(#2995): add changeset fragment for PR #2996

* chore(#2995): add changeset fragment for PR #2996

* fix(#2995): emit both NOT_INSTALLED + MISSING_FROM_REPO; clean up fixture leak (CR)

CodeRabbit on PR #2996 found two issues:

1. (Low value) auditWorkflowScriptPaths short-circuited on NOT_INSTALLED,
   masking MISSING_FROM_REPO for the same ref. Removed the `continue` so
   both findings emit in one run; added a regression test.

2. (Low value) bug-2995 test created tmpRoot in before() but never wrote
   into it; per-fixture mkdtempSync dirs leaked. Rooted fixture repos
   under tmpRoot so the after() cleanup actually frees them.
 2026-05-01 21:13:45 -04:00
Tom Boucher
55ae8e42d2 test(#2986): mutation-killer suite for config-schema.cjs (95 typed assertions) (#3005) 
* test(#2986): mutation-killer suite for config-schema.cjs (95 typed assertions)

Stryker measured 4.62% mutation score on config-schema.cjs (6 killed,
124 survived). Surviving mutants documented that existing tests were
exercising paths without verifying outputs.

Adds tests/bug-2986-config-schema-mutation-killers.test.cjs (95 tests,
4 suites) targeting each surviving mutant class:

- M1/M4: parameterized isValidConfigKey(key) === true for every member
  of VALID_CONFIG_KEYS. Kills static-key-fast-path mutations
  (if (VALID_CONFIG_KEYS.has(...)) return true; -> if (false) return true;)
  because no static key matches any DYNAMIC_KEY_PATTERN by design.

- M2: representative dynamic-pattern keys (one per pattern). Each matches
  exactly one pattern. Kills .some -> .every mutation: with .every, no
  single key matches all patterns -> all dynamic keys would be rejected.

- M3: strictEqual against the literal boolean true/false (not assert.ok
  truthy checks). Kills polarity-flip mutations.

- Anchor-tightening: keys that differ from valid by one char beyond the
  documented shape (trailing dot-segment, empty agent name, non-enum tier,
  etc.). Kills regex-loosening mutations on ^, $, charset boundaries.

Tests assert on typed boolean return values from the lib's public surface.
Zero source-grep, zero raw-text matching.

* chore(#2986): add changeset fragment for PR #3005

* test(#2986): use dynamic-only rep key for features pattern (CR feedback)

CodeRabbit on PR #3005: features.thinking_partner is in the static
VALID_CONFIG_KEYS set, so the static fast-path returns true before
DYNAMIC_KEY_PATTERNS.some() is ever called. A Stryker mutant that
removed only the features entry from DYNAMIC_KEY_PATTERNS would
survive because the test only ever exercised the static path for
that key.

Replaced features.thinking_partner with features.some_dynamic_feature
which is NOT in static keys, so isValidConfigKey must reach the
dynamic path to return true. Added a per-rep invariant that asserts
each representative key is NOT a member of VALID_CONFIG_KEYS,
catching this class of mistake at test time on any future
representative-key change.
 2026-05-01 21:13:25 -04:00
Tom Boucher
3657c4ea9e fix(#3006): retarget PR-template CHANGELOG checkboxes at the changeset workflow (#3007) 
The three PR templates still asked contributors to tick `CHANGELOG.md
updated`, contradicting the post-#2978 rule (documented in
CONTRIBUTING.md and enforced by scripts/changeset/lint.cjs) that
`CHANGELOG.md` must not be edited directly.

Each checkbox now references `npm run changeset` with the appropriate
`--type` (Fixed/Changed/Added) and notes the `no-changelog` opt-out
label where applicable, so `gh pr create` users land in the correct
workflow by copy-paste.

Closes #3006

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-01 20:01:04 -04:00
Tom Boucher
918f987a19 feat(#2982): extend no-source-grep lint to catch var-binding readFileSync.includes() (#2985) 
* feat(#2982): extend no-source-grep lint to catch var-binding readFileSync.includes()

The base lint (scripts/lint-no-source-grep.cjs) only catches
readFileSync(...).<text-method>() chained directly. The much more
common var-binding form escapes it:

  const src = fs.readFileSync(p, 'utf8');
  // 50 lines later
  if (src.includes('foo')) {}        // ← still grep, lint missed it

Scan of the test suite found ~141 files using this pattern.

Implementation built TDD per #2982 with structured-IR assertions:

  scripts/lint-no-source-grep-extras.cjs
    - detectVarBindingViolations(src) — pure detector, two passes:
      pass 1 collects vars bound from readFileSync, pass 2 finds any
      <var>.<includes|startsWith|endsWith|match|search>( on those vars.
    - detectWrappedAssertOkMatch(src) — flags
      assert.ok(<expr>.match(...)) which escapes the assert.match rule.
    - VIOLATION enum exposes stable codes for tests to assert on.

  scripts/lint-no-source-grep.cjs
    - Wires the new detectors into the existing per-file check; one
      additional violation row per file with the first 3 sample tokens.

  tests/bug-2982-lint-var-binding.test.cjs
    - 13 tests, all assertions on typed VIOLATION enum / structured
      records. Covers all 5 text-match methods, multi-var, no-bind,
      string literal (must NOT trigger), wrapped assert.ok(.match),
      and assert.match (must NOT double-flag).

Migration backlog (#2974 expanded scope):

  - 42 files annotated `// allow-test-rule: source-text-is-the-product`
    (legitimate — they read .md/.json/.yml files whose deployed text
    IS the product)
  - 3 files annotated `// allow-test-rule: pending-migration-to-typed-ir [#2974]`
    (read .cjs/.js source — clear migration debt)
  - 95 files annotated `pending-migration-to-typed-ir [#2974]` with
    `Per-file review may reclassify as source-text-is-the-product
    during migration` (mixed — manual review under #2974)

After this lands the lint reports 0 violations on main; new
violations in PRs surface immediately.

Closes #2982
Refs #2974

* test(#2982): fix truncated test name per CR

The label ended with a bare '(' from a copy-paste mishap. Now reads
'does NOT flag .matchAll(...) — matchAll is not match, so
assert.ok(.matchAll(...)) is not flagged'.

* chore(#2982): add changeset fragment for PR #2985

* chore(#2982): add changeset fragment for PR #2985
 2026-05-01 19:50:10 -04:00
Tom Boucher
17a4321bf5 docs(#2989): promote v1.39.1 hotfix entries from [Unreleased] to dated section (#2991) 
Both v1.39.0 (stable, tagged 2026-05-01T03:05:33Z) and v1.39.1
(hotfix, tagged 2026-05-01T21:03:54Z) shipped to npm but the
CHANGELOG `[Unreleased]` link still pointed at `v1.38.5...HEAD` and
the entries that landed in v1.39.1 were still un-promoted.

Move the five v1.39.1 hotfix entries (#2917, #2949, #2954, #2962,
#2969) into a new `## [1.39.1] - 2026-05-01` section above
`## [1.38.5]`, with a one-line intro and install snippet matching
the conventions used in earlier dated sections.

Update the `[Unreleased]` link to point at `v1.39.1...HEAD`.

Out of scope (separate cleanup):
  - Backfilling a `## [1.39.0]` section. The CHANGELOG never had one;
    this PR doesn't make that worse but also doesn't try to invent
    release-note text from commit messages.
  - The eight v1.39.1 commits without `[Unreleased]` entries
    (#2942, #2944, #2924/#2941, #2940, #2947, #2950, #2948, #2957).
    These weren't in `[Unreleased]` to begin with; faithful
    promotion only moves what was already documented.
  - Adding a `docs/RELEASE-v1.39.1.md` file. The `docs/RELEASE-*.md`
    pattern in this repo is RC-only; stable patches historically
    don't have a counterpart.

The post-v1.39.1 hardening entries (#2980, #2983, #2987 from this
session, plus #2976 which was pre-skipped from the v1.39.1
cherry-pick set after #2980 landed) remain in the new
`[Unreleased]` section — they ship in the next release.

Closes #2989
 2026-05-01 18:21:09 -04:00
Tom Boucher
9d5db87249 feat(#2975): adopt changeset-fragment workflow to eliminate CHANGELOG conflicts (#2978) 
* feat(#2975): adopt changeset-fragment workflow to eliminate CHANGELOG conflicts

Two PRs that both edit `### Fixed` in CHANGELOG.md always conflict on merge.
Recently bit on #2960/#2972 in the same session — fix-the-conflict-and-rebase
tax. Replace the shared-file model with per-PR fragment files that never
share lines.

Implementation built TDD per #2975, vertical slices with structured-IR
assertions throughout:

  scripts/changeset/parse.cjs       - fragment text → typed record + frozen
                                      FRAGMENT_ERROR enum (8 tests)
  scripts/changeset/render.cjs      - fragments → structured IR with
                                      Keep-a-Changelog section ordering
                                      (2 tests)
  scripts/changeset/serialize.cjs   - IR ↔ markdown round-trip pair
                                      (parse(serialize(ir)) === ir,
                                      3 tests)
  scripts/changeset/cli.cjs         - file-I/O wrapper with --json mode;
                                      reads .changeset/, folds into
                                      CHANGELOG.md, deletes consumed
                                      fragments. Idempotent. (1 test)
  scripts/changeset/lint.cjs        - pure verdict (changedFiles, labels)
                                      → { ok, reason } via LINT_REASON
                                      enum. Honors `no-changelog` label.
                                      (5 tests)
  scripts/changeset/new.cjs         - fragment scaffolder with random
                                      adjective-noun-noun filename. Tests
                                      assert via parseFragment round-trip.
                                      (3 tests)

Total: 22 tests, all assertions on typed structured fields. No regex on
text, no String#includes on file content. Lint clean across 356 test files.

Supporting:

  .changeset/README.md              - format spec + workflow docs
  .changeset/eager-hawks-rally.md   - dogfood fragment for THIS PR (will
                                      be the first thing the new release
                                      tool consumes)
  .github/workflows/changeset-required.yml
                                    - CI: every PR runs lint.cjs
  package.json                      - npm run changeset, changelog:render,
                                      lint:changeset
  CONTRIBUTING.md                   - new "CHANGELOG Entries — Drop a
                                      Fragment" section between PR
                                      Guidelines and Testing Standards

Closes #2975

* fix(#2975): address CodeRabbit findings on changeset workflow

7 valid findings (4 Major, 3 Minor); all addressed:

scripts/changeset/parse.cjs
  - Preserve fragment body verbatim. Previously body.trim() ate
    intentional leading whitespace (code blocks, etc.); now trim() is
    used only for the emptiness check, and a single trailing newline
    is stripped (the editor-added one) so well-formed fragments
    round-trip byte-for-byte. Added a regression test asserting a
    code-block-leading body is preserved.

scripts/changeset/cli.cjs
  - Validate flag values during argument parsing. parseArgs now returns
    { ok, opts | error }; rejects `--repo` etc. with no following value
    or with another flag as the value. main() surfaces the error
    message before exiting 2.
  - Handle post-write fragment-deletion failures. After CHANGELOG.md
    is written, any unlink failure is captured into a structured
    deleteFailures list with reason 'fail_fragment_delete'; cmdRender
    returns exitCode=1 with the partial-failure detail instead of
    leaving the changelog updated and fragments behind (which would
    cause double-consumption on rerun).

scripts/changeset/lint.cjs
  - Treat CHANGELOG.md as a linted user-facing path. Direct edits to
    CHANGELOG.md (the bypass route around the new workflow) now fail
    the lint with FAIL_MISSING_FRAGMENT. Added a regression test for
    that case.
  - Use cp.execFileSync instead of cp.execSync for the git diff call.
    Eliminates the shell-interpolation surface on GITHUB_BASE_REF;
    git's own arg parser remains the validator.

scripts/changeset/new.cjs
  - Atomic fragment creation. existsSync() + writeFileSync was racy
    under concurrent invocations. Now writeFileSync uses { flag: 'wx' }
    which fails EEXIST on collision; the random-name retry loop
    catches EEXIST and re-rolls. Throws explicitly after 16 attempts
    rather than silently overwriting.

.changeset/README.md
  - Add language tag `md` to the format example fence (markdownlint
    MD040).

All 25 changeset tests pass; lint clean (356 test files, 0 violations).

* fix(#2975): sanitize --type and validate flag values in new.cjs (CR fixes)

Two CR findings on scripts/changeset/new.cjs:

1. (Minor) `type` was embedded in frontmatter without sanitization. A
   newline in the value (e.g. `--type 'Fixed\ntype: Added'`) would
   corrupt the fragment. scaffoldFragment now validates `type` against
   the Keep-a-Changelog ALLOWED_TYPES set BEFORE writing — same set
   parse.cjs uses on consume. Throws with a typed error referencing
   the allowed values; tests cover the newline case + 4 other
   non-allowed values.

2. (Minor) `--repo` (and other value-taking flags) without a value
   silently set opts.repo to undefined, which produced a cryptic
   ERR_INVALID_ARG_TYPE deep inside path.join. parseArgs now mirrors
   the cli.cjs convention: returns { ok, opts | error }, validates
   that the next token exists and is not itself another flag, and
   surfaces a precise "missing value for --repo" message before exit.
   Added 3 tests: missing-trailing-value, flag-as-value, well-formed.

29 tests pass across the changeset suite (4 new regression tests).
 2026-05-01 18:12:20 -04:00
Tom Boucher
cb98a88139 fix(#2987): skip dry-run publish validation when version is already on npm (#2988) 
The `Dry-run publish validation` step ran `npm publish --dry-run` with
no `if:` guard. `npm publish --dry-run` contacts the registry and
exits 1 with "You cannot publish over the previously published
versions" when the target version exists.

The earlier `Detect prior publish (reconciliation mode)` step already
discovers this case and sets steps.prior_publish.outputs.skip_publish=true.
The actual publish step (further down) is gated on that. The
rehearsal step was missing the gate, so any re-run of an
already-published hotfix blew up at the rehearsal before reaching
the reconciliation logic — exactly when an operator is trying to
recover from a later-step failure (merge-back, summary, etc.).

Add `if: ${{ steps.prior_publish.outputs.skip_publish != 'true' }}`
matching the publish step's gate. The rehearsal still runs on first
publishes where it has value.

Trigger: run 25233855236.

Closes #2987
 2026-05-01 17:39:35 -04:00
Tom Boucher
fb92d1e596 fix(#2983): classifier exit-code discipline, base-tag staging, drop vestigial merge-back (#2984) 
* fix(#2983): classifier exit-code discipline, base-tag staging, drop vestigial merge-back

Three issues surfaced by CodeRabbit's post-merge review of #2981 plus
a production failure on the v1.39.1 release run.

(1) Overloaded classifier exit code

scripts/diff-touches-shipped-paths.cjs reused exit 1 for both the
legitimate "no shipped paths" result and Node's default exit on
uncaught throw, so any classifier failure (corrupt package.json,
EPERM, etc.) was indistinguishable from a normal skip — the workflow's
`if ! ... ; then skip` idiom would silently drop the commit.

Distinct exit codes now:
  0  shipped       — at least one path is in the npm `files` whitelist
  1  not shipped   — CI / test / docs / planning only
  2  classifier error — workflow MUST fail-fast

uncaughtException + unhandledRejection + try/catch around fs/JSON
parsing all route to exit 2 with stderr context.

(2) Classifier missing at the base tag (CRITICAL)

`Prepare hotfix branch` runs `git checkout -b "$BRANCH" "$BASE_TAG"`
BEFORE the cherry-pick loop, replacing the working tree with the base
tag's contents. Base tags predating #2980 (notably v1.39.0, the most
likely next hotfix base) don't have scripts/diff-touches-shipped-paths.cjs
at all — `node <missing>` exits non-zero — `if !` skips every commit —
empty hotfix branch published. Strictly worse than the original #2980
push-rejection, which at least failed loudly.

Stage the classifier from the dispatched ref's working tree into
$RUNNER_TEMP at the top of the run script (before any working-tree-
mutating git command). The cherry-pick loop now references $CLASSIFIER
(staged) instead of the in-tree path. Sanity guards: refuse to start
if scripts/diff-touches-shipped-paths.cjs is missing in the dispatched
ref, refuse to proceed if cp didn't materialize $CLASSIFIER.

The cherry-pick loop captures node's exit via ${PIPESTATUS[1]} and
dispatches via explicit case:
  0  proceed with cherry-pick
  1  skip into NON_SHIPPED_SKIPPED
  *  emit ::error:: + exit "$CLASSIFIER_RC"

(3) Drop the merge-back PR step

Auto-cherry-pick only picks commits already on main (`git cherry HEAD
origin/main` outputs the unmerged ones; we filter fix:/chore: from
main). By construction every code commit on the hotfix branch is
already on main. The only hotfix-branch-only commit is `chore: bump
version to X.Y.Z for hotfix`, which either no-ops against main or
rewinds main's in-progress version. The merge-back PR was vestigial.

It also failed in production on run 25232968975 with `GitHub Actions
is not permitted to create or approve pull requests (createPullRequest)`
— org policy blocks PR creation from the workflow's GH_TOKEN. Even
without that block, the PR would have nothing useful to merge.

Step removed. The `pull-requests: write` permission granted solely
for the merge-back step has been dropped from the release job
(least-privilege).

Regression coverage

tests/bug-2983-classifier-exit-codes-and-base-tag-staging.test.cjs
adds 12 assertions across two describe blocks:

  - 5 classifier behavioral: exit 0/1 preserved, exit 2 on missing
    package.json, exit 2 on malformed JSON, exit-code constants
    exported.
  - 7 workflow contract: classifier staged before checkout, target
    is $RUNNER_TEMP, missing-source guard, missing-staged guard,
    PIPESTATUS-based dispatch, error branch fails workflow, loop uses
    staged path (not in-tree).

tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs updated
where it asserted the pre-#2983 `if ! ... ; then` shape: now accepts
the post-#2983 case-dispatch form. The test still proves the
classifier participates; bug-2983 enforces the specific shape.

Run summary references for the curious reviewer:

  - Run 25232010071 — original #2980 trigger (workflow-file push
    rejection)
  - Run 25232968975 — failed merge-back step that prompted the
    "is this even useful?" question that drove the removal

Closes #2983

* fix(#2983): address CodeRabbit findings on PR #2984

Two findings, both real, both fixed.

(1) [Critical] PIPESTATUS capture clobbered by `|| true`

Pre-fix shape:
  git diff-tree ... | node "$CLASSIFIER" || true
  CLASSIFIER_RC="${PIPESTATUS[1]}"

When the classifier exits 1 ("not shipped" — common case) or 2
(error), `|| true` triggers the right-hand side. `true` is a
one-command "pipeline" that overwrites PIPESTATUS to (0).
${PIPESTATUS[1]} on the next line is therefore unset (or stale
under set -u). The case dispatch then matched the empty string —
falling into `*)` and failing the workflow on every non-shipped
commit, OR matching `0)` after some shells default-init unset
to 0 and silently picking commits that don't ship.

Local repro confirms the issue:

  $ bash -c 'set -euo pipefail; false | sh -c "exit 7" || true; \
       echo "PIPESTATUS: ${PIPESTATUS[*]}"; \
       echo "[1]: ${PIPESTATUS[1]:-<unset>}"'
  PIPESTATUS: 0
  [1]: <unset>

Fix: bracket the pipeline in `set +e`/`set -e`, snapshot
PIPESTATUS into a local array on the very next line, then
dispatch on the snapshot:

  set +e
  git diff-tree ... | node "$CLASSIFIER"
  PIPE_RC=("${PIPESTATUS[@]}")
  set -e
  DIFFTREE_RC="${PIPE_RC[0]}"
  CLASSIFIER_RC="${PIPE_RC[1]}"

The snapshot must happen on the first line after the pipeline;
any intervening simple command resets PIPESTATUS. The array form
is invariant against that.

Bonus from the new shape: $DIFFTREE_RC is now also captured.
git diff-tree is unlikely to fail on a known-good $SHA, but if
it does, we no longer feed partial/empty input to the classifier
and call it "not shipped." A non-zero DIFFTREE_RC emits
::error::git diff-tree failed and exits.

(2) [Minor] Stale "Merge-back PR opened against main" summary line

The hotfix run summary still printed:
  echo "- Merge-back PR opened against main"

But the merge-back step itself was removed in the previous commit
on this branch. Operators reading the summary would expect a PR
that doesn't exist. Replaced with explicit non-action text:

  echo "- No merge-back PR (auto-picked commits are already on main)"

Test coverage

bug-2983 test file gains 3 assertions:
  - PIPE_RC array-snapshot pattern is required (regex matches the
    exact `PIPE_RC=("${PIPESTATUS[@]}")` form).
  - The `pipeline || true; ${PIPESTATUS[1]}` antipattern is
    explicitly forbidden via assert.doesNotMatch.
  - DIFFTREE_RC is captured from PIPE_RC[0] and a non-zero value
    triggers ::error::git diff-tree failed.
  - Run summary forbids `Merge-back PR opened against main` and
    requires the new non-action sentence.

bug-2964 test's loop-anchor window bumped 6 KB → 8 KB to
accommodate the additional pre-pick scaffolding (the test's own
comment had already anticipated this kind of growth, citing prior
precedents from #2970 and #2980).

Mark CodeRabbit comments resolved post-commit.

Refs CR finding ids 3175253571, 3175253578 on PR #2984.
 2026-05-01 17:25:20 -04:00
Tom Boucher
7424271aa0 fix(#2980): hotfix cherry-pick only picks commits that change what ships (#2981) 
* fix(#2980): pre-skip workflow-file cherry-picks in release-sdk hotfix loop

The default GITHUB_TOKEN issued to the release-sdk run lacks the
`workflow` scope, so the prepare job's `git push origin "$BRANCH"` is
rejected by GitHub when any cherry-picked commit modifies a file under
`.github/workflows/`:

  ! [remote rejected] hotfix/X.YY.Z -> hotfix/X.YY.Z
    (refusing to allow a GitHub App to create or update workflow ...
     without `workflows` permission)

Pre-#2980 behavior: the auto_cherry_pick loop happily picked
workflow-file commits, then the trailing push exploded with no clear
signal which commit was the culprit. v1.39.1 hit this on PR #2977
(run 25232010071) — earlier release-sdk fixes (#2965, #2967, #2970)
had been skipped on conflict so their workflow-file changes never
reached the push step, masking the bug; #2977 was the first
workflow-file commit to apply cleanly and the push immediately
exploded.

Fix: pre-pick guard in the cherry-pick loop. Inspect each candidate
commit's file list via `git diff-tree --no-commit-id --name-only -r`
BEFORE attempting the pick. If any path matches `^\.github/workflows/`,
skip the commit, emit a `::warning::` annotation naming the dropped
commit, and append to a new `WORKFLOW_SKIPPED` bucket. The run summary
surfaces this bucket in its own section, distinct from `CONFLICT_SKIPPED`
(real merge conflicts) and `POLICY_SKIPPED` (feat/refactor exclusions),
so operators reviewing the run never confuse the remediation paths.

The loud-warning piece is non-negotiable: silent drops were explicitly
rejected as a failure mode during the option-1/2/3 tradeoff discussion.
If a workflow-file fix genuinely needs to ship in a hotfix, the
operator applies it manually on the hotfix branch using a token with
`workflow` scope, or lands it on main and re-cuts the release.

Regression covered by tests/bug-2980-skip-workflow-file-cherrypicks.test.cjs
(5 assertions: pre-pick guard exists, uses `git diff-tree`, emits
`::warning::`, lands in dedicated bucket, surfaces in summary).

The bug-2964 test's 4 KB window after the cherry-pick-loop anchor was
nudged to 6 KB to accommodate the new pre-pick scaffolding — the test's
own comment had already anticipated this kind of growth (citing #2970's
merge-commit pre-skip as prior precedent).

Closes #2980

* refactor(#2980): replace workflow-file pre-skip with shipped-paths filter

The previous commit on this branch caught only the .github/workflows/*
subset of the bug, treating the symptom (push rejection on workflow-file
changes) rather than the root cause (the fix:/chore: filter is too broad
— it picks any commit with that conventional-commit type even when the
diff cannot affect the published npm package).

CI-only fixes (release-sdk.yml itself, hotfix tooling, test-only
commits) shouldn't flow through hotfix runs at all — they cannot change
what `npm install get-shit-done-cc@X.YY.Z` produces. The
.github/workflows/* push rejection is just the loudest of these
"shouldn't have been picked" cases; tests/, docs/, .planning/ commits
get picked silently with the same lack of effect on consumers.

Replace the workflow-file pre-skip with a shipped-paths filter:

- New scripts/diff-touches-shipped-paths.cjs reads package.json `files`,
  plus package.json itself (always-shipped per `npm pack` semantics),
  and exits 0 iff any input path is in the shipped set. Lockfile is
  not shipped (npm pack excludes it unless explicitly in `files`).
- Workflow loop now pipes `git diff-tree --no-commit-id --name-only -r`
  through the classifier; on exit 1 the commit is skipped and
  appended to a new NON_SHIPPED_SKIPPED bucket (replaces
  WORKFLOW_SKIPPED).
- Run summary surfaces NON_SHIPPED_SKIPPED as informational — no
  ::warning:: annotation. A non-shipping commit cannot affect the
  package, so a yellow alert would imply remediation is possible
  and would mislead operators.

The classifier in a separate .cjs file (rather than inline bash
heredoc) is so its rules — directory-prefix vs exact-match,
package.json-always-shipped, lockfile-not-shipped — are unit-testable
in tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs (11 new
assertions: 4 static workflow + 6 classifier behavioral + 1 mixed-
diff edge case).

Why this dissolves the original push-rejection bug: workflow files
aren't in `files`, so workflow-only commits are skipped pre-pick.
The push step never sees them.

If a workflow-file fix genuinely needs to ship in a hotfix release
(extremely rare — the hotfix workflow is read from main's ref, not
the hotfix branch's), the operator applies it manually using a token
with `workflow` scope. The pre-skip puts that requirement in the run
summary explicitly.

Closes #2980
 2026-05-01 16:59:49 -04:00
Tom Boucher
7a416b10d4 fix(#2976): allow same-version bump in release-sdk hotfix release job (#2977) 
The release job's "Bump in-tree version (not committed)" step ran
`npm version "$VERSION" --no-git-tag-version` without --allow-same-version,
so on real hotfix runs it failed with `npm error Version not changed` —
because the prepare job had already committed the bump on the hotfix
branch (the release job checks out BRANCH on real runs vs BASE_TAG on
dry-runs, which is why dry-run never caught it).

Pass --allow-same-version to both bumps, matching release.yml:326.

Closes #2976
 2026-05-01 16:32:18 -04:00
Tom Boucher
ef43f5161f fix(#2969): deterministic Step 5 verification gate for /gsd-reapply-patches (#2972) 
* fix(#2969): deterministic Step 5 verification gate for /gsd-reapply-patches

The prior Step 5 "Hunk Verification Gate" was prescribed correctly in the
workflow text — but executed laxly by the LLM, which filled in `verified: yes`
without actually checking content presence. The reporter observed three
distinct files (skills/gsd-discuss-phase/SKILL.md, skills/gsd-autonomous/
SKILL.md, get-shit-done/workflows/new-project.md) where archives contained
substantive user-added blocks that did not survive into the merged result, yet
the gate reported clean.

Move verification from LLM-driven prose into a deterministic Node script the
workflow calls. The script can't be shortcut.

Changes:

- scripts/verify-reapply-patches.cjs (new): pure Node, no external deps.
  For each file in the patches dir, computes user-added significant lines as
  the line-set diff between backup and pristine baseline (when available;
  falls back to "every significant backup line" when no pristine — over-broad
  but the safe direction for this bug class). Asserts each line appears
  literally in the merged installed file via String.prototype.includes.
  Filters trivial lines (length < 12 chars, pure punctuation, decorative
  comments) so harmless drift doesn't trigger false failures. Exits 0 on
  pass, 1 on any miss with per-file diagnostic, 2 on usage error.
  Supports --json for workflow consumption.

- get-shit-done/workflows/reapply-patches.md: rewrite Step 5 to call the
  script and parse its JSON output. The Step 4 Hunk Verification Table
  remains as advisory Claude-readable summary, but the gate is now the
  script's exit code.

- tests/bug-2969-verify-reapply-patches.test.cjs (new): 6 tests covering
  (a) pass when every line survives, (b) fail when a line is missing,
  (c) fail when the merged file is deleted entirely, (d) --json structured
  report shape, (e) backup-meta.json is correctly skipped as metadata,
  (f) no-pristine-dir fallback exercises the safe over-broad path. All pass.

Out of scope: the manifest-baseline tightening described in #2969 Failure 1
(saveLocalPatches comparing against the wrong baseline so prior silent wipes
poison subsequent updates). That's a separate, bigger architectural change
involving pristine-content infrastructure; this PR addresses the gate fidelity
half so users at least see the diagnostic when content goes missing.

Closes #2969 (partial — Failure 2 only)

* fix(#2969): preserve #1999 Hunk Verification Table assertions alongside new script gate

CI failure on PR #2972 surfaced that tests/reapply-patches.test.cjs (the
#1999 contract) asserts Step 5 references:
  - "Hunk Verification Table"
  - `verified: no` failure condition
  - explicit STOP/halt/abort directive
  - "table absent / missing" halt path

My initial Step 5 rewrite for #2969 substituted the deterministic script
for the table-based gate entirely, stripping those references. The script
is the strictly stronger gate, but the existing #1999 test enforces the
table-based safety net as a defense-in-depth contract.

Restore both gates as a layered Step 5:

  - 5a (binding): deterministic verifier script — script gate, exits
    non-zero on any miss, cannot be shortcut by the LLM
  - 5b (advisory): Hunk Verification Table review — preserved as
    redundant safety net for the case where the script has a bug or the
    pristine baseline is unavailable

Both gates must pass. Verified: tests/reapply-patches.test.cjs (5 tests
in the #1999 suite) and tests/bug-2969-verify-reapply-patches.test.cjs
(6 tests in the #2969 suite) all pass — 21/21 total in this fixture.

* fix(#2969): address CodeRabbit findings on workflow + script

Five CR findings on PR #2972, all valid; addressed in this commit:

1. (Major) Stderr was merged into VERIFY_OUTPUT via `2>&1`, so any Node
   warning, deprecation notice, or stack trace would corrupt the JSON
   parse downstream. Capture stdout only; stderr remains on the
   controlling terminal for operator visibility.

2. (Major) verifyFile() crashed with EISDIR/EACCES instead of producing
   a structured diagnostic when the installed path was a directory or
   unreadable. Wrap statSync/readFileSync in try/catch and emit a
   per-file fail row; the whole-run gate continues with structured
   output. Added test case asserting the directory-at-installed-path
   case fails with `not a regular file` diagnostic instead of crashing.

3. (Minor) PRISTINE_FLAG built as a single string + unquoted expansion
   would split paths with spaces. Switched to a bash array (VERIFY_ARGS)
   that preserves whitespace through expansion.

4. (Minor) Fenced code block missing language tag (markdownlint MD040).
   Added `text` tag to the error message block.

5. (Minor) Usage comment said pristine fallback was "backup-meta lookup"
   but the actual code path falls back to significant-line checks from
   backup content. Corrected the comment to match implementation.

Verified all 21 tests in tests/reapply-patches.test.cjs (#1999 contract)
+ tests/bug-2969-verify-reapply-patches.test.cjs (now 7 tests with the
new directory case) pass.

* test(#2969): structured JSON assertions, no substring matching on script output

Replace every assert.match(r.stdout, /pattern/) call with structured
assertions on the parsed JSON report from the script's own --json mode.
The script's --json contract IS the structured shape we test against —
the test author should never depend on the human-readable formatter
output, just as no test should depend on substring presence in source.

Changes:

  - All 7 tests now run the verifier with --json (via a runVerifier()
    helper) and parse the resulting JSON document into { status, report,
    stderr }. Diagnostic stderr is preserved as a separate channel for
    debug output but is not used for assertions.
  - Each previously substring-matched diagnostic ("Failures: 1",
    "not a regular file", "installed file missing after merge",
    file path, dropped line) is now a deepEqual / equal / Array.includes
    against typed report fields: report.failures, report.results[i].status,
    report.results[i].reason, report.results[i].file,
    report.results[i].missing[].
  - Added an explicit "documented shape" test asserting the JSON output
    has exactly the keys { file, missing, reason, status } per result —
    locks the public contract of the --json mode.
  - DRY'd up fixture reset into a resetFixture() helper since every test
    starts with a fresh patches/installed/pristine triple.

Linter: scripts/lint-no-source-grep.cjs reports 0 violations across 348
test files. Combined run of bug-2969-...test.cjs (7 tests) +
reapply-patches.test.cjs (5 tests in the #1999 suite) all pass —
22/22 in the relevant fixture.

* fix(#2969): typed REASON enum + raw-text-matching rule shipped repo-wide

This commit closes the loop on the no-source-grep discipline:

1. scripts/verify-reapply-patches.cjs:
   - Frozen REASON enum exposes the diagnostic surface as stable codes:
     OK_NO_USER_LINES_VS_PRISTINE, OK_NO_SIGNIFICANT_BACKUP_LINES,
     FAIL_INSTALLED_MISSING, FAIL_INSTALLED_NOT_REGULAR_FILE,
     FAIL_READ_ERROR, FAIL_USER_LINES_MISSING.
   - Each result.reason is now a code from this enum, not free text.
     Tests assert via REASON.X equality, not regex on prose.
   - REASON exported from module.exports.

2. tests/bug-2969-verify-reapply-patches.test.cjs:
   - Full rewrite. Every assertion on typed structured fields:
     report.results[0].status === 'fail',
     report.results[0].reason === REASON.FAIL_INSTALLED_NOT_REGULAR_FILE,
     report.results[0].missing.includes(droppedLine) (Array set membership,
     not String substring).
   - Locks the REASON enum surface via Object.keys(REASON).sort() deepEqual.
   - Locks the JSON report shape via Object.keys(report).sort() deepEqual.
   - Zero regex, zero String#includes, zero startsWith/endsWith on text.

3. CONTRIBUTING.md:
   - New section "Prohibited: Raw Text Matching on Test Outputs" with
     concrete BAD/GOOD examples (substring on file content; assert.match
     on stdout; "structured parser" hiding string ops; regex on free-form
     reason fields).
   - The rule statement: "Tests assert on typed structured values. If
     the code under test produces text, the code under test must also
     expose a structured intermediate representation, and the test must
     assert on that IR — never on the rendered text."
   - Required structured-surface table: file IR, --json mode, frozen
     enum, fs facts.
   - "Hiding grep behind a function is still grep" callout — the
     parser-wrapper anti-pattern.
   - New `pre-existing-text-matching` exemption category for the 8
     grandfathered files. Marked Transitional; new tests cannot use it.

4. scripts/lint-no-source-grep.cjs:
   - Three new patterns enforced (in addition to the existing .cjs-source
     readFileSync rule):
     - assert.match/doesNotMatch on .stdout/.stderr
     - .stdout/.stderr.<includes|startsWith|endsWith>(
     - readFileSync(...).<includes|startsWith|endsWith>(
   - Aggregated violations per file (multiple findings now report together).
   - Updated diagnostic message references both CONTRIBUTING.md sections.

5. 8 pre-existing tests annotated with `// allow-test-rule:
   pre-existing-text-matching` so the lint passes on this commit; each
   carries the prose "Tracked for migration to typed-IR assertions; do
   not copy this pattern." Files: bug-2649, bug-2687, bug-2796, bug-2838,
   bug-2943, graphify, hooks-opt-in, security-scan.

Verification: lint 0 violations across 348 test files; full suite passes.

* fix(#2969): rename exemption category to pending-migration-to-typed-ir + cite tracking issue

Per maintainer feedback:
1. "Grandfathered" / "legacy" framing is wrong — both terms imply
   permanent or condoned exemption. The 8 files are tracked for
   correction, not exempted.
2. Each annotated file must cite the tracking issue so the migration
   work is auditable.

Changes:
- CONTRIBUTING.md: rename exemption category from
  `pre-existing-text-matching` to `pending-migration-to-typed-ir`. Update
  prose to "Tracked for correction, not exempted" and require each
  annotation to cite the open migration issue (e.g.
  `// allow-test-rule: pending-migration-to-typed-ir [#NNNN]`).
- 8 test files: update annotation to cite #2974 (the tracking issue
  opened for migrating these files to typed-IR assertions).
 2026-05-01 16:14:39 -04:00
Tom Boucher
e9a66da1e7 fix(#2962): write npm-style gsd-sdk shim on Windows under --sdk install (#2971) 
* fix(#2962): write npm-style gsd-sdk shim on Windows under --sdk install

trySelfLinkGsdSdk previously contained `if (process.platform === 'win32')
return null;` — a missed gap from #2775's POSIX self-link rather than an
intentional design choice. As a result, `npx get-shit-done-cc@latest
--claude --global --sdk` on Windows left `gsd-sdk` off PATH despite the
installer reporting success, and the obvious recovery (`npm i -g
@gsd-build/sdk`) lands the stale 0.1.0 publication that lacks the `query`
subcommand the agents call ~40 times.

This PR addresses the shim half. The npm-publish half (publishing
@gsd-build/sdk at parity with the get-shit-done-cc version) requires
maintainer credentials and is left for separate action.

Changes:

- bin/install.js: replace the unconditional Windows return-null with
  dispatch to a new trySelfLinkGsdSdkWindows() that:
  * resolves npm's global bin via `execFileSync('npm', ['prefix', '-g'])`
    (no shell interpolation; npm is the only PATH-resolved binary)
  * verifies write access with a probe before producing partial state
  * writes the standard npm shim triple to npm's global bin:
    - gsd-sdk.cmd (cmd.exe; CRLF line endings)
    - gsd-sdk.ps1 (PowerShell)
    - gsd-sdk    (Bash wrapper for Cygwin/MSYS/Git-Bash)
  * each shim invokes `node "<absolute path to bin/gsd-sdk.js>"` with the
    passed args, decoupling shim location from SDK location — same logical
    structure as the POSIX wrapper-via-require() fallback above
  * unlinks any stale shims before writing so prior installs don't pin
    callers to a now-absent path
  * returns the .cmd path on success (handle the existing onPath check
    looks for) or null on any failure, falling through to the existing
    "gsd-sdk is not on your PATH" warning at line 8704

- tests/bug-2962-windows-sdk-shim.test.cjs (new): 5 tests exercising
  trySelfLinkGsdSdkWindows directly with cp.execFileSync mocked to redirect
  npm prefix to a temp dir. Asserts shim contents reference the absolute
  path, .cmd uses CRLF, stale shims are replaced not appended, and null is
  returned when `npm prefix -g` fails.

- tests/no-unconditional-win32-skip.test.cjs (new): regression guard
  that fails CI if any future commit re-introduces
  `if (process.platform === 'win32') return null;` (or similar
  skip-only branches) in bin/install.js. Negative test verified by
  transiently re-introducing the bad pattern → guard fired → restored
  → passes.

Out of scope: publishing @gsd-build/sdk@<current> to npm so the natural
`npm i -g @gsd-build/sdk` recovery also lands a usable SDK. That requires
maintainer credentials and is the second half of the issue.

Closes #2962

* fix(#2962): address CodeRabbit findings — execSync for npm.cmd, behavior-based regression guard

CR finding 1 (🟠 Major): Node's child_process docs explicitly call out that
.cmd/.bat files cannot be spawned via execFile/execFileSync without a shell
("Spawning .bat and .cmd files on Windows" section). Since `npm` on Windows
is `npm.cmd`, my use of execFileSync('npm', ['prefix', '-g'], { shell: false })
would have failed on the very platform this PR is meant to fix.

Switched to cp.execSync('npm prefix -g', ...) — matching the existing
convention at line ~8718 which makes the same lookup. Args are static literals
so shell interpolation is not an injection vector.

CR finding 2 (🟠 Major): the source-grep regression test in
tests/no-unconditional-win32-skip.test.cjs violated the repo's no-source-grep
testing standard (CONTRIBUTING.md). Replaced with a behavior-based test that:

  - overrides process.platform to 'win32' via Object.defineProperty
  - mocks cp.execSync to return a temp-dir as npm prefix
  - calls trySelfLinkGsdSdk(shimSrc) and asserts it returns non-null AND
    materializes gsd-sdk.cmd on disk

The behavior guard is strictly stronger than the regex version: it would
catch any equivalent skip pattern (e.g. os.platform() === 'win32', a
typeof-based guard, etc.), not just literal `if (process.platform === 'win32')`
text. Negative-tested by re-introducing the `return null` skip → test fails
with maintainer-quoted diagnostic "trySelfLinkGsdSdk must not silently
return null on Windows; a no-op skip is a missed-parity regression"; restored
→ passes.

Test for Windows shim materialization (bug-2962-windows-sdk-shim.test.cjs)
also updated to mock cp.execSync (matching the new production code path)
instead of cp.execFileSync.

Full suite: 6480/6480 pass.

* test(#2962): make Windows shim tests self-contained per CR

Each test now invokes trySelfLinkGsdSdkWindows() itself before reading
the shim files, so they don't implicitly depend on the earlier test's
side effects. Addresses CR's order-dependence finding.

* test(#2962): structured shim parsing — eliminate substring source-grep

CR found that even after the prior refactor, three tests in the suite
still used .includes()/.startsWith() against shim file content
(cmdContent.includes(\`@node ${jsonQuoted} %*\`) etc.). Substring matching
on file text is the same anti-pattern the no-source-grep standard
forbids — even when the file is one this test wrote — because it asserts
a literal exists rather than that the structured shape is correct.

Replace with three small parsers (parseCmdShim, parsePs1Invocation,
parseBashInvocation) that split each shim into header + invocation
tokens and assert via deepEqual on structured records. The assertions
now check that the .cmd has @ECHO OFF / @SETLOCAL / @node <abs> %* in
that order with exactly 3 meaningful lines, and that the .ps1 and bash
wrappers split into the expected (call, nodeCmd, target, argToken)
tuples.

The stale-shim replacement test was hardened the same way: instead of
proving the absence of a sentinel substring, it now proves the parsed
target equals the new shimSrc and != the old path.

Verified: scripts/lint-no-source-grep.cjs reports 0 violations across
348 test files. The 6-test windows-sdk-shim + win32-skip-guard suite
all pass.

* fix(#2962): expose pure shim IR + tests assert on typed fields, not rendered text

Earlier "structured parser" approach (parseCmdShim / parsePs1Invocation /
parseBashInvocation) was still raw-text manipulation behind a function
wrapper — split('\\r\\n'), trim().split(/\\s+/), content.includes('\\r\\n').
Maintainer was right: hiding grep behind a parser is still grep.

Real fix: refactor production code to expose the structured intermediate
representation, and have tests assert on the IR fields directly.

Production:
- New buildWindowsShimTriple(shimSrc) — pure function, no fs/spawn.
  Returns { invocation: { interpreter, target }, eol: { cmd, ps1, sh },
  fileNames: { cmd, ps1, sh }, render: { cmd: () => string, ... } }.
  The IR is the contract; rendered text is an implementation detail of
  the renderers.
- trySelfLinkGsdSdkWindows now calls buildWindowsShimTriple, looks up
  filenames from triple.fileNames, and writes triple.render[kind]() to
  each target. Same observable behavior, structurally separated.
- buildWindowsShimTriple added to test-mode exports.

Tests (full rewrite — no shim file content is read at any point):
- Layer 1: pure-IR tests assert on triple.invocation.target,
  triple.eol === { cmd: '\\r\\n', ps1: '\\n', sh: '\\n' },
  triple.fileNames === { cmd: 'gsd-sdk.cmd', ... }, and the
  documented IR shape via Object.keys().sort() deepEqual.
- Layer 2: fs/spawn driver tests assert filesystem FACTS:
  - return value equals expected path
  - all three target files exist as regular non-empty files
  - rendered file byte length === Buffer.byteLength of triple.render(kind)
    output (proves the writer writes what the renderer produces, no
    mutation, no truncation, no double-write — without comparing content)
  - mtime advances on rewrite (proves stale-replace behavior)
  - returns null when npm prefix -g throws

No more split, .includes, .startsWith, .endsWith, or substring matching
anywhere in the test suite. Lint clean. 10/10 tests pass.
 2026-05-01 16:10:30 -04:00
Tom Boucher
b8d9bd69b2 fix(release-sdk): skip all cherry-pick conflicts in hotfix loop (full automation) (#2970) 
* fix(release-sdk): skip all cherry-pick conflicts in hotfix loop

Full-automation policy: any conflict the cherry-pick can't auto-resolve
— context-missing (#2966) or real merge conflict — is now skipped, not
aborted. The hotfix run completes with whatever applies cleanly; the
SKIPPED list in the run summary becomes the operator's post-hoc review
queue.

Surfaced in run 25227493387 (1.39.1 dry-run): commit 0fb992d
("fix(git): add git.base_branch config") produced real conflicts in
config.cjs / ship.md / complete-milestone.md / tests/config.test.cjs.
v1.39.0 was tagged on the feat/hermes-runtime-2841 branch (#2920),
which restructured those files. 0fb992d was authored against the
pre-restructure shape, so cherry-pick can't auto-resolve.

Pre-#2968 behavior: the workflow distinguished context-missing (skip)
from real (abort + push partial + exit 1). Real conflicts blocked every
hotfix from a base tag whose lineage diverged from main — exactly the
v1.39.x situation. The user has called explicitly for full automation:
"this needs to be fully automated, no one is going to sit there and
tag fixes."

Behavior change:
  - Both classification branches now `git cherry-pick --skip` and
    append to SKIPPED with a reason category:
      * "context absent at base" — empty-HEAD markers (#2966)
      * "merge conflict — manual review" — non-empty HEAD (#2968)
  - Removed: `git cherry-pick --abort`, partial-state push,
    "Cherry-pick conflict" GITHUB_STEP_SUMMARY block, `exit 1`.
  - Operator's manual recovery path via `auto_cherry_pick=false`
    remains intact.

Trade-off (acknowledged in #2968): a critical fix can be silently
dropped if no one reviews the SKIPPED list. The release job's
install-smoke + full test suite still runs and would catch any
test-covered regression. Fixes that aren't test-covered could ship
missing — accepted cost of full automation per the issue.

Tests:
  - tests/bug-2968-cherry-pick-skip-on-any-conflict.test.cjs (new) —
    extracts the cherry-pick failure block via bash if/fi nesting walk
    (no raw-text grep) and asserts the abort path is removed, --skip
    is unconditional, and "merge conflict" + "context absent at base"
    annotations both exist.
  - tests/bug-2966-cherry-pick-context-missing.test.cjs (renamed
    describe + first test name) — assertions still valid since the
    classifier survives for skip-reason annotation.
  - tests/bug-2964-release-sdk-empty-cherry-pick.test.cjs — unchanged
    and still green.

Local: `node --test tests/bug-2964-...test.cjs tests/bug-2966-...test.cjs
tests/bug-2968-...test.cjs` → 8/8 pass.
Local: `npm run lint:tests` → 0 violations.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

* fix(release-sdk): split cherry-pick conflict skips from policy skips

CodeRabbit flagged on PR #2970 that conflict skips and policy skips
share the SKIPPED bucket. The run summary heading
"Skipped (feat/refactor/etc — not auto-included)" buries manual-review
conflicts (which the operator must triage) under the same list as
intentional policy exclusions (commits that don't match fix/chore by
design and need no action). Operators reviewing the summary can't
distinguish the two without reading every entry.

Split into two variables:
  - POLICY_SKIPPED — feat/refactor/docs/etc filtered out by the
    fix/chore regex (informational, no action needed)
  - CONFLICT_SKIPPED — fix/chore commits whose cherry-pick failed and
    were skipped per the full-automation policy (#2968) (manual
    review queue)

Run summary now emits two sections with distinct headings:
  - "Skipped — cherry-pick conflict (manual review)"
  - "Not auto-included (feat/refactor/docs/etc)"

The new bug-2968 test asserts both buckets are populated correctly:
  - failure path appends to CONFLICT_SKIPPED, not SKIPPED
  - both bucket variables are echoed in the summary
  - both section headings are present

Local: `node --test tests/bug-2964-...test.cjs tests/bug-2966-...test.cjs
tests/bug-2968-...test.cjs` → 9/9 pass.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

* fix(release-sdk): handle merge commits and guard cherry-pick --skip

CodeRabbit flagged a real major issue on PR #2970: merge commits with
fix:/chore: titles fail BEFORE entering cherry-pick state because they
need `-m <parent>` to specify the diff base. Without it, the cherry-pick
errors out and CHERRY_PICK_HEAD is never created. The unconditional
`git cherry-pick --skip` call that follows then fails too (no in-progress
cherry-pick to skip), bricking the loop — defeating the full-automation
policy this PR set out to deliver.

Two guards added:

1. Pre-skip merge commits before invoking cherry-pick. The loop checks
   parent count via `git rev-list --parents -n 1 "$SHA"`; if > 1, the
   commit goes straight to CONFLICT_SKIPPED with reason "merge commit —
   manual -m parent selection required". Operator decides which parent
   to keep when reviewing the run summary.

2. Guard `git cherry-pick --skip` with a CHERRY_PICK_HEAD existence
   check. Catches any other failure mode where the cherry-pick aborts
   before entering conflict state (unreadable commit, ref problems,
   etc.) so the loop still continues cleanly.

Also bumped the bug-2964 test's regex slice window from 2000 to 4000
chars so the merge-commit pre-skip block doesn't push the cherry-pick
line out of the test's match range.

Tests added in tests/bug-2968-cherry-pick-skip-on-any-conflict.test.cjs:
  - merge-commit detection: workflow must call
    `git rev-list --parents -n 1 "$SHA"` before cherry-pick and annotate
    skips with the distinct "manual -m parent selection required"
    reason.
  - guard: failure block must check CHERRY_PICK_HEAD before --skip.

Local: `node --test tests/bug-2964-...test.cjs tests/bug-2966-...test.cjs
tests/bug-2968-...test.cjs` → 11/11 pass.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

* fix(release-sdk): guard awk classifier against degenerate unmerged paths

CodeRabbit raised two issues on PR #2970:

1. Major (workflow): the `awk` classifier runs under `set -euo pipefail`.
   If a CONFLICTED path is missing/unreadable, awk exits non-zero and
   terminates the entire step — bricking the loop on a degenerate file.
   Also, an unmerged path with no `<<<<<<< ` markers (path-level conflict
   or anomalous git state) was misclassified as "context absent at base"
   (the auto-skip path), letting potentially-real conflicts skip silently.

   Fix: before invoking awk, check `[ ! -r "$CONFLICTED" ]` and
   `grep -q '^<<<<<<< ' "$CONFLICTED"`. Either failure marks
   ALL_EMPTY_HEAD=false → REASON falls through to "merge conflict —
   manual review", landing the pick in the operator review queue.
   Also added `2>/dev/null || echo "real"` on the awk call so a
   transient awk failure can't slip into the auto-skip bucket.

2. Nitpick (tests): regex assertions on `failureBlock` could match
   commented lines (e.g. comment text mentioning "CONFLICT_SKIPPED"
   or "git cherry-pick --skip" satisfied the assertions without the
   real command being present).

   Fix: anchor with `^\s*...` + `m` flag so only executable shell lines
   count.

Plus a new test asserting all three workflow guards
(`[ ! -r "$CONFLICTED" ]`, `grep -q '^<<<<<<< '`, `awk ... || echo
"real"`) are present in the failure block.

Local: `node --test tests/bug-2964-...test.cjs tests/bug-2966-...test.cjs
tests/bug-2968-...test.cjs` → 12/12 pass.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
 2026-05-01 15:15:20 -04:00
Tom Boucher
0d25ef0c47 fix(release-sdk): skip cherry-picks whose target context is absent at base (#2967) 
* fix(release-sdk): skip cherry-picks whose target context is absent at base

When auto_cherry_pick processed a fix:/chore: commit whose patch modified
code that didn't exist at the hotfix base tag — typically because the
surrounding infrastructure was added later in a feat/refactor commit
excluded by the filter — `git cherry-pick` failed with a conflict that
no operator could meaningfully resolve, and the loop bricked the run.

Discovered re-running the 1.39.1 dry-run after #2965 merged: cherry-pick
of `a3467792` (the #2965 merge itself) failed because the auto_cherry_pick
block it modifies was added in #2956 ("Add automated cherry-pick + SDK-
bundle parity to hotfix flow") — an Add/feat commit, so the fix/chore
filter excludes it. v1.39.0 has no such block, so the patch had no
anchor.

The conflict is unmistakably distinguishable from a real content conflict:
git emits marker blocks where every `<<<<<<< HEAD ... =======` HEAD
section is empty (no anchor lines to reconcile against), while real
conflicts have content on both sides.

After cherry-pick fails:
  1. List unmerged paths via `git diff --diff-filter=U`.
  2. For each, scan conflict markers with awk. If every HEAD section is
     blank/whitespace-only across every block, classify as
     context-missing.
  3. Context-missing → `git cherry-pick --skip` and append to SKIPPED
     list with reason "(context absent at base)".
  4. Otherwise fall through to the existing abort/push-partial/error
     path that surfaces the conflict for operator resolution.

Real conflicts still surface with the same workflow as before.

Tests in tests/bug-2966-cherry-pick-context-missing.test.cjs cover:
  - Static — extracts the "Prepare hotfix branch" run block via
    indentation-aware YAML parsing (no raw-text grep) and asserts the
    classification predicate, --skip call, and skipped-reason annotation
    are present.
  - Behavioral — synthetic repo reproducing the real shape of the
    failure, asserts cherry-pick exits non-zero and produces the
    empty-HEAD marker shape.
  - Predicate — pulls the awk script out of the deployed workflow and
    feeds it sample conflict shapes (empty-HEAD, real, mixed,
    whitespace-only); asserts each is classified as the workflow will
    behave.

Local: `node --test tests/bug-2966-...test.cjs` → 3/3 pass.
Local: `npm run lint:tests` → 0 violations.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

* fix(release-sdk): pin merge.conflictStyle=merge on hotfix cherry-pick

CodeRabbit flagged on #2967 that the awk classifier introduced for #2966
assumes default conflict-marker style (plain `<<<<<<< HEAD ... ======= ...
>>>>>>>`). If a runner has merge.conflictStyle=diff3 or zdiff3 set
(globally, repo-config, or via git defaults shift), the marker emits an
extra `||||||| ancestor` section between HEAD and =======. The awk's
`in_head` mode would accumulate that ancestor content into the HEAD
buffer, and a context-missing conflict would misclassify as real —
sending the workflow into the abort path on a pick that should be
silently skipped.

Pass `-c merge.conflictStyle=merge` on the cherry-pick command itself
(scoped to that one git invocation; doesn't leak to other commands).
This guarantees marker shape regardless of the runner's git config.

Updated the existing static assertion in
tests/bug-2966-cherry-pick-context-missing.test.cjs to require the pin —
a future edit dropping it fails the test.

Local: `node --test tests/bug-2966-...test.cjs` → 3/3 pass.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

* test(#2964): allow git options between `git` and `cherry-pick`

The previous commit on this branch (d6530190) added
`git -c merge.conflictStyle=merge cherry-pick ...` to release-sdk.yml.
The bug-2964 static test's regex `/git cherry-pick[^\n]*"\$SHA"/`
required `cherry-pick` to be the literal next token after `git`, so it
no longer matched the line and CI failed on Node 22 / Node 24 / macOS.

Loosen to `/git\b[^\n]*?cherry-pick[^\n]*"\$SHA"/` so any options
between `git` and `cherry-pick` (e.g. `-c key=value`) are tolerated.
The flag assertions on the matched line still verify --allow-empty and
--keep-redundant-commits are present, which is what bug-2964 actually
guards.

Local: `node --test tests/bug-2964-...test.cjs tests/bug-2966-...test.cjs`
→ 5/5 pass.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

* test(#2966): pin merge.conflictStyle in test git wrapper, assert awk status

CodeRabbit raised two issues on PR #2967:

1. The synthetic-repo cherry-pick reproducer asserted `<<<<<<< HEAD ...`
   blocks have empty HEAD sections, but the cherry-pick itself didn't
   pin `merge.conflictStyle`. A developer or CI runner with global
   diff3/zdiff3 config would inject `||||||| ancestor` lines into the
   HEAD scan and the test would fail for environment reasons rather
   than the bug premise. Pin the style on the test's `git()` wrapper
   so every git operation in the test is deterministic regardless of
   user config.

2. `classify()` ran awk and consumed `r.stdout.trim()` without checking
   `r.status` or `r.error`. A failed awk invocation (missing binary,
   syntax error, signal) returns empty stdout, which would falsely
   classify as "context-missing" and the test would silently pass on
   broken predicates. Add `assert.ok(!r.error, ...)` and
   `assert.equal(r.status, 0, ...)` before reading stdout.

Local: `node --test tests/bug-2966-...test.cjs tests/bug-2964-...test.cjs`
→ 5/5 pass.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
 2026-05-01 14:35:18 -04:00
Tom Boucher
a346779213 fix(release-sdk): allow empty/redundant commits during hotfix cherry-pick (#2965) 2026-05-01 13:56:24 -04:00
Tom Boucher
0d6abb87ac fix(#2954): align help.md with post-#2824 skill consolidation (#2959) 2026-05-01 13:36:44 -04:00
Tom Boucher
c5dfdbe42e fix(#2957): claude+global post-install instructs restart and skill fallback (#2960) 
* fix(#2957): claude+global post-install instructs restart and skill fallback

`npx get-shit-done-cc --claude --global` writes skills to
`~/.claude/skills/gsd-*/SKILL.md` (CC 2.1.88+ format) and removes the
legacy `~/.claude/commands/gsd/`. The post-install message still told
users to type `/gsd-new-project` without mentioning the required Claude
Code restart or the skill-name fallback. On configurations where CC
does not auto-surface skills in the slash menu, users hit "no commands
appear" and assumed the install failed.

Split the post-install message: the existing single-line instruction
stays for every non-Claude runtime and for `--claude --local`. For
`--claude --global` it now reads:

  Restart Claude Code, then in any directory either type
  /gsd-new-project or ask Claude to run the gsd-new-project skill.

This covers both invocation paths and surfaces the restart requirement.

Add tests/bug-2957-claude-global-postinstall-message.test.cjs as a
regression guard: captures the printed message for claude+global,
claude+local, and opencode+global; asserts content for each. Verified
the test fails on main (pre-fix) and passes after the fix.

Closes #2957

* test(#2957): assert legacy generic instruction is replaced not extended

CodeRabbit flagged that the test would still pass if the new restart/
fallback copy were printed *alongside* the old 'open a blank directory'
instruction. Adding a doesNotMatch assertion proves the claude+global
branch replaces the legacy line rather than appending to it.
 2026-05-01 13:04:39 -04:00
javeroff
9d0d085a17 fix(query/agent-skills): emit raw <agent_skills> block instead of JSON-wrapped string (#2917) 
* fix(query/agent-skills): emit raw <agent_skills> block instead of JSON-wrapped string

The CLI dispatcher (`cli.ts`) JSON-stringifies all query handler results via
`console.log(JSON.stringify(result.data, null, 2))`.  For the `agent-skills`
handler this produced a JSON-quoted string literal — e.g.
`"<agent_skills>\n…</agent_skills>"` — which workflows embedded verbatim via
`$(gsd-sdk query agent-skills gsd-planner)`, breaking all `<agent_skills>`
injection into spawned subagent prompts.

Fix: add an optional `format: 'json' | 'text'` field to `QueryResult`.  When a
handler returns `format: 'text'` and `--pick` is not active, the CLI writes the
string directly via `process.stdout.write` instead of JSON-stringifying it.
`agentSkills` sets `format: 'text'` for non-empty blocks.

Regression guard: two new CLI integration tests in `skills.test.ts` spawn the
CLI as a child process and assert that (a) a mapped agent type receives the raw
XML block on stdout and (b) an unmapped agent type produces the existing JSON
empty-string output.

Fixes #2914.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs(changelog): add #2917 entry under Unreleased Fixed

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-01 12:21:06 -04:00
Tom Boucher
53cda93a01 Add automated cherry-pick + SDK-bundle parity to hotfix flow (#2956) 
* feat(workflows): hotfix auto-cherry-pick + SDK-bundle parity (#2955)

hotfix.yml:
- create: auto-cherry-picks fix:/chore: commits from origin/main since
  BASE_TAG, oldest-first. Patch-equivalents skipped via git cherry.
  feat:/refactor: never auto-included. Conflicts halt with offending SHA.
- finalize: install-smoke gate, sdk-bundle/gsd-sdk.tgz parity with
  release-sdk.yml, tightened next dist-tag re-point, --latest on gh
  release create. SDK package.json bumped in lockstep.

release-sdk.yml:
- New action input (publish | hotfix) and auto_cherry_pick boolean.
- New prepare job branches hotfix/X.YY.Z from highest vX.YY.* tag,
  cherry-picks same logic as hotfix.yml, outputs effective ref.
- install-smoke and release consume prepare.outputs.ref.
- Hotfix mode forces tag=latest, opens merge-back PR. Idempotent if
  branch already exists.

VERSIONING.md: documents the cumulative-tag invariant
(vX.YY.Z anchors vX.YY.{Z+1}) and both workflow paths.

Closes #2955

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(code-review): wire --fix dispatch and update stale command references (#2947)

* fix(#2893): surface non-canonical plan filenames instead of silently returning zero plans

Reporter saw `plan_count: 0` from `/gsd:execute-phase` even though five
plan files existed on disk. Investigation showed the planner had written
files like `01-PLAN-01-foundation.md`, while `phase-plan-index`'s strict
filter (`f.endsWith('-PLAN.md') || f === 'PLAN.md'`) rejected them
silently — collapsing two distinct states into the same `plans: []`
return:

  - directory truly has no plans (legit empty)
  - directory has plans but the filter rejected them (user/agent error)

The canonical contract is documented in three places:
  - `agents/gsd-planner.md` write_phase_prompt step (lines 1063-1080)
  - `commands/gsd/plan-phase.md`
  - `references/universal-anti-patterns.md` (rule 26)

It mandates `{padded_phase}-{NN}-PLAN.md` and explicitly forbids
`PLAN-NN.md` / `01-PLAN-01.md` / `plan-NN.md` etc. The strict filter is
correct per that contract. The bug is that the executor never tells the
user when the contract was violated — they just see `plan_count: 0`
with no signal.

Fix: add a diagnostic helper `describeNonCanonicalPlans()` that scans
the phase directory for files matching `*PLAN*.md` (the diagnostic net)
that the canonical filter rejected, excluding legit derivatives like
`*-PLAN-OUTLINE.md` and `*-PLAN.pre-bounce.md`. When offenders exist,
return a `warning` field naming each one and citing the canonical
pattern so the user knows what to rename to.

Wired into the three filter sites:
  - `phase-plan-index` (the executor's main entry point)
  - `phases list --type plans`
  - `find-phase`

The strict filter itself is unchanged — existing canonical plans behave
identically. This is purely a diagnostic that converts silent-empty
into loud-with-actionable-error.

Tests:
  - `phase-plan-index returns warning for reporter's exact filename
    pattern (`01-PLAN-01-foundation.md`)`
  - `truly empty dir does not emit a warning`
  - `canonical plans + outline + pre-bounce files do not emit a warning`

Closes #2893

* test(#2893): add parity tests for find-phase and phases list --type plans warnings

CodeRabbit's only finding on the prior commit: I wired the warning into
three filter sites (`phase-plan-index`, `find-phase`,
`phases list --type plans`) but only `phase-plan-index` had test
coverage for the warning shape. The other two paths could silently
diverge during future refactors — exactly the silent-drift class of bug
this fix exists to prevent.

Add four parity tests mirroring the existing two:

  - find-phase: non-canonical filenames produce a warning naming each
    offender + citing the canonical pattern.
  - find-phase: canonical plan + derivative files (PLAN-OUTLINE,
    pre-bounce) produce no warning.
  - phases list --type plans: same non-canonical case, but assert the
    warning is prefixed with `${dir}: ` (this path aggregates across
    phase directories so each offender is tagged with its dir).
  - phases list --type plans: canonical case, no warning.

`node --test tests/phase.test.cjs`: 98/98 pass (was 94, +4 new).

* docs(changelog): hotfix flow auto-cherry-pick + SDK bundle parity (#2955)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(workflows): address CodeRabbit findings on hotfix flow (#2955)

5 findings, all real:

1. BASE_TAG selection used lexicographic awk compare, breaking on
   multi-digit patches (v1.27.10 wrongly < v1.27.2). Fixed in both
   hotfix.yml and release-sdk.yml: append TARGET_TAG to candidate list,
   sort -V, take preceding entry. Semver-correct.

2,4. Cherry-pick conflict aborted locally with no remote branch to
   resolve from. Now the skeleton branch is pushed up-front (real runs);
   on conflict we abort, push the partial-pick state with
   --force-with-lease, and emit operator instructions in the run summary.

3. release-sdk.yml dry_run exited before cherry-pick, defeating the
   purpose. Now dry_run still applies cherry-picks locally (catches
   conflicts), just skips push. Downstream install-smoke runs against
   BASE_TAG; the cherry-pick verification itself is the dry-run signal.

5. release-sdk.yml release job missing pull-requests: write — gh pr
   create for the merge-back PR would have failed under restricted
   token defaults. Permission added.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(workflows): CR round 2 — dry-run signal + post-publish reconciliation (#2955)

3 findings, all real:

6. hotfix.yml create dry_run skipped every step (branch creation,
   cherry-pick, version bump) — a green dry-run gave no signal at all.
   Now the local checkout/cherry-pick/bump always runs; only the git
   push calls are gated on dry_run. Conflicts surface in dry-run too.

7,8. "Refuse if version already on npm" preflight hard-failed reruns,
   so a transient failure between npm publish and a later step (tag
   push, GH release, merge-back PR, dist-tag re-point) left the release
   half-shipped with no path to reconcile. Replaced with a
   prior_publish detect step that warns and sets skip_publish=true; the
   publish step is gated on that flag, but tag/release/PR/dist-tag
   continue. GitHub Release create is now idempotent (edit --latest if
   already exists).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(workflows): CR round 3 — preserve dry-run cherry-pick history in conflict guidance (#2955)

Dry-run conflict path discarded successful picks with the runner, but
the message told operators to rerun with auto_cherry_pick=false — which
recreates the branch from BASE_TAG and silently loses every pick that
had succeeded before the conflict.

Updated both hotfix.yml and release-sdk.yml: dry-run conflict summary
now lists the lost SHAs and recommends re-running with
auto_cherry_pick=true (real, not dry-run) to materialize the partial
branch on origin. Real-run guidance unchanged.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
 2026-05-01 11:51:45 -04:00
Tom Boucher
ec07861228 fix(#2948): wire spike --wrap-up flag dispatch (#2951) 
* fix(#2948): wire spike --wrap-up flag dispatch

Add dispatch block to commands/gsd/spike.md so that /gsd-spike --wrap-up
routes to the spike-wrap-up workflow instead of silently no-oping. Also
add spike-wrap-up.md to execution_context so the runtime can load it, and
update both companion references in workflows/spike.md from the deleted
/gsd-spike-wrap-up entry-point to /gsd-spike --wrap-up.

Fixes #2948

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(#2948): rewrite dispatch test using parseFrontmatter + section extraction

Replace raw fs.readFileSync + text.includes() / regex assertions with structural
parsing: parseFrontmatter extracts the YAML frontmatter fields and _body,
extractSection pulls named XML blocks, and parseExecutionContextRefs resolves
the @-prefixed workflow references. Assertions now target the argument-hint
frontmatter field, the execution_context @-ref list, and the routing text within
<context>/<process> sections — not arbitrary substrings in the raw file.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(#2948): tighten dispatch assertion to line-level rule check

Replace the co-occurrence check (dispatchText.includes('--wrap-up') &&
dispatchText.includes('spike-wrap-up')) with line-level assertions that parse
the <process> section's rules array, find the exact '- If it is `--wrap-up`:'
line, verify it includes 'strip the flag' and 'spike-wrap-up', and assert the
'- Otherwise:' fallback still routes to the spike workflow.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(#2948): anchor parseFrontmatter to line 0 to avoid mid-file --- delimiters

parseFrontmatter was scanning the whole file for the first two '---' lines,
which can match a mid-document horizontal rule as the opening delimiter.
Now requires lines[0].trim() === '---'; returns { _body: content } for files
with no frontmatter, and searches for the closing '---' from line 1 onward.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
 2026-05-01 11:25:26 -04:00
Tom Boucher
3ba17e872e fix(#2950): update stale deleted-command references in workflow files (#2952) 
* fix(#2950): update stale deleted-command references in workflow files

Eight workflow files (help.md, do.md, settings.md, discuss-phase.md,
new-project.md, plan-phase.md, spike.md, sketch.md) referenced command
names removed in #2790. Updated all occurrences to canonical new forms:
  /gsd-phase (--insert / --remove), /gsd-capture, /gsd-config (--profile
  / --integrations / --advanced), /gsd-spike --wrap-up,
  /gsd-sketch --wrap-up, /gsd-code-review --fix.
Adds regression test (124 assertions) in tests/bug-2950-stale-command-refs.test.cjs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(#2950): update pre-existing assertions to accept new consolidated command forms

gsd-settings-advanced.test.cjs and settings-integrations.test.cjs were checking
settings.md for the old micro-skill names (/gsd-settings-advanced,
/gsd-settings-integrations). Now that #2950 updates settings.md to use the
consolidated equivalents, broaden the assertions to accept both old and new forms.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(#2950): require canonical command forms and forbid legacy variants

The broadened OR assertions added to unblock CI were too permissive — they
could pass with legacy names still present. Now assert the canonical form is
present (gsd-config --advanced / gsd-config --integrations) AND the legacy
forms are absent (gsd-settings-advanced, gsd:settings-advanced,
/gsd-settings-integrations).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
 2026-05-01 11:25:10 -04:00
Tom Boucher
4d628b306a fix(#2949): wire sketch --wrap-up flag dispatch (#2953) 
* fix(#2949): wire sketch --wrap-up flag dispatch

Add dispatch logic to commands/gsd/sketch.md so --wrap-up routes to the
sketch-wrap-up workflow instead of silently falling through to the normal
sketch workflow. Also adds sketch-wrap-up.md to execution_context and
updates companion references in workflows/sketch.md from the deleted
/gsd-sketch-wrap-up command to /gsd-sketch --wrap-up.

Fixes #2949

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(#2949): use exact-match "If it is" instead of "If it contains" for --wrap-up dispatch

Aligns with the established pattern across all consolidated commands
(workspace.md, update.md, progress.md) where the first-token check uses
"If it is `--flag`" for exact equality, not substring matching.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
 2026-05-01 11:06:24 -04:00
Tom Boucher
b328f3269f fix(code-review): wire --fix dispatch and update stale command references (#2947) 
* fix(#2893): surface non-canonical plan filenames instead of silently returning zero plans

Reporter saw `plan_count: 0` from `/gsd:execute-phase` even though five
plan files existed on disk. Investigation showed the planner had written
files like `01-PLAN-01-foundation.md`, while `phase-plan-index`'s strict
filter (`f.endsWith('-PLAN.md') || f === 'PLAN.md'`) rejected them
silently — collapsing two distinct states into the same `plans: []`
return:

  - directory truly has no plans (legit empty)
  - directory has plans but the filter rejected them (user/agent error)

The canonical contract is documented in three places:
  - `agents/gsd-planner.md` write_phase_prompt step (lines 1063-1080)
  - `commands/gsd/plan-phase.md`
  - `references/universal-anti-patterns.md` (rule 26)

It mandates `{padded_phase}-{NN}-PLAN.md` and explicitly forbids
`PLAN-NN.md` / `01-PLAN-01.md` / `plan-NN.md` etc. The strict filter is
correct per that contract. The bug is that the executor never tells the
user when the contract was violated — they just see `plan_count: 0`
with no signal.

Fix: add a diagnostic helper `describeNonCanonicalPlans()` that scans
the phase directory for files matching `*PLAN*.md` (the diagnostic net)
that the canonical filter rejected, excluding legit derivatives like
`*-PLAN-OUTLINE.md` and `*-PLAN.pre-bounce.md`. When offenders exist,
return a `warning` field naming each one and citing the canonical
pattern so the user knows what to rename to.

Wired into the three filter sites:
  - `phase-plan-index` (the executor's main entry point)
  - `phases list --type plans`
  - `find-phase`

The strict filter itself is unchanged — existing canonical plans behave
identically. This is purely a diagnostic that converts silent-empty
into loud-with-actionable-error.

Tests:
  - `phase-plan-index returns warning for reporter's exact filename
    pattern (`01-PLAN-01-foundation.md`)`
  - `truly empty dir does not emit a warning`
  - `canonical plans + outline + pre-bounce files do not emit a warning`

Closes #2893

* test(#2893): add parity tests for find-phase and phases list --type plans warnings

CodeRabbit's only finding on the prior commit: I wired the warning into
three filter sites (`phase-plan-index`, `find-phase`,
`phases list --type plans`) but only `phase-plan-index` had test
coverage for the warning shape. The other two paths could silently
diverge during future refactors — exactly the silent-drift class of bug
this fix exists to prevent.

Add four parity tests mirroring the existing two:

  - find-phase: non-canonical filenames produce a warning naming each
    offender + citing the canonical pattern.
  - find-phase: canonical plan + derivative files (PLAN-OUTLINE,
    pre-bounce) produce no warning.
  - phases list --type plans: same non-canonical case, but assert the
    warning is prefixed with `${dir}: ` (this path aggregates across
    phase directories so each offender is tagged with its dir).
  - phases list --type plans: canonical case, no warning.

`node --test tests/phase.test.cjs`: 98/98 pass (was 94, +4 new).
 2026-05-01 10:28:05 -04:00
Tom Boucher
e2792536d9 feat(workflows): atomic Write+commit ordering for SUMMARY.md (#2806) (#2939) 
* feat(workflows): add atomic Write+commit ordering directive for SUMMARY.md

Adds explicit prompt-ordering language to executor spawn prompts and
plan-execution steps so agents commit SUMMARY.md before emitting any
concluding narrative. Mitigates the truncation-between-Write-and-commit
failure mode that has made the #2070 rescue net load-bearing.

Refs #2806

* fix(workflows): condense REQUIRED ORDER blocks to fit XL budget

The two REQUIRED ORDER directives added in bd1956df pushed
execute-phase.md to 1712 lines, exceeding the 1700-line XL budget.

Collapse each 6-line block into a single line that preserves the
semantic intent (Write SUMMARY.md → commit → narration; no text
between Write and commit; #2070 rescue is not primary defense).

File is now exactly 1700 lines; workflow-size-budget test passes.

* fix(execute-plan): move self-check before commit to preserve atomic Write+commit (#2939)
 2026-05-01 09:32:21 -04:00
Tom Boucher
7cc6358f91 fix(install): honour --minimal across every runtime + manifest fix for Claude local (#2940) 
* fix(install): record commands/gsd in manifest for Claude local + per-runtime --minimal coverage

writeManifest gated commands/gsd/ recording to Gemini, leaving Claude
Code local installs with an incomplete manifest. Audit during #2923
investigation showed every runtime adapter correctly honours --minimal
on disk (6 skills, 0 agents) — but Claude local manifest reported 0
skills, breaking saveLocalPatches() drift detection and any downstream
tooling that reads manifest.files for the installed surface.

Drop the isGemini gate so any runtime that writes commands/gsd/ has
those files hashed into the manifest.

Adds tests/install-minimal-all-runtimes.test.cjs: spawns the installer
end-to-end for all 14 supported runtimes in both --global and --local
modes, parses the manifest JSON, and asserts mode === 'minimal',
skill set equals MINIMAL_SKILL_ALLOWLIST, and zero gsd-* agents are
recorded. Cross-checks the manifest against on-disk skill files.

Closes #2923

* test(install): address CR feedback on bug-2923 minimal-runtime tests

- Assert installer exit status in runInstall() so failing installs do not
  produce misleading downstream artifact assertions; include stderr in the
  failure message for debuggability.
- Guard the on-disk vs manifest parity loop with assert.ok(manifest, ...)
  so the equality check cannot pass accidentally when the manifest is
  missing.
 2026-05-01 09:23:20 -04:00