fix(install): wire --sdk flag into installSdkIfNeeded (#3033)

hasSdk was parsed in bin/install.js but never passed to installSdkIfNeeded, so `npx get-shit-done-cc@latest --sdk` silently skipped SDK deployment via the isLocal early-return and emitted a misleading "✓ GSD SDK ready" message. installSdkIfNeeded now accepts opts.forceSdk. When true (set from hasSdk at the call site in installAllRuntimes), the local-install soft-skip is bypassed so the full shim-link path runs regardless of install mode. When dist is also missing with forceSdk=true, the fail-fast diagnostic fires instead of silently returning. The #2678 soft-skip (isLocal + missing dist + no --sdk) is preserved. Closes #3033 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-06 21:04:03 -04:00
parent f4d0208abb
commit 2bc49b0aec
4 changed files with 194 additions and 15 deletions
--- a/.changeset/3033-sdk-flag-wired.md
+++ 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)
--- 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 <N>` 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 <N>` (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)
--- 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) {
--- a/tests/bug-3033-sdk-flag-wired.test.cjs
+++ 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)',
+    );
+  });
+});