760 Commits

Author	SHA1	Message	Date
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	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	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	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	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	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
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	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
Tom Boucher	8de8acee46	fix(workflows): assert HEAD on per-agent branch before worktree commits (#2924) (#2941) ... * fix(workflows): assert HEAD on per-agent branch before worktree commits Worktree-mode setup could leave HEAD attached to a protected branch (master), causing agent commits to land there. The previous response was a destructive self-recovery via 'git update-ref refs/heads/master <sha>', which silently rewinds the protected branch and destroys concurrent commits in multi-active scenarios (parallel agents, user committing while agent runs). - Reorder <worktree_branch_check> in execute-phase.md and quick.md to assert HEAD via 'git symbolic-ref' BEFORE any 'git reset --hard'. HALT with a blocker if HEAD is on main/master/develop/trunk/release/* or detached. - Add a per-commit HEAD assertion (step 0) to gsd-executor.md <task_commit_protocol>; HEAD attachment can drift after 'git checkout <sha>'. - Forbid 'git update-ref refs/heads/<protected>' in <destructive_git_prohibition>; surface the blocker rather than self-heal. - Remove '--no-verify' as the worktree-mode default in execute-phase.md, execute-plan.md, quick.md, and references/git-integration.md. Hooks now run on every executor commit; opt out only via workflow.worktree_skip_hooks. - Add regression test that parses the worktree_branch_check blocks structurally and asserts the symbolic-ref check precedes the reset --hard, no workflow performs update-ref on a protected ref, and --no-verify is no longer the default in any parallel-execution prompt. * fix(#2924): address CodeRabbit review findings on worktree HEAD PR - Add positive worktree-agent-* allow-list to <task_commit_protocol> step 0 in gsd-executor.md and to <worktree_branch_check> in execute-phase.md and quick.md. The deny-list (main|master|develop|trunk|release/*) silently allowed feature/* and other arbitrary branches outside the agent namespace. - Register workflow.worktree_skip_hooks in both config schemas (sdk/src/query/config-schema.ts and get-shit-done/bin/lib/config-schema.cjs) and document it in docs/CONFIGURATION.md so config-set accepts it. - Fix stash lifecycle in execute-phase.md post-wave hook validation: stash under a named ref and pop after the hook run; warn on pop failure. - Pre-dispatch PLAN.md commit in quick.md: gate on git diff --cached --quiet for idempotency and exit 1 with a clear error on commit failure (both the --no-verify and the normal branches) — no more swallowing real errors. - Test fixes (tests/bug-2924-worktree-head-attachment.test.cjs): - Parse the protected-branch alternation structurally and require main, master, develop, trunk, release/.* (release/* was previously skipped by the \\b...\\b regex). - Use fs.readdirSync(dir, { recursive: true }) so workflows in nested subdirectories are also asserted against the update-ref ban. - Add allow-list assertions for execute-phase.md, quick.md, and gsd-executor.md to lock in the new positive namespace check. * test(#2924): assert sub-section end marker exists before slicing * test(#2924): use section boundary instead of fixed window for parallel-agents slice	2026-05-01 09:23:02 -04:00
Tom Boucher	2cc8796265	fix(config-get): return schema default for context_window when absent (#2944) ... * fix(config-get): return schema default for context_window when absent (#2943) cmdConfigGet in bin/lib/config.cjs now consults a SCHEMA_DEFAULTS map before emitting "Key not found", so context_window (and any future schema-defaulted keys) return their default value (exit 0) when not set in config.json. Also updates the stale subagent-timeout.test.cjs assertion that expected the old broken behavior (exit 1 / "Key not found") to match the corrected behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: use distinct sentinel to prove --default wins over schema default (#2943) * docs: update CHANGELOG.md for #2943 fix --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-01 09:22:45 -04:00
Tom Boucher	faee0287a0	fix(detect-custom-files): add skills/ to GSD_MANAGED_DIRS (#2942) (#2945) ... After v1.39.0 skill consolidation (#2790), skills/ became a GSD-managed root that the installer wipes on update. GSD_MANAGED_DIRS in gsd-tools.cjs was missing 'skills', so user-added skill directories (e.g. skills/custom-skill/SKILL.md) were never walked and silently destroyed during /gsd-update. - Add 'skills' to GSD_MANAGED_DIRS so the directory is walked - Add tests/bug-2942-detect-custom-skills.test.cjs with 5 targeted tests - Update tests/update-custom-backup.test.cjs: replace the now-incorrect "skills/ must NOT be scanned" assertion (written pre-#2790) with a test that verifies custom skills ARE detected and GSD-owned skills are not falsely flagged Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-01 09:22:13 -04:00
Tom Boucher	5abf46ac1c	Merge pull request #2920 from gsd-build/feat/hermes-runtime-2841 ... feat(install): add Hermes Agent runtime support	2026-04-30 23:02:15 -04:00
Tom Boucher	372d3453f5	fix(install): tokenize before ALL_RUNTIMES_OPTION check + isolate HERMES_HOME in test ... Two CodeRabbit findings on PR #2920: 1. parseRuntimeInput previously only matched the bare "16" exactly for the all-runtimes shortcut. Inputs the prompt explicitly encourages — "16,", "16 1", "1,16" — fell through to per-token parsing and silently installed only Claude or a partial subset. Move the ALL_RUNTIMES_OPTION check after tokenization so any token equal to "16" expands. Added regression coverage in tests/multi-runtime-select.test.cjs for the four mixed-input forms. 2. The "maps Hermes to ~/.hermes for global installs" test invoked getGlobalDir('hermes') without isolating HERMES_HOME. On a developer machine that exports HERMES_HOME the assertion would fail even though getGlobalDir was behaving correctly. Save/clear/restore the env var around the assertion, mirroring the pattern the later describe block already uses. Full suite: 6128/6128 pass.	2026-04-30 22:48:08 -04:00
Tom Boucher	c9d6306981	fix(hermes): rewrite CLAUDE.md → HERMES.md (revert from .hermes.md per spec) ... Per the issue spec for #2841 and CodeRabbit feedback on PR #2920, the project-context filename rewrite should produce HERMES.md, not .hermes.md. Reverts the earlier .hermes.md change at all 5 substitution sites in bin/install.js and updates the corresponding regression test in tests/hermes-install.test.cjs to assert HERMES.md. Full suite: 6127/6127 pass.	2026-04-30 22:30:16 -04:00
Tom Boucher	1168e9f59a	Merge pull request #2921 from gsd-build/fix/2916-handle-branching-default-base ... fix(#2916): branch new phases off origin/HEAD instead of current HEAD	2026-04-30 22:25:03 -04:00
Tom Boucher	ace61869d0	test(#2916): parameterize fixtures so both main and trunk are exercised ... Two follow-ups on commit 80f14cac (which hardened quick-branching with a trunk fixture): 1. quick-branching.test.cjs: add a `defaultBranch` parameter to setupFixture and run the "branches off origin/HEAD" assertion against both `main` and `trunk`. The wholesale switch to trunk in 80f14cac removed coverage of the conventional `main` path; parameterizing restores it without giving up the symbolic-ref guarantee. 2. bug-2916-handle-branching-default-base.test.cjs: apply the same parameterization here. handle_branching has the same default-branch detection logic as Step 2.5, so it deserves the same trunk regression guard. Previously this file only exercised `main`. A regression that silently defaults to `main` instead of consulting `git symbolic-ref refs/remotes/origin/HEAD` now fails the `trunk` variant in both files. Tests: 10/10 in the touched suites.	2026-04-30 21:57:27 -04:00
Tom Boucher	80f14cac1f	test(#2916): scope branch_name scan to init step and harden fixture ... - Restrict the "init parse list includes branch_name" assertion to the bash blocks inside Step 2 (Initialize) so an unrelated step that mentions branch_name cannot mask the contract. - Switch the fixture's default branch from main to trunk so the symbolic-ref code path is locked in: a regression that silently defaults to "main" instead of consulting origin/HEAD now fails. Addresses CodeRabbit review on PR #2921. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>	2026-04-30 21:48:43 -04:00
Tom Boucher	e5cd523e7b	test(hermes): use parseFrontmatter for agent assertion (CR #2920)	2026-04-30 21:44:12 -04:00
Tom Boucher	861a7d972b	test(install): replace source-grep prompt assertions with structured checks ... Two test files were asserting installer prompt behavior by regex/.includes() against bin/install.js source. Per CONTRIBUTING.md "no-source-grep" testing standard, replace with structured assertions: - tests/kilo-install.test.cjs: import runtimeMap and buildRuntimePromptText from the install module; assert runtimeMap['11'] === 'kilo' and that the rendered prompt lists Kilo above OpenCode without marketing copy. - tests/multi-runtime-select.test.cjs: import runtimeMap, allRuntimes, parseRuntimeInput, buildRuntimePromptText. Assert exported runtimeMap matches the canonical option list, allRuntimes contains every runtime exactly once, prompt text lists Hermes (10), Qwen Code (13), Trae (14), All (16), and parser splits/dedupes by exercising parseRuntimeInput rather than regexing source code. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>	2026-04-30 21:30:48 -04:00
Tom Boucher	bd0511988b	fix(hermes): nest GSD skills under skills/gsd/ category (#2841) ... Per spec in #2841, all 86 GSD skills must collapse into a single "gsd" category in Hermes' system prompt. Previous code passed skills/ as the install root, producing a flat skills/gsd-*/ layout that inflated Hermes' loader output to 86 top-level entries. Changes: - Install path now writes to skills/gsd/{DESCRIPTION.md, gsd-*/SKILL.md} - Uninstall removes the entire skills/gsd/ category dir plus any leftover flat-layout gsd-*/ from older installs (graceful migration) - writeManifest emits skills/gsd/<skill>/<file> paths for Hermes - --skills-root hermes returns the nested category path so /gsd-sync-skills syncs into the right directory - DESCRIPTION.md at category root carries name/version/description so Hermes' skill loader surfaces the GSD category in the system prompt Also extracts promptRuntime's runtimeMap, allRuntimes, parseRuntimeInput, and buildRuntimePromptText to module scope and exports them so tests can assert structurally instead of grepping bin/install.js source. Existing hermes-install tests updated to expect the nested layout and to verify the category DESCRIPTION.md frontmatter (name, version, description) using the shared parseFrontmatter helper. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>	2026-04-30 21:30:48 -04:00
Tom Boucher	4a5f36df5e	Merge pull request #2919 from gsd-build/fix/2911-audit-open-output-references ... fix(#2911): audit-open emits raw human report and parseable JSON	2026-04-30 21:23:30 -04:00
Tom Boucher	140d334dab	test(#2916): replace string-grep assertions with behavioral fixture test ... CodeRabbit nitpick (per project policy `feedback_no_source_grep_tests`): the prior `tests/quick-branching.test.cjs` asserted branching correctness by `.includes()`-grepping the raw markdown content for literal command substrings. Those assertions stayed green even when the underlying behavior regressed (e.g. when `git checkout -b` was unconditionally run from the wrong HEAD). Replace with the same pattern as `bug-2916-handle-branching-default-base .test.cjs`: - Structurally extract the Step 2.5 bash block from quick.md by walking the markdown for fenced ```bash blocks under the heading (no regex on prose). - Spin up a fixture git repo with a bare origin, a clone whose `origin/HEAD` points at `main`, and a checked-out previous-task branch carrying its own unmerged commit. - Execute the extracted bash block via `bash -c` and assert that the new branch's tip equals `origin/main` (0 commits inherited from the previous-task HEAD). - Add a reuse test that pre-creates the target branch with its own commit and verifies the script switches back to it without a rebase or reset. The two informational tests (workflow file exists, branching runs before task-directory creation) are retained, plus the `branch_name` parsing assertion is rewritten to walk fenced bash blocks rather than substring-grep arbitrary content.	2026-04-30 21:22:56 -04:00