From 2bc49b0aece3f3ce7901f066d0513d40e134f8a1 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 6 May 2026 21:04:03 -0400 Subject: [PATCH] fix(install): wire --sdk flag into installSdkIfNeeded (#3033) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hasSdk was parsed in bin/install.js but never passed to installSdkIfNeeded, so `npx get-shit-done-cc@latest --sdk` silently skipped SDK deployment via the isLocal early-return and emitted a misleading "✓ GSD SDK ready" message. installSdkIfNeeded now accepts opts.forceSdk. When true (set from hasSdk at the call site in installAllRuntimes), the local-install soft-skip is bypassed so the full shim-link path runs regardless of install mode. When dist is also missing with forceSdk=true, the fail-fast diagnostic fires instead of silently returning. The #2678 soft-skip (isLocal + missing dist + no --sdk) is preserved. Closes #3033 Co-Authored-By: Claude Sonnet 4.6 --- .changeset/3033-sdk-flag-wired.md | 5 + CHANGELOG.md | 14 +- bin/install.js | 10 +- tests/bug-3033-sdk-flag-wired.test.cjs | 180 +++++++++++++++++++++++++ 4 files changed, 194 insertions(+), 15 deletions(-) create mode 100644 .changeset/3033-sdk-flag-wired.md create mode 100644 tests/bug-3033-sdk-flag-wired.test.cjs diff --git a/.changeset/3033-sdk-flag-wired.md b/.changeset/3033-sdk-flag-wired.md new file mode 100644 index 000000000..78c7f1ede --- /dev/null +++ b/.changeset/3033-sdk-flag-wired.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3033 +--- +**`--sdk` flag now wired into SDK deployment** — `hasSdk` was parsed in `bin/install.js` but never passed to `installSdkIfNeeded`, so `npx get-shit-done-cc@latest --sdk` silently skipped SDK deployment and produced a misleading "✓ GSD SDK ready" message. `installSdkIfNeeded` now accepts `forceSdk: true` (set when `--sdk` is passed), which bypasses the local-install soft-skip and runs the full shim-link path so `gsd-sdk` is materialized on PATH. The `#2678` soft-skip for local installs without `--sdk` is preserved. (#3033) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6445d081f..ceec0aeb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,22 +8,11 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed -- **`/gsd-quick` worktree-merge resurrection guard no longer deletes brand-new `.planning/` files** — the inverted `PRE_MERGE_FILES` grep that caused any file absent from the pre-merge snapshot (including freshly created `SUMMARY.md`) to be deleted has been replaced with the git-history check already used by `execute-phase.md` since PR #2510; only files with a confirmed deletion event in main's ancestry are now removed. (#3195) +- **`--sdk` flag now wired into SDK deployment** — `hasSdk` was parsed in `bin/install.js` but never passed to `installSdkIfNeeded`, so `npx get-shit-done-cc@latest --sdk` silently skipped SDK deployment and produced a misleading "✓ GSD SDK ready" message. `installSdkIfNeeded` now accepts `forceSdk: true` (set when `--sdk` is passed), which bypasses the local-install soft-skip and runs the full shim-link path so `gsd-sdk` is materialized on PATH. The `#2678` soft-skip for local installs without `--sdk` is preserved. (#3033) - **Milestone-archive layout support** — `validate consistency`, `validate health`, and `find-phase` now scan `.planning/milestones/v*-phases/` directories in addition to the flat `.planning/phases/` layout. Projects that have graduated to milestone-archive layout no longer receive spurious W006 "Phase N in ROADMAP.md but no directory on disk" warnings for every active phase. (#3164) ### Feature -- **Vertical MVP discovery & progress surfaces.** `/gsd new-project` now prompts the user to choose between **Vertical MVP** (each phase delivers an end-to-end user capability — recommended for new products) and **Horizontal Layers** (build complete technical layers, assemble at the end). Picking Vertical MVP writes `**Mode:** mvp` on every initial roadmap phase. `/gsd progress` adds a user-flow status sub-block sourced from PLAN.md task names when a phase has `**Mode:** mvp`. `/gsd stats` adds a 'Phases: N total | M MVP | K standard' summary line when at least one MVP phase exists. `/gsd graphify` renders MVP-mode phase nodes with a distinct green fill (`#22c55e`) and a ` (MVP)` label suffix — two-channel signaling for color-blind and grayscale renders. Closes the umbrella PRD #2826. (#2826) -- **MVP-mode UAT framing in `/gsd verify-work`** — when the phase under verification has `**Mode:** mvp` in ROADMAP.md, the generated UAT script asks "can a real user complete the feature?" before any technical checks. User-flow steps (open, fill, click, observe) run first; technical checks (endpoint schemas, error states) only run AFTER the user flow passes. Adds a goal-backward "User Flow Coverage" section to VERIFICATION.md that maps user-story steps to evidence in the codebase. User-story-format guard refuses to verify a `mode: mvp` phase whose `**Goal:**` line is not in user-story format. (#2826) -- **MVP+TDD runtime gate in `/gsd execute-phase`** — when both `MVP_MODE` and `TDD_MODE` are active for a phase, the executor refuses to advance a behavior-adding task until a failing-test commit exists for it. The existing end-of-phase TDD review (advisory by default) escalates to **blocking** under the same condition: phases with missing RED→GREEN commits cannot be marked complete without `--force-mvp-gate`. Pure doc-only / config-only / test-only tasks are exempt. (#2826) -- **`/gsd mvp-phase ` command** — guided MVP planning entry point. Prompts for an "As a / I want to / So that" user story (three structured fields), runs SPIDR splitting check (full interactive flow per PRD Q3) if the story is too large, writes `**Mode:** mvp` and the formatted goal to ROADMAP.md, then delegates to `/gsd plan-phase ` (which auto-detects MVP via the roadmap mode field shipped in PRD Phase 1). The `gsd-planner` agent now emits a `## Phase Goal` section with bolded **As a** / **I want to** / **so that** keywords as the first content under the phase header in PLAN.md when MVP_MODE is active. (#2826) -- **`--mvp` flag on `/gsd plan-phase`** — opt-in vertical-slice planning. Plans are - organized as feature slices (UI→API→DB) instead of horizontal layers, so each task - moves a real user-visible capability forward. Persistable per-phase via `**Mode:** mvp` - in ROADMAP.md. New-project Phase 1 + `--mvp` triggers Walking Skeleton output - (`SKELETON.md`) capturing architectural decisions for subsequent phases. Single - planner agent, mode-switched (no new agent surface). PRD Phases 2–4 (`mvp-phase` - command, TDD wiring, discovery/UX) deferred to follow-up plans. (#2826) - **Six namespace meta-skills with keyword-tag descriptions** — replace the flat 86-skill listing with two-stage hierarchical routing. Model sees 6 namespace routers (`gsd:workflow`, `gsd:project`, `gsd:review`, `gsd:context`, `gsd:manage`, @@ -122,7 +111,6 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fix -- **`generate-claude-md` now writes to `AGENTS.md` on Codex runtime** — when `config.runtime` is `codex` (or `GSD_RUNTIME=codex`), the handler overrides the output target to `AGENTS.md` regardless of `claude_md_path`, so Codex projects no longer have GSD sections written to `CLAUDE.md` by mistake. Explicit `--output` flags are still honoured. Regression covered by `tests/bug-3163-codex-agents-md.test.cjs`. (#3163) - **`/gsd-graphify build` now runs inline instead of spawning a sub-agent** — graphify v0.7+ split the build into a fast AST-extraction phase (cached) followed by a separate clustering + report-write phase. The cached extraction phase survived sub-agent isolation, but the post-extraction phase was SIGTERM'd when the agent exited, leaving the cache populated and no `graph.json` / `graph.html` / `GRAPH_REPORT.md` artifacts written to `.planning/graphs/`. The skill now runs `graphify update .`, the three artifact copies, the snapshot, and the status report as a single foreground Bash call so the entire pipeline survives to completion. The CLI's `graphify build` pre-flight still returns `action: "spawn_agent"` so external callers and existing tests keep working. Regression covered by `tests/bug-3166-graphify-inline-build.test.cjs` (4 structural assertions that parse `commands/gsd/graphify.md` YAML frontmatter and body to fence against re-introducing `Task` to `allowed-tools` or `Task(` invocation syntax). (#3166) - **`gsd-pristine/` is now populated by the installer when local patches are detected** — `saveLocalPatches` declared a `pristineDir` variable and JSDoc'd "saves pristine copies (from manifest) to gsd-pristine/ to enable three-way merge during reapply-patches", but no code ever wrote to that directory. Effect: the `/gsd-reapply-patches` Step 5 verifier (#2972) silently degraded to its over-broad fallback heuristic ("every significant backup line"), exactly the silent-success-on-lost-content failure mode #2969 was designed to prevent. Fix: new `populatePristineDir({ packageSrc, pristineDir, modified, runtime, pathPrefix, isGlobal })` helper runs the install transform pipeline (`copyWithPathReplacement`) into a tmp staging dir, then copies out only the modified-file paths into `gsd-pristine/`. `saveLocalPatches` now accepts a `pristineCtx` and calls the helper when local patches are detected; the install entry point passes the package source root, runtime, pathPrefix, and isGlobal so transforms produce byte-identical output to what `copyWithPathReplacement` would have written under normal install. Soft-fails on transform errors (logs a warning, continues with empty pristine — no worse than pre-fix behavior). Pristine reflects the about-to-install version's content, which is what the verifier needs as the "what would survive without the user's modifications" baseline. Regression covered by `tests/bug-2998-pristine-dir-populated.test.cjs` (6 tests across two suites): asserts the helper is exported, returns 0 for empty modified list, writes one pristine file per source-existing path, skips ghost paths without corrupting pristine, and produces deterministic output (two runs with same inputs yield byte-identical pristine — the property `pristine_hashes` in `backup-meta.json` depends on). (#2998) - **`release-sdk` hotfix re-run no longer fails at `Dry-run publish validation` when the version is already on npm** — the `Detect prior publish (reconciliation mode)` step sets `skip_publish=true` when the package version is already on the registry, and the actual publish step honors that gate. The `Dry-run publish validation` step was missing the same guard, so any operator re-run of an already-published hotfix (the typical recovery path when later steps fail mid-flight) hit `npm publish --dry-run` first and got `npm error You cannot publish over the previously published versions: X.Y.Z` — `npm publish --dry-run` contacts the registry and rejects existing-version targets even though it doesn't actually publish. The dry-run validation step is now gated on the same `steps.prior_publish.outputs.skip_publish != 'true'` condition as the publish step. The rehearsal still runs on first publishes (where it has value); it skips only in the specific reconciliation case where the publish itself would be skipped. Trigger run: [25233855236](https://github.com/gsd-build/get-shit-done/actions/runs/25233855236/job/73995605643). Regression covered by `tests/bug-2987-dry-run-validation-skip-on-reconciliation.test.cjs`. (#2987) diff --git a/bin/install.js b/bin/install.js index 24677a299..b7fd94d16 100755 --- a/bin/install.js +++ b/bin/install.js @@ -9338,7 +9338,12 @@ function installSdkIfNeeded(opts) { // so a self-link into a user-writable PATH dir makes `gsd-sdk` callable // from local-mode installs too. Only when the dist is genuinely missing // do we bail out with a non-fatal warning. - if (opts.isLocal && !fs.existsSync(sdkCliPath)) { + // + // #3033: --sdk (opts.forceSdk) overrides the local-install early-return — + // the user explicitly requested SDK deployment, so treat the missing-dist + // case like a global install (fail fast with an actionable diagnostic) + // instead of silently skipping. + if (opts.isLocal && !opts.forceSdk && !fs.existsSync(sdkCliPath)) { console.warn(`\n ${yellow}⚠${reset} Skipping SDK check for local install — sdk/dist/cli.js not found at ${sdkCliPath}.`); return; } @@ -9813,7 +9818,8 @@ function installAllRuntimes(runtimes, isGlobal, isInteractive) { // prebuilt in the tarball (fix/2441-sdk-decouple); gsd-sdk reaches users via // the parent package's bin/gsd-sdk.js shim, so no sub-install is needed. // Skip with --no-sdk. Skip with isLocal (#2678 — local installs don't own global npm). - installSdkIfNeeded({ isLocal: !isGlobal }); + // #3033: pass forceSdk so --sdk overrides the local-install skip. + installSdkIfNeeded({ isLocal: !isGlobal, forceSdk: hasSdk }); const printSummaries = () => { for (const result of results) { diff --git a/tests/bug-3033-sdk-flag-wired.test.cjs b/tests/bug-3033-sdk-flag-wired.test.cjs new file mode 100644 index 000000000..dd4a4a162 --- /dev/null +++ b/tests/bug-3033-sdk-flag-wired.test.cjs @@ -0,0 +1,180 @@ +/** + * Regression test for #3033: --sdk flag parsed but never used. + * + * `hasSdk` was set in bin/install.js but never passed to `installSdkIfNeeded`, + * so `npx get-shit-done-cc@latest --sdk` produced a misleading "✓ GSD SDK ready" + * message while still silently skipping SDK deployment for local installs. + * + * Fix: `installSdkIfNeeded` now accepts `opts.forceSdk`. When true, the + * early-return for `isLocal=true` + missing dist is bypassed and the + * fail-fast diagnostic fires (same as a global install with missing dist), + * and when dist IS present the full shim-link path runs regardless of + * install mode. + * + * Tests here call `installSdkIfNeeded` directly with `forceSdk: true` + * and assert on filesystem state and console output — no source-grep. + */ + +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); + +const { installSdkIfNeeded } = require('../bin/install.js'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +function captureConsole(fn) { + const stdout = []; + const stderr = []; + const origLog = console.log; + const origWarn = console.warn; + const origError = console.error; + console.log = (...a) => stdout.push(a.join(' ')); + console.warn = (...a) => stderr.push(a.join(' ')); + console.error = (...a) => stderr.push(a.join(' ')); + let threw = null; + try { + fn(); + } catch (e) { + threw = e; + } finally { + console.log = origLog; + console.warn = origWarn; + console.error = origError; + } + if (threw) throw threw; + const strip = (s) => s.replace(/\x1b\[[0-9;]*m/g, ''); + return { + stdout: stdout.map(strip).join('\n'), + stderr: stderr.map(strip).join('\n'), + }; +} + +describe('bug #3033: --sdk flag (opts.forceSdk) must be wired into installSdkIfNeeded', () => { + let tmpRoot; + let sdkDir; + let pathDir; + let homeDir; + let savedEnv; + + beforeEach(() => { + tmpRoot = createTempDir('gsd-3033-'); + sdkDir = path.join(tmpRoot, 'sdk'); + pathDir = path.join(tmpRoot, 'somebin'); + homeDir = path.join(tmpRoot, 'home'); + fs.mkdirSync(pathDir, { recursive: true }); + fs.mkdirSync(homeDir, { recursive: true }); + savedEnv = { PATH: process.env.PATH, HOME: process.env.HOME }; + process.env.PATH = pathDir; + process.env.HOME = homeDir; + }); + + afterEach(() => { + if (savedEnv.PATH == null) delete process.env.PATH; + else process.env.PATH = savedEnv.PATH; + if (savedEnv.HOME == null) delete process.env.HOME; + else process.env.HOME = savedEnv.HOME; + cleanup(tmpRoot); + }); + + test('forceSdk=true + isLocal=true + dist present: self-links gsd-sdk into PATH dir', () => { + // Stage a valid dist so the installer can proceed past the missing-dist gate. + fs.mkdirSync(path.join(sdkDir, 'dist'), { recursive: true }); + fs.writeFileSync( + path.join(sdkDir, 'dist', 'cli.js'), + '#!/usr/bin/env node\nconsole.log("0.0.0-test");\n', + { mode: 0o755 }, + ); + + // Put ~/.local/bin on PATH so the shim-link step can succeed. + const localBin = path.join(homeDir, '.local', 'bin'); + fs.mkdirSync(localBin, { recursive: true }); + process.env.PATH = `${localBin}${path.delimiter}${pathDir}`; + + const { stdout, stderr } = captureConsole(() => { + installSdkIfNeeded({ sdkDir, isLocal: true, forceSdk: true }); + }); + const combined = `${stdout}\n${stderr}`; + + // Shim must be materialized on PATH. + const linkPath = path.join(localBin, 'gsd-sdk'); + assert.ok( + fs.existsSync(linkPath), + `forceSdk=true must materialize gsd-sdk shim at ${linkPath}. Output:\n${combined}`, + ); + + // Must report "GSD SDK ready" — not the legacy "Skipping SDK check" message. + assert.ok( + /GSD SDK ready/.test(combined), + `forceSdk=true must print "GSD SDK ready" once shim is on PATH. Output:\n${combined}`, + ); + assert.ok( + !/Skipping SDK check for local install/.test(combined), + `forceSdk=true must NOT print the local-skip message. Output:\n${combined}`, + ); + }); + + test('forceSdk=true + isLocal=true + dist missing: fails fast instead of silently skipping', () => { + // No dist directory — simulate a broken/missing SDK. + fs.mkdirSync(sdkDir, { recursive: true }); + // dist/cli.js intentionally absent. + + let exitCode = null; + const origExit = process.exit; + process.exit = (code) => { + exitCode = code; + throw new Error(`process.exit(${code})`); + }; + + let threw = false; + try { + captureConsole(() => { + installSdkIfNeeded({ sdkDir, isLocal: true, forceSdk: true }); + }); + } catch { + threw = true; + } finally { + process.exit = origExit; + } + + // With forceSdk=true the missing-dist early-return is bypassed and the + // fail-fast path fires, calling process.exit(1). + assert.ok( + threw && exitCode === 1, + `forceSdk=true with missing dist must call process.exit(1) — exitCode=${exitCode}, threw=${threw}`, + ); + }); + + test('forceSdk=false (default) + isLocal=true + dist missing: retains #2678 soft-skip', () => { + // Verify the #2678 contract is not broken for the default (no --sdk) path. + fs.mkdirSync(sdkDir, { recursive: true }); + + let exitCalled = false; + const origExit = process.exit; + process.exit = (code) => { + exitCalled = true; + throw new Error(`process.exit(${code}) — local install without --sdk must not exit`); + }; + + try { + captureConsole(() => { + installSdkIfNeeded({ sdkDir, isLocal: true }); + }); + } catch (e) { + // If process.exit was called, the test should fail below. + } finally { + process.exit = origExit; + } + + assert.strictEqual( + exitCalled, + false, + 'isLocal=true without forceSdk must not call process.exit (preserves #2678 contract)', + ); + }); +});