b9e3979fc152a7883f67b9bbaecc6da23f78cdd2
2300 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
b9e3979fc1 | refactor: introduce explicit query error seam contracts | ||
|
Tom Boucher
|
c7d3f83b8b | refactor: reduce failure-classification API surface | ||
|
Tom Boucher
|
bc289fad4a | refactor: type native adapter error seam to GSDToolsError | ||
|
Tom Boucher
|
9bee4dce4a | test: adopt typed GSDToolsError constructors across failure tests | ||
|
Tom Boucher
|
9a469fa05c | refactor: centralize query tools error construction in factory | ||
|
Tom Boucher
|
abf7779088 | test: cover typed timeout mapping in query dispatch | ||
|
Tom Boucher
|
16bf552037 | test: lock typed timeout no-fallback transport behavior | ||
|
Tom Boucher
|
009cfb1562 | refactor: split native adapter timeout and failure seams | ||
|
Tom Boucher
|
6fe4af2546 | refactor: split subprocess timeout and failure error seams | ||
|
Tom Boucher
|
41683b2f53 | refactor: centralize typed GSDToolsError construction | ||
|
Tom Boucher
|
7dcafbc211 | refactor: consolidate failure classification constructors | ||
|
Tom Boucher
|
ccda572ade | refactor: default typed failure classification across query errors | ||
|
Tom Boucher
|
1ca7f58831 | test: cover tools error mapping and unify timeout fallback check | ||
|
Tom Boucher
|
7298a76b20 | refactor: centralize dispatch error projection from failure signals | ||
|
Tom Boucher
|
5cfd874058 | refactor: add typed query failure signals | ||
|
Tom Boucher
|
ba6100c548 | refactor: deepen query failure classification module | ||
|
Tom Boucher
|
9f5b011b35 | refactor: use internal gsdtools error type import | ||
|
Tom Boucher
|
1037b82a98 | test: address remaining coderabbit findings and notes | ||
|
Tom Boucher
|
ac883f8150 | fix: address coderabbit query seam findings | ||
|
Tom Boucher
|
3e22c70fac | docs: fix changeset summary text | ||
|
Tom Boucher
|
12fc34689e | docs: add changeset for query seam deepening | ||
|
Tom Boucher
|
9d096b9925 | refactor: deepen gsdtools query execution seams | ||
|
Tom Boucher
|
42ed7cee8d |
refactor: deepen GSDTools query execution seams (#3085)
* refactor: deepen gsdtools query execution seams * docs: add changeset for query seam deepening * docs: fix changeset summary text * fix: address coderabbit query seam findings * test: address remaining coderabbit findings and notes * refactor: use internal gsdtools error type import |
||
|
Tom Boucher
|
5e21bf7567 |
Deepen query dispatch seam with Command Topology Module (#3078)
* Deepen query dispatch seam with command topology module * Stabilize SDK parity defaults and integration test gating * docs(architecture): record pre-project config policy and e2e gate * refactor(query): stop injecting native adapter in CLI dispatch path * fix(config): align workflow auto-chain typing and docs |
||
|
Tom Boucher
|
9c92c32f6e |
refactor(query): deepen runtime context/native adapter/output seams (#3076)
* refactor(query): deepen runtime context, native adapter, and cli output seams * chore(changeset): add fragment for query seam deepening continuation * refactor(query): converge internal command-resolution imports on canonical seam * refactor(query): remove dead seam wrappers and converge on canonical modules * docs(architecture): update context and adr for query seam completion * fix(query): preserve gsd-tools stderr in cli output and clarify static ws test scope * test(query): cover whitespace stderr and null exitCode fallback |
||
|
Tom Boucher
|
5c9f34bd31 |
refactor(cli): extract Query CLI Adapter Module seam (#3074)
* refactor(cli): extract query adapter seam from cli entrypoint * test: update ws forwarding guard for query-cli-adapter seam * fix(query): close remaining CodeRabbit findings on cli adapter * test: address remaining CodeRabbit nitpicks on ws forwarding coverage |
||
|
Tom Boucher
|
b6c401dc90 |
refactor(query): deepen command/dispatch seams and resolve coderabbit findings
* refactor(query): deepen command definition seam and fold fallback mapping cleanup * refactor(query): add shared dispatch formatting module seam * fix(query): restore QueryResult type import in dispatch deps * test/query: align raw-output policy and definition normalization contracts * refactor(query): deepen diagnosis, invariant report, and error taxonomy seams * refactor(query): deepen dispatch plan, fallback bridge, policy snapshot, and hints seams * refactor(query): deepen validation, fallback policy, capability, and result builder seams * refactor(query): deepen resolution strategy, output classifier, observability, and policy-capability seams * refactor(query): finalize deep strategy/classifier/observability/capability seams * test/query: address coderabbit inline and out-of-diff dispatch nits * fix(query): address remaining coderabbit input-validation and bridge stderr threads * fix(query): address remaining coderabbit dispatch and strategy/output nits |
||
|
Tom Boucher
|
c3f896f311 | docs(contributing): codify CONTEXT + ADR contribution and testing standards | ||
|
Tom Boucher
|
f104dab332 |
refactor(query): deepen dispatch policy seam with structured result contract (#3066)
* refactor(query): deepen dispatch policy seam with structured result contract Closes #3065. - unify query dispatch outcome as typed success/failure union - include error kind/details + final exit_code in failure path - align native and fallback paths under one dispatch policy seam - make CLI query path consume seam result (thin adapter) - add ADR + context term for Dispatch Policy Module * refactor(query): strengthen dispatch seam with shared error mapper and typed details - add query-dispatch-error-mapper module shared by native/fallback paths - remove ad-hoc inline mapping in dispatch/fallback executors - lock error-details schema in mapper + dispatch tests - document structured dispatch contract in QUERY-HANDLERS.md * fix(query): return structured fallback failure when path resolution throws - guard resolveGsdToolsPath in cjs dispatch path - map thrown resolution errors to fallback_failure result - add regression test for structured failure contract |
||
|
Tom Boucher
|
5975f06b6a |
refactor(query): extract command catalog seam for registry wiring (#3060)
* refactor(sdk): extract gsdtools transport seam with per-command policy * refactor(query): centralize registry command catalog wiring * refactor(query): unify command resolution seam across sdk callers * fix(sdk): address CodeRabbit transport policy and timeout findings * refactor(query): extract mutation event mapper seam * refactor(query): converge mutation and transport policy data * refactor(query): share fallback orchestration across cli and sdk * refactor(query): split static registry catalog by domain clusters * refactor(query): extract mutation event emission decorator seam * refactor(query): extract alias-family handler catalog module * refactor(query): extract cjs fallback execution adapter * refactor(query): deepen command semantics seam * refactor(query): extract deep dispatch seam * refactor(query): deepen cjs fallback execution seam * refactor(query): merge routing plan into dispatch seam * fix(query): address CodeRabbit review findings on PR #3060 Critical: prevent double-execution race by checking timeout errors before subprocess fallback (gsd-transport.ts). Major: fix execRaw() to respect transport policy outputMode instead of hardcoding 'raw' (gsd-tools.ts). Major: add explicit 30s timeout to subprocess fallback execution (query-fallback-executor.ts). Major: remove raw args from stderr banner to prevent secret leakage (query-fallback-executor.ts). Minor: ensure native text output has trailing newline for CLI parity (query-dispatch.ts). Update gsd-tools.test.ts to match new execRaw() behavior. * fix(tests): update CLI integration tests for catalog-based registration The refactoring moved handler registration from inline registry.register() calls to catalog-based registration (registerStaticCatalog/registerAliasCatalog). - gsd-sdk-query-registry-integration.test.cjs: collectRegisteredNames() now also scans catalog files for handler names registered via the new system. - bug-2492-context-coverage-gate.test.cjs: checks for catalog-based registration (DECISION_ROUTING_STATIC_CATALOG) instead of inline strings. - bug-2524-sdk-query-ws-flag.test.cjs: checks for dispatchNative callback pattern instead of direct registry.dispatch() call. * fix(query): address remaining CodeRabbit review findings - query-command-semantics.ts: guard stats/progress rewrite so option tokens (e.g. --pick) are not turned into subcommands, preserving the top-level handler dispatch. - query-dispatch.ts: formatOutput now skips --pick for text-format responses (matching CJS fallback behavior) and surfaces a proper error when extractField returns undefined instead of silently producing 'undefined'. - query-dispatch.ts: fix backwards error message — 'registered' is the restrictive policy that disables fallback, not enables it. - tests/bug-2492-context-coverage-gate.test.cjs: check VERIFY_DECISION_STATIC_CATALOG (the correct catalog for plan-gate handlers) instead of DECISION_ROUTING_STATIC_CATALOG. - tests/gsd-sdk-query-registry-integration.test.cjs: resolve catalog variable before loading entries so the drift guard checks each referenced catalog individually. * refactor(query): deepen registry assembly module with strict invariants - extract registry assembly into dedicated module - split build vs mutation decoration internals - add strict assembly invariants: 1) no duplicate keys 2) alias canonicals must have handlers 3) mutation commands must be registered 4) raw-output policy commands must be registered - slim query index to thin re-export seam - add focused registry assembly tests - update drift-guard tests to target new seam * test(query): add thin-seam coverage for query index re-exports * fix(query): return structured native dispatch errors + tighten decisions.parse guard - runQueryDispatch native path now catches adapter errors and returns QueryDispatchResult.error instead of throwing. - preserve legacy CLI exit contract by using code=1 for native dispatch failures. - strengthen bug-2492 guard: decisions.parse assertion now checks VERIFY_DECISION_STATIC_CATALOG OR explicit command token. |
||
|
Tom Boucher
|
0f98952a3d |
refactor(sdk): extract GSDTools transport seam + policy (#3058)
* refactor(sdk): extract gsdtools transport seam with per-command policy * fix(sdk): address CodeRabbit transport policy and timeout findings * fix(sdk): harden raw transport formatting and raw-path coverage |
||
|
Tom Boucher
|
eb365f7336 |
docs: audit and update docs/ for v1.40.0 release (#3048)
* docs(en): update FEATURES/USER-GUIDE/COMMANDS for v1.40.0 surface - FEATURES.md: append v1.40.0 section (#122 skill consolidation, #123 namespace meta-skills, #124 context-window guard, #125 phase-lifecycle status-line read-side); add to TOC. - USER-GUIDE.md: add slash-command form (hyphen vs colon) primer and namespace routing primer; replace deleted slash forms in walkthroughs (`/gsd-add-backlog`, `/gsd-plant-seed`, `/gsd-add-phase`, `/gsd-set-profile`, `/gsd-list-workspaces`, etc.) with consolidated forms (`/gsd-capture --backlog`, `/gsd-phase --insert`, `/gsd-config --profile`, `/gsd-workspace --list`, etc.); fix `/gsd-spike-wrap-up` and `/gsd-sketch-wrap-up` to flag form. - COMMANDS.md: clarify Command Syntax (Gemini = colon form, others = hyphen form); add Namespace Meta-Skills section with all six routers; add `--context` to /gsd-health flag table. Refs #3047 * docs(en): refresh INVENTORY/CLI-TOOLS/STATE-MD-LIFECYCLE for v1.40.0 - INVENTORY.md: workflow-row "Invoked by" column updated to point at consolidated commands (`/gsd-phase` family, `/gsd-workspace --list`, `/gsd-config --advanced/--integrations/--profile`, `/gsd-sketch --wrap-up`, `/gsd-spike --wrap-up`); CLI-modules row for `secrets.cjs` updated to `/gsd-config --integrations`. Command count and namespace meta-skills section already reflect 65 shipped (= 59 consolidated sub-skills + 6 ns-* routers). - CLI-TOOLS.md: add `validate context` row under Validation Commands with the 60 %/70 % threshold envelope used by `/gsd-health --context`. - STATE-MD-LIFECYCLE.md: flip status header from "proposed" to "shipped in v1.40.0" since `parseStateMd()` and `formatGsdState()` now read and render `active_phase`, `next_action`, `next_phases`, and `progress`. `docs/AGENTS.md` audited and verified clean — `gsd-code-fixer` row already lists the correct `/gsd-code-review --fix` spawner; no deleted-skill references found. `docs/INVENTORY-MANIFEST.json` audited and verified clean — already enumerates the 65 commands (including six ns-* routers) and contains no deleted slash forms. Refs #3047 * docs(en): cleanup ARCHITECTURE/CONFIGURATION for v1.40.0 - ARCHITECTURE.md: split Commands install-target list to call out the Gemini colon form (`/gsd:command-name`) vs hyphen form for every other runtime. Add a new subsection covering two-stage hierarchical routing via the six namespace meta-skills (#2792) and a paired note on the MCP token-budget interaction so readers see the two big per-turn cost levers in one place. - CONFIGURATION.md: rewrite three references to the deleted `/gsd-settings-advanced` and `/gsd-settings-integrations` slash forms to use the consolidated `/gsd-config --advanced` / `/gsd-config --integrations` invocations. Add a new "STATE.md Frontmatter (Phase Lifecycle)" section documenting the four optional fields (`active_phase`, `next_action`, `next_phases`, `progress`) read by the v1.40 status-line, with a pointer to STATE-MD-LIFECYCLE.md for the full reference. `docs/manual-update.md` audited and verified clean — already documents `/gsd-update --reapply` (the consolidated form), no reference to the deleted `/gsd-reapply-patches`. Refs #3047 * docs(i18n): mirror v1.40.0 slash-command rename into ja-JP/ko-KR/zh-CN/pt-BR Mechanical token-level renames only — every reference to a deleted micro-skill slash form is rewritten to the consolidated form on the matching parent skill. No prose was machine-translated; new prose sections (slash-form primer, namespace routing primer, v1.40 feature entries, STATE.md frontmatter) were left for human translator follow-up. Renames applied uniformly across all four trees: /gsd-add-todo, /gsd-add-note, /gsd-add-backlog, /gsd-plant-seed, /gsd-check-todos → /gsd-capture[ --note| --backlog|--seed|--list] /gsd-add-phase, /gsd-insert-phase, /gsd-remove-phase, /gsd-edit-phase → /gsd-phase[ --insert| --remove|--edit] /gsd-new-workspace, /gsd-list-workspaces, /gsd-remove-workspace → /gsd-workspace[ --new| --list|--remove] /gsd-settings-advanced, /gsd-settings-integrations, /gsd-set-profile → /gsd-config[ --advanced| --integrations|--profile] /gsd-sketch-wrap-up → /gsd-sketch --wrap-up /gsd-spike-wrap-up → /gsd-spike --wrap-up /gsd-reapply-patches → /gsd-update --reapply /gsd-code-review-fix → /gsd-code-review --fix /gsd-plan-milestone-gaps → /gsd-audit-milestone Refs #3047 * docs(changelog): regroup [Unreleased] under Feature/Enhancement/Fix Replace the existing Keep-a-Changelog \`Added\` / \`Changed\` / \`Performance\` / \`Removed\` / \`Fixed\` sub-headers in the [Unreleased] block with the issue/PR template taxonomy: Added → Feature Changed / Performance → Enhancement Removed → Enhancement Fixed → Fix Order within the release: Feature → Enhancement → Fix. Every bullet preserved verbatim — only headers and grouping changed; the awkward inline-versioned headers (\`### Added — 1.40.0-rc.1\`, \`### Changed — 1.40.0-rc.1\`, \`### Fixed — 1.40.0-rc.1\`) folded into the same buckets with the \`— 1.40.0-rc.1\` suffix dropped, since the [Unreleased] block IS 1.40.0-rc.1. The [1.39.2] hotfix block called out in #3047's spec does not yet exist in CHANGELOG.md (the previously released hotfix is [1.39.1]), so this commit only regroups [Unreleased]. Older release blocks ([1.39.1] and earlier) are frozen and untouched. Refs #3047 * docs(changeset): add fragment for v1.40.0 doc audit Refs #3047 * docs(en): strip leading / from deleted slash-command tokens in FEATURES REQ-CONSOLIDATE-03 and REQ-CONSOLIDATE-04 listed deleted commands by their `/gsd-foo` form for the historical record. The docs-parity tests in bug-3010, bug-3029-3034, and bug-3042-3044 use the regex `/\/gsd-[a-z0-9][a-z0-9-]*/g` to scan user-facing surfaces for any remaining mention of removed slash forms — they cannot tell prose about a deleted command from a live recommendation. Strip the leading slash from the bare-name references (preserve the historical text otherwise). Tests now require a `/` prefix to match, so `gsd-add-todo` reads identically to a human but no longer trips the parser. Verified locally: 65/65 tests pass across the three docs-parity suites that were red on CI run 25270072600. Refs #3047 * docs(en): fix CR feedback + drop literal /gsd:plan-phase from USER-GUIDE CI: tests/bug-2543-gsd-slash-namespace.test.cjs flagged docs/USER-GUIDE.md:35 for embedding the literal `/gsd:plan-phase` token in the parenthetical Gemini-form example. The test scans every .md under docs/ for `/gsd:<live-cmd>` because non-Gemini surfaces must not advertise the colon form. Replaced the literal example with a prose substitution rule. CR: docs/ARCHITECTURE.md:125 — the namespace meta-skills were listed by file-prefix (`gsd-ns-workflow`) but the invocable frontmatter `name:` is the bare form (`gsd-workflow`). Verified against the six `commands/gsd/ns-*.md` files. Replaced with the canonical names and noted the file/name disagreement in-line. CR: docs/COMMANDS.md:723 — `v1.40` aligned to canonical `v1.40.0`. CR: docs/FEATURES.md:2679 — REQ-CTX-GUARD-02 advertised the wrong invocation (`gsd-tools validate context`). The shipped handler is exposed via `gsd-sdk query validate.context` and requires explicit `--tokens-used <int>` + `--context-window <int>` flags (verified against sdk/src/query/validate.ts:849-882 and get-shit-done/bin/lib/validate-command-router.cjs:19-36). CR: docs/zh-CN/README.md:533 — added `inherit` to the profile-options parenthetical to match the canonical set (verified against model-profiles.cjs:29 `VALID_PROFILES = […MODEL_PROFILES['gsd-planner'], 'inherit']`). Verified locally: 74/74 tests pass across the four docs-parity suites that were red on CI runs 25270072600 and 25270182903. Refs #3047 |
||
|
Tom Boucher
|
1e6737cd8e |
feat(plan-phase): --research-phase flag + scrub stale slash-command refs (#3042, #3044) (#3045)
* feat(plan-phase): --research-phase flag absorbs deleted /gsd-research-phase + scrub stale refs (#3042, #3044) #3042 (orphaned research-phase): /gsd-research-phase had a workflow file but no slash-command stub. Rather than restore the orphan, the research- only capability is now a flag on /gsd-plan-phase: /gsd-plan-phase --research-phase <N> When set, the workflow scopes to phase N, runs the research step (Section 5 of the existing plan-phase workflow), then early-exits before the planner/plan-checker/verifier chain. Per RCA against the deleted standalone, the flag adds two modifiers to fully cover the original surface (Option B from the RCA discussion): - --view : print existing RESEARCH.md to stdout, no spawn. Cheapest mode for the correction-without-replanning loop the issue reporter explicitly called out. Errors with a clear hint if RESEARCH.md is missing. - --research : reuse the existing "force re-research" semantics. In research-only mode this skips the existing-RESEARCH.md prompt and re-spawns unconditionally. - Neither flag, RESEARCH.md exists : prompt update/view/skip. Mirrors the deleted standalone's existing-artifact menu (#3042 RCA). #3044 (stale slash-command refs): scrubbed five deleted commands from all user-facing surfaces, including English docs, 4 localized doc sets (ja-JP, ko-KR, zh-CN, pt-BR), workflows, templates, and references. /gsd-check-todos → /gsd-capture --list /gsd-new-workspace → /gsd-workspace --new /gsd-status → /gsd-progress /gsd-plan-milestone-gaps → table rows / orphan sections removed (PR #3038 only scrubbed workflows/agent; missed the docs surfaces this PR covers) /gsd-research-phase → /gsd-plan-phase --research-phase Includes a fix to docs/issue-driven-orchestration.md (PR #3036) which itself referenced /gsd-new-workspace 4 times — self-correction. Removed: - get-shit-done/workflows/research-phase.md (orphan, capability absorbed into --research-phase flag) Tests: - tests/bug-3042-3044-research-flag-and-stale-refs.test.cjs — 46 structural-IR tests across both bugs: - argument-hint advertises --research-phase + --view - workflow parses --research-phase, sets RESEARCH_ONLY, early-exits before planner - --view prints RESEARCH.md without spawning - --research forces refresh in research-only mode - existing-RESEARCH.md prompt path with update/view/skip - workflows/research-phase.md is removed - 5 deleted slash-commands absent from 17 English user-facing surfaces + 16 localized doc surfaces (4 locales × 4 docs each) - replacement command tokens present where deleted ones lived 6950/6950 full suite pass. Lints clean. Closes #3042 Closes #3044 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: address all 8 CR findings on PR #3045 Major (3): - get-shit-done/workflows/plan-phase.md:344 — added explicit early-exit guard at Section 5.1: "Skip if RESEARCH_ONLY=true". Without it, an LLM could fall through "use existing, skip to step 6" → planner spawn, violating the research-only contract. The guard makes the early-exit unreachable from any non-research-only branch. - get-shit-done/references/continuation-format.md (3 examples) + zh-CN/.../continuation-format.md (3 examples) — pointed to `/gsd-plan-phase --research-phase` but docs/COMMANDS.md didn't document the flag. Added a full --research-phase + --view + --research modifier section to the /gsd-plan-phase flag table in COMMANDS.md so the canonical reference matches the continuation examples. Minor (5): - docs/FEATURES.md:1632 — `/gsd-plan-phase --research-phase` → `/gsd-plan-phase --research-phase <N>` (include required arg). - get-shit-done/templates/README.md:46 — NN-VALIDATION.md producer reverted from `/gsd-plan-phase --research-phase` (Nyquist) to plain `/gsd-plan-phase` (Nyquist). VALIDATION.md is created during normal Nyquist flow, not research-only mode — the bulk replacement was wrong for that line. - get-shit-done/workflows/help.md:89 — signature line was missing `--research`; added it alongside `--research-phase` and `--view`. - tests/bug-3042-3044-...:197 — promptHasView/promptHasSkip were tautological (matched anywhere in 1700-line workflow). Tightened to a proximity check anchored on "RESEARCH.md already exists" prompt header within a 600-char window. Updated workflow to emit that literal phrase. - tests/feat-2840-...:95 — workspace assertion used `/gsd-workspace` but the documented replacement is `/gsd-workspace --new`. Tightened to require both tokens (in 3 places: requiredCommands list, regex in conceptPairs, error message). 6950/6950 full suite pass. Lint clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> |
||
|
Tom Boucher
|
dca12242b5 |
fix(install): skip Gemini local commands/gsd when global GSD present (#3037) (#3041)
* fix(install): skip Gemini local commands/gsd when global GSD present (#3037) Reporter showed that running `npx get-shit-done-cc --gemini --global` followed by `--gemini --local` in a project creates the same 65 GSD command files in both Gemini scopes: - ~/.gemini/commands/gsd/ (user scope) - <project>/.gemini/commands/gsd/ (workspace scope) Gemini conflict-detects by command name across scopes and renames every overlapping /gsd:* command to /workspace.gsd:* and /user.gsd:*, breaking the documented /gsd:* namespace. Fix: in bin/install.js, when handling --gemini --local, detect whether ~/.gemini/commands/gsd/ already exists with managed-shape content. If so, skip the local copy and print a clear three-line warning explaining the conflict avoidance. The user-scope install already provides the same /gsd:* commands in this project; the local copy adds zero value. Sibling fixes (test isolation): - tests/install-minimal-all-runtimes.test.cjs: pass HOME/USERPROFILE through the spawned installer's env so the developer's real ~/.gemini/commands/gsd/ doesn't trigger the new skip path during test runs that want to assert the local-install populates commands/gsd/. - tests/gemini-namespacing.test.cjs: the "Gemini Install (Behavioral)" describe block now creates an isolated tmpHome and points process.env.HOME at it before calling install(false, 'gemini'), with proper restore in afterEach. Test: - tests/bug-3037-gemini-duplicate-commands.test.cjs — 4 structural tests: 1. global install populates HOME/.gemini/commands/gsd 2. local install AFTER global skips the local copy 3. local install with NO existing global still populates locally (no-regression) 4. local install when HOME has .gemini/ but no GSD-managed commands/gsd/ still populates locally (non-GSD-Gemini-user no-regression) 6909/6909 full suite pass. Lints clean. Closes #3037 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: address CR feedback on PR #3041 — narrower detection + USERPROFILE restore CR findings: 1. **bin/install.js (Major)** — userScopeHasGsd used `fs.readdirSync(homeGeminiGsd).length > 0` which would skip the local install for any non-empty directory, including a user who hand-dropped a single override at ~/.gemini/commands/gsd/<thing> .toml without ever running --gemini --global. Narrowed the detection to require at least 3 canonical GSD command files (help.toml, progress.toml, new-project.toml) — a marker that ships in every GSD Gemini install (minimal mode included) and is structurally impossible to produce by accident. 2. **tests/bug-3037-...:59 (Minor)** — beforeEach overwrites process.env.USERPROFILE but afterEach only restores HOME, leaking the temp home into later tests on Windows or any code path that reads USERPROFILE. Added save/restore symmetric with HOME. Plus added a 5th regression test covering the narrowed detection: "local install when HOME has hand-dropped overrides UNDER commands/gsd/ (but no full GSD) still populates locally" — directly exercises the edge case CR identified. 5/5 targeted tests pass. 6910/6910 full suite pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> |
||
|
Tom Boucher
|
7714b5244b |
fix(workflows,docs): scrub stale /gsd-code-review-fix and /gsd-plan-milestone-gaps refs (#3029, #3034) (#3038)
* fix(workflows,docs): scrub stale /gsd-code-review-fix and /gsd-plan-milestone-gaps refs (#3029, #3034) #2790 consolidated /gsd-code-review-fix into /gsd-code-review --fix and deleted /gsd-plan-milestone-gaps in favor of inline gap planning as part of /gsd-audit-milestone's output. The deletion was propagated through some surfaces (#2950 covered help/do/settings/discuss-phase/etc.) but several user-facing surfaces still emitted the old forms: #3029 — /gsd-code-review-fix references in: - agents/gsd-code-fixer.md (description, "Spawned by", recovery prose) - get-shit-done/workflows/code-review.md (offer text) - get-shit-done/workflows/execute-phase.md (offer text) - get-shit-done/workflows/code-review-fix.md (internal retry hints) - docs/INVENTORY.md (agent + workflow rows) - docs/CONFIGURATION.md (workflow.code_review row) - docs/USER-GUIDE.md (3 occurrences in walkthrough) - docs/AGENTS.md (gsd-code-fixer agent stub) - docs/FEATURES.md (commands list + REQ-REVIEW-04) All replaced with /gsd-code-review --fix. Internal retry hints in the workflow file itself updated to point at the new form. Release notes (docs/RELEASE-*.md) and gsd-ns-review's "absorbed by" deletion note left unchanged — historical/explanatory content. #3034 — /gsd-plan-milestone-gaps references in: - get-shit-done/workflows/audit-milestone.md (<offer_next> blocks for gaps_found and tech_debt: lines 281, 323) - commands/gsd/complete-milestone.md (gaps_found pre-flight: lines 46, 57) Replaced with inline closure path: /gsd-phase --insert <N> "Close gap: <REQ-ID> ..." /gsd-discuss-phase <N> /gsd-plan-phase <N> /gsd-execute-phase <N> Plus a Nyquist-coverage hint pointing at /gsd-validate-phase / /gsd-secure-phase for retroactive audit-chain hygiene gaps. The gsd-ns-project SKILL.md "deleted by #2790" note is preserved (it's the canonical pointer for future readers asking what happened to the command). Tests: - tests/bug-3029-3034-stale-command-routes.test.cjs — parser-based assertions per fixed surface, plus a structural cross-check that gsd-ns-project keeps the deletion note. 15 tests, all green. - 6905/6905 full suite passes. Closes #3029 Closes #3034 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: address CR feedback on PR #3038 — argument order, structural tests, agent count CR findings on PR #3038: 1. **docs/USER-GUIDE.md (Major)** — `--fix` examples used flag-first form (`/gsd-code-review --fix 3`), but the supported CLI grammar is phase-first (`/gsd-code-review 3 --fix`). The original sed-based replacement preserved the position of the `gsd-code-review-fix` token, producing the wrong order. Fixed in USER-GUIDE.md (3 occurrences) and the same drift in the workflow surfaces: - get-shit-done/workflows/code-review-fix.md (2 retry hints) - get-shit-done/workflows/code-review.md (offer text) - get-shit-done/workflows/execute-phase.md (offer text) 2. **docs/AGENTS.md (Minor)** — internal count drift: line 483 said "Ten additional agents" but line 725 said "12 advanced/specialized". Filesystem reality: 33 agents total, 21 primary, 12 specialized (count of `### ` stubs in the Advanced and Specialized section). Updated lines 3, 13, 483 to use 12/33 and added the two missing names (doc-classifier, doc-synthesizer) to the inline list at line 13. 3. **tests:94 (Major refactor suggestion)** — `.includes()` token checks were source-grep style. Refactored to a typed-IR pattern: extract the SET of slash-command tokens via regex, assert membership on the parsed Set instead of substring scanning the raw file text. Added the `allow-test-rule` comment explaining the IR-build vs IR-assertion split per scripts/lint-no-source-grep.cjs convention. 4. **tests:130 (Major)** — replacement-path assertion was file-wide and could false-pass on generic mentions of "inline" elsewhere in the file. Refactored: `extractOfferBlocks(content)` returns the typed list of `<offer_next>` and "Pre-flight" blocks where the deleted command previously lived, and the assertion runs against those blocks specifically. Now requires `/gsd-phase --insert` or inline-audit prose to appear in the same offer block, not just somewhere in the file. 15/15 targeted tests pass. 6905/6905 full suite pass. Lints clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> |
||
|
Tom Boucher
|
117b3ec009 |
docs: add issue-driven orchestration guide (#2840) (#3036)
* docs: add issue-driven orchestration guide (#2840) Adds docs/issue-driven-orchestration.md — a recipe for driving GSD from a GitHub / Linear / Jira issue using existing primitives. Maps Symphony-style orchestration concepts onto GSD commands without vendoring code, adding a daemon, or introducing tracker integration. Concept mapping covers: - WORKFLOW.md → ROADMAP.md / STATE.md / phase CONTEXT.md / phase PLAN.md - isolated agent workspace → /gsd-new-workspace --strategy worktree - agent dispatch → /gsd-manager (interactive), /gsd-autonomous (unattended) - per-phase steps → /gsd-discuss-phase → /gsd-plan-phase → /gsd-execute-phase - proof-of-work → /gsd-verify-work (UAT.md persists across /clear) - adversarial review → /gsd-review (cross-AI peer review) - human merge gate → /gsd-ship - follow-up capture → /gsd-note, /gsd-plant-seed, /gsd-new-milestone End-to-end flow walks through 7 numbered steps from picking the tracker issue to capturing follow-ups. Safety boundaries (isolated worktrees, explicit human review, no automatic public posting, verification before ship) and non-goals (no vendoring, no daemon, no mandatory tracker, no gate bypass, no command-surface expansion) are spelled out explicitly so the doc cannot drift into "let's just add one more flag". Cross-linked from docs/README.md (Documentation Index) and docs/USER-GUIDE.md (Table of Contents preamble). Tests: tests/feat-2840-issue-driven-orchestration-guide.test.cjs — 9 structural-IR tests parse the guide into a typed record and assert on flags (commandsPresent, conceptPairs, nonGoalFlags, safetyFlags, numberedSteps). Fence-language MD040 check enforced. Cross-link presence enforced. No raw-text assertions on prose. 6890/6890 tests pass. Lint:tests clean (allow-test-rule comment justifies the doc-shape parser per scripts/lint-no-source-grep.cjs escape hatch). Lint:changeset clean. Closes #2840 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(test): guard USER-GUIDE.md existsSync before read (CR #3036) CR Minor: cross-linked-from-USER-GUIDE.md test called fs.readFileSync directly without first asserting fs.existsSync, asymmetric with the README.md test above. A missing USER-GUIDE.md would throw ENOENT instead of producing a meaningful assertion message. Mirror the null-guard pattern. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> |
||
|
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> |
||
|
Tom Boucher
|
35fffe7f31 |
docs(out-of-scope): record #2758 agent-template-rendering decision
Closed on the technical merits: the determinism claim is theoretical (no observed misinterpretation), token waste is small and unmeasured, and PR #2279's orchestrator-embedding path already serves the deterministic-gating need without a parallel templating subsystem. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> |
||
|
Tom Boucher
|
d137ce86ec |
docs(out-of-scope): record #2756 temporal-context decision
Reporter did not return to clarify the actual ask after the narrowing-then- retraction in the comment thread. Closing as wontfix per .out-of-scope/ temporal-context.md with re-open criteria spelled out. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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> |
||
|
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. |
||
|
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). |
||
|
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 |