Refactor SDK-first architecture seams (#3316)

* refactor: tighten sdk-first architecture seams Refs #3312 * refactor: finish state document seam cleanup Refs #3312 * test: harden minimal install cleanup assertion * ci: support sdk-scoped package lock * fix(3316): restore root package-lock.json and align changeset pr ref Reverts dec57a83 ("ci: support sdk-scoped package lock") and restores the root package-lock.json that c249d34d deleted. The deletion was the wrong direction: - The root package.json declares its own runtime and dev deps (@anthropic-ai/claude-agent-sdk, ws, c8). Without a root lockfile, `npm install --no-package-lock` resolves whatever satisfies semver at install time — CI today and CI in six months can install different transitive trees, defeating reproducibility. - The lockfile has been part of every release on this repo (long history on main); removing it loses the npm audit / Dependabot target without compensating benefit. - The CI workaround pattern (cache-dependency-path: sdk/package-lock.json + `npm install --no-package-lock`) papered over the symptom rather than fix the cause. Also fix the changeset pr: from 3312 (issue) to 3316 (PR). CONTEXT.md flags this exact failure mode as a recurring CodeRabbit finding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address coderabbit review findings * fix: close remaining coderabbit threads --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-09 14:21:42 -04:00
parent 2e87c60afc
commit 26dcdb1ad0
34 changed files with 1693 additions and 738 deletions
--- a/tests/bug-2647-outer-tarball-sdk-dist.test.cjs
+++ b/tests/bug-2647-outer-tarball-sdk-dist.test.cjs
@@ -28,6 +28,7 @@ const { describe, test } = require('node:test');
 const assert = require('node:assert/strict');
 const fs = require('fs');
 const path = require('path');
+const os = require('os');
 const { execFileSync } = require('child_process');

 const REPO_ROOT = path.join(__dirname, '..');
@@ -88,6 +89,8 @@ describe('bug #2647: outer tarball ships sdk/dist so gsd-sdk query works', () =>
  });

  test('npm pack dry-run includes sdk/dist/cli.js after build:sdk', { timeout: 180_000 }, () => {
+    const npmCache = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-npm-cache-'));
+    const npmEnv = { ...process.env, npm_config_cache: npmCache };
    // Ensure the sdk is built so the pack reflects what publish would ship.
    // The outer prepublishOnly chains through build:sdk, which does `npm ci && npm run build`
    // inside sdk/. We emulate that here without full ci to keep the test fast:
@@ -98,24 +101,28 @@ describe('bug #2647: outer tarball ships sdk/dist so gsd-sdk query works', () =>
      // Build requires node_modules; install if missing, then build.
      const sdkNodeModules = path.join(sdkDir, 'node_modules');
      if (!fs.existsSync(sdkNodeModules)) {
-        execFileSync('npm', ['ci', '--silent'], { cwd: sdkDir, stdio: 'pipe' });
+        execFileSync('npm', ['ci', '--silent'], { cwd: sdkDir, stdio: 'pipe', env: npmEnv });
      }
-      execFileSync('npm', ['run', 'build'], { cwd: sdkDir, stdio: 'pipe' });
+      execFileSync('npm', ['run', 'build'], { cwd: sdkDir, stdio: 'pipe', env: npmEnv });
    }
    assert.ok(fs.existsSync(cliJs), 'sdk build must produce sdk/dist/cli.js');

-    const out = execFileSync(
-      'npm',
-      ['pack', '--dry-run', '--json', '--ignore-scripts'],
-      { cwd: REPO_ROOT, stdio: ['ignore', 'pipe', 'pipe'] },
-    ).toString('utf-8');
-    const manifest = JSON.parse(out);
-    const files = manifest[0].files.map((f) => f.path);
-    const cliPresent = files.includes('sdk/dist/cli.js');
-    assert.ok(
-      cliPresent,
-      `npm pack must include sdk/dist/cli.js in the tarball (so "gsd-sdk query" resolves after install). sdk/dist entries found: ${files.filter((p) => p.startsWith('sdk/dist')).length}`,
-    );
+    try {
+      const out = execFileSync(
+        'npm',
+        ['pack', '--dry-run', '--json', '--ignore-scripts'],
+        { cwd: REPO_ROOT, stdio: ['ignore', 'pipe', 'pipe'], env: npmEnv },
+      ).toString('utf-8');
+      const manifest = JSON.parse(out);
+      const files = manifest[0].files.map((f) => f.path);
+      const cliPresent = files.includes('sdk/dist/cli.js');
+      assert.ok(
+        cliPresent,
+        `npm pack must include sdk/dist/cli.js in the tarball (so "gsd-sdk query" resolves after install). sdk/dist entries found: ${files.filter((p) => p.startsWith('sdk/dist')).length}`,
+      );
+    } finally {
+      fs.rmSync(npmCache, { recursive: true, force: true });
+    }
  });

  test('built sdk CLI exposes the `query` subcommand', { timeout: 60_000 }, () => {
--- a/tests/bug-3017-codex-hook-absolute-node.test.cjs
+++ b/tests/bug-3017-codex-hook-absolute-node.test.cjs
@@ -118,18 +118,19 @@ describe('Bug #3017: buildCodexHookBlock emits absolute node runner', () => {
      'must return null on missing runner so caller can warn-and-skip instead of writing a broken hook');
  });

-  test('integrates with resolveNodeRunner() in the live process — runner equals process.execPath (#3022 CR)', () => {
+  test('integrates with resolveNodeRunner() in the live process — runner equals resolved node runner (#3022 CR)', () => {
    const runner = resolveNodeRunner();
    assert.ok(runner, 'resolveNodeRunner returns a usable value in this test env');
    const block = buildCodexHookBlock('/tmp/x/.codex', { absoluteRunner: runner });
    const parsed = parseCodexHookBlock(block);
    assert.equal(parsed.ok, true);
-    // Strict canonical-runner equality: the parsed runner (after
-    // stripping toml + JSON escape layers) must be exactly process.execPath
-    // (forward-slashed, since resolveNodeRunner normalizes that way).
-    const expected = process.execPath.replace(/\\/g, '/');
+    // Strict canonical-runner equality: the parsed runner (after stripping
+    // toml + JSON escape layers) must be exactly the normalized runner that
+    // resolveNodeRunner selected. Homebrew Cellar execPath values intentionally
+    // normalize to the stable Homebrew symlink (#3181).
+    const expected = JSON.parse(runner);
    assert.equal(unescapeRunner(parsed.runner), expected,
-      `parsed runner must equal process.execPath, got: ${parsed.runner}, want: ${expected}`);
+      `parsed runner must equal resolveNodeRunner(), got: ${parsed.runner}, want: ${expected}`);
  });
 });

