530 Commits

Author	SHA1	Message	Date
Tom Boucher	e32a53b974	feat(113): detect javascript:/data:/userinfo/token-in-query in markdown links (#133) ... * test(113): add per-rule failing tests + hostile fixture for markdown link payloads RED phase for issue #113 — scanForInjection() currently returns { clean: true } for markdown links containing javascript:, data:text/html, userinfo credentials, and token-in-query payloads. Changes: - tests/fixtures/adversarial/security/context-malicious-markdown-link.md: Extended to contain one hostile example per rule class (MD-LINK-JS-SCHEME, MD-LINK-DATA-SCHEME, MD-LINK-USERINFO, MD-LINK-TOKEN-IN-QUERY) plus benign negative controls (data:image/png, mailto:, https://github.com, port-only URL). - tests/security-prompt-injection.test.cjs: - Flipped PINNED "malicious-markdown-link fixture is NOT flagged" assertion to "malicious-markdown-link fixture is flagged by scanner" (forward-looking). - Added 4×positive + 4×negative per-rule unit tests asserting structuredFindings with ruleId, file, line, match fields. - Added parity guard: every MARKDOWN_LINK_PATTERNS source string from security.cjs must appear in gsd-read-injection-scanner.js hook source. D3 false-positive grep: 0 legitimate matches — no allowlist entries needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(113): detect javascript:/data:/userinfo/token-in-query in markdown links (security.cjs + hook) GREEN phase for issue #113. Rule details (all with primary source citations): MD-LINK-JS-SCHEME Flags ](javascript:...) regardless of case. Source: OWASP XSS Prevention Cheat Sheet https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html MD-LINK-DATA-SCHEME Flags data: URIs NOT in the explicit safe-list. Safe-list: image/(png|jpeg|gif|webp|bmp|ico|avif|heic) and font/(woff2?|otf|ttf). data:image/svg+xml is intentionally BLOCKED — SVG can host <script>. Source: OWASP File Upload Cheat Sheet — SVG Files https://cheatsheetseries.owasp.org/cheatsheets/File_Upload_Cheat_Sheet.html#svg-files MD-LINK-USERINFO Flags https?://user:pass@host in markdown link targets. Does NOT fire on: mailto:user@host (no :// before user) or https://host:443/path (port, not userinfo). Source: RFC 3986 §3.2.1 (userinfo syntax) https://www.rfc-editor.org/rfc/rfc3986#section-3.2.1 RFC 9110 §4.2.4 (HTTP deprecates userinfo) https://www.rfc-editor.org/rfc/rfc9110#section-4.2.4 MD-LINK-TOKEN-IN-QUERY Flags key NAMES: token, access_token, id_token, refresh_token, api_key, apikey, secret, password, client_secret, code — regardless of value. Source: RFC 9700 OAuth 2.0 Security BCP §4.3.1 https://www.rfc-editor.org/rfc/rfc9700#section-4.3.1 D3 false-positive grep: 0 legitimate matches in codebase — no allowlist needed. Architecture: - scripts/security.cjs: canonical MARKDOWN_LINK_PATTERNS export, scanForInjection() extended with structuredFindings (ruleId, file, line, match) via opts.file. - hooks/gsd-read-injection-scanner.js: patterns inlined for hook independence (same pattern sources, verified by parity test). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(113): flip PINNED malicious-markdown-link assertion and add parity guard REFACTOR phase — tightening test rigor after test-rigor skill review: 1. Fixture assertion now enumerates all 4 expected ruleIds explicitly: [MD-LINK-JS-SCHEME, MD-LINK-DATA-SCHEME, MD-LINK-USERINFO, MD-LINK-TOKEN-IN-QUERY]. Previously findings.length > 0 would pass even if 3 of 4 rules were broken. 2. line field assertions tightened: `f.line >= 1` (meaningful lower bound for 1-based line numbers) instead of `typeof f.line === 'number'` (vacuous). 3. match field assertions tightened to check the hostile content is present: - MD-LINK-JS-SCHEME: /javascript:/i in match - MD-LINK-DATA-SCHEME: /data:/i in match - MD-LINK-USERINFO: /@/ in match (the @ character is the definitive userinfo marker) - MD-LINK-TOKEN-IN-QUERY: /token=/i in match 4. Parity test checks actual RegExp .source strings (not just lengths), verifying the hook contains the exact canonical pattern sources character-for-character. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#113): add changeset fragment + Windows/Node 24 state.test compatibility 1. .changeset/113-malicious-markdown-links.md — required Security fragment for the user-facing markdown-link scanner changes in this PR (changeset-lint was failing with FAIL_MISSING_FRAGMENT). 2. get-shit-done/bin/lib/state-command-router.cjs — add OUTPUT_ON_SDK_ERROR set for mutation state subcommands whose CJS contract is always exit-0. On Windows/Node 24 the SDK bridge returns result.ok===false for validation failures (e.g. state record-metric --phase 1 with no --plan/--duration), causing dispatchViaSdk() to call error() (exit 1) instead of output({error}) (exit 0). The fix maps SDK non-ok results to JSON output for the affected mutation commands (record-metric, advance-plan, record-session, add-decision, add-blocker, resolve-blocker, update-progress), restoring the exit-0 CJS contract on all platforms. tests/state.test.cjs:1161 "returns error when required fields missing" passes locally (104/104 pass). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-23 10:15:03 -04:00
Tom Boucher	8b6ffca51f	fix(3785): case-insensitive depends_on resolution in phase resolver (#88) ... * fix(3785): case-insensitive depends_on resolution in phase resolver planMap, canonicalToId, and shortFormToId in phasePlanIndex used strict Map.has() with no case normalization. A depends_on ref in mixed/lowercase against an uppercase-suffix plan ID (e.g. '20-01-auth' → '20-01-Auth') dropped the DAG edge, assigning the dependent plan to wave 1 instead of wave 2. Fix: normalize all keys and lookup values to lowercase so the three-tier resolution is case-insensitive. Adds regression test (#3785). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(changeset): add PR 3798 changelog fragment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3785): detect case-fold collisions; apply lowercase-both to CJS path - Add collision guard in both sdk/src/query/phase.ts (phasePlanIndex) and get-shit-done/bin/lib/phase.cjs (cmdPhasePlanIndex): when two plan IDs in the same phase are identical after toLowerCase(), throw/error immediately with a clear message naming both files instead of silently overwriting one in planMap and misrouting depends_on edges. - Apply the same lowercase-both normalization (#3785) to cmdPhasePlanIndex in phase.cjs, which was missing from the original PR — planMap and canonicalToId keys are now lowercased on write; dep strings are lowercased before lookup. - Add regression tests to tests/phase.test.cjs: case-insensitive resolution test (runs on all platforms) and collision-detection test (skipped on macOS/Windows where FS is case-insensitive). - Update changeset to describe both the SDK and CJS fixes. Identified via Codex adversarial review of PR #3798. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3785): address review — KNOWN GAP comment for CJS shortFormToId, canonical-casing tests, depends_on output normalization - F1: Reword changeset for accuracy (plannerID drift trigger; two-tier CJS gap honest). Add KNOWN GAP comment in phase.cjs before Kahn's loop noting CJS lacks shortFormToId (tracked as follow-up parity gap, out of scope for #3785). - F2: Add strict planA.id === '20-01-Auth' assertions in both SDK (vitest) and CJS (node --test) tests — a future regression that silently lowercases stored IDs would now fail the test. - F3: Normalize depends_on output to canonical plan IDs in both SDK phase.ts and CJS phase.cjs. User-typed '20-01-auth' in depends_on resolves to '20-01-Auth' in output via planMap lookup. Add planB.depends_on === ['20-01-Auth'] assertions in both test suites. - F5: Add seenLower guard-scope comment (full-ID collisions only; shared-prefix collisions handled by first-write-wins from sorted planFiles). - F6: Add ASCII-safe toLowerCase comment at first call site in both SDK and CJS. - F7: Add intentional-separation comment on seenLower vs planMap in both SDK and CJS. Reviewers: gsd-code-reviewer (MN-01/NT-1) + sonnet adversarial. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(3785): cover case-insensitive depends_on resolution branches Add 4 focused test cases exercising branches introduced by #3785: - All-uppercase depends_on ref resolving to lowercase plan ID via planMap - External cross-phase dep preserved as-is in Pass 3 output (planMap miss) - Mixed-case short canonical prefix resolving via canonicalToId - Plans with undefined/empty depends_on emit empty array correctly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-22 16:54:10 -04:00
Tom Boucher	334a64168e	chore(npm): rebrand packages to @opengsd scope (#127) ... * chore(npm): rebrand packages to @opengsd scope Rename: - get-shit-done-redux → @opengsd/get-shit-done-redux - @gsd-redux/sdk → @opengsd/gsd-sdk Add publishConfig.access=public for first-time scoped publish. CLI binary names (get-shit-done-redux, gsd-sdk, gsd-tools) unchanged. Sweeps install commands, npx invocations, CI publish/version-check workflows, tests, docs, READMEs (all translations), and the PACKAGE_NAME constant in check-latest-version. Bumps qs 6.15.1 → 6.15.2 to clear a moderate advisory surfaced by the audit-clean test (GHSA-q8mj-m7cp-5q26). Closes #126 * chore: pin 2.0.0 release + remove canary workflow - Bump both packages 1.50.0-canary.0 → 2.0.0 for first @opengsd publish - Remove .github/workflows/canary.yml and canary dist-tag handling in release.yml / release-sdk.yml - Drop canary section from VERSIONING.md Refs #126 * chore: address review findings + harden tarball-smoke timeout - .changeset/opengsd-org-rename.md: match project's custom parse.cjs frontmatter (type: Changed / pr: 127); the scoped @changesets/cli keys were silently rejected. - CONTEXT.md: drop two canary-stream policy lines and a dangling DEFECT.CANARY-VERSION-LEAK.cross-ref now that canary.yml is gone. - tests/release-tarball-smoke.install.test.cjs: pass timeout: 600_000 for npm pack + global install; the 3-minute runNpm default was timing out on slower Docker hosts (cartographer). Refs #126 * fix(sdk): add missing type/runtime devDependencies for build prepublishOnly invokes tsc which couldn't resolve @types/node, @types/ws, or synckit. They had been hoisted from root but were not declared in sdk/'s own package.json — first publish from a clean SDK tree failed. Refs #126 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ci): use npm pack stdout instead of glob to find tarball `npm pack --silent` for a scoped package (@opengsd/get-shit-done-redux) produces `opengsd-get-shit-done-redux-*.tgz`, not `get-shit-done-redux-*.tgz`. Capture the filename from stdout instead of a hardcoded glob so the step works regardless of package name format. Fixes smoke (ubuntu-latest, 22, false) CI failure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * ci: treat workflow-file changes as test-skip eligible `.github/workflows/install-smoke.yml` (and other workflow files) were in neither `test.yml` paths nor `test-skip.yml` paths-ignore, so neither workflow ran on a workflow-only commit — leaving the required test-skip check perpetually missing. Refs #126 * chore: reset version to 1.0.0 for first @opengsd publish Nothing has been published yet under the @opengsd scope, so the inaugural release uses 1.0.0 rather than 2.0.0. The "major bump" in the changeset reflects the breaking install-command change for users migrating from the prior unscoped `get-shit-done-redux`, not a numeric continuation from a 1.x line under the new identity. Refs #126 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-22 16:22:41 -04:00
Tom Boucher	76dd22deed	fix(3774): treat 999 as exact sentinel in phase-lifecycle-policy (#93) ... * fix(3774): treat 999 as exact sentinel, not lower bound, in phase-lifecycle-policy scanSequentialMaxPhaseFromMilestone and scanSequentialMaxPhaseFromDirs used `num >= 999` to skip the backlog lane, but this incorrectly excluded every phase ≥ 1000, causing computeNextSequentialPhaseId to return 1 for projects using canonical phase IDs in the 1000+ range. Change both guards to `num === 999` so only the backlog sentinel is skipped. Adds regression test: project with phases 1000–1500 must produce 1501, not 1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: add changeset for fix #3792 (phase.add returns 1 on 1000+ projects) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3774): address review — fix 4 CJS scanner twins + tighten regression test Addresses gsd-code-reviewer BLOCKER (4 CJS scanner twins in phase.cjs:610,624,688,698 still carried >= 999, reachable via GSD_WORKSTREAM / absent SDK build) and MAJOR (regression test couldn't distinguish === 999 from === 1000 — added [999, 1000] fixture asserting result === 1001). Decrement helpers at :893, :922, :930, :936 left unchanged — intentional 999-lane protection per dual-review analysis. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-22 14:54:27 -04:00
Tom Boucher	2a915c1b82	chore: migrate references from gsd-build to open-gsd/get-shit-done-redux (#120) (#121) ... Security-motivated migration of all stale repository and npm-scope references. Three categories of changes (58 files, 174 substitutions): 1. gsd-build → open-gsd (security-critical): - .github/workflows/release-sdk.yml — npm token comment, tarball filename pattern - .github/workflows/hotfix.yml — same - .changeset/fix-3406-detect-stale-sdk-shadow.md — @gsd-build/sdk → @open-gsd/sdk - .changeset/sharp-quails-leap.md — same - get-shit-done/workflows/update.md — CHANGELOG raw GitHub URL 2. GSD-redux org slug → open-gsd (canonical rename): - package.json + sdk/package.json — repository/homepage/bugs metadata - All README.*.md — live badge and link sections - CONTRIBUTING.md, CONTEXT.md, QUICK-WINS-CONFIRMED-BUGS.md - .coderabbit.yaml, .release-monitor.sh, scripts/sync-rulesets.sh - docs/** — all live agent/ADR/user-facing documentation - tests/** — repo slug assertions and test fixtures - scripts/changeset/cli.cjs + github-release-notes.cjs - .github/ISSUE_TEMPLATE/*, .github/pull_request_template.md - bin/install.js, get-shit-done/bin/lib/model-catalog.cjs - sdk/HANDOVER-*.md, sdk/src/*.test.ts 3. CLAUDE.md (gitignored local file — not in this commit): Updated separately outside git: --repo gsd-build/get-shit-done → --repo open-gsd/get-shit-done-redux with security warning. Intentionally unchanged: CHANGELOG.md, docs/RELEASE-*.md, .changeset/README.md, .changeset/build-hooks-atomic-write.md, README.md migration table (historical fork record), tests/changeset-serialize.test.cjs line 78 (serialization fixture). The gsd-build/get-shit-done repo is compromised (rug-pull documented in README.md). Do not push to or interact with that repo. Closes #120	2026-05-22 12:28:16 -04:00
Tom Boucher	8d1788020a	fix(3691): address review — drop no-op Bug 2 change, anchor Plans regex, guard leading-dot IDs ... Addresses gsd-code-reviewer (Bug 2 no-op proven empirically; unanchored Plans regex) and sonnet adversarial (leading-dot silent wave-1 default; multi-decimal + bare-bold test gaps). - F1: Drop "Bug 2" nextPhaseOffset regex change (\d[\d.]* → \d): confirmed no-op by reverting and verifying all 7 existing tests still pass — phase headings always start with a digit so \d already matches decimal phases like 02.3. - F2: Anchor plansBlockMatch to start-of-line via (?:^|\n) prefix so mid-line occurrences like `***Plans:***` in prose or `OpenPlans:` prefixes do not produce false matches. - F3: Add leading-dot plan ID validation guard before planData.find() — malformed IDs that fail /^\w[\w.-]*$/ are skipped rather than silently defaulting to wave 1. - F4: Add adversarial test cases for 001.10-PLAN.md (multi-decimal leading-zero ID) and **Plans:** bare-bold (no trailing text); delete vacuous Bug 2 describe block. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-22 11:53:40 -04:00
Tom Boucher	69a3427189	fix(3691): match all Plans-block variants in annotate-dependencies ... Three regex defects in cmdRoadmapAnnotateDependencies (roadmap.cjs): 1. Plans-block detection (line ~553): `Plans:\s*\n` required no text after the colon, silently skipping `Plans: 3 plans\n` and `**Plans:** N\n`. Fixed: `\*{0,2}Plans\*{0,2}:[^\n]*\n` + require `+` checklist lines so a bold summary line above a bare `Plans:` block doesn't consume the match. 2. Phase-section boundary (line ~542): `\d` matched only one digit, so `### Phase 02.3:` was not recognised as a section terminator, allowing plan-list content from adjacent decimal phases to bleed in. Fixed with `\d[\d.]*`. Same one-digit boundary also patched in roadmap.cjs (analyze path), phase.cjs (insert-after path), and init.cjs (section-slice path). 3. Plan-ID extraction (line ~566): `[\w-]+?` excluded `.`, capturing `02` from `02.3-01-PLAN.md` instead of `02.3-01`, so planData.find never resolved and every plan defaulted to wave 1. Fixed: `[\w.-]+?`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-22 11:53:40 -04:00
Colin Johnson	6f123778d2	Merge pull request #94 from open-gsd/feat/phase-uat-passed-3184 ... feat(sdk): isPhaseUatPassed predicate + phase.uat-passed query (#3184)	2026-05-22 11:49:33 -04:00
Tom Boucher	4a19d4db2b	fix(3815): phase.insert handles checked-bullet ROADMAP format (#79) ... * fix(3815): phase.insert parser handles checked-bullet ROADMAP format phaseInsert (TS) and cmdPhaseInsert (CJS) previously used a heading-only regex (#{2,4}\s*Phase\s+N:) to locate the target phase. On projects whose ROADMAP uses the checked-bullet format (- [ ] **Phase N: name** or - [ ] Phase N: name), the lookup always failed with "Phase N not found". Extend the locator to also accept the bullet form — mirroring the patterns already used by phaseRemove and phaseComplete. When bullet-style is detected, insert a new bullet entry after the matched line (preserving bold/plain style to match surrounding entries). The heading-style code path is unchanged. Also fix a pre-existing test timeout: the first registry-integration test in phase-lifecycle.test.ts was failing with STACK_TRACE_ERROR (masked timeout) because the cold import of index.js takes >5 s. Added { timeout: 30_000 }. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3815): refine hybrid-ROADMAP detection, preserve #3098 parity Tighten the bullet-style branch guard: only treat a ROADMAP as bullet-style (and apply the bullet-insert path) when it contains ZERO heading-style phase entries (anyHeadingPattern test). A mixed (hybrid) ROADMAP — headings for some phases, bullet summaries for others — is the #3098 case where the detail section is absent; that path must still error with "missing a detail section". Adds a regression test (#3098 preserved) in both TS and CJS to confirm that a heading-style ROADMAP with a bullet-only entry for the target phase still fires the "missing a detail section" error, not the bullet-insert path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: add changeset for #3815 phase.insert bullet-roadmap fix Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-22 11:39:45 -04:00
Tom Boucher	b647f44eb0	fix(3806): port W005/W006/I001 fixes from validate.ts to verify.cjs (#83) ... PR #3479 fixed three false-positive classes in sdk/src/query/validate.ts but the fixes never propagated to get-shit-done/bin/lib/verify.cjs — the hand-maintained CJS runtime bundle that gsd-tools.cjs actually executes. W005: widened phase-dir regex from \d{2} to \d{2,} so 3+-digit prefixes like 999.1-foo are accepted. W006: adds forEachArchivedPhaseToken call after collectDiskPhases so phases whose directories live in a milestone archive are not flagged as missing. I001: adds canonicalPlanStem helper and uses it in summaryBases Set construction so 68-01-scaffolding-PLAN.md correctly matches 68-01-SUMMARY.md. Adds five regression tests (TDD red→green) covering each false-positive path. Fixes #3806 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-22 11:23:50 -04:00
Tom Boucher	cda3d7a5ab	fix(3804): worktree.cleanup-wave rescues uncommitted SUMMARY.md (#81) ... * fix(3804): rescue uncommitted SUMMARY.md in executeWorktreeWaveCleanupPlan Ports the shell-fallback SUMMARY rescue logic from quick.md into executeWorktreeWaveCleanupPlan. Before the dirty-state check, all *SUMMARY.md files under <worktree>/.planning/ are copied to the main tree (if absent or divergent), then filtered out of the git-status porcelain output. A worktree whose only dirty file is the executor's uncommitted SUMMARY.md now proceeds to merge+remove instead of returning cleanup_blocked/worktree_dirty. Adds two TDD tests (#3804): - Rescue-only dirty state (SUMMARY.md alone) → cleanup succeeds - SUMMARY + non-SUMMARY dirty files → cleanup still blocks Refs: #2296, #2070, #2838, #3804 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3804): normalize relPath to forward slashes for Windows porcelain match On Windows, `path.join` produces backslash separators while `git status --porcelain` always emits forward slashes. The rescued-paths Set would never match porcelain output, causing the dirty-check filter to ignore SUMMARY rescue and block cleanup on Windows. Also normalize the test assertion for `rescued[0].dest` to use forward slashes so the test passes on both platforms. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-22 11:23:34 -04:00
Tom Boucher	dff176bfd2	chore: rebrand to GSD-redux/get-shit-done-redux ... Mirror of code, issues, and PRs from the upstream gsd-build/get-shit-done, which appears compromised or abandoned (maintainer unreachable since 2026-04-01; $GSD token linked to rug-pull). - Adds rebrand notice block at top of English README - Removes $GSD token badge and @gsd_foundation X badge (keeps Discord) - Renames npm packages: get-shit-done-cc -> get-shit-done-redux, @gsd-build/sdk -> @gsd-redux/sdk - Updates all repo URLs across docs, workflows, package.json, bin/ - Updates ci@gsd-build -> ci@gsd-redux in workflow git identities - Leaves CHANGELOG and .changeset/* alone (historical, time-stamped)	2026-05-22 08:27:07 -04:00
Tom Boucher	b533f71857	chore: introduce CommandRoutingHub and migrate phase-command-router (PoC) (#3828) ... * feat(routing): add CommandRoutingHub with behavioral test suite (#3788) Introduces createHub({ mode, sdkLoader, cjsRegistry, manifest }) and hub.dispatch({ family, subcommand, args, cwd, raw }) -> Result with a closed 6-value ERROR_KINDS frozen enum. Hub never throws, never prints, and enforces no transparent fallback between sdk/cjs modes. 34 behavioral tests cover all errorKind values, mode fixation, and the no-throw contract. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(routing): migrate phase-command-router to CommandRoutingHub (#3788) Rewrites phase-command-router.cjs to dispatch through CommandRoutingHub. Public entry point routePhaseCommand({ phase, args, cwd, raw, error }) is unchanged. The adapter determines mode (sdk/cjs) from env + tryLoadSdk(), constructs a hub, dispatches, and translates the pure Result back to output()/error() calls. New behavioral test suite (23 tests) replaces the old mock-heavy approach and includes two integration tests through the real hub. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(routing): ADR + glossary + changeset for CommandRoutingHub (#3788) Adds ADR-3788 documenting the hub's design contract (pure result, fixed mode, closed 6-value errorKind enum, no transparent fallback). Adds Command Routing Hub glossary entry to CONTEXT.md and a one-paragraph reference to ARCHITECTURE.md. Changeset fragment records the Changed entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(docs): rename ADR to sequential convention 0012 (#3788) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(inventory): register CommandRoutingHub in INVENTORY (#3788) Add command-routing-hub.cjs row to docs/INVENTORY.md CLI Modules table, bump headline count from 72 to 73, and regenerate INVENTORY-MANIFEST.json via scripts/gen-inventory-manifest.cjs --write. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(adr): add 0012 to ADR index (#3788) Add entry for 0012-command-routing-hub.md to the index table in docs/adr/README.md so the enh-3271-sdk-adr-structure lint passes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(lint): bump phase test-file ceiling to accommodate command-router suite (#3788) phase-command-router.test.cjs added by the CommandRoutingHub migration pushes the phase prefix cluster from 4 to 5 test files. Bump the allowlist ceiling from 4 to 5 (issue 3788) so lint-test-file-count passes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(routing): preserve phase.mvp-mode JSON error and ROADMAP scan through hub (#3788) mvp-mode was never registered in the SDK; the pre-#3788 CJS router always dispatched it via the CJS handler even when sdkAvailable was true. After the hub migration, SDK-mode hubs (Docker, where the SDK build exists) sent mvp-mode to the SDK bridge, which returned SdkDispatchFailed with reason 'unknown' instead of the expected 'usage' code, and failed ROADMAP lookups. Fix by short-circuiting mvp-mode to the CJS handler before hub construction, matching the pre-migration observable behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(adr): note SDK-incomplete subcommand limitation in ADR-0012 (#3788) * fix(inventory): bump CLI Modules headline to 74 after rebase onto main (#3788) Upstream added code-review-flags.cjs (72→73) at the same time our branch added command-routing-hub.cjs. After rebase both modules exist (74 total) but the headline stayed at 73; bump to 74. * fix(routing): remove dead mvp-mode handler from cjsRegistry (#3788) The cjsRegistry['phase']['mvp-mode'] handler (previously lines 65–68) was unreachable: the early-return bypass at line 56 intercepts mvp-mode before hub construction in CJS mode, and in SDK mode cjsRegistry is passed as undefined. Remove the dead handler; all 57 tests still pass. * docs(adr): correct router count in ADR-0012 (#3788) The context section cited "eight" routers including "frontmatter" but there is no frontmatter-command-router.cjs. The actual count is seven: phase, phases, roadmap, state, verify, validate, init. * fix(routing): guard missing subcommand + use ERROR_KINDS constant (#3788) Two fixes in phase-command-router.cjs: 1. Add early-return for missing subcommand before hub construction. Pre-#3788 the routeCjsCommandFamily fell through to error() for undefined args[1]; post-#3788 the hub's manifest check skips falsy subcommands, which would have sent bare 'phase' into SDK dispatch in SDK mode instead of the expected "Available: ..." error message. 2. Switch on ERROR_KINDS.UnknownCommand instead of bare 'UnknownCommand' string, per ADR-0012's closed-enum contract ("callers switch on ERROR_KINDS values, not bare string literals"). * docs(routing): fix factual errors in ARCHITECTURE, ADR-0012, changeset (#3788) Three corrections: 1. ARCHITECTURE.md: softened "All CJS command family routers dispatch through CommandRoutingHub" — only phase-command-router.cjs is migrated in this PR; remaining routers still use routeCjsCommandFamily and migrate in follow-up issues. 2. ADR-0012: corrected the SDK mvp-mode claim. The ADR said "the SDK has no equivalent entry" but sdk/src/query/command-static-catalog- domain.ts:104-105 registers phase.mvp-mode. The actual reason for the early-return bypass is divergent ROADMAP scan behaviour and error reason codes, not SDK absence. 3. .changeset/mellow-tigers-gather.md: corrected pr: 1 → pr: 3828. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-21 23:32:10 -04:00
Tom Boucher	2d3027767b	fix(3426): Codex Windows hooks use .cmd shim to avoid POSIX exec fail (#3768) ... * test(#3426): add RED test for Codex Windows hooks .cmd shim requirement Drive buildCodexHookWindowsShimIR (typed IR) + ensureCodexHooksJsonSessionStart integration against mocked win32 platform. Counter-tests confirm darwin/linux paths remain unchanged. NOTE: Windows wall-clock verification depends on Docker matrix Windows runners. Local test exercises the generator IR shape only. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#3426): Codex Windows hooks use .cmd shim to avoid bash.exe POSIX-exec failure Root cause: Codex on Windows runs hook commands from PowerShell/cmd. The previous hooks.json command format was `"node.exe" "script.js"`. Codex's hook-dispatch shell (Git Bash / MSYS) tried to POSIX-exec node.exe (a Windows PE binary) via execvp(), which fails with ENOEXEC — reported as `bash.exe: cannot execute binary file`. Fix: `ensureCodexHooksJsonSessionStart` now calls `buildCodexHookWindowsShimIR` on win32 to write a .cmd shim alongside the .js hook file. cmd.exe executes .cmd files natively via CreateProcess, bypassing the POSIX exec layer entirely. Non-Windows paths (darwin, linux) are unchanged: they continue to use the node-runner command. Also adds `gsd-check-update.cmd` to the codex-hooks-json managed-basename set so reconcileCodexHooksJsonSessionStart correctly replaces stale node-runner entries on reinstall. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(3426): update changeset to reference PR #3768 * fix(3426): fail-loud on Codex Windows shim-write failure instead of silently restoring broken command Replace the silent fallback to `projectManagedHookCommand` (the old `node.exe script.js` form) with an explicit warn-and-skip path. When `atomicWriteFileSync` fails to write the `.cmd` shim, the previous code silently called `reconcileCodexHooksJsonSessionStart` with the legacy node-runner command. That command triggers the exact `bash.exe: cannot execute binary file` POSIX-exec failure that #3426 exists to fix — so a successful-looking install was secretly restoring the original bug. New behaviour: - Emit `console.warn` with the failure reason and a remediation hint, matching the `${yellow}⚠${reset} Skipped …` idiom used at line 9098. - Return `{ changed: false, wrote: false }` to skip registration for this runtime entirely, so the outer caller can surface "NOT installed" instead of "installed (but broken)". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(3426): typed-IR assertions on .cmd shim eol/quoting/passthrough + IR extension Extend `buildCodexHookWindowsShimIR` to expose two new typed fields on the returned IR object (CONTRIBUTING.md L558-L565 IR-first discipline): eol: { cmd: '\r\n' } — CRLF is canonical for cmd.exe .cmd files passthroughArgs: true — shim forwards all args via %* Add a new describe block (Step 2b) with three IR-level assertions: 1. `eol.cmd === '\r\n'` — prevents silent EOL regression that could break parsing on Windows versions that require CRLF. 2. `invocation.target` is the raw unquoted path (no shell-metachar leakage) — quoting happens only at render time. 3. `passthroughArgs === true` — the %* forwarding contract is explicitly typed so regressions fail before the text is rendered. All assertions operate on the typed IR returned by the generator, NOT on the rendered `.cmd` file content — text-matching is the anti-pattern CONTRIBUTING.md L522-582 prohibits. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(3426): fix Windows CI failures — update hook-command filter patterns Four test files filtered for managed hooks in hooks.json using the literal string `gsd-check-update.js`. On Windows the PR-introduced .cmd shim changes the hooks.json command to `"path/gsd-check-update.cmd"` (no node prefix, .cmd extension), so those filters matched 0 entries and 24 Windows subtests failed. Fixes: - bug-2760-codex-install-defensive.test.cjs (7 filters): change `/gsd-check-update\.js/` → `/gsd-check-update/` to match both .js (POSIX) and .cmd (Windows) commands. - bug-3357-codex-legacy-hooks-json-migration.test.cjs (3 filters): same `.js` → no-extension change. - bug-3427-3433-codex-install-shape.test.cjs (2 filters): same fix; add explanatory comment to uninstall assertion. - codex-config.test.cjs (9 filters + 1 exact-command assertion): bulk-replace all `hooksJsonCommands.filter(cmd => cmd.includes('gsd-check-update.js'))` with `gsd-check-update`; make the `fresh CODEX_HOME` test platform- aware — on win32 assert `.cmd` shim path, on POSIX assert the existing `"runner" "script.js"` form (#3017). All four suites pass locally (macOS / darwin). Windows subtests verified against the Windows CI failure log patterns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(3426): address pr-review-toolkit + codex review findings - fix(uninstall): add gsd-check-update.cmd to gsdHooks cleanup list so the .cmd shim is removed from disk on Windows uninstall (was left as orphan artifact — silent failure post-uninstall) - test(3426): add uninstall test asserting gsd-check-update.cmd is deleted from hooks dir after `uninstall(true, 'codex')` (no coverage existed) - fix(comment): correct JSDoc on buildCodexHookWindowsShimIR — shim content is three-line @ECHO OFF/@SETLOCAL/@runner snippet, not bare `@node "script.js" %*` as the old comment claimed - fix(comment): update stale assertion message in codex-config.test.cjs L1457 — said "config.toml references it" but the hook is in hooks.json Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-20 23:13:50 -04:00
Tom Boucher	99b52a302a	fix(3659): applySurface prunes skill dirs on cluster disable (#3766) ... * test(3659): add regression tests for applySurface skill-dir pruning on cluster disable Tests that applySurface with claude global scope correctly prunes ~/.claude/skills/gsd-STEM/ dirs for disabled clusters, preserves gsd-STEM dirs in enabled clusters, leaves non-gsd user dirs untouched, and is idempotent across two consecutive calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3659): applySurface now prunes ~/.claude/skills/gsd-STEM/ on cluster disable Root cause: surface.md directed the AI to use RUNTIME_CONFIG_DIR=~/.claude/skills (the skills sub-directory) instead of the base Claude config dir (~/.claude). When runtimeConfigDir=~/.claude/skills and scope=global, the layout computes dest=~/.claude/skills/skills — the wrong target — so pruning never reached the actual gsd-STEM dirs in ~/.claude/skills/. Fix: - surface.md: correct RUNTIME_CONFIG_DIR to use the base config dir (~/.claude), add explicit SCOPE=global, and update all path references in execution_context. Surface state file moves from ~/.claude/skills/.gsd-surface.json to ~/.claude/.gsd-surface.json, matching install/uninstall conventions. - surface.cjs: extract pruneSkillDirs() as a shared helper (single point of truth for gsd-STEM dir removal). _syncGsdDir now delegates to it instead of having the ownership/prune logic inline. Export pruneSkillDirs for callers that need stand-alone pruning without a full applySurface pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(3659): update changeset to reference PR #3766 * fix(3659): manifest-membership gate on pruneSkillDirs prevents user gsd-* dir data loss Finding 1 (CRITICAL): the prefixed branch previously deleted any on-disk dir that matched the 'gsd-' prefix and was not in retainedNames. A user-created gsd-mything/ would be silently destroyed. Fix: deletion now requires BOTH prefix match AND manifest membership (stem present in manifest). Dirs that match the prefix but are not manifest-known are preserved with a process.stderr warning so the user knows the dir was kept. Finding 2 (type guard): the Hermes (empty-prefix) branch passed manifest directly to new Set([...manifest.keys()]) without verifying it is actually a Map. A truthy non-Map would throw. Fix: safeManifest = (manifest instanceof Map) ? manifest : null, used in both branches. Non-Map manifest triggers the same conservative no-deletions path already used when manifest is absent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(3659): counter-test for all-clusters-disabled + user gsd-* dir preservation Finding 3: add test (e) that disables every cluster (Object.keys(CLUSTERS)) and asserts three things: 1. All GSD-owned skill dirs (gsd-explore/, gsd-help/) are removed. 2. Non-gsd user dir (my-custom-skill/) is preserved. 3. User-created gsd-mything/ (prefix match, not in manifest) is preserved — this is the critical regression guard for the Finding 1 data-loss fix. Also update the existing _syncGsdDir skills-kind test in surface-apply.test.cjs to pass a manifest that declares old-skill as GSD-owned. Without a manifest the new conservative path correctly preserves all unknown gsd-* dirs, which broke the pre-existing no-manifest assertion; supplying the manifest restores the expected pruning behavior and documents the required calling contract. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3659): address pr-review-toolkit + codex review findings - Collapse redundant if/else in _syncGsdDir - Collapse duplicate canonicalStems branches in pruneSkillDirs - Update stale module-header comment (config-dir root) - Clarify dead isGsdOwned guard comment - Log rmSync failures to stderr - Add pruneSkillDirs to module-header Exports JSDoc - Remove unused imports in bug-3659 test file Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-20 23:12:44 -04:00
Tom Boucher	172e6920eb	fix(3670): break migration lock self-deadlock on Windows (#3765) ... * test(installer): add regression tests for #3670 migration lock self-deadlock - T1: same-process PID re-entry reclamation (primary regression) - T2: dead-PID stale lock reclamation - T3: unlinkSync EPERM surfaces (not silently swallowed via force:true) - T4: counter-test — normal round-trip still works - T5: counter-test — genuinely-held live lock still errors clearly - Update existing 'reports lock release failures' test to mock fs.unlinkSync (not fs.rmSync) matching the fixed release path Windows wall-clock deadlock repro depends on Docker matrix Windows runners. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(installer): break migration lock self-deadlock on Windows (#3670) Root cause: `acquireInstallMigrationLock` release closure called `fs.rmSync(lockPath, { force: true })`. On Windows NTFS, a file recently closed via `closeSync(fd)` may still return EPERM from `unlink` until the OS fully releases the handle. The `force: true` flag silently swallows EPERM, leaving the lock file on disk. The subsequent `runInstallerMigrations` call in the same install() invocation hits EEXIST, spins for 30 s, then throws "installer migration lock is held". Fix: 1. Release closure uses `fs.unlinkSync` (not rmSync+force) so EPERM propagates via releaseError instead of being swallowed. 2. `acquireInstallMigrationLock` closes the fd before writing the payload (path-based write), eliminating the open handle that caused the deferred EPERM on Windows. 3. Stale-lock reclamation: on EEXIST, parse the on-disk PID and reclaim immediately if it matches process.pid (same-process re-entry, the primary #3670 failure mode) or if the PID is dead (ESRCH). Live alien PIDs still trigger the 30 s timeout. 4. Error message on a genuinely-held lock now includes the holder PID and acquiredAt timestamp for operator diagnostics. No public API change. All callers of runInstallerMigrations are inside installer-migrations.cjs and bin/install.js. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(3670): update changeset to reference PR #3765 * fix(3670): timeout on reclaim-unlink failure to prevent spin-loop regression When unlinkSync throws (e.g. Windows EPERM on an open handle) in the same-PID / dead-PID reclamation path, the original code continued unconditionally — bypassing the timeout check and reintroducing the exact deadlock the PR is supposed to fix. Guard the continue behind a `reclaimed` flag: only loop back to openSync if unlink SUCCEEDED. On failure, fall through to the existing bounded sleep + timeout, which surfaces "installer migration lock is held" within lockTimeoutMs instead of spinning indefinitely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(3670): tighten T5 — assert bounded failure when reclaim unlink fails on live lock The old T5 accepted BOTH success and throw, which allowed over-reclamation of a genuinely un-reclaimable lock to pass undetected. Rewrite T5 to force deterministically unreclaimable conditions: - Pre-seed lock with process.pid (triggers isSameProcess path) - Mock fs.unlinkSync via mock.method() to throw EPERM for the lock file With the production fix: reclaimed=false → falls through to timeout → throws "installer migration lock is held" within ~200ms. Without the production fix: unlink throws but continue runs anyway → process spins and eventually OOMs (confirmed RED: 136s runtime, V8 heap exhaustion from infinite readLockFile + new Error() allocations). assert.throws() now makes success a hard failure, closing the over-reclamation gap. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3670): clean up orphan lock file when writeFileSync fails after closeSync If closeSync(fd) succeeds (fd=null) but the subsequent writeFileSync throws, the empty lock file was left on disk. readLockFile returns null for an empty/invalid-JSON file, so the stale-lock reclamation path skips it, causing the next acquire attempt to spin to timeout. Track ownership with lockCreatedByUs flag; add a second cleanup branch in the catch block for the fd-already-closed case. Also fix changeset body to use the bold-prefix format required by all other fragments in .changeset/. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-20 23:12:08 -04:00
Tom Boucher	26773bd669	fix(3735): add surface to PROFILES.core (restore ADR-0011 expand contract) (#3744) ... * test(3735): add failing test that PROFILES.core includes surface Regression test asserting that resolveProfile({ modes: ['core'] }) includes 'surface' in its transitive closure — the ADR-0011 contract that --profile=core users can expand via /gsd:surface enable <cluster>. Also updates stale hardcoded skill-count assertions (7→8) across install-minimal*.test.cjs and install-profiles-resolve.test.cjs to reflect the correct post-fix baseline. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3735): add 'surface' to PROFILES.core to restore ADR-0011 expand contract PROFILES.core omitted 'surface', silently breaking the documented contract in ADR-0011 that --profile=core users can expand their skill surface via /gsd:surface enable <cluster>. The sub-command is only available if surface.md is staged — which requires it to appear in the resolved set for the core profile. Added 'surface' to both PROFILES.core and PROFILES.standard (standard is a documented superset of core; omitting it from standard would break the "standard must include all core skills" invariant and the resolveProfile tests). The MINIMAL_SKILL_ALLOWLIST back-compat shim is derived from PROFILES.core so it picks up surface automatically, ensuring --minimal and --core-only installs also stage surface.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(3735): update changeset to reference PR #3744 The fix commit landed with the linked-issue number as the pr: field placeholder. Updating to the actual PR number now that the PR is open. * docs(install-profiles): fix stale 'six skills' count in module header comment After #3735 added surface to PROFILES.core the skill count became eight, but the module-level comment still said "six skills covering the main project loop". Update the description to reflect the correct count and reference the ADR-0011 expand contract that surface fulfils. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-20 23:10:09 -04:00
Tom Boucher	86e067924f	fix(3727): wire --fix flag dispatch in code-review workflow (#3743) ... * test(3727): add failing test for --fix flag dispatch in code-review workflow Adds bug-3727-code-review-fix-flag-dispatch.test.cjs with: - Pure-function tests on parseCodeReviewFlags() / resolveCodeReviewWorkflow() from new code-review-flags.cjs typed IR module - Structural docs-parity tests asserting dispatch_fix step exists in code-review.md and that initialize step references code-review-flags.cjs Structural tests FAIL today (RED): workflow has no dispatch_fix step and no reference to code-review-flags.cjs in initialize. IR-level tests pass because the lib module is introduced in this commit as the typed seam. Fixes #3727 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3727): wire --fix/--all/--auto flags through code-review workflow The initialize step now parses --fix, --all, and --auto from argv (via the code-review-flags parser seam added in the previous test commit) and the workflow dispatches gsd-code-fixer when --fix is truthy and the review returned findings. Fixes the regression introduced when PR #2947 closed #2946 without actually shipping the workflow dispatch. Fixes #3727 * chore(3727): update changeset to reference PR #3743 The fix commit landed with the linked-issue number as the pr: field placeholder. Updating to the actual PR number now that the PR is open. * fix(3727): register code-review-flags.cjs in INVENTORY.md and manifest Add missing row for get-shit-done/bin/lib/code-review-flags.cjs to the CLI Modules table in docs/INVENTORY.md (count 72→73) and regenerate docs/INVENTORY-MANIFEST.json to fix three failing CI tests: - cli-modules-doc-parity: every CLI module must have a row in INVENTORY.md - inventory-counts: headline "CLI Modules (N shipped)" must match file count - inventory-manifest-sync: INVENTORY-MANIFEST.json must match filesystem Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-20 23:09:42 -04:00
Tom Boucher	c1c8b0d109	fix(3739): gap-checker now detects padded-prefix CONTEXT.md (#3764) ... * test(3739): add RED tests for padded-prefix CONTEXT.md gap-checker miss Covers bare and padded (01-CONTEXT.md, 02.1-CONTEXT.md) forms, an uncovered-decision counter-test, and unit tests for the upcoming findContextMdIn() helper. All 6 new tests fail before the fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3739): extract findContextMdIn() helper; fix gap-checker bare lookup gap-checker.cjs:136 used a bare path.join(absPhaseDir, 'CONTEXT.md') that silently returned '' for any phase using the padded-prefix convention (01-CONTEXT.md, 02.1-CONTEXT.md, etc.). Extract findContextMdIn(absDir) to planning-workspace.cjs — the module already imported by gap-checker, init, roadmap, and core — and wire gap-checker.cjs to call it instead of the bare lookup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(3739): replace inline dual-form predicate with findContextMdIn() at all 4 remaining sites The dual-form predicate `f.endsWith('-CONTEXT.md') || f === 'CONTEXT.md'` existed verbatim at 5 sites across init.cjs (×3), roadmap.cjs, and core.cjs — Rule of Three mandates extraction at ≥3 sites. All 4 remaining call sites now delegate to findContextMdIn() from planning-workspace.cjs. No behaviour change; all existing tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(3739): update changeset to reference PR #3764 * fix(3739): findContextMdIn prefers bare CONTEXT.md over padded form (deterministic precedence) `.find()` with `f.endsWith('-CONTEXT.md') || f === 'CONTEXT.md'` returned the first match in `readdirSync` order — undefined on most filesystems. When both `CONTEXT.md` and `01-CONTEXT.md` exist the winner was arbitrary. The old gap-checker.cjs:136 code always used bare `CONTEXT.md` first (existsSync on the bare path). Restore that invariant: check `files.includes('CONTEXT.md')` before falling through to the padded scan. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(3739): dual-file precedence test — bare CONTEXT.md wins over padded form Adds two test cases for the scenario where both CONTEXT.md and 01-CONTEXT.md exist in the same phase directory: 1. Helper level: findContextMdIn() must return 'CONTEXT.md' (not the padded filename) when both files are present on disk. 2. Integration level: gap-analysis must resolve decisions from the bare form only; D-PADDED (from 01-CONTEXT.md) must not appear when the bare form shadows it. Without these tests a future change to findContextMdIn could silently regress the precedence guarantee. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3739): bug-2798 tests skip cleanly when sdk/dist is absent (was hard-fail) The 3 tests in bug-2798-context-window-config-key.test.cjs invoke the built SDK CLI (sdk/dist/cli.js) and require sdk/dist/query/config-schema.js. When dist is absent, they threw hard errors rather than observable skips. Apply the same `if (!existsSync(...)) { t.skip(...); return; }` guard used in bug-2767-gsd-sdk-commit-files-flag.test.cjs (c2812313). Tests 1 & 2 guard on sdk/dist/cli.js; test 3 guards on sdk/dist/query/config-schema.js. When dist is present all 3 run; when absent all 3 emit actionable skip lines. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3739): eliminate double readdirSync in findContextMdIn callers findContextMdIn now accepts either a directory path or an already-read files array, allowing callers that already hold a directory listing (core.cjs:getPhaseFileStats, roadmap.cjs:countPhasePlansAndSummaries, gap-checker.cjs:runGapAnalysis) to skip redundant readdirSync calls. Test coverage added for the array-argument overload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: redesign flaky concurrent add-blocker test with deterministic barrier Previous design relied on OS scheduler to interleave two subprocess writes, producing a flake under CI load. Redesigned using a file-barrier (Option A) that forces both subprocesses to reach a ready-gate before either proceeds, guaranteeing true concurrent lock contention and eliminating timing dependency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-20 22:39:34 -04:00
Tom Boucher	479839147e	fix(state): acquireStateLock retries on transient Docker/NFS errno codes (#3777) ... * fix(state): acquireStateLock retries on transient Docker/NFS errno codes Expands retry allowlist to ENOENT/EINVAL/EIO/ESTALE/EAGAIN/EINTR in addition to existing EPERM/EBUSY. Truly fatal codes (EMFILE/ENOSPC/EROFS/EACCES) still throw. Resolves the locking-bugs regression seen across all 8 open consolidation PRs (#3738, #3741, #3752, #3754, #3756, #3759, #3760, #3769). Closes #3776 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(changeset): update pr number to 3777 in retry-allowlist fragment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-20 15:56:22 -04:00
Tom Boucher	ca2644a71a	fix(worktree): unlock-retry on locked cleanup + startup orphan sweep (#3707) (#3719) ... * fix(worktree): unlock-retry on locked cleanup + startup orphan sweep (#3707) Two root causes fixed: 1. **In-session cleanup blocked**: `executeWorktreeWaveCleanupPlan` now attempts `git worktree unlock <path>` then retries `git worktree remove --force` when the initial single-force remove fails on a locked worktree. Previously every cleanup after a successful merge was silently blocked. 2. **Cross-session orphan accumulation**: new `reapOrphanWorktrees` helper sweeps `.git/worktrees/*/locked` at startup. It reaps entries where the pid is dead, the branch tip is an ancestor of the default branch (ancestry guard prevents data loss on squash-merge repos), and the lock mtime is older than 5 minutes (race guard). Wired into `quick.md` and `execute-phase.md` startup blocks guarded by `USE_WORKTREES != false`. SDK: adds `worktree.reap-orphans` query command (routes through gsd-tools.cjs). Tests: 11 real-fs tests covering unlock-retry, dead-pid reap, live-pid skip, unmerged skip, fresh-mtime skip, idempotent double-call, and structural wiring. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(changeset): add Fixed fragment for PR #3707 (worktree orphan cleanup) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(worktree): fix test portability on Windows + macOS for bug-3707 reap tests - worktreeMeta helper: replace /\/\.git$/ with /[/\\]\.git$/ so the gitdir path suffix is stripped on both Windows (backslash) and Unix. - worktreeMeta helper: normalize CRLF→LF before splitting porcelain blocks, fixing block parsing when git emits CRLF on Windows. - reapOrphanWorktrees: replace single 'main' rev-parse with a [defaultBranch, 'main', 'master'] candidate loop so test fixtures without a remote origin (where branch may be 'master') don't bail early. Intentionally excludes 'HEAD' to prevent false reaping when HEAD is detached or on a feature branch (Codex adversarial finding). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(worktree): CI green — macOS symlink path, Windows test helper, pid portability, EPERM liveness Four fixes to get macOS + Windows CI from red to green: 1. **macOS symlink mismatch** (worktree-safety.cjs): `reapOrphanWorktrees` now builds a canonical→listed path map from `git worktree list --porcelain` using `fs.realpathSync.native`. Uses the listed path (as git knows it) for `git worktree unlock/remove`, not the gitdir-derived path. Fixes the `/var/folders` vs `/private/var/folders` discrepancy on GitHub macOS runners where `git worktree unlock <realpath>` was silently failing because git's list stored the unresolved symlink path. 2. **Windows path separator in test helper** (test file): `worktreeMeta` `.replace(/\/\.git$/, '')` → `.replace(/[/\\]\.git$/, '')`. On Windows, git writes backslash separators in the gitdir file; the Unix-only regex was causing `Cannot find .git/worktrees/<name>` for all Suite 2 tests. 3. **Non-portable PID in tests** (test file): All `'999999'` dead-PID literals replaced with `deadPid()` helper that spawns a real short-lived child, captures its PID, and returns it after exit. Eliminates flakiness on Linux systems where `pid_max` can reach 4194304, making 999999 a live PID. 4. **EPERM fail-closed in isPidAlive** (worktree-safety.cjs): `catch { return false }` → checks `err.code === 'EPERM'` and returns `true` (alive). On Windows and cross-user scenarios, `process.kill(pid, 0)` throws EPERM for live but inaccessible processes; treating that as dead would reap a live worktree. Adversarial review via codex confirmed: - Squash-merge repos: fail-closed (CONCERN, not BUG — by design, not data-loss) - canonicalToListed map: SAFE (fail-closed on realpathSync error) - Concurrent reapers: SAFE (both prune; second gets skipped: remove_failed) - Startup blocking: CONCERN (no global cap, 10s/call × N worktrees) — tracked, not fixed here (requires separate perf work) - gsd-sdk missing: SAFE (quick.md checks and fails fast with guidance) All 27 local tests + Docker (holodeck) green. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(worktree): address codex adversarial findings — fail-closed default branch + CRLF map Two fixes from codex adversarial review of PR 3718: 1. **Default branch resolution (data-loss risk)**: `reapOrphanWorktrees` now uses `refs/remotes/origin/<branch>` exclusively when a remote is configured. If `origin/HEAD` is absent but a remote exists, we bail out (fail-closed) rather than falling back to a local `main`/`master` that may not be the real integration branch. The `main`/`master` fallback is only used when there is provably no remote (local-only test fixtures). 2. **CRLF normalization in canonical-path mapper**: The `worktree list --porcelain` output was split on '\n\n' without normalizing CRLF first. On Windows, git emits CRLF, which caused block-splitting to fail and left the canonicalToListed map only partially populated, weakening the symlink/path-mismatch fix introduced earlier. 3. **Windows 8.3 short-path fix (test helper)**: Both `beforeEach` blocks now call `resolvedTmpDir()` which pre-resolves `os.tmpdir()` via `fs.realpathSync.native` so temp paths avoid RUNNER~1-style short names that git stores in long form, causing worktreeMeta path comparisons to fail on Windows CI. All 11 real-fs + 16 unit tests green locally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(worktree): adversarial findings + macOS CI path-mismatch fix ## Root cause (macOS CI fail) `reapOrphanWorktrees` stored `worktreePath` (gitdir-derived, real path via git's symlink resolution, e.g. `/private/var/folders/…`) in results, while the test's `wtDir` used the unresolved symlink form (`/var/folders/…`). After reaping, `canonicalPath(wtDir)` can no longer call `realpathSync.native` (directory gone), so it falls back to `path.resolve` — which returns the symlink form — causing the `result.find()` comparison to miss. ## Fixes applied ### Source — worktree-safety.cjs 1. **Finding 1 (fail-closed PID check)**: Non-parseable lock content (e.g. `"Locked by claude-code agent-xxx"`) is now treated as ALIVE with reason `lock_owner_unknown`, not as dead. Previously it fell through as dead. 2. **Finding 1b (EPERM safe)**: `isPidAlive` call wrapped in try/catch; any thrown error (EPERM = process exists but cross-user on Windows) → ALIVE. 3. **Finding 2 (startup warning)**: `cmdWorktreeReapOrphans` now writes a one-line stderr warning when ≥1 entry is skipped or when reaper throws, while keeping exit-zero so workflows don't break. 4. **Finding 3 (default-branch discovery)**: Local-only fallback now tries `init.defaultBranch` config and HEAD symref before `main`/`master`, so repos configured with `trunk`, `dev`, etc. get correct orphan detection. 5. **macOS path fix**: Result entry for reaped worktrees now uses `gitKnownPath` (from `git worktree list`) instead of `worktreePath` (from gitdir file), ensuring the caller always sees the path git uses for the worktree. ### Test — bug-3707-locked-worktree-cleanup.test.cjs 6. **macOS CI fix**: Pre-compute `wtDirCanonical = canonicalPath(wtDir)` before calling `reapOrphanWorktrees` so the comparison works after removal. 7. **Gap 1**: New test — Claude Code lock format (`"Locked by claude-code …"`) must not be reaped; asserts `status=skipped, reason=lock_owner_unknown`. 8. **Gap 2**: New test — `isPidAlive` throwing EPERM → must not reap. 9. **Gap 3**: New test — repo with `init.defaultBranch=trunk`; merged worktree must be reaped (verifies trunk is discovered as the integration branch). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): raise waitForStoppedAt timeout 2 s → 5 s for Windows/Node22 CI load Subprocess write latency exceeds 2 s on loaded windows-latest/Node22 runners (test duration was 6181 ms); 5 s gives sufficient headroom without changing any production behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-20 15:22:12 -04:00
Tom Boucher	ab24d80b68	fix(state): acquireStateLock throws on non-EEXIST openSync errors (#3773) ... * fix(state): acquireStateLock throws on non-EEXIST openSync errors Closes #3772 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(changeset): update pr reference to #3773 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(state): retry on EPERM/EBUSY in acquireStateLock and withPlanningLock Resolves state update TOCTOU failure on macos-22 and config-set concurrency failure on windows-24. Root cause: openSync(O_CREAT|O_EXCL) can return EPERM or EBUSY transiently on some CI OS+AV combinations when the lock file is briefly held open by the deleting process; the new throw-on-non-EEXIST guard from #3772 propagated these transient errors, killing child processes and causing lost updates in the concurrency tests. Fix: guard EPERM/EBUSY with an explicit continue before the throw-on-non-EEXIST line in both acquireStateLock and withPlanningLock; the C1 source-audit test still passes because the throw pattern is preserved for all other non-EEXIST codes. Refs #3772 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-20 13:59:20 -04:00
Tom Boucher	473c279c23	fix(state): acquireStateLock must not unlink live locks on retry exhaustion (#3714) (#3717) ... PR #3711 fixed a Windows phantom-lock bug by making the last-retry path unconditionally unlink the existing lock and re-acquire. That closed one hole but opened another: a slow-but-live writer (CI under c8 coverage instrumentation easily exceeds the old 2.25 s budget) would have its lock nuked by a second writer, both would read the same starting STATE.md, and the second write would clobber the first append. Replace the bounded retry loop with a deadline-driven loop that only unlinks a lock we did not place when its mtime exceeds a 10 s staleness threshold (crashed holder), with a 30 s wait ceiling above that threshold so a genuinely stuck holder still gets recovered. Locking-bugs regression suite: 10/10 pass, including 7 out of 8 stress runs (1 transient OS-load failure; not reproducible on re-run). Fixes #3714 Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>	2026-05-19 00:00:44 -04:00
Tom Boucher	dde529f0b1	fix(sdk): regenerate command-aliases CJS surface for phase.uat-passed (#3184) ... Cycle 16 hand-edited command-aliases.generated.ts to register the new query — but command-aliases is auto-generated and has a CJS counterpart that must stay in sync. Running `npx tsx scripts/gen-command-aliases.ts` from sdk/ syncs both surfaces; feat-3598 parity tests now pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-18 23:30:39 -04:00
Tom Boucher	6a5fa59129	feat(3081): auto-trim review prompts for small-context model reviewers (#3708) ... * feat(3081): auto-trim review prompts for small-context model reviewers Adds review.max_prompt_tokens and review.max_prompt_tokens_per_reviewer config keys. When configured, the /gsd-review workflow deterministically trims the assembled prompt before sending to each reviewer (drop CONTEXT → RESEARCH → REQUIREMENTS; head-shrink PROJECT.md; tail-truncate PLANs proportionally; reserve disclosure-note tokens upfront). Trim metadata is recorded in REVIEWS.md frontmatter. Reviewer is skipped with a warning if even the minimum review set exceeds the budget. Closes #3081 * fix(3081): register prompt-budget in SDK query registry and update inventory manifest review.md references `gsd-sdk query prompt-budget` at three call sites, but the command had no handler in the SDK registry — failing the registry-integration drift-guard test on all 6 CI matrix legs. Added a native TypeScript SDK handler (sdk/src/query/prompt-budget.ts) that ports the applyBudget logic from the CJS module, registered it in DOMAIN_STATIC_CATALOG, and regenerated docs/INVENTORY-MANIFEST.json to include the new cli_modules/prompt-budget.cjs entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3081): bump ws to 8.20.1 and allowlist prompt-budget sibling pair Two additional CI failures after the registry fix: 1. ws moderate CVE (GHSA-58qx-3vcg-4xpx, uninitialized memory disclosure): The advisory covers ws >=8.0.0 <8.20.1. Both root and sdk/package.json pinned ^8.20.0 which resolved to 8.20.0. Bumped both to 8.20.1 to clear the npm audit drift-guard test (bug-3588-npm-audit-clean.test.cjs). 2. lint-shared-module-handsync detected the new prompt-budget.ts / prompt-budget.cjs sibling pair without an allowlist entry. Added a cooperatingSiblings entry to scripts/shared-module-handsync-allowlist.json with classification and justification matching the established pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3081): align prompt-budget skip semantics across CJS and SDK dispatch paths Replace brittle `[ $EXIT -eq 2 ]` guards with `[ $EXIT -ne 0 ]` in all three local-reviewer blocks (Ollama, LM Studio, llama.cpp) in workflows/review.md. Any non-zero exit from prompt-budget now triggers a skip with a descriptive warning — exit 2/11 prints "budget too small", any other non-zero prints "unexpected exit code". This ensures the SDK bridge dispatch path (exit 11 via GSDError(Blocked)) triggers the same skip as the CJS path (exit 2). The SDK handler (sdk/src/query/prompt-budget.ts) already writes both metadata and prompt files before throwing, so no change needed there. The Ollama block also gains the missing OLLAMA_SKIP guard so the reviewer invocation is actually skipped (previously the block only suppressed the OLLAMA_PROMPT_FILE update but still ran the curl invocation). SDK integration path (hardFailed via GSDError(Blocked) → exit 11) is covered by handler unit tests in tests/prompt-budget.test.cjs; no gsd-sdk-*.test.cjs exercising the full bridge dispatch for this command exists yet — that gap remains and is documented here. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix prompt-budget trim ordering and review guard follow-ups * perf: optimize prompt-budget and dedup reviewer trim workflow * fix(3708): drop source-grep theater tests to satisfy lint-no-source-grep All four test files added in commit 2df566ed were pure source-grep theater: they read .cjs / .ts / .md source files and asserted that specific string literals were present or absent. None exercised runtime behaviour. Deleted: - tests/gsd-tools-memory-optimizer.test.cjs — 7 includes() on gsd-tools.cjs - tests/prompt-budget-hotpath-optimizer.test.cjs — includes() on prompt-budget.cjs + .ts - tests/prompt-budget-io-optimizer.test.cjs — includes() on prompt-budget.ts + gsd-tools.cjs - tests/review-workflow-budget-dedup.test.cjs — includes() on review.md Behavioural coverage for the prompt-budget feature already exists in tests/prompt-budget.test.cjs and tests/prompt-budget-cli.test.cjs (also added by this PR). No replacement tests needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3708): correct budget-pressure threshold and minSet accounting Two bugs in applyBudget caused premature trimming and false hard-fails: 1. UNNEEDED_TRIM: budgetUnderPressure compared baseTokens against effectiveBudget - NOTE_RESERVE_TOKENS, triggering trim pressure 80 tokens before the budget was actually exceeded. Fix: compare against effectiveBudget directly; NOTE_RESERVE_TOKENS are still reserved in contentBudget once real pressure is confirmed. 2. FALSE_HARDFAIL: minSet included NOTE_RESERVE_TOKENS unconditionally, treating the note as mandatory even when no trim would occur and no note would be injected. Fix: exclude NOTE_RESERVE_TOKENS from minSet; a prompt that fits untrimmed needs no note and must not hard-fail. Both fixes applied in CJS and TypeScript implementations. Two regression tests added (cycles 11 and 12) that reproduce each case behaviorally. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-18 23:13:09 -04:00
Tom Boucher	77b6bb62db	fix(state): acquireStateLock last-retry now re-acquires before proceeding (#3711)	2026-05-18 17:55:06 -04:00
Tom Boucher	cb154569cf	revert: fix(surface) #3666 — CONTRIBUTING.md test-pattern violations (#3688) ... Reverts c5657fcbfd (squash of PR #3666). The production change was correct in intent. The new test file tests/surface-state.test.cjs violates two CONTRIBUTING.md rules that the project enforces specifically because they produce passing-but-useless tests: 1. ~15 try { ... } finally { cleanup(dir); } blocks in test bodies — CONTRIBUTING.md "Never use try/finally inside test bodies." Approved forms are beforeEach/afterEach hooks or t.after(...). 2. 17 assert.match calls on rendered console.warn prose — CONTRIBUTING.md "Prohibited: Raw Text Matching on Test Outputs." Tests must assert on a typed structured surface (frozen reason enum) instead of the rendered prose, otherwise they pass-but-rot the moment a warning string is reworded. The .changeset/witty-birds-gather.md fragment is also reverted so the v1.43 changelog does not advertise a fix that is not on main. A reworked PR addressing items 1-4 from the re-review (linked on the closed PR thread) is welcome — the intent of the fix is right. Refs #3666 #3662 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-18 00:20:36 -04:00
Cristian Uibar	c2dc9e2532	Fix W002 false positives on archived phase references in STATE.md body (#3655) ... * W002 health check: cross-reference milestones archive for STATE.md phase refs After /gsd-complete-milestone, phase dirs move into milestones/vX.Y-phases/ and their `#### Phase N:` headings in ROADMAP.md are collapsed inside <details> blocks. The ROADMAP heading scan misses them, so W002 fired for every archived phase number mentioned in STATE.md's historical narrative body ("Recent", "Decisions", "Deferred Items") — leaving every project that ever ran /gsd-complete-milestone permanently degraded with proportional W002 noise. Union archived milestone phase directories into the validPhases set used by the W002 check, mirroring the same milestone-archive lookup that W006 already uses (sdk/src/query/validate.ts:723-736). Closes #3652 * Address review: use shared regex constants and mirror W002 archive fix into CJS path CodeRabbit (PR #3655 review 1): the ad-hoc /^(\d+[A-Z]?(?:\.\d+)*)/i used to extract phase tokens from archived phase dirs would skip project-code-prefixed names like `CK-64-...`. Switch the new SDK block to the shared PHASE_TOKEN_FROM_DIR_RE / MILESTONE_ARCHIVE_DIR_RE constants (defined at sdk/src/query/validate.ts:32-33) so prefixed archives are recognised. Added a companion regression test using `CK-`-prefixed dirs. Codex review: the shipped CJS health command path (get-shit-done/bin/lib/verify.cjs cmdValidateHealth, routed by validate-command-router.cjs) only unions collectDiskPhases (active archive only) plus ROADMAP heading scan — same bug as the SDK had. Port the all-archive scan into the CJS path via listMilestoneArchiveDirs + PHASE_TOKEN_FROM_DIR_RE (already declared at verify.cjs:401-402). Added a CJS regression test covering the multi-sub-milestone (v1.3a + v1.3b) scenario from the issue report. Adds .changeset/lucky-lynx-wave.md. * Address Gemini findings: shared regex + helper reuse + cross-platform path P1 #2 — refactor the new W002 archive-scan block to reuse the existing listMilestoneArchiveDirs helper (sdk/src/query/validate.ts:40) instead of re-implementing readdir + filter inline. Eliminates duplication and prevents the two call sites from drifting apart. P2 #4 — listMilestoneArchiveDirs sorted by `a.slice(a.lastIndexOf('/') + 1)`, which returns the full path on Windows where path.join produces backslashes. Switch to path.basename(a) so the numeric version sort works cross-platform. Brings the SDK helper in line with the CJS sibling at get-shit-done/bin/lib/verify.cjs:411 which already uses path.basename. P1 #1 / P2 #5 — the pre-existing W006/W007 archive + active phase scans (Check 8) used an ad-hoc `^(\d+[A-Z]?(?:\.\d+)*)` regex that silently skipped project-code-prefixed phase dirs like `CK-64-foo`, so W006 fired for a correctly-archived phase and W007 fired for a correctly-on-disk phase. Switch both scans to the shared PHASE_TOKEN_FROM_DIR_RE constant declared at sdk/src/query/validate.ts:32. The W006 archive loop also now reuses listMilestoneArchiveDirs for consistency. P2 #6 — strengthen the CK-prefix regression test to also assert no W006 fires for `#### Phase 64: Prior shipped` (placed inside <details> so the heading scan picks it up while the on-disk scan does not), pinning the shared-regex behaviour in the W006 path. * CI: switch retired /gsd-<cmd> comment syntax to canonical /gsd:<cmd> The bug-2543 slash-namespace invariant lint scans get-shit-done/bin/lib/** for /gsd-<cmd> patterns and fails CI when one slips into a comment. Use the canonical /gsd:complete-milestone form in the new verify.cjs comment (and mirror the change in the SDK + tests + changeset entry so all docstrings referencing the milestone-completion command share one spelling). Also: extract a small forEachArchivedPhaseToken helper in validate.ts (Gemini P3 finding from review pass 2) so Check 4 (W002) and Check 8 (W006) share the archive-walking loop instead of inlining it twice. * Address rev3 review: shared regex parity, numeric sort, drop dead try/catch Gemini P1 — Check 4's flat phases/ scan still used the ad-hoc /^(\d+[A-Z]?(?:\.\d+)*)/ regex while the archive scan used the shared PHASE_TOKEN_FROM_DIR_RE. Project-code-prefixed dirs (e.g. CK-65-current) on the flat layout would have slipped past validity, so the W002 check could still mis-classify them. Use PHASE_TOKEN_FROM_DIR_RE here too. Gemini P3 — `[...validPhases].sort()` ordered tokens alphabetically, producing error messages like "phases 1, 10, 19, 2, 20" instead of "1, 2, 10, 19, 20". Switch both SDK and CJS to numeric localeCompare so the displayed list is human-readable. Mirrored in both paths. Grok P3 — the CJS Check 4 archive block wrapped listMilestoneArchiveDirs in an outer try/catch even though the helper already swallows ENOENT/EACCES into []. The outer catch was unreachable. Removed; only the per-archive readdir needs a catch. Grok P2 / Gemini P1 (CJS Check 8 archive scan) — the assertion that CJS Check 8 needs the same archive union as Check 4 was repeatedly raised across review passes. It is incorrect: Check 8 filters ROADMAP.md through extractCurrentMilestone() before scanning headings, which strips shipped milestones (collapsed in <details> or not) so archived phase numbers never reach `roadmapPhases`. Added an inline note documenting this and a positive regression assertion in the CJS test that W006 does NOT fire for the archived phases in the multi-sub-milestone fixture. (Skipped a parallel W007 assertion because the active-archive fallback in getActiveMilestoneArchiveDir is pre-existing behavior unrelated to #3652.) * Port forEachArchivedPhaseToken helper to verify.cjs for SDK parity Gemini rev5 P2 — the CJS Check 4 inlined the archive-walking loop while the SDK already factored it into forEachArchivedPhaseToken(). Add a mirror helper in verify.cjs so both seams use the same primitive, matching the cooperating-sibling pattern documented in scripts/shared-module-handsync-allowlist.json. * ci: retrigger to clear unrelated TOCTOU flake The previous CI run failed at the pre-existing #1925 concurrency test (state add-blocker concurrent calls) on macos-24 and ubuntu-22 but passed on ubuntu-24 — and the same test passed on the prior CI run of this branch (commit 8a246916). The state add-blocker code path is completely independent of the W002 archive-union changes in this PR.	2026-05-18 00:14:10 -04:00
Cristian Uibar	c5657fcbfd	fix(surface): default missing optional fields in readSurface, normalize writeSurface input (#3666) ... * surface: default missing optional fields in readSurface, normalize writeSurface input readSurface used to reject any .gsd-surface.json missing one of its four fields and return null with no diagnostic, so the active surface silently degraded to the 'full' profile. Optional array fields (disabledClusters, explicitAdds, explicitRemoves) now default to [] when missing or wrong-typed; hard failures (malformed JSON, non-object root, missing/non-string baseProfile) still return null but emit a console.warn naming the file + reason. writeSurface now normalizes its input to the full SurfaceState shape and throws on missing baseProfile, so partial writes can no longer land on disk and trip readSurface later. Tests extended to cover the new lenient and warn-on-hard-fail behavior plus the writer guard. Fixes #3662 * surface: reject whitespace-only baseProfile + migrate new tests to helpers.cjs Applies CodeRabbit findings on PR #3666: 1. readSurface and writeSurface now reject baseProfile values that are blank after trim() (e.g. " "), not only the empty string. Whitespace-only strings would split-by-comma to [''] downstream and silently produce an unresolvable profile mode. Both guards updated symmetrically; warn/error messages reworded to "missing, non-string, or blank". 2. tests/surface-state.test.cjs now uses createTempDir + cleanup from tests/helpers.cjs instead of local mkdtempSync + fs.rmSync, aligning with the repo coding guideline for root-level tests. The local tmpDir() helper delegates to createTempDir for backward-compat with the existing test bodies. Per-test cleanup calls swapped to cleanup(dir). 3. Added regression tests: - readSurface rejects whitespace-only baseProfile and warns - writeSurface rejects whitespace-only baseProfile, non-string baseProfile, and null surfaceState Refs #3662 * surface: warn on unknown baseProfile mode names in read and write Applies a Codex review finding on PR #3666: readSurface and writeSurface used to accept any non-blank string as baseProfile. A typo like {"baseProfile":"standrad"} would pass validation, then resolveProfile() in install-profiles.cjs would silently fall back to 'full' with no diagnostic — the same silent-degradation symptom that #3662 was filed to fix, just through a different code path. Both functions now split baseProfile by comma, validate each mode against the registered PROFILES set ('core', 'standard', 'full'), and emit a single [gsd] console.warn line that names the unknown modes and lists the valid ones. The state is still parsed/written — resolveProfile() decides the actual resolution fallback. Composed profiles where some modes are valid and some are not warn only about the unknown subset. Side note: the pre-existing 'round-trips composed base profile' test used 'core,audit' as a stand-in composed string. 'audit' is not a registered profile (the three known profiles are 'core', 'standard', 'full'), so the test was relying on the old lack of validation. Switched to 'core,standard' to preserve the round-trip intent without producing diagnostic noise. Refs #3662 * changeset: include blank/typo baseProfile in documented read failure cases CodeRabbit minor finding on PR #3666 — the changeset wording only mentioned "missing/non-string baseProfile" but the implementation also rejects blank (including whitespace-only) baseProfile values, and warns on typo'd / unknown profile mode names. Updated to match actual behavior. * changeset: pr field should be PR number, not issue number Codex review finding on PR #3666. The changeset's `pr: 3662` was the linked issue (#3662), but the convention across other .changeset/*.md files is that `pr:` carries the PR number. Verified by spot-checking other changesets (2937 → pr: 3515, 3298 → pr: 3306, 3541 → pr: 3547 — all PR numbers). Updated to `pr: 3666`. The body text still references the issue. * surface: reject comma-only baseProfile + warn on wrong-typed optional fields Three Gemini review findings on PR #3666 — all spirit-of-#3662 edge cases: 1. Comma-only baseProfile bypasses validation. readSurface used to accept baseProfile: ", ," because trim() returned "," (non-empty). Downstream resolveProfile() would split-and-filter to [] and silently fall back to 'full'. Added effectiveProfileModes() helper that splits, trims, filters empty — both readSurface and writeSurface now reject when the result is empty. Same silent-degradation symptom as the original bug. 2. Wrong-typed optional fields silently coerced. readSurface used to coerce {disabledClusters: 42} to {disabledClusters: []} with no diagnostic. Now warns via mistypedOptionalFields() before normalizeSurfaceState() does the coercion, in both reader and writer. 3. Missing test coverage for the EACCES branch in readSurface. Added a chmod-000 unreadable-file test, skipped on Windows and root accounts (mode bits are ignored on those platforms). 48/48 surface tests pass. Final codex pass returned LGTM on the prior state; these three additions strengthen the same lenient/loud contract. Refs #3662	2026-05-18 00:14:07 -04:00
Tom Boucher	7e6ba56985	fix(3678): executor must respect commit_docs:false; teach SDK skip envelope (#3679) ... * fix(3678): executor must respect commit_docs:false; teach SDK skip envelope Closes #3678 When `commit_docs: false` in `.planning/config.json`, the SDK's `cmdCommit` correctly short-circuits and returns `{committed: false, hash: null, reason: 'skipped_commit_docs_false'}` without staging or committing anything. The agent prompt at `agents/gsd-executor.md:710-720` (final_commit block) tells the executor to call `gsd-sdk query commit "docs(...)" --files .planning/...` but says NOTHING about how to interpret a skipped return. With no explicit instruction, the LLM improvises raw `git add` / `git add -f` / `git commit` to "fulfill" the per-plan commit step it was told to make, which leaks gitignored `.planning/` artifacts into the user's git history (exactly what the reporter observed). Three coordinated fixes: 1. **agents/gsd-executor.md final_commit block** — adds explicit handling text for all three SDK return envelopes (`committed:true`, `skipped:true commit_docs`, `skipped:true gitignored`, `committed:false other reasons`). States plainly: "Do not fall back to raw `git add` / `git commit` / `git add -f` when the SDK returns `skipped: true`." 2. **get-shit-done/bin/lib/commands.cjs cmdCommit** — adds `skipped: true` to both skip-path envelopes so agents see "skipped" as a first-class success signal rather than inferring "no commit happened, I must improvise" from absent `hash` / `committed:false`. Backward-compatible: existing callers reading `committed` / `hash` / `reason` are unaffected. 3. **tests/bug-3678-executor-commit-docs-respect.test.cjs** — 7-test regression covering: - A1/A2: agent prompt mentions the skip envelope AND explicitly forbids raw-git fallback (`source-text-is-the-product` exception) - B1: SDK envelope carries `committed:false`, `skipped:true`, canonical `reason: 'skipped_commit_docs_false'` (frozen enum) - B2: git index empty after commit_docs:false skip (no `.planning/` staged) - B3: HEAD unchanged after commit_docs:false skip - C1/C2: structural ban on `git add -f` / `git add --force` in any agent or workflow body (prohibition-sentence exception preserves audit prose) Verification: - node --test tests/bug-3678-*: 7/7 pass - Targeted regression (10 commit/executor-adjacent files): 135/135 pass - Full docker suite (gsd-test-summary): 11751/11740 pass / 0 fail (the 11 added are this test plus a few collateral pickups) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(changeset): add fragment for #3678 fix (Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>) * chore(changeset): set PR number 3679 (Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>) * fix(3678): preserve skip-aware carve-out in executor completion checklist The new `final_commit` prose at lines 717-741 teaches the executor to treat `skipped:true` as success and forbids raw-git fallback, but the downstream completion checklist still contained an unconditional "Final metadata commit made" checkbox. An LLM executor reading an unchecked mandatory box may attempt to satisfy it via raw `git add`, re-introducing the exact regression this PR is meant to prevent. Update the checklist line to carve out the intentional-skip case and add a regression test asserting the carve-out remains present. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-18 00:14:03 -04:00
Tom Boucher	40a442b21f	Merge pull request #3649 from gsd-build/feat/3597-split-suites-node-matrix ... feat(3597): split test suites and add Node 22/24/26 OS matrix	2026-05-17 01:51:25 -04:00
Tom Boucher	b8fa89b5b6	fix(3663): address CodeRabbit surface/layout follow-ups	2026-05-17 01:39:10 -04:00
Tom Boucher	4d81532b74	fix(windows): use git show-prefix for nested worktree detection	2026-05-17 01:33:32 -04:00
Tom Boucher	af54a4ffe6	fix(windows): force root-equivalent cwd to non-nested	2026-05-17 01:24:45 -04:00
Tom Boucher	2782fd05fb	fix(windows): derive nested-worktree state from normalized paths	2026-05-17 01:14:57 -04:00
Tom Boucher	043cfd97b3	fix(3663): applySurface creates missing dest dirs (Codex P1-1) ... Remove the fs.existsSync(dest) guard in applySurface so _syncGsdDir is always called. _syncGsdDir already does mkdirSync(..., { recursive: true }) so the destination is created when absent — recovering partially-initialized or user-deleted runtime config dirs. Also threads manifest through to _syncGsdDir as optional 4th arg (used by P1-2 Hermes fix). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-17 01:12:00 -04:00
Tom Boucher	40e325b91f	fix(3663): pass runtime+cmdNames to Claude converter for Hermes/Qwen ... convertClaudeCommandToClaudeSkill(content, skillName, runtime, cmdNames) uses the runtime arg to gate Hermes/Qwen branding and version: frontmatter emission (#2808, #3583). Previously the layout module called it with only 2 args so the runtime-specific formatting was never applied. Changes: - skillsKind() gains a runtime param (5th arg after converterName). - stage() computes cmdNames = readGsdCommandNames() once per call (perf: avoids repeated fs.readdirSync in the converter) and wraps the real converter so all 4 args are forwarded. - readGsdCommandNames added to bin/install.js GSD_TEST_MODE exports block so the stage closure can call it without requiring the script separately. - All switch arms updated to pass the canonical runtime string. Converters that do not inspect runtime/cmdNames (Cursor, Codex, Copilot, etc.) accept and ignore the extra arguments — no behaviour change for those runtimes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-17 01:12:00 -04:00
Tom Boucher	be5fe61221	fix(3663): lazy-require converters; remove GSD_TEST_MODE env mutation at module load ... Previously runtime-artifact-layout.cjs set process.env.GSD_TEST_MODE at the top level before requiring bin/install.js. Any caller that required this module would silently inherit the GSD_TEST_MODE='1' side-effect for the lifetime of the process. Replace with a lazy loader (loadInstallExports / getInstallExports) that saves the current GSD_TEST_MODE, sets it to '1' only if it was undefined, calls require(), and restores the original value in a finally block. The exported module.exports cache (_installExports) ensures the require is run at most once. Converter names are now passed as strings to skillsKind; the stage closure resolves them via getInstallExports()[converterName] at call time, so the installer is not loaded until a stage() function is actually invoked. Verification: node -e "require('./...runtime-artifact-layout.cjs'); console.log(process.env.GSD_TEST_MODE)" prints undefined. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-17 01:12:00 -04:00
Tom Boucher	67dd09e509	fix(3663): restore .gsd-source marker resolution in findInstallSourceRoot ... Both findInstallSourceRoot and findAgentsSourceRoot now accept an optional runtimeConfigDir. When provided, they first check <runtimeConfigDir>/.gsd-source: read the stored path, verify it exists, and return it. Fall through to the path.dirname walk-up only when the marker is absent or points to a missing path. The factory functions (commandsKind, agentsKind, skillsKind) now receive configDir from the switch arms and pass it through to the finders. listSurface in surface.cjs passes its runtimeConfigDir argument to findInstallSourceRoot so installed-from-source layouts use the marker rather than the walk-up. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-17 01:12:00 -04:00
Tom Boucher	3711a4f04f	fix(3663): replace ../../.. fallback with descriptive error per CLAUDE.md ... Both findInstallSourceRoot and findAgentsSourceRoot previously returned a path.join(__dirname, '..', '..', '..', ...) fallback when the walk-up loop found nothing. Replace with throw per CLAUDE.md "No Relative Path Traversal" policy: .. chains silently break on CWD changes; a throw with the failing __dirname in the message is an unambiguous diagnostic. The walk-up loop already uses path.dirname iteratively (no literal ..); only the dead-end fallback used the banned pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-17 01:12:00 -04:00
Tom Boucher	8c16b1d338	fix(3663): preserve Hermes user-skill dirs under skills/gsd/ namespace ... When kindPrefix === '' (Hermes: destSubpath=skills/gsd, no per-skill prefix), startsWith('') always returns true so the prior removal loop would delete any dir not in the staged set — including user-owned skill dirs. Guard the entire removal block behind kindPrefix !== '' so non-staged dirs are never pruned when there is no prefix to distinguish GSD-owned from user-owned entries. TDD: failing test added first asserting user-custom-skill is preserved through a _syncGsdDir call with kindPrefix=''. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-17 01:12:00 -04:00
Tom Boucher	4d578394eb	refactor(3663): migrate applySurface to layout-driven loop; add skills kind to _syncGsdDir ... - applySurface(runtimeConfigDir, commandsDir, agentsDir, manifest, clusterMap) → applySurface(runtimeConfigDir, layout, manifest, clusterMap) - _syncGsdDir extended to handle skills kind (dirs not files, prefix-gated removal) - _findInstallSource/_findAgentsSource deleted; listSurface now uses findInstallSourceRoot() from runtime-artifact-layout.cjs - findInstallSourceRoot exported from runtime-artifact-layout.cjs - test(3663): update surface-apply.test.cjs to new layout-passing shape + add skills kind test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-17 01:12:00 -04:00
Tom Boucher	dc2a6278e5	feat(3663): add runtime-artifact-layout.cjs + resolve tests for all 15 runtimes ... - New module: get-shit-done/bin/lib/runtime-artifact-layout.cjs - resolveRuntimeArtifactLayout(runtime, configDir, scope) → Layout - 15-runtime table (grok intentionally excluded, throws TypeError) - findInstallSourceRoot / findAgentsSourceRoot walk-up-only (no .gsd-source marker in Phase 1) - Loads bin/install.js converters via GSD_TEST_MODE guard - Tests: 16 fixtures covering all runtimes + both claude scopes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-17 01:12:00 -04:00
Tom Boucher	235f527635	feat(3663): implement stageSkillsForRuntimeAsSkills iteration+write loop ... RED-2/GREEN-2: wildcard skills='*' stages all *.md as <prefix><stem>/SKILL.md with converter applied. Includes existence guard and error cleanup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-17 01:12:00 -04:00
Tom Boucher	11e74f9a25	test(3663): add export check for stageSkillsForRuntimeAsSkills ... RED-1/GREEN-1: export assertion + stub function + STAGED_DIRS export. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-05-17 01:12:00 -04:00
Tom Boucher	dac60313e2	fix(windows): treat root-equivalent show-cdup as non-nested	2026-05-17 01:06:06 -04:00
Tom Boucher	157f791d45	fix(windows): derive nested worktree state from show-cdup	2026-05-17 00:36:26 -04:00
Tom Boucher	3c51e760f8	fix(windows): harden nested-root and archive-warning assertions	2026-05-17 00:17:31 -04:00
Tom Boucher	5ee2a148ca	fix(windows): stabilize init/workflow tests across path and npm edge cases	2026-05-16 14:00:34 -04:00
Tom Boucher	1a1ad2ec87	Merge origin/main into feat/3597-split-suites-node-matrix ... Resolves conflict in .github/workflows/test.yml: keep the 6 new drift-check steps from main (plan-scan, secrets, schema-detect, decisions, workstream-name-policy, Shared Module hand-sync) before the split-lane test runs from this PR. PR's dedicated `coverage` job replaces main's per-matrix `Run tests with coverage` step. Other conflicting files (CONTRIBUTING.md, get-shit-done/bin/lib/init.cjs, package.json) auto-merged cleanly. Changeset files (.changeset/*) brought in from main as adds.	2026-05-16 13:22:07 -04:00