fb6633ceda4f41cb0bd7ccfef03e5710d44a9daa
273 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
ba625c0978 |
feat(shell-projection): add exec* dispatch and platform* file I/O seam (#3465) (#3470)
* feat(shell-projection): add exec* dispatch and platform* file I/O seam (Phase 1, #3465) Extends shell-command-projection.cjs with two new sections: Subprocess dispatch: - execGit(args, opts) → { exitCode, stdout, stderr } - execNpm(args, opts) → same; owns shell:true on Windows (npm.cmd) - execTool(program, args, opts) → same; ENOENT → exitCode 127, no throw - probeTty(opts) → string | null; returns null on Windows and non-tty Platform file I/O: - normalizeContent(filePath, content) → { content, encoding }; pure function; .md → full normalizeMd pass; other → CRLF→LF + trailing newline - platformWriteSync(filePath, content, opts) → ensureDir + normalizeContent + atomic write - platformReadSync(filePath, opts) → null on ENOENT; throws when required:true - platformEnsureDir(dirPath) → mkdirSync recursive, idempotent Absorbs normalizeMd fence-tracking logic from core.cjs (no circular dep). Existing rendering exports untouched. core.cjs compat exports unchanged until Phase 4. Updates CONTEXT.md with Shell Command Projection Module canonical definition. 31 new behavioral tests; full suite green. Closes #3465 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(context): record shell-projection expansion session learnings * chore(changeset): add entry for shell-projection I/O seam (#3465) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(adr): add ADR-0010 skill-surface budget module and ADR-0011 review default reviewers (Phase 1, #3465) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(context): record phase 1 rebase and PR session learnings (#3465) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(shell-projection): drop substring match on stderr — assert typed shape only The lint-no-source-grep rule prohibits substring matching on stderr (or any test-output text). The exitCode === 127 assertion already proves the ENOENT path; the stderr content was implementation-detail of the OS. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(pr3470): address CodeRabbit findings and minimal-core drift * docs(adr0010): align profile interface with phase-1 scope * docs(adr): index newly added 0010/0011 ADR drafts in README enh-3271 invariant requires every file in docs/adr/ to be linked from the README. Adds entries for the three ADR drafts committed earlier in this PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tom Boucher
|
d0f916728b |
feat(skill-surface): install-time profiles + runtime /gsd:surface (#3408) (#3456)
* feat(skill-deps): add requires: frontmatter to all 51 skills with cross-skill references Mechanical migration from docs/research/data/2026-05-12-skill-audit.json. Every skill whose body references another GSD skill now declares those dependencies in `requires:` YAML frontmatter (flow-style array). Notable: discuss-phase, plan-phase, and execute-phase all reference `phase`, which confirms the latent gap in MINIMAL_SKILL_ALLOWLIST — `phase` is pulled by the core loop but was never in the allowlist. The profile closure model (ADR-0010 Phase 1) resolves this automatically. Closes part of #3408. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(skill-surface-budget): add PROFILES map, resolveProfile, loadSkillsManifest, staging, marker IO Implements the Skill Surface Budget Module core (ADR-0010, Phase 1): - PROFILES Object.freeze map: core (6 skills), standard (~13), full ('*') - loadSkillsManifest: parses requires: frontmatter from commands/gsd/*.md into a Map<stem, string[]> without external YAML dep - resolveProfile({modes, manifest}): computes transitive closure over the requires: graph; composable (modes=['core','audit'] unions closures) - stageSkillsForProfile / stageAgentsForProfile: filesystem staging with same exit-cleanup machinery as the legacy stageSkillsForMode - readActiveProfile / writeActiveProfile: .gsd-profile marker round-trip - Back-compat shims preserved: MINIMAL_SKILL_ALLOWLIST, isMinimalMode, shouldInstallSkill (overloaded), stageSkillsForMode — all legacy tests pass The phase latent bug is now resolved by closure: discuss-phase, plan-phase, and execute-phase all require phase, so any profile including any of them automatically includes phase via transitive closure. Tests: 22 manifest+resolve, 9 stage, 10 marker (41 new tests, all green). Back-compat anchor: 80/80 passing. Closes part of #3408. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(skill-surface-budget): add lint-skill-deps.cjs CI gate and fix 19 missed requires: entries Two lint checks (scripts/lint-skill-deps.cjs): a) Frontmatter-body consistency: skill body references must appear in requires: b) Profile closure: every requires: dep of any profile skill must be in closure Running the lint revealed 19 body references missed by the audit JSON (the audit used static analysis; some bodies have conditional references). Fixed: complete-milestone: +audit-milestone, discuss-phase, plan-phase, execute-phase, new-milestone fast: +quick health: +thread map-codebase: +new-project, plan-phase new-milestone, new-project, review, ultraplan-phase: +plan-phase ship: +verify-work sketch, spike: +new-project verify-work: +execute-phase workstreams: +new-milestone, resume-work Wired into package.json as lint:skill-deps and added to pretest. 8 fixture-based tests: all green. Closes part of #3408. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(skill-surface-budget): wire --profile= arg, profile marker write/read in bin/install.js - Add --profile=<name> / --profile=<n1>,<n2> arg parsing (composable). Mutually exclusive with --minimal / --core-only (aliases for --profile=core). Default (no flag): full. - Import readActiveProfile / writeActiveProfile from install-profiles.cjs. - After writeManifest: persist active profile to .gsd-profile marker. - gsd update path: if no --profile flag given, read existing .gsd-profile marker so non-full profiles are not silently re-expanded to full (ADR-0010). - Update --help block to document --profile= with per-tier token costs. New test: install-minimal-backcompat.test.cjs (6 tests): - PROFILES.core === MINIMAL_SKILL_ALLOWLIST (contract) - --minimal writes .gsd-profile marker "core" - --profile=core, --profile=standard write correct markers - default install writes marker "full" Closes part of #3408. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(changeset): add feat-3408-skill-profiles changelog fragment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(install-profiles): derive agents from skill body refs and wire into resolveProfile Deviation 1 of ADR-0010 phase 1b: tiered profiles (core, standard) now produce a non-empty agents Set instead of always returning empty. resolveProfile() scans each skill body for gsd-* agent name references (via new parseCallsAgents()), stores them in _calls_agents_<stem> manifest entries, and unions them across the resolved skill closure. stageAgentsForProfile() already checked resolvedProfile.agents — it now gets real data so tiered profiles install the correct subset of agents instead of zero. Closes #3408 (partial — Deviation 1 only) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(install): honor .gsd-profile marker on update, add resolveEffectiveProfile/mostRestrictiveProfile Deviation 2 of ADR-0010 phase 1b: the marker written during installation is now actually honored when re-running without explicit flags (e.g. gsd update). The dead-end logging block is replaced by resolveEffectiveProfile(), which picks the marker profile over 'full' when no explicit --profile= flag was given. The resolved profile is piped through to all 13 stageSkillsForMode dispatch sites (now _stageSkills) so updates install only the previously-chosen skill subset. --minimal retains its back-compat behavior (strict 6-skill allowlist, no closure) while writing 'core' to the marker. mostRestrictiveProfile() is exported for callers that need to reconcile disagreeing markers across runtimes (smallest skill set wins). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(surface): add CLUSTERS data + state IO module Add clusters.cjs with 10 named skill groups covering all 66 skills (verified by surface-clusters.test.cjs). Add surface.cjs with readSurface/ writeSurface atomic IO, resolveSurface, applySurface, and listSurface. Tests: 17 passing (11 state IO + 6 cluster integrity). Closes #3408 * docs(adr): add ADR-0011 Skill Surface Budget Module (Phase 1 accepted, Phase 2 amendment) Records the install-time profile staging decision (Phase 1, landed) and the runtime /gsd:surface cluster-toggle decision (Phase 2, in flight) as an amendment. Updates the ADR README index. Closes #3408 * docs(install-profiles): update module docblock for Phase 2 and ADR-0011 Corrects the ADR reference from 0010 to 0011, documents the three-profile model and back-compat aliases, adds resolveEffectiveProfile precedence rule, and notes the companion surface.cjs Phase 2 engine. * docs(context): add Skill Surface Budget Module canonical entry Adds the Domain terms entry for the Skill Surface Budget Module covering both Phase 1 (install-time profiles, .gsd-profile marker) and Phase 2 (runtime /gsd:surface cluster toggles, clusters.cjs, .gsd-surface.json), per ADR-0011 Consequences requirement. * feat(surface): add resolveSurface and applySurface engine + tests Tests cover: profile → surface equivalence, cluster disable/enable, explicitAdds transitive closure, applySurface file sync (add missing, remove superseded, preserve non-gsd files), listSurface token cost. 16 new tests passing. * docs(readme): document --profile= flag and /gsd:surface command Brief user-facing mention of install profiles (core/standard/full) and the /gsd:surface slash command in the Commands table. Points to ADR-0011 for details. * feat(surface): add /gsd:surface slash command runbook New skill: gsd:surface — runtime profile/cluster toggle without reinstall. Sub-commands: list, status, profile <name>, disable/enable <cluster>, reset. Persists state to .gsd-surface.json (independent of .gsd-profile). Description 96 chars (≤100 limit). lint:descriptions + lint:skill-deps: 0 violations. * feat(surface): add changeset fragment for /gsd:surface runtime toggle * feat(surface): add surface skill stem to utility cluster surface.md is a new skill; add it to the utility cluster so the surface-clusters.test.cjs coverage invariant stays satisfied. * docs(adr): fix ADR references to 0011 and record Phase 2 as shipped ADR-0010 number was already claimed by the file-operation-engine ADR; this ADR landed as 0011-skill-surface-budget-module.md. Update inline ADR references in clusters.cjs, surface.cjs, install-profiles.cjs, and the Phase 2 changeset to ADR-0011. Update the ADR Status section to record Phase 2 artifacts as shipped on this branch rather than "in progress". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(research): port skill-surface-budget memo and audit data ADR-0011 references docs/research/2026-05-12-skill-surface-budget.md and docs/research/data/2026-05-12-skill-audit.json, which only existed in the research worktree. Port both onto this branch so the ADR's References section resolves and reviewers can read the cluster taxonomy (§3.2), dependency topology (§3.1), and option grading (§4) that justify Phase 1 and Phase 2 decisions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(registration): register surface/clusters in INVENTORY, COMMANDS, and help.md - surface.md: convert allowed-tools from inline YAML array to block style (was parsed as a single tool name "[Read, Write, Bash]" by test harness) - docs/INVENTORY.md: add CLI module rows for clusters.cjs and surface.cjs; add Commands row for /gsd-surface; bump CLI Modules count 55→57, Commands 66→67 - docs/INVENTORY-MANIFEST.json: add entries for clusters.cjs, surface.cjs, and /gsd-surface (filename-based command key) - docs/COMMANDS.md: add ### `/gsd-surface` heading in Configuration Commands - get-shit-done/workflows/help.md: add /gsd:surface entry in Configuration section Fixes registration failures introduced by Phase 2 of #3408. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(surface,docs): scrub .claude leakage and escape hypothetical slash tokens Two PR regressions introduced earlier on this branch: 1. surface.cjs JSDoc comments contained the canonical paths (~/.claude/commands/gsd, ~/.claude/agents) as example values, which the cline-install leak regex (~\/\.claude\/(?:get-shit-done|commands|agents |hooks)) flagged as install-time path leaks. Reworded the docblocks to describe runtime-resolved paths without literal ~/.claude tokens. 2. The ported research memo proposed hypothetical Option C dispatchers using slash syntax (/gsd:milestone, /gsd:research). The docs-parity-live-registry test enforces that every slash-command token in docs/ resolves to a real command. Rewrote the Option C sketch without the slash prefix and added a clarifying note that the dispatchers are illustrative, not shipped. Targeted tests now pass: tests/cline-install.test.cjs and tests/docs-parity-live-registry.test.cjs both green. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: remove raw output/source grep in lint tests * fix: close coderabbit profile and requires issues * test: align surface token-cost assertion wording * fix(install): align core profile alias and defer profile marker write --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tom Boucher
|
bf3c736029 |
feat(shell-projection): centralize managed hook command policy (#3450)
* feat(shell-projection): route persistent PATH hints through projection seam * refactor(shell-projection): unify managed hook command policy * fix(install): guard malformed settings hooks during uninstall * chore(changeset): add fragment for pr 3450 * fix(install): guard malformed settings hook entries |
||
|
Tom Boucher
|
16f5009270 |
feat(shell-projection): route PATH guidance through typed action projection (#3447)
* feat(shell-projection): route PATH guidance through typed action projection * chore(changeset): add PR 3447 changelog fragment |
||
|
Tom Boucher
|
7e91a861ee |
feat: deepen shell command projection seam (#3445)
* feat: deepen shell command projection seam * chore: add changeset for shell projection seam * feat: centralize local and portable hook path projection * docs: avoid prompt-scan false positive in installer migrations * docs(adr): fix path-style token punctuation |
||
|
Tom Boucher
|
589318ce0a |
feat: route Codex hook commands through projection seam (#3444)
* feat: route Codex hook commands through projection seam * docs: add changeset for Codex hook projection slice * docs: set changeset pr for #3444 * fix(codex): decode toml-escaped legacy hook path * test(codex): use structured assertion for hook projection |
||
|
Tom Boucher
|
98cc1a84c0 |
fix: scope Windows hook syntax to Gemini runtime (#3438)
* test: cover Windows hook shell drift for Claude (#3413) * fix: scope Windows hook syntax to Gemini runtime (#3413) * docs: add changeset for #3413 * docs: set changeset pr for #3413 * fix: route Windows hook formatting through runtime-aware projection seam * test: cover runtime projection edge cases for Windows hooks * fix(docs): add shell-command-projection to inventory parity * docs: align CLI module shipped count after rebase |
||
|
Tom Boucher
|
656fb5868c |
Fix(hooks): resolve Windows Bash hook runner (#3397)
* fix: resolve windows bash hook runner * chore: add changeset for codex bash hooks * fix: avoid hardcoded windows bash paths * fix: skip unavailable bash hook registrations |
||
|
Tom Boucher
|
1f99085559 |
Fix(hooks): normalize Windows managed hook script paths (#3396)
* fix: normalize windows managed hook script paths * chore: add changeset for codex hook paths * docs: hyphenate windows managed hooks changeset |
||
|
Tom Boucher
|
7a0a7f1300 | Merge main into phase 5 installer migrations | ||
|
Tom Boucher
|
d40c576dd0 | Merge main into phase 4 installer migrations | ||
|
Tom Boucher
|
6a93b14186 |
Merge remote-tracking branch 'origin/main' into codex/installer-migrations-phase-two
# Conflicts: # bin/install.js # get-shit-done/bin/lib/installer-migrations.cjs |
||
|
Tom Boucher
|
041e94c43b | fix: roll back installer migration state | ||
|
Tom Boucher
|
c4fe891391 | fix: tighten installer migration authoring guards | ||
|
Tom Boucher
|
908a19cd04 | fix: harden installer migration integration | ||
|
Tom Boucher
|
4e40ee8b2f | fix: harden installer migration paths | ||
|
Tom Boucher
|
7fe75c2c3e | Feat(installer): harden phase 4 migration integration | ||
|
Tom Boucher
|
0b62129847 | Wire installer migrations into install flow | ||
|
Tom Boucher
|
3943146484 | feat: migrate legacy codex hooks cleanup | ||
|
Tom Boucher
|
6d33055756 | feat: add installer migration framework | ||
|
Tom Boucher
|
cb27c18026 | fix(installer): warn on stale gsd-sdk path | ||
|
Tom Boucher
|
19295f5ab6 | fix(gemini): make Windows hooks and agent tools valid | ||
|
Tom Boucher
|
b55848bd7d | fix(codex): block unsupported execute worktrees | ||
|
Tom Boucher
|
14e47acd9c | fix(codex): remove legacy hooks json update hook | ||
|
Tom Boucher
|
4cb5649e8a | fix(gemini): drop Agent dispatcher tool (#3349) | ||
|
Tom Boucher
|
e14ef535aa |
fix(install): allow codex hooks.state.<key> as regular table (#3285) (#3289)
* test: codex hooks.state.<key> tables must validate as regular tables (#3285 RED) Drive validateCodexConfigSchema with a fixture containing both [hooks.state] and [[hooks.SessionStart]] entries. Expect the state tables to pass as regular tables. Currently fails — validator over-classifies every hooks.* path as AoT. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(install): treat codex hooks.state as regular table not AoT (#3285) validateCodexConfigSchema was over-classifying every hooks.* section header as an event-handler array-of-tables, rejecting the hooks.state.* namespace that Codex CLI 0.130.0+ uses for per-hook trust persistence. Fix: 1. Section-header check: carve out `hooks.state` and `hooks.state.*` from the AoT-required rule — only paths that are neither of those still require double-bracket form. 2. Parsed-object check: skip the `state` key when iterating Object.entries (parsed.hooks) so the "must be array" guard does not fire for the trust namespace object. All other hooks.<EVENT> validation (SessionStart AoT, handler-field placement) is unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * changeset: pr=3289 for #3285 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(install): codex hooks.state must be regular-table, reject AoT/scalar (CR finding 1) - migrateCodexHooksMapFormat: exclude hooks.state and hooks.state.* from legacy-map detection so [hooks.state] is never promoted to [[hooks.state]] AoT - validateCodexConfigSchema: reject [[hooks.state]] / [[hooks.state.*]] AoT at section level; reject Array/scalar values at parsed-object level - Accept only plain-object shape for hooks.state and hooks.state.* (Codex CLI 0.130.0+ trust-persistence namespace) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: assert codex hooks.state trust entry preserved with original values (CR finding 2) Strengthen post-install preservation assertion to verify the actual trust entry key and its enabled/trusted_hash values survive — not just that hooks.state is an object. Add two validator-reject tests for [[hooks.state]] and [[hooks.state.foo]] AoT forms. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tom Boucher
|
b08e57c92d |
fix(install): copy sdk/shared/model-catalog.json + resolve chain in CJS (#3288) (#3293)
* test: reproduce model-catalog MODULE_NOT_FOUND in install layout (#3288) Tests A/B/C exercise the install-layout regression introduced by #3230: - A: confirms the old 3-level __dirname path fails when sdk/shared/ is absent - B: confirms the new co-located bin/shared/ path resolves correctly (RED) - C: confirms install() copies model-catalog.json to co-located path (RED) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(install): copy sdk/shared/model-catalog.json + resolve chain in CJS (#3288) Two-part fix for the CRITICAL regression introduced by #3230: Install-side: bin/install.js now copies sdk/shared/model-catalog.json into get-shit-done/bin/shared/model-catalog.json immediately after the main get-shit-done/ copy step. Every runtime install (Claude Code, Codex, OpenCode, Gemini, etc.) now includes this file in the payload. CJS-side: model-catalog.cjs replaces the brittle single-path require with a resolve-chain that checks candidates in order: 1. get-shit-done/bin/shared/model-catalog.json (co-located, preferred post-install) 2. sdk/shared/model-catalog.json (source-repo dev path, legacy fallback) 3. GSD_MODEL_CATALOG env override (custom deployments / test harnesses) When no candidate resolves, throws with a diagnostic listing all tried paths (PRED.k301 — throw must include candidate paths for debuggability). REFACTOR audit: three other __dirname traversals in bin/lib/ were inspected: - core.cjs:1242 (3 levels up → agents/) — safe; agents/ IS copied to targetDir - profile-output.cjs:547,740 (2 levels up → templates/) — safe; templates/ is inside get-shit-done/ and IS copied Only model-catalog.cjs traversed outside the installed payload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * changeset: pr=3293 for #3288 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(model-catalog): narrow catch to missing-file errors; clear env in test (#3288) Two CR findings from PR #3293 review: 1. model-catalog.cjs catch block swallowed ALL errors — malformed JSON, permission errors, and other real failures were silently absorbed into the fallback chain. Now only MODULE_NOT_FOUND (with matching path in message) and ENOENT are treated as recoverable; any other error is rethrown immediately. 2. test beforeEach saved GSD_EXPLICIT_CONFIG_DIR but didn't clear it — a CI-set value could leak into install() and redirect the install to an unexpected directory, making test C non-deterministic. Added `delete process.env.GSD_EXPLICIT_CONFIG_DIR` to beforeEach. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tom Boucher
|
d241227129 |
fix(install): Windows persistent SDK shim; replace legacy gsd-tools.cjs shim (#3211) (#3282)
* test: reproduce Windows SDK not found after fresh npx install (#3211) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(install): Windows persistent Path probe + npx-PATH filter on Windows (#3211) Add getUserShellWindowsPersistentPath() — the Windows counterpart to getUserShellPath(). Probes the user-level 'Path' registry key via powershell.exe so the installer can verify gsd-sdk is reachable from PowerShell/cmd.exe/Git Bash post-install, not just in the transient npx subprocess PATH. Wire it into installSdkIfNeeded: on Windows, use the registry-derived persistent Path (with npx dirs stripped) as the cross-shell reachability gate, instead of skipping the check entirely. This is the Windows sibling of the Linux fix in #3249/#3231. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * changeset: pr=3282 for #3211 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(install): include Machine+User Path in Windows persistent probe (#3211) getUserShellWindowsPersistentPath now merges Machine-level and User-level registry Path entries (matching the effective PATH that PowerShell, cmd.exe, and Git Bash inherit), instead of reading only User-level. Reading User-only would produce a false warning when gsd-sdk is installed in a machine-level bin dir (e.g. C:\Program Files\nodejs). Addresses CodeRabbit finding on PR #3282. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tom Boucher
|
deeb6deb67 |
fix(install): accept Codex TOML floats; idempotent rollback (#3245) (#3254)
* test: reproduce extractFrontmatter LAST-block bug (#3240) * test: reproduce state.update progress trampling and percent formula (#3242) Two failing regression tests: - Bug A: state.update "Last Activity" tramples curated progress.* frontmatter via readModifyWriteStateMd → syncStateFrontmatter - Bug B: 12 declared ROADMAP phases / 6 realized / 6/6 plans done → percent: 100 instead of 50 (phase-fraction ignored) * test: reproduce TOML float rejection and partial rollback (#3245) Two failing regression tests: 1. parseTomlToObject rejects valid Codex TOML floats (tool_timeout_sec = 20.0) 2. Post-install validation failure leaves skills/, agents/, VERSION on disk despite restoring config.toml — hybrid state after abort Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(install): accept TOML floats; idempotent codex rollback (#3245) Two fixes for the Codex install failure introduced by #2760 CR4 finding 3: 1. parseTomlValue now accepts TOML 1.0 float literals (decimals, exponents, underscore separators, signed). Codex CLI's serde schema requires f64 for tool_timeout_sec / startup_timeout_sec — the prior strict-integer-only check was the inverse of what Codex requires, causing every config with a float to trigger a fatal schema validation failure. Date/time separators (-/:T/Z) are still rejected. 2. restoreCodexSnapshot is extended into a unified idempotent rollback that reverts ALL Codex-specific mutations on failure: - config.toml (existing behavior) - skills/gsd-* directories (new) - agents/gsd-*.{md,toml} files (new) - get-shit-done/VERSION (new) - orphaned atomic-write temp files (new) Pre-install state is captured before the first Codex write so the rollback reflects the true pre-GSD state. Non-gsd-* user content is untouched. The rollback is safe to call multiple times and before any snapshots are captured. Fixes #3245 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * changeset: pr=3254 for #3245 * test: fix source-grep lint violation in bug-3242 test (#3242) Replace content.includes() check with line-by-line parse of STATE.md body. The lint enforces structural assertions over raw text matching. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: mark #3242 RED tests as todo pending fix (#3242) The three failing tests are intentional regression tests for bugs in state.cjs that will be fixed in a separate PR. Mark them { todo: true } so they don't block CI on this branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(install): tighten TOML underscore placement validation (CR finding 1) The float regex used [\d_]* which accepts invalid forms like 1__0, 1_.0, and 1._0. TOML 1.0 §2 requires underscores only between digits. Switch both the integer pre-check and the full float pattern to (?:_?\d)* so consecutive underscores, leading underscores on a segment, and trailing underscores on a segment are all rejected before replace(/_/g,'') can silently normalize them into valid JS numbers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(install): restore pre-existing gsd-* content on rollback (CR finding 2) The snapshot only recorded names of pre-existing skills/gsd-* dirs and agents/gsd-* files. On a failed reinstall the rollback could delete newly-created dirs but could not restore the bytes of dirs/files that were overwritten, leaving the user in a hybrid state (old config.toml, new skill files). Now snapshot the full file tree of every pre-existing gsd-* skill dir into codexPreInstallSkillContents (Map<name, Map<relPath, Buffer>>) and every pre-existing agent file into codexPreInstallAgentContents (Map<filename, Buffer>). restoreCodexSnapshot() uses these maps to wipe-and-restore overwritten entries and only removes entries that had no pre-install state, giving a true atomic rollback guarantee. Reads are best-effort so a partial snapshot is still better than none. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(install): scope temp-file cleanup to installer-owned writes (CR finding 3) _cleanTmpFiles() was deleting any *.tmp-<pid>-<n> file found under targetDir. This is too broad: other tools in the user's Codex/home directory may create temp files matching the same suffix pattern, and a GSD install rollback would silently delete them. Add __atomicWrittenTmps (a module-level Set<string>) populated by atomicWriteFileSync for every temp path it creates. _cleanTmpFiles() now checks __atomicWrittenTmps.has(full) before unlinking, so only temp files this installer process actually wrote are eligible for cleanup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): remove no-op doesNotThrow wrapping try/catch (CR finding 4) assert.doesNotThrow(() => { try { f(); } catch(_){} }) always passes because the catch block swallows every exception before the outer assertion can see it. This meant the rollback-idempotency guarantee was never actually verified. Replace with an explicit threw flag around runCodexInstall, assert that the install did throw (validation failure is expected), and add a post-rollback state assertion that skills/ was not created. This gives a loud failure surface if runCodexInstall starts crashing from inside the rollback path, matching the intent described in the test comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): correct describe title for float-acceptance tests (CR nitpick 1) The describe block title said 'rejects malformed input that previously slipped through', but the test inside now asserts that TOML floats are accepted (the #3245 inversion). This misled readers expecting every sub-test to assert rejection. Update the title to reflect the mixed behaviour: floats are accepted; dates and trailing-garbage are rejected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): rename test to match what the assertion actually checks (CR nitpick 2) The test name 'post-install config retains float literal form (20.0 not truncated to 20)' promised a string-form invariant, but the assertion uses numeric equality (assert.strictEqual(parsed.tool_timeout_sec, 20)) which cannot distinguish 20 from 20.0 in JS. Rename to 'post-install config round-trips tool_timeout_sec as numeric 20' so the description matches what the test actually verifies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): replace raw text scan with state json assertion (CR nitpick 3) The 'Last Activity updates the body field' test was reading STATE.md as raw text, splitting on newlines, and using lines.find/startsWith to locate the 'Last Activity:' line — the exact pattern-match-on-source approach prohibited by the no-source-grep testing standard. Replace with runGsdTools('state json', tmpDir) which surfaces the body- extracted Last Activity value as fm.last_activity in its JSON output, and assert against that structured field instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): correct post-rollback state assertion for early-failure case The previous assertion checked that skills/ didn't exist, but the installer writes skills/ before the schema validator fires. Rollback removes gsd-* dirs inside skills/, not skills/ itself. Update the assertion to verify that no gsd-* skill dirs survive rollback, which is the actual invariant the test name describes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * changeset: document full rollback scope (CR finding 1) Adds config.toml restoration and orphaned atomic-write temp-file cleanup to the changeset description — the previous text only listed skills/, agents/, and VERSION. * fix(install): wrap post-snapshot scope in rollback handler (CR finding 2) Any throw between the pre-install snapshot capture and the Codex config block (skills copy, agents copy, VERSION write, manifest write, leaked- path scan, etc.) now triggers _codexPreConfigRollback() so the caller is never left in a partially-installed state. Previously only the later config.toml mutation paths had rollback wired in. Introduces _codexPreConfigRollback (defined right after snapshot capture) and wraps the intervening operations in a try/catch that invokes it on error for Codex installs; non-Codex paths are unaffected. * test: assert threw=true to prevent vacuous pass (CR finding 4) Two tests used bare try/catch without asserting threw === true, so they would silently pass even if runCodexInstall never threw (k060 pattern). Each bare catch block is replaced with a threw flag and a strictEqual(threw, true, ...) assertion. CR findings 2+3 are both addressed in the preceding install commit: finding 3 (restore from snapshot manifest, not current FS state) lands alongside the rollback-wrapper change as part of the restoreCodexSnapshot refactor. * fix(install): reject leading zeros in TOML float integer part per TOML 1.0 (CR finding round 4) TOML 1.0 §2 disallows leading zeros in the integer part of numeric literals — `01`, `00`, `01.5`, `00e2`, `+01.0`, `-01.0` are all invalid. The pre-check and float regexes in parseTomlValue used `\d(?:_?\d)*` which accepted any digit as the leading digit. Both regexes are tightened to `(0|[1-9](?:_?\d)*)` for the integer part: - `0` alone is valid - a non-zero leading digit followed by optional underscored digits is valid - `01`, `00`, and any variant with a leading zero and further digits is rejected The "still rejects bare time (07:32:00)" test assertion is broadened from `/unsupported TOML value/` to `/unsupported TOML value|trailing bytes/` because the parser now stops at `0` and the remainder `7:32:00` is rejected as trailing bytes — the invariant (time literals are not accepted) is unchanged. 25 new regression tests cover all rejection cases and valid TOML forms. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tom Boucher
|
c4d3fe62a5 |
fix(install): require persistent SDK reachability before reporting ready (#3231) (#3249)
* test: reproduce false GSD SDK ready signals on Linux (#3231) * fix(install): require persistent SDK reachability before reporting ready (#3231) * changeset: pr=3249 for #3231 * fix(install): filter _npx from login-shell PATH probe (CR finding 1) Apply filterNpxFromPath() to the getUserShellPath() result before passing it to isGsdSdkOnPath(), mirroring the same filtering already applied to process.env.PATH. Without this, a transient _npx entry in the login-shell PATH can falsely satisfy the cross-shell reachability check and reintroduce the false-ready condition this PR fixes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): unconditional legacy-shim replacement assertion (CR finding 2) Replace readFileSync+includes source-grep check with isLegacyGsdSdkShim() and add an else branch asserting that when sdkReady is false, a warning/error was emitted. Previously the sdkReady===false path had no assertion at all, allowing the test to pass without verifying any postcondition. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: replace text-grep assertions with structured ones (CR finding 2 + nitpick) Finding 2: restructure the legacy-shim replacement assertion to branch on isLegacyGsdSdkShim() state (a behavioral fact) rather than console output, and add an unconditional postcondition for both branches. Nitpick 3 (4 locations): - lines 149-153: replace /GSD SDK ready/.test(combined) with isGsdSdkOnPath(filterNpxFromPath(PATH)) === false - lines 167-169, 185-189: split filterNpxFromPath result into segments array and use array.includes() instead of string.includes() on the raw PATH string - lines 375-377: replace /GSD SDK ready/.test(combined) with fs.existsSync(shimPath) + isGsdSdkOnPath(filterNpxFromPath(localBin)) All 8 tests pass. lint-no-source-grep: 0 violations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(build-hooks): per-PID staging dir eliminates concurrent-cleanup TOCTOU race When multiple test before() hooks spawned build-hooks.js concurrently (--test-concurrency=4), a race existed: Process A would finish all copies, call rmdirSync('.dist-staging/') in cleanup, then Process B — still in its copy loop — would call copyFileSync(src, '.dist-staging/hook.pid.ts') and get ENOENT because the staging directory was gone. On macOS/Linux, copyFileSync reports the SOURCE path in ENOENT errors when the destination directory is missing, making the failure appear to be a missing source file (hooks/gsd-statusline.js) rather than a missing destination directory. This misled the diagnosis. Fix: make STAGE_DIR per-PID ('.dist-staging-<pid>/') so each builder owns its own staging directory. No other process touches it, eliminating all contention on staging-dir creation and cleanup. Update .gitignore to match the new 'hooks/.dist-staging-*/' glob. Reproduces as: CI test matrix (macos-24, ubuntu-22, ubuntu-24) all failing with ENOENT on hooks/gsd-statusline.js in bug-2136 before() hook. The new test file added in this PR (bug-3231) shifts the concurrency schedule just enough to expose the race on every CI run. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: assert on captured console output, not tautological PATH state (CR finding) The two discarded `captureConsole()` return values in the bug-3231 test were flagged by CodeRabbit as tautological assertions. Fix: - Test 1 (transient _npx PATH): capture stdout/stderr and assert the installer does NOT emit "GSD SDK ready" (the false-positive the PR fixes), and that it does emit some diagnostic output instead. - Test 3 (clean install): capture stdout/stderr and assert the installer DOES emit "GSD SDK ready" after successfully self-linking into a persistent PATH dir — confirming the positive path works correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tom Boucher
|
739b95ef80 | fix(install): normalize Homebrew node@NN Cellar paths | ||
|
Tom Boucher
|
69aa7ec04e |
fix(install): prefer stable Homebrew symlinks over versioned Cellar paths in node runner
process.execPath on Homebrew resolves symlinks and returns the versioned Cellar path (e.g. /usr/local/Cellar/node/25.8.1/bin/node). After brew upgrade node, the old Cellar binary fails with dyld: Library not loaded because shared libraries have changed SOVERSION. - Add normalizeNodePath() helper that maps Cellar paths to stable Homebrew symlinks (/usr/local/bin/node or /opt/homebrew/bin/node) - resolveNodeRunner() now calls normalizeNodePath() before quoting - rewriteLegacyManagedNodeHookCommands() also normalizes baked Cellar runner paths in existing hook commands so reinstall doesn't re-bake them - Export normalizeNodePath for testability - Add 22 tests covering all cases (Cellar paths, stable symlinks, NVM, system node, Windows, null/empty, both function surfaces) Closes #3181 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tom Boucher
|
2bc49b0aec |
fix(install): wire --sdk flag into installSdkIfNeeded (#3033)
hasSdk was parsed in bin/install.js but never passed to installSdkIfNeeded, so `npx get-shit-done-cc@latest --sdk` silently skipped SDK deployment via the isLocal early-return and emitted a misleading "✓ GSD SDK ready" message. installSdkIfNeeded now accepts opts.forceSdk. When true (set from hasSdk at the call site in installAllRuntimes), the local-install soft-skip is bypassed so the full shim-link path runs regardless of install mode. When dist is also missing with forceSdk=true, the fail-fast diagnostic fires instead of silently returning. The #2678 soft-skip (isLocal + missing dist + no --sdk) is preserved. Closes #3033 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Otavio Salvador
|
8ca86b5e24 |
fix: use #!/usr/bin/env bash in community .sh hooks for distro portability
The three opt-in bash hooks (gsd-phase-boundary.sh, gsd-session-state.sh,
gsd-validate-commit.sh) shipped with #!/bin/bash, which fails on distros
that don't ship bash at /bin/bash (NixOS, minimal Alpine images, some
container runtimes). POSIX guarantees /bin/sh but not /bin/bash.
This is latent in the default install path because Claude Code wires the
hooks as `bash <path>` from settings.json (PATH-resolved — the script's
own shebang is read as a comment by bash). The fix matters when scripts
are run directly: tests, future installer changes, or manual debugging.
Changes:
- hooks/gsd-{phase-boundary,session-state,validate-commit}.sh: shebang
switched to #!/usr/bin/env bash, matching the convention already used
in scripts/*.sh.
- tests/bug-2136-sh-hook-version.test.cjs: assertion updated to expect
the new shebang; comment updated to spell out the rationale.
- tests/bug-2979-hook-absolute-node.test.cjs: doc-comment updated — the
prior wording cited "POSIX std PATH always has /bin" as the reason
bare `bash` is OK. The actual reason is that bare `bash` is
PATH-resolved, which is portable across distros that don't ship
/bin/bash. POSIX std PATH guarantees /bin/sh, not /bin/bash.
- bin/install.js::buildHookCommand: comment block clarifying the same.
No behavior change in this file — bare `bash` was already correct.
- .changeset/portable-bash-shebang-hooks.md: changeset entry.
Verified locally on NixOS:
- npm run build:hooks: hooks/dist/*.sh shebangs propagate correctly.
- node --test tests/bug-2136-*.cjs tests/bug-2979-*.cjs
tests/bug-1817-*.cjs tests/bug-1834-*.cjs tests/bug-1906-*.cjs
tests/bug-2557-*.cjs tests/bug-3017-*.cjs tests/security-scan.test.cjs
tests/hooks-doc-parity.test.cjs: 126/126 pass.
- node scripts/run-tests.cjs (full suite): 6944 pass / 0 fail / 5 skip.
|
||
|
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
|
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
|
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
|
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 |
||
|
Tom Boucher
|
ca78b65de7 |
fix(#2973): /gsd-profile-user writes dev-preferences.md to skills/, not legacy commands/gsd/ (#3003)
* fix(#2973): /gsd-profile-user writes dev-preferences.md to skills/ not legacy commands/gsd/ v1.39.0's install summary claimed the legacy ~/.claude/commands/gsd/ directory had been removed in favor of skills-only architecture, but the cmdGenerateDevPreferences writer at profile-output.cjs:781 still defaulted to the legacy path. Every /gsd-profile-user --refresh deterministically re-created the legacy directory. Missed in PR #1540's migration because dev-preferences is a runtime-generated user artifact, not a GSD-shipped command file. Fix: - Writer default: ~/.claude/skills/gsd-dev-preferences/SKILL.md - profile-user.md Display message + artifact list reference new path - New migrateLegacyDevPreferencesToSkill(targetDir, saved) installer helper. Called at all 5 skills-aware install branches. Copies preserved legacy dev-preferences.md into skills/gsd-dev-preferences/ SKILL.md, but ONLY if no SKILL.md already exists -- never clobbers user-customized skill content. Tests: bug-2973-profile-user-skills-path.test.cjs runs the writer in a subprocess (core.cjs:output uses fs.writeSync(1, ...) which bypasses in-process stubbing), asserts the writer's command_path field is the skills location, the file is on disk at that path, the legacy path is NOT created. Tests for migration helper assert it writes when no skill exists and skips when one does. Closes #2973 * chore(#2973): add changeset fragment for PR #3003 * fix(#2973): rephrase comment to avoid cline-install leaked-path lint The new comment at line 780 of profile-output.cjs literally contained the string '~/.claude/commands/gsd/' which the cline-install leaked-path regression test (tests/cline-install.test.cjs:175) correctly flagged. Cline transforms .claude/skills/ -> .cline/skills/ in installed .cjs files but does not transform .claude/commands/. The new comment talks about the legacy 'commands/gsd' subdirectory without the ~/.claude/ prefix, so the lint passes. The path semantics are unchanged -- the runtime construction at line 787 still uses path.join(os.homedir(), '.claude', 'skills', ...) which the lint regex does not match. * test(#2973): add timeout to spawnSync to prevent CI hangs (CR feedback) CodeRabbit on PR #3003: without a timeout, a regression that hangs the writer or dispatcher would block CI indefinitely. Added a 30s timeout (generous for what should complete in <1s) and an explicit signal assertion so a timeout trip surfaces as a clear test failure with context rather than a hung worker. * test(#2973): add allow-test-rule annotation for legitimate product-text parsing The new var-binding lint from #2982/#2985 caught readFileSync(...).match() and readFileSync(...).includes() calls in this test. Both are legitimate structural assertions against the product workflow markdown, not source-grep: - match() extracts the path from a structured Display: "..." line and asserts on the typed path value (same pattern as bug-2470's installer scanForLeakedPaths regex test). - includes() asserts the absence of a legacy path literal. profile-user.md IS the shipped workflow artifact, and its Display: line IS what the user sees. Per the existing test-rigor convention, this is the source-text-is-the-product justification category. Annotated with allow-test-rule citing that category. * chore(#3003): drop direct CHANGELOG.md edit; release entry now lives in .changeset/ The changeset-fragment workflow (#2975) renders fragments into CHANGELOG.md at release time. Direct edits to [Unreleased] on each PR caused merge conflicts on every concurrent PR. This commit restores CHANGELOG.md to match origin/main; the release entry for this fix is preserved in the .changeset/*.md fragment(s) on this branch, which the release workflow consolidates. * fix(#2973): preserve user-owned gsd-dev-preferences skill across wipe (CR) CodeRabbit on PR #3003 caught a real bug: copyCommandsAsClaudeSkills() wipes ALL gsd-* skill directories at the top of every install, then reinstalls from the package source. Since gsd-dev-preferences is user-generated (written by /gsd-profile-user --refresh) and NOT shipped by the npm package, the wipe deletes the user's customized SKILL.md with nothing to restore from. Fix: USER_OWNED_SKILLS allow-list in copyCommandsAsClaudeSkills. Snapshot files under skills/gsd-dev-preferences/ before the wipe, restore after. Same preserve/restore pattern as PR #1924. Tests: bug-2973 grows by 2 cases: - user-customized SKILL.md survives the wipe - non-user-owned gsd-* skills are still wiped (preservation is opt-in) |
||
|
Tom Boucher
|
9f09246f3b |
fix(#2998): populate gsd-pristine/ from install transform pipeline so verifier has a real baseline (#3004)
* fix(#2998): populate gsd-pristine/ from install transform pipeline so verifier has a real baseline saveLocalPatches declared a pristineDir variable and JSDoc'd 'saves pristine copies to gsd-pristine/' but no code ever wrote there. Effect: /gsd-reapply-patches Step 5 verifier (#2972) silently fell back to its over-broad heuristic ('every significant backup line') -- exactly the silent-success-on-lost-content failure mode #2969 was designed to prevent. Fix: new populatePristineDir({...}) helper runs copyWithPathReplacement (the install transform pipeline) into a tmp staging dir, then copies out only the modified-file paths into gsd-pristine/. saveLocalPatches now accepts a pristineCtx and calls the helper when local patches are detected. Soft-fails on transform errors (logs warning, continues with empty pristine -- no worse than pre-fix). Pristine reflects the about-to-install version's content, which is the right baseline for 'what would survive without the user's modifications'. Tests: bug-2998-pristine-dir-populated.test.cjs asserts the helper is exported, no-ops on empty input, writes one pristine file per source- existing path, skips ghost paths, and produces deterministic output (byte-identical across runs -- the property pristine_hashes depends on). Closes #2998 * chore(#2998): add changeset fragment for PR #3004 * fix(#2998): expand pristine to all manifest install roots; clear stale pristine on populate (CR) CodeRabbit on PR #3004 caught two issues: 1. populatePristineDir only staged packageSrc/get-shit-done/ but manifest.files records edits under several install roots (commands/, agents/, hooks/, skills/, root files like .clinerules). Modified paths outside get-shit-done/ were silently skipped, leaving the verifier with no baseline for those edits. Fixed by computing the set of top-level dirs from the modified set and staging each one that exists in source. Root-level files (no slash) bypass the transform pipeline and are copied directly. 2. populatePristineDir did not wipe pre-existing gsd-pristine/ before populating. A previous run's stale pristine could survive into the current run's diff baseline. Now wipe before populate AND in the catch path so soft-failures don't leave half-populated data on disk. Tests: bug-2998-pristine-dir-populated.test.cjs grows by 2 cases: - agents/ paths are staged and copied (was silently skipped pre-fix) - mixed get-shit-done/ + agents/ in same modified list both stage |
||
|
Tom Boucher
|
e9a66da1e7 |
fix(#2962): write npm-style gsd-sdk shim on Windows under --sdk install (#2971)
* fix(#2962): write npm-style gsd-sdk shim on Windows under --sdk install trySelfLinkGsdSdk previously contained `if (process.platform === 'win32') return null;` — a missed gap from #2775's POSIX self-link rather than an intentional design choice. As a result, `npx get-shit-done-cc@latest --claude --global --sdk` on Windows left `gsd-sdk` off PATH despite the installer reporting success, and the obvious recovery (`npm i -g @gsd-build/sdk`) lands the stale 0.1.0 publication that lacks the `query` subcommand the agents call ~40 times. This PR addresses the shim half. The npm-publish half (publishing @gsd-build/sdk at parity with the get-shit-done-cc version) requires maintainer credentials and is left for separate action. Changes: - bin/install.js: replace the unconditional Windows return-null with dispatch to a new trySelfLinkGsdSdkWindows() that: * resolves npm's global bin via `execFileSync('npm', ['prefix', '-g'])` (no shell interpolation; npm is the only PATH-resolved binary) * verifies write access with a probe before producing partial state * writes the standard npm shim triple to npm's global bin: - gsd-sdk.cmd (cmd.exe; CRLF line endings) - gsd-sdk.ps1 (PowerShell) - gsd-sdk (Bash wrapper for Cygwin/MSYS/Git-Bash) * each shim invokes `node "<absolute path to bin/gsd-sdk.js>"` with the passed args, decoupling shim location from SDK location — same logical structure as the POSIX wrapper-via-require() fallback above * unlinks any stale shims before writing so prior installs don't pin callers to a now-absent path * returns the .cmd path on success (handle the existing onPath check looks for) or null on any failure, falling through to the existing "gsd-sdk is not on your PATH" warning at line 8704 - tests/bug-2962-windows-sdk-shim.test.cjs (new): 5 tests exercising trySelfLinkGsdSdkWindows directly with cp.execFileSync mocked to redirect npm prefix to a temp dir. Asserts shim contents reference the absolute path, .cmd uses CRLF, stale shims are replaced not appended, and null is returned when `npm prefix -g` fails. - tests/no-unconditional-win32-skip.test.cjs (new): regression guard that fails CI if any future commit re-introduces `if (process.platform === 'win32') return null;` (or similar skip-only branches) in bin/install.js. Negative test verified by transiently re-introducing the bad pattern → guard fired → restored → passes. Out of scope: publishing @gsd-build/sdk@<current> to npm so the natural `npm i -g @gsd-build/sdk` recovery also lands a usable SDK. That requires maintainer credentials and is the second half of the issue. Closes #2962 * fix(#2962): address CodeRabbit findings — execSync for npm.cmd, behavior-based regression guard CR finding 1 (🟠 Major): Node's child_process docs explicitly call out that .cmd/.bat files cannot be spawned via execFile/execFileSync without a shell ("Spawning .bat and .cmd files on Windows" section). Since `npm` on Windows is `npm.cmd`, my use of execFileSync('npm', ['prefix', '-g'], { shell: false }) would have failed on the very platform this PR is meant to fix. Switched to cp.execSync('npm prefix -g', ...) — matching the existing convention at line ~8718 which makes the same lookup. Args are static literals so shell interpolation is not an injection vector. CR finding 2 (🟠 Major): the source-grep regression test in tests/no-unconditional-win32-skip.test.cjs violated the repo's no-source-grep testing standard (CONTRIBUTING.md). Replaced with a behavior-based test that: - overrides process.platform to 'win32' via Object.defineProperty - mocks cp.execSync to return a temp-dir as npm prefix - calls trySelfLinkGsdSdk(shimSrc) and asserts it returns non-null AND materializes gsd-sdk.cmd on disk The behavior guard is strictly stronger than the regex version: it would catch any equivalent skip pattern (e.g. os.platform() === 'win32', a typeof-based guard, etc.), not just literal `if (process.platform === 'win32')` text. Negative-tested by re-introducing the `return null` skip → test fails with maintainer-quoted diagnostic "trySelfLinkGsdSdk must not silently return null on Windows; a no-op skip is a missed-parity regression"; restored → passes. Test for Windows shim materialization (bug-2962-windows-sdk-shim.test.cjs) also updated to mock cp.execSync (matching the new production code path) instead of cp.execFileSync. Full suite: 6480/6480 pass. * test(#2962): make Windows shim tests self-contained per CR Each test now invokes trySelfLinkGsdSdkWindows() itself before reading the shim files, so they don't implicitly depend on the earlier test's side effects. Addresses CR's order-dependence finding. * test(#2962): structured shim parsing — eliminate substring source-grep CR found that even after the prior refactor, three tests in the suite still used .includes()/.startsWith() against shim file content (cmdContent.includes(\`@node ${jsonQuoted} %*\`) etc.). Substring matching on file text is the same anti-pattern the no-source-grep standard forbids — even when the file is one this test wrote — because it asserts a literal exists rather than that the structured shape is correct. Replace with three small parsers (parseCmdShim, parsePs1Invocation, parseBashInvocation) that split each shim into header + invocation tokens and assert via deepEqual on structured records. The assertions now check that the .cmd has @ECHO OFF / @SETLOCAL / @node <abs> %* in that order with exactly 3 meaningful lines, and that the .ps1 and bash wrappers split into the expected (call, nodeCmd, target, argToken) tuples. The stale-shim replacement test was hardened the same way: instead of proving the absence of a sentinel substring, it now proves the parsed target equals the new shimSrc and != the old path. Verified: scripts/lint-no-source-grep.cjs reports 0 violations across 348 test files. The 6-test windows-sdk-shim + win32-skip-guard suite all pass. * fix(#2962): expose pure shim IR + tests assert on typed fields, not rendered text Earlier "structured parser" approach (parseCmdShim / parsePs1Invocation / parseBashInvocation) was still raw-text manipulation behind a function wrapper — split('\\r\\n'), trim().split(/\\s+/), content.includes('\\r\\n'). Maintainer was right: hiding grep behind a parser is still grep. Real fix: refactor production code to expose the structured intermediate representation, and have tests assert on the IR fields directly. Production: - New buildWindowsShimTriple(shimSrc) — pure function, no fs/spawn. Returns { invocation: { interpreter, target }, eol: { cmd, ps1, sh }, fileNames: { cmd, ps1, sh }, render: { cmd: () => string, ... } }. The IR is the contract; rendered text is an implementation detail of the renderers. - trySelfLinkGsdSdkWindows now calls buildWindowsShimTriple, looks up filenames from triple.fileNames, and writes triple.render[kind]() to each target. Same observable behavior, structurally separated. - buildWindowsShimTriple added to test-mode exports. Tests (full rewrite — no shim file content is read at any point): - Layer 1: pure-IR tests assert on triple.invocation.target, triple.eol === { cmd: '\\r\\n', ps1: '\\n', sh: '\\n' }, triple.fileNames === { cmd: 'gsd-sdk.cmd', ... }, and the documented IR shape via Object.keys().sort() deepEqual. - Layer 2: fs/spawn driver tests assert filesystem FACTS: - return value equals expected path - all three target files exist as regular non-empty files - rendered file byte length === Buffer.byteLength of triple.render(kind) output (proves the writer writes what the renderer produces, no mutation, no truncation, no double-write — without comparing content) - mtime advances on rewrite (proves stale-replace behavior) - returns null when npm prefix -g throws No more split, .includes, .startsWith, .endsWith, or substring matching anywhere in the test suite. Lint clean. 10/10 tests pass. |
||
|
Tom Boucher
|
c5dfdbe42e |
fix(#2957): claude+global post-install instructs restart and skill fallback (#2960)
* fix(#2957): claude+global post-install instructs restart and skill fallback `npx get-shit-done-cc --claude --global` writes skills to `~/.claude/skills/gsd-*/SKILL.md` (CC 2.1.88+ format) and removes the legacy `~/.claude/commands/gsd/`. The post-install message still told users to type `/gsd-new-project` without mentioning the required Claude Code restart or the skill-name fallback. On configurations where CC does not auto-surface skills in the slash menu, users hit "no commands appear" and assumed the install failed. Split the post-install message: the existing single-line instruction stays for every non-Claude runtime and for `--claude --local`. For `--claude --global` it now reads: Restart Claude Code, then in any directory either type /gsd-new-project or ask Claude to run the gsd-new-project skill. This covers both invocation paths and surfaces the restart requirement. Add tests/bug-2957-claude-global-postinstall-message.test.cjs as a regression guard: captures the printed message for claude+global, claude+local, and opencode+global; asserts content for each. Verified the test fails on main (pre-fix) and passes after the fix. Closes #2957 * test(#2957): assert legacy generic instruction is replaced not extended CodeRabbit flagged that the test would still pass if the new restart/ fallback copy were printed *alongside* the old 'open a blank directory' instruction. Adding a doesNotMatch assertion proves the claude+global branch replaces the legacy line rather than appending to it. |
||
|
Tom Boucher
|
7cc6358f91 |
fix(install): honour --minimal across every runtime + manifest fix for Claude local (#2940)
* fix(install): record commands/gsd in manifest for Claude local + per-runtime --minimal coverage writeManifest gated commands/gsd/ recording to Gemini, leaving Claude Code local installs with an incomplete manifest. Audit during #2923 investigation showed every runtime adapter correctly honours --minimal on disk (6 skills, 0 agents) — but Claude local manifest reported 0 skills, breaking saveLocalPatches() drift detection and any downstream tooling that reads manifest.files for the installed surface. Drop the isGemini gate so any runtime that writes commands/gsd/ has those files hashed into the manifest. Adds tests/install-minimal-all-runtimes.test.cjs: spawns the installer end-to-end for all 14 supported runtimes in both --global and --local modes, parses the manifest JSON, and asserts mode === 'minimal', skill set equals MINIMAL_SKILL_ALLOWLIST, and zero gsd-* agents are recorded. Cross-checks the manifest against on-disk skill files. Closes #2923 * test(install): address CR feedback on bug-2923 minimal-runtime tests - Assert installer exit status in runInstall() so failing installs do not produce misleading downstream artifact assertions; include stderr in the failure message for debuggability. - Guard the on-disk vs manifest parity loop with assert.ok(manifest, ...) so the equality check cannot pass accidentally when the manifest is missing. |
||
|
Tom Boucher
|
372d3453f5 |
fix(install): tokenize before ALL_RUNTIMES_OPTION check + isolate HERMES_HOME in test
Two CodeRabbit findings on PR #2920: 1. parseRuntimeInput previously only matched the bare "16" exactly for the all-runtimes shortcut. Inputs the prompt explicitly encourages — "16,", "16 1", "1,16" — fell through to per-token parsing and silently installed only Claude or a partial subset. Move the ALL_RUNTIMES_OPTION check after tokenization so any token equal to "16" expands. Added regression coverage in tests/multi-runtime-select.test.cjs for the four mixed-input forms. 2. The "maps Hermes to ~/.hermes for global installs" test invoked getGlobalDir('hermes') without isolating HERMES_HOME. On a developer machine that exports HERMES_HOME the assertion would fail even though getGlobalDir was behaving correctly. Save/clear/restore the env var around the assertion, mirroring the pattern the later describe block already uses. Full suite: 6128/6128 pass. |
||
|
Tom Boucher
|
c9d6306981 |
fix(hermes): rewrite CLAUDE.md → HERMES.md (revert from .hermes.md per spec)
Per the issue spec for #2841 and CodeRabbit feedback on PR #2920, the project-context filename rewrite should produce HERMES.md, not .hermes.md. Reverts the earlier .hermes.md change at all 5 substitution sites in bin/install.js and updates the corresponding regression test in tests/hermes-install.test.cjs to assert HERMES.md. Full suite: 6127/6127 pass. |