--- a/tests/bug-3242-state-update-progress-trample.test.cjs
+++ b/tests/bug-3242-state-update-progress-trample.test.cjs
@@ -104,7 +104,7 @@ describe('#3242 Bug A: body-only state.update preserves curated progress frontma
    cleanup(tmpDir);
  });

-  test('state.update "Last Activity" does not overwrite progress.completed_plans', { todo: 'fix pending: #3242 Bug A not yet implemented' }, (t) => {
+  test('state.update "Last Activity" does not overwrite progress.completed_plans', (t) => {
    const statePath = path.join(tmpDir, '.planning', 'STATE.md');
    fs.writeFileSync(statePath, buildStateWithCuratedProgress({
      completedPlans: 22,
@@ -181,6 +181,28 @@ describe('#3242 Bug A: body-only state.update preserves curated progress frontma
      'state.update should have written the new date to the Last Activity body field',
    );
  });
+
+  test('state.update "Progress" resyncs progress frontmatter from the updated body', () => {
+    const statePath = path.join(tmpDir, '.planning', 'STATE.md');
+    fs.writeFileSync(statePath, buildStateWithCuratedProgress({
+      completedPlans: 22,
+      totalPlans: 22,
+      completedPhases: 6,
+      totalPhases: 12,
+      percent: 50,
+    }).replace('Last Activity: 2026-01-01\n', 'Last Activity: 2026-01-01\nProgress: [█████░░░░░] 50%\n'));
+
+    const updateResult = runGsdTools(
+      ['state', 'update', 'Progress', '[████████░░] 80%'],
+      tmpDir,
+    );
+    assert.ok(updateResult.success, `state update failed: ${updateResult.error}`);
+
+    const jsonResult = runGsdTools('state json', tmpDir);
+    assert.ok(jsonResult.success, `state json failed: ${jsonResult.error}`);
+    const fm = JSON.parse(jsonResult.output);
+    assert.strictEqual(fm.progress.percent, 80);
+  });
 });

 // ─────────────────────────────────────────────────────────────────────────────
@@ -198,7 +220,7 @@ describe('#3242 Bug B: progress.percent reflects phase fraction when ROADMAP dec
    cleanup(tmpDir);
  });

-  test('12 declared phases / 6 realized / 6/6 plans done → percent is 50, not 100', { todo: 'fix pending: #3242 Bug B not yet implemented' }, (t) => {
+  test('12 declared phases / 6 realized / 6/6 plans done → percent is 50, not 100', (t) => {
    const statePath = path.join(tmpDir, '.planning', 'STATE.md');

    // Body: 6 realized phases visible to disk scan.
@@ -292,7 +314,7 @@ describe('#3242 Bug B: progress.percent reflects phase fraction when ROADMAP dec
    );
  });

-  test('state sync also reflects phase-fraction-capped percent in body Progress field', { todo: 'fix pending: #3242 Bug B not yet implemented' }, () => {
+  test('state sync also reflects phase-fraction-capped percent in body Progress field', () => {
    // state sync updates the body's Progress: field — it must use the same capped formula
    const statePath = path.join(tmpDir, '.planning', 'STATE.md');

--- a/tests/codex-config.test.cjs
+++ b/tests/codex-config.test.cjs
@@ -51,6 +51,7 @@ const {
  GSD_CODEX_MARKER,
  CODEX_AGENT_SANDBOX,
  parseTomlToObject,
+  resolveNodeRunner,
 } = require('../bin/install.js');

 function runCodexInstall(codexHome, cwd = path.join(__dirname, '..')) {
@@ -1437,10 +1438,10 @@ describe('Codex install hook configuration (e2e)', () => {
    // #3017: handler command now uses the absolute Node binary path so
    // GUI/minimal-PATH runtimes can resolve it. The shape is
    //   "<absolute-node-path>" "<hook-path>"
-    // where <absolute-node-path> is process.execPath (forward-slashed)
-    // and the hook path is also quoted. Same Node process runs the test
-    // and the installer, so process.execPath matches at both ends.
-    const expectedRunner = process.execPath.replace(/\\/g, '/');
+    // where <absolute-node-path> is the normalized runner selected by
+    // resolveNodeRunner() and the hook path is also quoted. Homebrew Cellar
+    // execPath values intentionally normalize to stable Homebrew symlinks.
+    const expectedRunner = JSON.parse(resolveNodeRunner());
    const expectedHookPath = path.join(codexHome, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/');
    const expectedCommand = `"${expectedRunner}" "${expectedHookPath}"`;
    assert.strictEqual(
--- a/tests/install-minimal.test.cjs
+++ b/tests/install-minimal.test.cjs
@@ -210,15 +210,25 @@ describe('install-profiles: cleanupStagedSkills', () => {
    cleanupStagedSkills();
    const src = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-stage-fullmode-'));
    fs.writeFileSync(path.join(src, 'plan-phase.md'), '# plan\n');
+    const realMkdtemp = fs.mkdtempSync;
+    let minimalStageDirCreated = false;
+    fs.mkdtempSync = (prefix, ...rest) => {
+      if (typeof prefix === 'string' && prefix.endsWith('gsd-minimal-skills-')) {
+        minimalStageDirCreated = true;
+      }
+      return realMkdtemp(prefix, ...rest);
+    };
    try {
-      const before = listTmpStageDirs();
      const result = stageSkillsForMode(src, 'full');
      assert.strictEqual(result, src, 'full mode returns original src unchanged');
      cleanupStagedSkills();
-      const after = listTmpStageDirs();
-      // No new gsd-minimal-skills- dirs should have been created.
-      assert.deepStrictEqual(after, before);
+      assert.equal(
+        minimalStageDirCreated,
+        false,
+        'full mode should not create a gsd-minimal-skills stage dir',
+      );
    } finally {
+      fs.mkdtempSync = realMkdtemp;
      fs.rmSync(src, { recursive: true, force: true });
    }
  });
--- a/tests/phase.test.cjs
+++ b/tests/phase.test.cjs
@@ -854,6 +854,35 @@ describe('phase add command', () => {
    assert.ok(roadmap.includes('**Requirements**: TBD'), 'new phase entry should include Requirements TBD');
  });

+  test('phase add ignores --raw instead of persisting it in the description', () => {
+    fs.writeFileSync(
+      path.join(tmpDir, '.planning', 'ROADMAP.md'),
+      `# Roadmap v1.0\n\n### Phase 1: Foundation\n**Goal:** Setup\n\n---\n`
+    );
+
+    const result = runGsdTools(['phase', 'add', '--raw', 'User', 'Dashboard'], tmpDir);
+    assert.ok(result.success, `Command failed: ${result.error}`);
+
+    const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8');
+    assert.ok(roadmap.includes('### Phase 2: User Dashboard'), 'description should exclude --raw');
+    assert.ok(!roadmap.includes('--raw'), 'raw flag must not be persisted into ROADMAP.md');
+  });
+
+  test('phase add rejects unsupported flags and dangling --id', () => {
+    fs.writeFileSync(
+      path.join(tmpDir, '.planning', 'ROADMAP.md'),
+      `# Roadmap v1.0\n\n### Phase 1: Foundation\n**Goal:** Setup\n\n---\n`
+    );
+
+    const unsupported = runGsdTools(['phase', 'add', '--unknown', 'Dashboard'], tmpDir);
+    assert.ok(!unsupported.success, 'unsupported flags should fail');
+    assert.match(unsupported.error, /phase add does not support --unknown/);
+
+    const dangling = runGsdTools(['phase', 'add', 'Dashboard', '--id'], tmpDir);
+    assert.ok(!dangling.success, 'dangling --id should fail');
+    assert.match(dangling.error, /--id requires a value/);
+  });
+
  test('skips 999.x backlog phases when calculating next phase number', () => {
    fs.writeFileSync(
      path.join(tmpDir, '.planning', 'ROADMAP.md'),
@@ -1176,6 +1205,22 @@ describe('phase add-batch command (#2165)', () => {
    const result = runGsdTools(['phase', 'add-batch', '--descriptions', '[]'], tmpDir);
    assert.ok(!result.success, 'should fail on empty array');
  });
+
+  test('returns error when --descriptions JSON is not an array', () => {
+    const result = runGsdTools(['phase', 'add-batch', '--descriptions', '{"one":"Alpha"}'], tmpDir);
+    assert.ok(!result.success, 'should fail on non-array JSON');
+    assert.match(result.error, /--descriptions must be a JSON array/);
+  });
+
+  test('returns error when --descriptions is missing its JSON value', () => {
+    const missing = runGsdTools(['phase', 'add-batch', '--descriptions'], tmpDir);
+    assert.ok(!missing.success, 'should fail on dangling --descriptions');
+    assert.match(missing.error, /--descriptions must be a JSON array/);
+
+    const flagValue = runGsdTools(['phase', 'add-batch', '--descriptions', '--raw'], tmpDir);
+    assert.ok(!flagValue.success, 'should fail when --descriptions value is another flag');
+    assert.match(flagValue.error, /--descriptions must be a JSON array/);
+  });
 });

 // ─────────────────────────────────────────────────────────────────────────────
@@ -2855,4 +2900,3 @@ describe('phase complete excludes 999.x backlog from next-phase (#2129)', () =>
 // ─────────────────────────────────────────────────────────────────────────────
 // milestone complete command
 // ─────────────────────────────────────────────────────────────────────────────
-
--- a/tests/profile-output.test.cjs
+++ b/tests/profile-output.test.cjs
@@ -110,7 +110,7 @@ describe('write-profile command', () => {
    const analysisPath = path.join(tmpDir, 'analysis.json');
    fs.writeFileSync(analysisPath, JSON.stringify(analysis));

-    const result = runGsdTools(['write-profile', '--input', analysisPath, '--raw'], tmpDir);
+    const result = runGsdTools(['write-profile', '--input', analysisPath, '--raw'], tmpDir, { HOME: tmpDir });
    assert.ok(result.success, `Failed: ${result.error}`);
    const out = JSON.parse(result.output);
    assert.ok(out.profile_path, 'should return profile_path');
@@ -206,7 +206,11 @@ describe('generate-dev-preferences command', () => {
    const analysisPath = path.join(tmpDir, 'analysis.json');
    fs.writeFileSync(analysisPath, JSON.stringify(analysis));

-    const result = runGsdTools(['generate-dev-preferences', '--analysis', analysisPath, '--raw'], tmpDir);
+    const result = runGsdTools(
+      ['generate-dev-preferences', '--analysis', analysisPath, '--raw'],
+      tmpDir,
+      { HOME: tmpDir }
+    );
    assert.ok(result.success, `Failed: ${result.error}`);
    const out = JSON.parse(result.output);
    assert.ok(out.command_path || out.command_name, 'should return command output');
--- a/tests/workstream.test.cjs
+++ b/tests/workstream.test.cjs
@@ -666,6 +666,28 @@ describe('workstream progress', () => {
    assert.strictEqual(data.workstreams[0].progress_percent, 50);
  });

+  test('clamps progress percent when completed phase dirs exceed roadmap count', () => {
+    const isolatedDir = createTempProject();
+    try {
+      const wsDir = path.join(isolatedDir, '.planning', 'workstreams', 'overflow');
+      for (const phase of ['01-one', '02-two']) {
+        const phaseDir = path.join(wsDir, 'phases', phase);
+        fs.mkdirSync(phaseDir, { recursive: true });
+        fs.writeFileSync(path.join(phaseDir, 'PLAN.md'), '# Plan\n');
+        fs.writeFileSync(path.join(phaseDir, 'SUMMARY.md'), '# Summary\n');
+      }
+      fs.writeFileSync(path.join(wsDir, 'STATE.md'), '# State\n**Status:** In progress\n');
+      fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), '# Roadmap\n### Phase 1: One\n');
+
+      const result = runGsdTools(['workstream', 'progress', '--raw'], isolatedDir);
+      assert.ok(result.success, `progress failed: ${result.error}`);
+      const data = JSON.parse(result.output);
+      assert.strictEqual(data.workstreams[0].progress_percent, 100);
+    } finally {
+      cleanup(isolatedDir);
+    }
+  });
+
  test('returns flat mode when no workstreams exist', () => {
    const emptyDir = createTempProject();
    try {