diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 0d9e8976f..fbf040169 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "gsd-core", "displayName": "GSD Core", - "version": "1.4.2", + "version": "1.4.3", "description": "GSD Core is a meta-prompting, context engineering, and spec-driven development system for AI coding agents.", "author": { "name": "open-gsd", diff --git a/.gitignore b/.gitignore index bbc12e1b1..ddde43901 100644 --- a/.gitignore +++ b/.gitignore @@ -113,6 +113,7 @@ build/ /gsd-core/bin/lib/model-profiles.cjs /gsd-core/bin/lib/installer-migrations/002-codex-legacy-hooks-json.cjs /gsd-core/bin/lib/installer-migrations/003-rename-get-shit-done-to-gsd-core.cjs +/gsd-core/bin/lib/installer-migrations/004-prune-stale-pristine-snapshots.cjs /gsd-core/bin/lib/observability/logger.cjs /gsd-core/bin/lib/active-workstream-store.cjs /gsd-core/bin/lib/adr-parser.cjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d1fbb669..75a3a4a77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,18 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +## [1.4.3] - 2026-06-09 + +### Fixed + +- Fix `--reapply` verifier false-positives on post-#604-rename installs caused by two gaps in pristine-baseline handling: + +**Gap 1** (`verify-reapply-patches.cjs`): when `backup-meta.json` records a `pristine_hash` for a file but `gsd-pristine/` has no corresponding snapshot on disk, the verifier fell to over-broad mode (every upstream-changed line treated as a user-added requirement) and produced `FAIL_USER_LINES_MISSING` false positives. Fix: return advisory `OK_NO_BASELINE` reason (non-blocking, exit 0) when a recorded hash is present but the pristine file is absent — the verifier cannot reason correctly without a baseline and must not block. + +**Gap 2** (new migration `004-prune-stale-pristine-snapshots`): migration 003 removed legacy `get-shit-done/` runtime files but left `gsd-pristine/get-shit-done/` orphan snapshots in place. Those stale snapshots referenced `get-shit-done/...` key paths that no longer match the active `gsd-core/...` layout, contributing to `FAIL_INSTALLED_MISSING` false reports. Fix: add a new migration (not editing 003, to preserve its checksum) that removes all files under `gsd-pristine/get-shit-done/`. (#934) (#937) +- **`/gsd-update` changelog preview no longer silently fails** — the installer now copies `scripts/changeset/` and `scripts/lib/` into the runtime config dir so `$GSD_DIR/scripts/changeset/cli.cjs` resolves at runtime; `update.md` was updated to use the correct installed path and to surface an explicit error if the CLI is missing rather than swallowing it. (#938) +- **`plan-review-convergence` now runs `gsd-plan-phase` inline instead of inside `Agent()`** — both sites that previously wrapped `gsd-plan-phase` in `Agent()` (initial planning + replan loop) have been changed to bare `Skill()` calls at depth 0. On Claude Code, a depth-1 Agent has no Agent tool, so a wrapped `plan-phase` could never spawn `gsd-planner` or `gsd-plan-checker` — the replan loop silently failed to produce a revised plan whenever HIGH concerns were found. Running plan-phase inline from the depth-0 orchestrator (which retains the Agent tool) restores the full planner→checker sub-agent chain. A new structural guard test (`bug-936-no-nested-spawner-wrap.test.cjs`) statically scans all workflow files and fails if any workflow wraps a spawner orchestrator in `Agent()` without a `RUNTIME != claude` carve-out, preventing regression. (#936) (#939) + ## [1.4.2] - 2026-06-09 ### Fixed diff --git a/bin/install.js b/bin/install.js index 50ba46122..751b4d950 100755 --- a/bin/install.js +++ b/bin/install.js @@ -8496,6 +8496,52 @@ function uninstall(isGlobal, runtime = 'claude') { } } + // 4a. Remove scripts/changeset/ and scripts/lib/ (#935) + // GSD-managed files only: enumerate the exact set the installer writes. + // Any file NOT in this set is user-owned and must survive uninstall. + // After removing GSD files, attempt to rmdir — if the directory is still + // non-empty (user has custom helpers) it stays; otherwise it goes cleanly. + const GSD_CHANGESET_FILES = [ + 'cli.cjs', 'parse.cjs', 'render.cjs', 'serialize.cjs', + 'github-release-notes.cjs', 'lint.cjs', 'new.cjs', + 'README.md', // documentation only — not user-authored + ]; + const GSD_SCRIPTS_LIB_FILES = ['cli-exit.cjs', 'allowlist-ratchet.cjs']; + + const changesetUninstallDir = path.join(targetDir, 'scripts', 'changeset'); + if (fs.existsSync(changesetUninstallDir)) { + let removedChangeset = 0; + for (const file of GSD_CHANGESET_FILES) { + const fp = path.join(changesetUninstallDir, file); + try { fs.unlinkSync(fp); removedChangeset++; } catch (_) { /* best-effort */ } + } + // Remove directory if empty after our cleanup + try { fs.rmdirSync(changesetUninstallDir); } catch (_) { /* Not empty — user content present */ } + if (removedChangeset > 0) { + removedCount++; + console.log(` ${green}✓${reset} Removed scripts/changeset/ GSD files`); + } + } + const scriptsLibUninstallDir = path.join(targetDir, 'scripts', 'lib'); + if (fs.existsSync(scriptsLibUninstallDir)) { + let removedScriptsLib = 0; + for (const file of GSD_SCRIPTS_LIB_FILES) { + const fp = path.join(scriptsLibUninstallDir, file); + try { fs.unlinkSync(fp); removedScriptsLib++; } catch (_) { /* best-effort */ } + } + // Remove directory if empty after our cleanup + try { fs.rmdirSync(scriptsLibUninstallDir); } catch (_) { /* Not empty — user content present */ } + if (removedScriptsLib > 0) { + removedCount++; + console.log(` ${green}✓${reset} Removed scripts/lib/ GSD files`); + } + } + // If scripts/ dir is now empty, remove it too + const scriptsUninstallDir = path.join(targetDir, 'scripts'); + if (fs.existsSync(scriptsUninstallDir)) { + try { fs.rmdirSync(scriptsUninstallDir); } catch (_) { /* Not empty — leave it */ } + } + // 5. Remove GSD package.json (CommonJS mode marker) const pkgJsonPath = path.join(targetDir, 'package.json'); if (fs.existsSync(pkgJsonPath)) { @@ -9169,6 +9215,24 @@ function writeManifest(configDir, runtime = 'claude', options = {}) { } } + // Track scripts/changeset/ and scripts/lib/ so saveLocalPatches() can detect drift + const changesetInstallDir = path.join(configDir, 'scripts', 'changeset'); + if (fs.existsSync(changesetInstallDir)) { + for (const file of fs.readdirSync(changesetInstallDir)) { + if (file.endsWith('.cjs')) { + manifest.files['scripts/changeset/' + file] = fileHash(path.join(changesetInstallDir, file)); + } + } + } + const scriptsLibInstallDir = path.join(configDir, 'scripts', 'lib'); + if (fs.existsSync(scriptsLibInstallDir)) { + for (const file of fs.readdirSync(scriptsLibInstallDir)) { + if (file.endsWith('.cjs')) { + manifest.files['scripts/lib/' + file] = fileHash(path.join(scriptsLibInstallDir, file)); + } + } + } + fs.writeFileSync(path.join(configDir, MANIFEST_NAME), JSON.stringify(manifest, null, 2)); return manifest; } @@ -10446,6 +10510,64 @@ function install(isGlobal, runtime = 'claude', options = {}) { console.log(` ${green}✓${reset} Installed hooks/lib/ helpers (git-cmd, graphify-rebuild, ...)`); } + // Install scripts/changeset/ and scripts/lib/ into /scripts/ + // so that `node "$GSD_DIR/scripts/changeset/cli.cjs"` resolves at runtime. + // + // The changeset CLI (scripts/changeset/cli.cjs) is invoked by the update + // workflow (gsd-core/workflows/update.md) to extract changelog ranges for + // the /gsd-update preview step. It was previously only present in the npm + // tarball root but never copied to the runtime config dir, causing the + // preview to always silently fail (#935). + // + // cli.cjs requires: + // - sibling files in scripts/changeset/ (parse/render/serialize/github-release-notes) + // - ../lib/cli-exit.cjs → scripts/lib/cli-exit.cjs + // - ../../gsd-core/bin/lib/semver-compare.cjs (already installed under gsd-core/) + // - ../../gsd-core/bin/lib/package-identity.cjs (already installed under gsd-core/) + // + // All runtimes that use the update workflow need this, so we copy unconditionally + // (same scope as gsd-core/ itself — every runtime that installs workflows gets it). + const changesetSrc = path.join(src, 'scripts', 'changeset'); + const scriptsLibSrc = path.join(src, 'scripts', 'lib'); + if (!fs.existsSync(changesetSrc)) { + // The changeset CLI source is missing from the package — mark as a hard failure + // so the user knows the changelog preview will not work rather than silently degrading. + failures.push('scripts/changeset/ (source missing from package — reinstall from npm)'); + } else { + const changesetDest = path.join(targetDir, 'scripts', 'changeset'); + const scriptsLibDest = path.join(targetDir, 'scripts', 'lib'); + fs.mkdirSync(changesetDest, { recursive: true }); + fs.mkdirSync(scriptsLibDest, { recursive: true }); + // Copy scripts/changeset/ — all .cjs and .md files + for (const entry of fs.readdirSync(changesetSrc)) { + const srcFile = path.join(changesetSrc, entry); + if (fs.statSync(srcFile).isFile()) { + fs.copyFileSync(srcFile, path.join(changesetDest, entry)); + } + } + // Copy scripts/lib/ — cli-exit.cjs (required by cli.cjs) and any future lib helpers. + // Hard-fail if missing: without cli-exit.cjs the installed CLI throws MODULE_NOT_FOUND. + if (!fs.existsSync(scriptsLibSrc)) { + failures.push('scripts/lib/ (source missing from package — reinstall from npm)'); + } else { + for (const entry of fs.readdirSync(scriptsLibSrc)) { + const srcFile = path.join(scriptsLibSrc, entry); + if (fs.statSync(srcFile).isFile()) { + fs.copyFileSync(srcFile, path.join(scriptsLibDest, entry)); + } + } + // Verify the critical dep cli-exit.cjs landed + if (!verifyFileInstalled(path.join(scriptsLibDest, 'cli-exit.cjs'), 'scripts/lib/cli-exit.cjs')) { + failures.push('scripts/lib/cli-exit.cjs'); + } + } + if (verifyFileInstalled(path.join(changesetDest, 'cli.cjs'), 'scripts/changeset/cli.cjs')) { + console.log(` ${green}✓${reset} Installed scripts/changeset/ (changelog preview CLI)`); + } else { + failures.push('scripts/changeset/cli.cjs'); + } + } + // Remove legacy get-shit-done-cc artifacts and stale update caches (#607). // cleanupLegacyGsdCc handles both the legacy shared cache and the per-package // cache (formerly an inline unlinkSync here). A cleanup failure must never diff --git a/commands/gsd/plan-review-convergence.md b/commands/gsd/plan-review-convergence.md index e75eaf46b..c13defcf9 100644 --- a/commands/gsd/plan-review-convergence.md +++ b/commands/gsd/plan-review-convergence.md @@ -17,11 +17,11 @@ requires: [phase, review] Cross-AI plan convergence loop — an outer revision gate around gsd-review and gsd-planner. Repeatedly: review plans with external AI CLIs → if HIGH concerns found → replan with --reviews feedback → re-review. Stops when no HIGH concerns remain or max cycles reached. -**Flow:** Agent→Skill("gsd-plan-phase") → Agent→Skill("gsd-review") → check HIGHs → Agent→Skill("gsd-plan-phase --reviews") → Agent→Skill("gsd-review") → ... → Converge or escalate +**Flow:** Skill("gsd-plan-phase") → Agent→Skill("gsd-review") → check HIGHs → Skill("gsd-plan-phase --reviews") → Agent→Skill("gsd-review") → ... → Converge or escalate -Replaces gsd-plan-phase's internal gsd-plan-checker with external AI reviewers (codex, gemini, etc.). Each step runs inside an isolated Agent that calls the corresponding existing Skill — orchestrator only does loop control. +Replaces gsd-plan-phase's internal gsd-plan-checker with external AI reviewers (codex, gemini, etc.). Plan-phase runs **inline** (bare Skill at depth 0) so it can spawn gsd-planner/gsd-plan-checker at depth 1. Review runs inside an isolated Agent (gsd-review is a Bash leaf — no sub-agents needed). Orchestrator only does loop control. -**Orchestrator role:** Parse arguments, validate phase, spawn Agents for existing Skills, check HIGHs, stall detection, escalation gate. +**Orchestrator role:** Parse arguments, validate phase, run plan-phase inline (Skill at depth 0), spawn an Agent for gsd-review, check HIGHs, stall detection, escalation gate. diff --git a/eslint.config.mjs b/eslint.config.mjs index d88a62b24..a94b33abe 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -75,6 +75,7 @@ export default tseslint.config( 'gsd-core/bin/lib/model-profiles.cjs', 'gsd-core/bin/lib/installer-migrations/002-codex-legacy-hooks-json.cjs', 'gsd-core/bin/lib/installer-migrations/003-rename-get-shit-done-to-gsd-core.cjs', + 'gsd-core/bin/lib/installer-migrations/004-prune-stale-pristine-snapshots.cjs', 'gsd-core/bin/lib/observability/logger.cjs', 'gsd-core/bin/lib/active-workstream-store.cjs', 'gsd-core/bin/lib/adr-parser.cjs', diff --git a/gemini-extension.json b/gemini-extension.json index 23cbd6dc8..29f593ed8 100644 --- a/gemini-extension.json +++ b/gemini-extension.json @@ -1,6 +1,6 @@ { "name": "gsd-core", - "version": "1.4.2", + "version": "1.4.3", "description": "GSD Core — a meta-prompting, context engineering, and spec-driven development system for AI coding agents. Loads gsd's operating context into every Gemini CLI session.", "contextFileName": "GEMINI.md" } diff --git a/gsd-core/bin/verify-reapply-patches.cjs b/gsd-core/bin/verify-reapply-patches.cjs index 79e7965bb..a495337c6 100755 --- a/gsd-core/bin/verify-reapply-patches.cjs +++ b/gsd-core/bin/verify-reapply-patches.cjs @@ -165,6 +165,19 @@ const REASON = Object.freeze({ // resolve this file; the guard here ensures the gate does not report spurious // failures in the meantime. OK_PRISTINE_DRIFT_DETECTED: 'ok_pristine_drift_detected', + // Bug #934: backup-meta.json records a pristine_hash for this file but the + // gsd-pristine/ file is absent from disk. This happens on post-#604-rename + // installs where saveLocalPatches discarded the only pristine candidate + // because its hash did not match the old-release hash (the file changed + // upstream between releases). Without a baseline the verifier cannot + // distinguish user-added lines from upstream-changed lines, so falling to + // over-broad mode would produce FAIL_USER_LINES_MISSING false positives for + // every upstream-removed line. The correct posture is advisory/non-blocking: + // report OK_NO_BASELINE so the caller can log a warning without halting the + // gate on a spurious failure. This is a bounded "cannot reason → do not + // block" rather than "ignore everything" — it only applies when the hash was + // recorded (modern installer) but the file is absent (specific gap). + OK_NO_BASELINE: 'ok_no_baseline', FAIL_INSTALLED_MISSING: 'fail_installed_missing', FAIL_INSTALLED_NOT_REGULAR_FILE: 'fail_installed_not_regular_file', FAIL_READ_ERROR: 'fail_read_error', @@ -208,11 +221,25 @@ function verifyFile({ relPath, patchesDir, configDir, pristineDir, pristineHashe return result; } + // Normalize to forward slashes so the key lookup matches on Windows + // where path.join produces backslash-separated relPath values but + // backup-meta.json stores keys written with forward slashes. + const hashKey = relPath.replace(/\\/g, '/'); + const recordedHash = pristineHashes && pristineHashes[hashKey]; + let pristineContent = null; if (pristineDir) { const pristinePath = path.join(pristineDir, relPath); + // Bug #934: track whether the pristine path EXISTS on disk (stat did not + // throw ENOENT). A regular file that fails to read, or a non-file path + // (e.g. a directory accidentally placed at the pristine path), is treated + // as "present but unusable" — we fall to over-broad mode (safe side). + // OK_NO_BASELINE is reserved for the strictly absent case: stat throws, + // meaning the file was never written (the gap the bug describes). + let pristinePathExists = false; try { const stat = fs.statSync(pristinePath); + pristinePathExists = true; // path exists (any type) if (stat.isFile()) { const candidate = fs.readFileSync(pristinePath, 'utf8'); // Bug #3657: if backup-meta.json recorded a pristine_hash for this @@ -226,11 +253,6 @@ function verifyFile({ relPath, patchesDir, configDir, pristineDir, pristineHashe // Over-broad mode never false-fails for a different reason because all // backup lines that are genuinely user-added will still be present in a // correctly merged install. - // Normalize to forward slashes so the key lookup matches on Windows - // where path.join produces backslash-separated relPath values but - // backup-meta.json stores keys written with forward slashes. - const hashKey = relPath.replace(/\\/g, '/'); - const recordedHash = pristineHashes && pristineHashes[hashKey]; if (recordedHash) { if (sha256(candidate) === recordedHash) { // Hash matches: the on-disk pristine is the correct baseline. @@ -251,8 +273,28 @@ function verifyFile({ relPath, patchesDir, configDir, pristineDir, pristineHashe pristineContent = candidate; } } + // Non-file at pristinePath (e.g. a directory): stat succeeded so + // pristinePathExists is true; we fall through to over-broad mode below, + // which is safe and conservative. } catch { - // Pristine missing or unreadable — fall through to over-broad mode. + // Pristine stat threw — path is absent (ENOENT) or inaccessible. + // pristinePathExists stays false. + } + + // Bug #934: recordedHash is present (modern installer) but the pristine + // path does not exist on disk at all (stat threw above). This means + // saveLocalPatches recorded a hash but could not write the corresponding + // gsd-pristine/ file (the only candidate was discarded because it was from + // a newer release). Falling to over-broad mode here would treat every + // upstream-changed line as a "user-added line that must survive", producing + // false FAIL_USER_LINES_MISSING for each upstream removal. Since we + // cannot reason correctly without a baseline, the safe answer is advisory/ + // non-blocking: return OK_NO_BASELINE and let the caller decide. + // NOTE: this guard fires ONLY when stat threw (path absent), not when the + // path is present but non-file — in that case over-broad mode is safer. + if (!pristinePathExists && recordedHash) { + result.reason = REASON.OK_NO_BASELINE; + return result; } } @@ -316,9 +358,17 @@ function main() { const drifted = driftedResults.length; const drifted_files = driftedResults.map((r) => r.file); + // Bug #934: aggregate no-baseline files into top-level report fields so the + // workflow can log a warning about files that could not be verified. Like + // drift, this is NOT a failure (exit code stays 0) but gives the caller + // structured data to surface the advisory condition. + const noBaselineResults = results.filter((r) => r.reason === REASON.OK_NO_BASELINE); + const no_baseline = noBaselineResults.length; + const no_baseline_files = noBaselineResults.map((r) => r.file); + if (opts.json) { process.stdout.write( - JSON.stringify({ checked: results.length, failures: failures.length, drifted, drifted_files, results }, null, 2) + '\n', + JSON.stringify({ checked: results.length, failures: failures.length, drifted, drifted_files, no_baseline, no_baseline_files, results }, null, 2) + '\n', ); } else { process.stdout.write(`# Hunk Verification Gate (#2969)\n\n`); diff --git a/gsd-core/workflows/plan-review-convergence.md b/gsd-core/workflows/plan-review-convergence.md index 953a2f350..552278d64 100644 --- a/gsd-core/workflows/plan-review-convergence.md +++ b/gsd-core/workflows/plan-review-convergence.md @@ -1,7 +1,8 @@ Cross-AI plan convergence loop — automates the manual chain: gsd-plan-phase N → gsd-review N --codex → gsd-plan-phase N --reviews → gsd-review N --codex → ... -Each step runs inside an isolated Agent that calls the corresponding Skill. +Plan-phase runs inline (bare Skill at depth 0) so it can spawn gsd-planner/gsd-plan-checker at depth 1. +Review runs inside an isolated Agent (leaf skill — Bash only, no sub-agents needed). Orchestrator only does: init, loop control, parse CYCLE_SUMMARY for HIGH count, stall detection, escalation. @@ -98,21 +99,15 @@ Display startup banner: **If `has_plans` is false:** -Display: `◆ No plans found — spawning initial planning agent... (runs in a subagent — no output until it returns, ~1–5 min; expected, not a freeze)` +Display: `◆ No plans found — running initial planning inline... (plan-phase runs here in the orchestrator — no output until planning is complete, ~1–5 min; expected, not a freeze)` ```text -Agent( - description="Initial planning Phase {PHASE}", - prompt="Run /gsd:plan-phase for Phase {PHASE}. - -Execute: Skill(skill='gsd-plan-phase', args='{PHASE} {GSD_WS}') - -Complete the full planning workflow. Do NOT return until planning is complete and PLAN.md files are committed.", - mode="auto" -) +Skill(skill="gsd-plan-phase", args="{PHASE} {GSD_WS}") ``` -After agent returns, verify plans were created: +Run plan-phase **inline** (do NOT wrap it in Agent()). The convergence orchestrator runs at depth 0 with Agent available, so inline plan-phase can spawn gsd-planner and gsd-plan-checker at depth 1 — the one level of nesting that works on Claude Code. Wrapping plan-phase in Agent() would push it to depth 1 where the Agent tool is absent, preventing it from spawning any sub-agents. Wait until plan-phase completes and PLAN.md files are committed before continuing. + +After plan-phase completes, verify plans were created: ```bash PLAN_COUNT=$(ls ${phase_dir}/${padded_phase}-*-PLAN.md 2>/dev/null | wc -l) ``` @@ -302,44 +297,35 @@ To restart loop: /gsd:plan-review-convergence {PHASE} {REVIEWER_FLAGS} ``` Exit workflow. -### 5d. Replan (Spawn Agent) +### 5d. Replan (Inline) **If under max cycles:** Update `prev_high_count = HIGH_COUNT`. -Display: `◆ Spawning replan agent with review feedback... (runs in a subagent — no output until it returns, ~1–5 min; expected, not a freeze)` +Display: `◆ Replanning inline with review feedback... (plan-phase runs here in the orchestrator — no output until replanning is complete, ~1–5 min; expected, not a freeze)` ```text -Agent( - description="Replan Phase {PHASE} with review feedback cycle {cycle}", - prompt="Run /gsd:plan-phase with --reviews for Phase {PHASE}. - -Execute: Skill(skill='gsd-plan-phase', args='{PHASE} --reviews --skip-research {GSD_WS}') - -This will replan incorporating cross-AI review feedback from REVIEWS.md. -Do NOT return until replanning is complete and updated PLAN.md files are committed. - -IMPORTANT: When gsd-plan-phase outputs '## PLANNING COMPLETE', that means replanning is done. Return at that point.", - mode="auto" -) +Skill(skill="gsd-plan-phase", args="{PHASE} --reviews --skip-research {GSD_WS}") ``` -After agent returns → go back to **step 5a** (review again). +Run plan-phase **inline** (do NOT wrap it in Agent()). Same rationale as step 4: the convergence orchestrator runs at depth 0 with Agent available, so inline plan-phase can spawn gsd-planner and gsd-plan-checker at depth 1. Wrapping in Agent() pushes plan-phase to depth 1 where the Agent tool is absent — the replan loop can never produce a revised plan when HIGHs are found. This is the root cause of bug #936. Wait until plan-phase completes (outputs '## PLANNING COMPLETE') and updated PLAN.md files are committed before continuing. + +After plan-phase completes → go back to **step 5a** (review again). - [ ] Config gate checked before running — exits with enable instructions if workflow.plan_review_convergence is false -- [ ] Initial planning via Agent → Skill("gsd-plan-phase") if no plans exist -- [ ] Review via Agent → Skill("gsd-review") — isolated, not inline; {GSD_WS} forwarded -- [ ] Replan via Agent → Skill("gsd-plan-phase --reviews") — isolated, not inline +- [ ] Initial planning via inline Skill("gsd-plan-phase") if no plans exist — NOT wrapped in Agent() (bug #936: depth-1 Agent has no Agent tool) +- [ ] Review via Agent → Skill("gsd-review") — isolated Agent is correct; gsd-review is a Bash leaf with no sub-agent spawns; {GSD_WS} forwarded +- [ ] Replan via inline Skill("gsd-plan-phase --reviews") — NOT wrapped in Agent(); inline lets plan-phase spawn gsd-planner/gsd-plan-checker at depth 1 - [ ] Orchestrator only does: init, config gate, loop control, parse CYCLE_SUMMARY for HIGH count, stall detection, escalation - [ ] HIGH count extracted from review agent's CYCLE_SUMMARY return message (not by grepping REVIEWS.md) - [ ] Review agent prompt defines CYCLE_SUMMARY: current_high= contract with PARTIALLY/FULLY RESOLVED definitions - [ ] Abort with clear error if CYCLE_SUMMARY is absent; distinguish malformed from absent - [ ] Warn if HIGH_COUNT > 0 but ## Current HIGH Concerns section is absent from return message -- [ ] Each Agent fully completes its Skill before returning +- [ ] The review Agent fully completes gsd-review before returning (plan-phase runs inline — no Agent wrap) - [ ] Loop exits on: no HIGH concerns (converged) OR max cycles (escalation) - [ ] Stall detection reported when HIGH count not decreasing - [ ] STATE.md updated on convergence completion diff --git a/gsd-core/workflows/reapply-patches.md b/gsd-core/workflows/reapply-patches.md index b69775eda..94494b33a 100644 --- a/gsd-core/workflows/reapply-patches.md +++ b/gsd-core/workflows/reapply-patches.md @@ -299,11 +299,28 @@ VERIFY_OUTPUT="$(node "${GSD_HOME}/gsd-core/bin/verify-reapply-patches.cjs" "${V VERIFY_STATUS=$? ``` -**Step 5a: drift check** — even when `VERIFY_STATUS` is 0, the report may signal that one or more files were skipped due to pristine-snapshot drift (Bug #3657). Parse the JSON and check: +**Step 5a: drift check** — even when `VERIFY_STATUS` is 0, the report may signal that one or more files were skipped due to pristine-snapshot drift (Bug #3657) or a missing baseline (Bug #934). Parse the JSON and check: ```bash DRIFTED_COUNT="$(echo "$VERIFY_OUTPUT" | node -e "const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8'));process.stdout.write(String(d.drifted||0))")" DRIFTED_FILES="$(echo "$VERIFY_OUTPUT" | node -e "const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8'));(d.drifted_files||[]).forEach(f=>process.stdout.write(f+'\n'))")" +NO_BASELINE_COUNT="$(echo "$VERIFY_OUTPUT" | node -e "const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8'));process.stdout.write(String(d.no_baseline||0))")" +NO_BASELINE_FILES="$(echo "$VERIFY_OUTPUT" | node -e "const d=JSON.parse(require('fs').readFileSync('/dev/stdin','utf8'));(d.no_baseline_files||[]).forEach(f=>process.stdout.write(f+'\n'))")" +``` + +**If `NO_BASELINE_COUNT` is greater than 0**, emit an advisory warning (non-blocking — the gate still exits 0 for these files). Do NOT halt: + +```text +ADVISORY: {NO_BASELINE_COUNT} file(s) could not be diff-verified because no pristine +baseline exists on disk despite a hash being recorded in backup-meta.json (Bug #934: +the installer discarded the only pristine candidate because it was from a newer release). +These files were skipped rather than false-failed; their user customisations may or +may not have survived the merge. + +Unverified files: + {each path in NO_BASELINE_FILES, one per line, indented two spaces} + +Recommended: manually inspect each file above and confirm your customisations survived. ``` **If `DRIFTED_COUNT` is greater than 0**, STOP and report to the user, then set `DRIFT_DETECTED=true` and halt — do not proceed to 5b or cleanup: diff --git a/gsd-core/workflows/update.md b/gsd-core/workflows/update.md index 4ff98df1c..16ffb9be0 100644 --- a/gsd-core/workflows/update.md +++ b/gsd-core/workflows/update.md @@ -195,24 +195,29 @@ CHANGELOG_TMP="/tmp/gsd-changelog-$$.md" curl -fsSL "https://raw.githubusercontent.com/open-gsd/gsd-core/main/CHANGELOG.md" -o "$CHANGELOG_TMP" 2>/dev/null \ || wget -qO "$CHANGELOG_TMP" "https://raw.githubusercontent.com/open-gsd/gsd-core/main/CHANGELOG.md" 2>/dev/null -EXTRACT_JSON=$(node "$GSD_DIR/gsd-core/scripts/changeset/cli.cjs" extract \ - --from "$INSTALLED_VERSION" \ - --to "$LATEST_VERSION" \ - --changelog "$CHANGELOG_TMP" \ - --json 2>/dev/null) -EXTRACT_EXIT=$? - -if [ "$EXTRACT_EXIT" -eq 2 ]; then - # Exit 2 = no releases in range (e.g. versions are equal or changelog is sparse) - CHANGELOG_PREVIEW="No changelog updates between v${INSTALLED_VERSION} and v${LATEST_VERSION}." -elif [ "$EXTRACT_EXIT" -ne 0 ] || [ -z "$EXTRACT_JSON" ]; then - CHANGELOG_PREVIEW="(Could not extract changelog — update will still proceed)" +GSD_CHANGESET_CLI="$GSD_DIR/scripts/changeset/cli.cjs" +if [ ! -f "$GSD_CHANGESET_CLI" ]; then + CHANGELOG_PREVIEW="(Changelog CLI not found at $GSD_CHANGESET_CLI — reinstall GSD to restore preview. Update will still proceed.)" else - # Re-run without --json to get the human-readable markdown for display - CHANGELOG_PREVIEW=$(node "$GSD_DIR/gsd-core/scripts/changeset/cli.cjs" extract \ + EXTRACT_JSON=$(node "$GSD_CHANGESET_CLI" extract \ --from "$INSTALLED_VERSION" \ --to "$LATEST_VERSION" \ - --changelog "$CHANGELOG_TMP" 2>/dev/null || echo "(changelog unavailable)") + --changelog "$CHANGELOG_TMP" \ + --json 2>&1) + EXTRACT_EXIT=$? + + if [ "$EXTRACT_EXIT" -eq 2 ]; then + # Exit 2 = no releases in range (e.g. versions are equal or changelog is sparse) + CHANGELOG_PREVIEW="No changelog updates between v${INSTALLED_VERSION} and v${LATEST_VERSION}." + elif [ "$EXTRACT_EXIT" -ne 0 ] || [ -z "$EXTRACT_JSON" ]; then + CHANGELOG_PREVIEW="(Could not extract changelog — update will still proceed)" + else + # Re-run without --json to get the human-readable markdown for display + CHANGELOG_PREVIEW=$(node "$GSD_CHANGESET_CLI" extract \ + --from "$INSTALLED_VERSION" \ + --to "$LATEST_VERSION" \ + --changelog "$CHANGELOG_TMP" 2>/dev/null || echo "(changelog unavailable)") + fi fi # Clean up temp changelog now that both extract runs are done rm -f "$CHANGELOG_TMP" diff --git a/package-lock.json b/package-lock.json index 56835eeb4..e2ecfdde2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@opengsd/gsd-core", - "version": "1.4.2", + "version": "1.4.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@opengsd/gsd-core", - "version": "1.4.2", + "version": "1.4.3", "license": "MIT", "dependencies": { "@anthropic-ai/claude-agent-sdk": "^0.2.84", diff --git a/package.json b/package.json index c5a4c7ad1..1dc75d00a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@opengsd/gsd-core", - "version": "1.4.2", + "version": "1.4.3", "description": "GSD Core is a meta-prompting, context engineering, and spec-driven development system for AI coding agents.", "bin": { "gsd-core": "bin/install.js", diff --git a/scripts/changeset/cli.cjs b/scripts/changeset/cli.cjs index 92fc65af8..9ddcb667c 100755 --- a/scripts/changeset/cli.cjs +++ b/scripts/changeset/cli.cjs @@ -324,7 +324,7 @@ function resolveChangelogPath(opts) { * 1 — I/O error or missing required flags. * * Fix for #3496: provides a deterministic range-aware helper so the - * `/gsd:update` show_changes_and_confirm step no longer relies on + * `/gsd-update` show_changes_and_confirm step no longer relies on * vague/manual extraction that can silently skip intermediate versions. */ function cmdExtract(opts) { diff --git a/src/installer-migrations/004-prune-stale-pristine-snapshots.cts b/src/installer-migrations/004-prune-stale-pristine-snapshots.cts new file mode 100644 index 000000000..c239da07a --- /dev/null +++ b/src/installer-migrations/004-prune-stale-pristine-snapshots.cts @@ -0,0 +1,145 @@ +/** + * Installer migration 004: remove stale gsd-pristine/get-shit-done/ snapshot // gsd-allow-legacy-name + * files after the get-shit-done → gsd-core rename (#604, #934). // gsd-allow-legacy-name + * + * Background: migration 003 removed legacy runtime files from + * get-shit-done/ but did not touch gsd-pristine/get-shit-done/, the // gsd-allow-legacy-name + * parallel directory that holds pristine snapshots captured before the rename. + * These snapshot files are GSD-managed (written by the installer, never by the + * user) and reference stale get-shit-done/... key paths that no longer exist // gsd-allow-legacy-name + * in the active layout. When verify-reapply-patches.cjs looks up a backup entry + * keyed under gsd-core/... it finds no matching gsd-pristine/ snapshot, falls + * to over-broad mode, and reports false FAIL_INSTALLED_MISSING / // gsd-allow-legacy-name + * FAIL_USER_LINES_MISSING for every backed-up pre-rename file (#934). + * + * Fix: walk gsd-pristine/get-shit-done/ and emit remove-managed for each file. // gsd-allow-legacy-name + * These files are always GSD-written snapshots — users never place their own + * files inside gsd-pristine/ — so the classification override + * (managed-pristine) is safe: there is no user content to protect. + * + * Checksum safety: migration 003's body is left untouched. Adding this + * separate migration avoids modifying 003's checksum, which would break + * upgrade state for any user who already applied 003 (root cause of #670). + * + * Per-file approach: the migration framework has no recursive directory-removal + * primitive — all actions operate on individual files. Empty directory shells + * left after removal can be cleaned up manually; this is the intentional ADR-0008 + * limitation. + */ + +import fs from 'node:fs'; +import path from 'node:path'; + +interface MigrationAction { + type: string; + relPath: string; + reason: string; + ownershipEvidence: string; + classification?: string; +} + +interface MigrationPlanContext { + configDir: string; + classifyArtifact(relPath: string): { classification: string; [key: string]: unknown }; +} + +interface InstallerMigration { + id: string; + title: string; + description: string; + introducedIn: string; + scopes: string[]; + destructive: boolean; + plan(ctx: MigrationPlanContext): MigrationAction[]; +} + +function walkPristineFiles(root: string, relDir: string, baseResolved: string, results: string[]): void { + const dir = path.join(root, relDir); + let entries; + try { + entries = fs.readdirSync(dir, { withFileTypes: true }); + } catch { + return; // directory absent or unreadable — nothing to do + } + for (const entry of entries) { + // Do not follow symlinks — skip to avoid out-of-tree traversal. + if (entry.isSymbolicLink()) continue; + const relPath = path.posix.join(relDir, entry.name); + // Bounds check: ensure the resolved path stays under configDir. + const resolved = path.resolve(root, relPath); + if (resolved !== baseResolved && !resolved.startsWith(baseResolved + path.sep)) continue; + if (entry.isDirectory()) { + walkPristineFiles(root, relPath, baseResolved, results); + } else if (entry.isFile()) { + results.push(relPath); + } + } +} + +const REASON = 'stale pristine snapshot from legacy get-shit-done/ dir, orphaned by rename migration 003 (#604, #934)'; // gsd-allow-legacy-name + +const migration: InstallerMigration = { + id: '2026-06-09-prune-stale-pristine-get-shit-done', // gsd-allow-legacy-name + title: 'Remove stale gsd-pristine/get-shit-done/ snapshot files (#934)', // gsd-allow-legacy-name + description: + 'Migration 003 removed runtime files from get-shit-done/ but left the matching pristine snapshot ' + // gsd-allow-legacy-name + 'directory gsd-pristine/get-shit-done/ intact. Those snapshots reference stale key paths and cause ' + // gsd-allow-legacy-name + 'verify-reapply-patches false positives (#934). Remove all files under gsd-pristine/get-shit-done/ ' + // gsd-allow-legacy-name + 'as they are GSD-managed snapshots, never user content.', + introducedIn: '1.4.3', + scopes: ['global', 'local'], + destructive: true, + plan(ctx: MigrationPlanContext): MigrationAction[] { + const pristineGsdRoot = path.join(ctx.configDir, 'gsd-pristine', 'get-shit-done'); // gsd-allow-legacy-name + + // Idempotency: if the stale pristine subdir doesn't exist, nothing to do. + if (!fs.existsSync(pristineGsdRoot)) return []; + + // Safety: reject symlinks in ANY ancestor component of the path we will walk + // to prevent following a symlink out of configDir. Check both gsd-pristine/ + // and gsd-pristine/get-shit-done/ — either being a symlink could redirect // gsd-allow-legacy-name + // the walk to an out-of-tree location. + const pristineParent = path.join(ctx.configDir, 'gsd-pristine'); + try { + if (fs.lstatSync(pristineParent).isSymbolicLink()) return []; + } catch { + return []; + } + try { + if (fs.lstatSync(pristineGsdRoot).isSymbolicLink()) return []; // gsd-allow-legacy-name + } catch { + return []; + } + + const baseResolved = path.resolve(ctx.configDir); + const relPaths: string[] = []; + walkPristineFiles(ctx.configDir, path.posix.join('gsd-pristine', 'get-shit-done'), baseResolved, relPaths); // gsd-allow-legacy-name + + const actions: MigrationAction[] = []; + for (const relPath of relPaths) { + // Bounds-check each relPath before emitting any action. + const resolved = path.resolve(ctx.configDir, relPath); + if (resolved !== baseResolved && !resolved.startsWith(baseResolved + path.sep)) continue; + + // These files are GSD-managed pristine snapshots — the installer writes + // them during install/upgrade; users never place personal files inside + // gsd-pristine/. Pass classification: 'managed-pristine' explicitly so + // the framework does not downgrade remove-managed to preserve-user when + // the manifest has no entry (these paths were never in the manifest since + // they live under gsd-pristine/, not the tracked runtime dir). + actions.push({ + type: 'remove-managed', + relPath, + reason: REASON, + ownershipEvidence: + 'GSD-written pristine snapshot under gsd-pristine/get-shit-done/; ' + // gsd-allow-legacy-name + 'installer is the sole author of gsd-pristine/ contents; no user content lives here', + classification: 'managed-pristine', + }); + } + + return actions; + }, +}; + +export = migration; diff --git a/tests/bug-2969-verify-reapply-patches.test.cjs b/tests/bug-2969-verify-reapply-patches.test.cjs index b9746068b..918c147f4 100644 --- a/tests/bug-2969-verify-reapply-patches.test.cjs +++ b/tests/bug-2969-verify-reapply-patches.test.cjs @@ -88,6 +88,7 @@ describe('Bug #2969: deterministic Step 5 verification gate', () => { // Locks the public diagnostic surface — adding a code requires updating // this assertion, removing one breaks consumers that switch on the enum. // Bug #3657 added OK_PRISTINE_DRIFT_DETECTED. + // Bug #934 added OK_NO_BASELINE. assert.deepEqual( Object.keys(REASON).sort(), [ @@ -95,6 +96,7 @@ describe('Bug #2969: deterministic Step 5 verification gate', () => { 'FAIL_INSTALLED_NOT_REGULAR_FILE', 'FAIL_READ_ERROR', 'FAIL_USER_LINES_MISSING', + 'OK_NO_BASELINE', 'OK_NO_SIGNIFICANT_BACKUP_LINES', 'OK_NO_USER_LINES_VS_PRISTINE', 'OK_PRISTINE_DRIFT_DETECTED', @@ -178,7 +180,8 @@ describe('Bug #2969: deterministic Step 5 verification gate', () => { assert.equal(status, 1); // Bug #3657 (Finding 1): drifted + drifted_files are additive fields added to surface // pristine-drift skips distinctly from failures. Shape-lock updated to include them. - assert.deepEqual(Object.keys(report).sort(), ['checked', 'drifted', 'drifted_files', 'failures', 'results']); + // Bug #934: no_baseline + no_baseline_files are additive fields for missing-pristine advisory. + assert.deepEqual(Object.keys(report).sort(), ['checked', 'drifted', 'drifted_files', 'failures', 'no_baseline', 'no_baseline_files', 'results']); const r0 = report.results[0]; assert.deepEqual(Object.keys(r0).sort(), ['file', 'missing', 'reason', 'status']); assert.equal(typeof r0.file, 'string'); diff --git a/tests/bug-3657-verify-reapply-patches-pristine-drift.test.cjs b/tests/bug-3657-verify-reapply-patches-pristine-drift.test.cjs index 10293dc6f..463f3d6a8 100644 --- a/tests/bug-3657-verify-reapply-patches-pristine-drift.test.cjs +++ b/tests/bug-3657-verify-reapply-patches-pristine-drift.test.cjs @@ -332,6 +332,7 @@ describe('Bug #3657: pristine-drift does not produce false FAIL_USER_LINES_MISSI 'FAIL_INSTALLED_NOT_REGULAR_FILE', 'FAIL_READ_ERROR', 'FAIL_USER_LINES_MISSING', + 'OK_NO_BASELINE', 'OK_NO_SIGNIFICANT_BACKUP_LINES', 'OK_NO_USER_LINES_VS_PRISTINE', 'OK_PRISTINE_DRIFT_DETECTED', @@ -525,3 +526,156 @@ describe('Bug #3657: pristine-drift does not produce false FAIL_USER_LINES_MISSI ); }); }); + +// --------------------------------------------------------------------------- +// Bug #934: OK_NO_BASELINE — pristine dir provided, hash recorded, but file absent +// --------------------------------------------------------------------------- + +describe('Bug #934: OK_NO_BASELINE when recordedHash present but pristine file absent', () => { + + /** + * Core regression: backup-meta.json has a pristine_hash for the file but + * the gsd-pristine/ snapshot is absent from disk (the installer's + * saveLocalPatches discarded the only candidate because its hash did not + * match the old-release hash — the file changed upstream between releases). + * Without the fix the verifier falls to over-broad mode and treats every + * upstream-removed line as a "user-added line that must survive", producing + * FAIL_USER_LINES_MISSING false positives. + * With the fix the verifier returns OK_NO_BASELINE (non-blocking, advisory). + */ + test('exits 0 with reason=OK_NO_BASELINE when recordedHash present but pristine absent', () => { + resetFixture(); + + const FILE = 'gsd-core/workflows/execute-phase.md'; + + // The backup contains both the old upstream content and the user's line. + const backupContent = + 'upstream line that was present in 1.4.0 but removed in 1.4.2 release\n' + + 'another upstream line removed upstream between gsd-core releases here\n' + + 'model: sonnet in frontmatter — this is the real user customisation line\n'; + + // The installed file has the new upstream content + the user's real line. + const installedContent = + 'brand-new upstream line that replaced the old content in gsd-core 1.4.2\n' + + 'model: sonnet in frontmatter — this is the real user customisation line\n'; + + // backup-meta.json records a hash (modern installer) but gsd-pristine/ is absent. + writeBackupMeta({ pristine_hashes: { [FILE]: 'sha256:deadbeef00000000000000000000000000000000000000000000000000000001' } }); + writeFile(path.join(patchesDir, FILE), backupContent); + writeFile(path.join(configDir, FILE), installedContent); + // Deliberately do NOT write a pristine file — this is the gap-1 scenario. + + const { status, report } = runVerifier(); + + // Must exit 0: cannot reason without baseline → non-blocking advisory. + assert.equal(status, 0, `expected exit 0; got ${status}; report=${JSON.stringify(report)}`); + assert.equal(report.failures, 0, `expected 0 failures; got ${report.failures}`); + const r0 = report.results[0]; + assert.equal(r0.status, 'ok', `expected status ok; got ${r0.status}`); + assert.equal(r0.reason, REASON.OK_NO_BASELINE, + `expected OK_NO_BASELINE; got ${r0.reason}`); + assert.deepEqual(r0.missing, []); + }); + + /** + * Counter-test: when pristine is absent but NO recordedHash is present + * (pre-fix installer that never wrote backup-meta.json), the verifier must + * still fall to over-broad mode — the old behaviour for untracked backups. + * OK_NO_BASELINE must NOT fire in this case. + */ + test('falls through to over-broad mode when pristine absent AND no recordedHash', () => { + resetFixture(); + + const FILE = 'gsd-core/workflows/plan-phase.md'; + const droppedLine = 'user-added instruction that was dropped from the install output'; + const backupContent = + 'stock upstream line long enough to be significant in the file\n' + + droppedLine + '\n'; + const installedContent = 'stock upstream line long enough to be significant in the file\n'; + + // No backup-meta.json — simulates pre-fix installer with no hash records. + writeFile(path.join(patchesDir, FILE), backupContent); + writeFile(path.join(configDir, FILE), installedContent); + // No pristine file. + + const { status, report } = runVerifier(); + + // Over-broad mode catches the genuinely dropped user line. + assert.equal(status, 1, 'over-broad mode should catch the dropped user line'); + assert.equal(report.failures, 1); + const r0 = report.results[0]; + assert.equal(r0.status, 'fail'); + assert.equal(r0.reason, REASON.FAIL_USER_LINES_MISSING); + assert.ok(r0.missing.includes(droppedLine), + `dropped line must appear in .missing[]; got ${JSON.stringify(r0.missing)}`); + // Must NOT be OK_NO_BASELINE — that only fires when a hash WAS recorded. + assert.notEqual(r0.reason, REASON.OK_NO_BASELINE); + }); + + /** + * Presence check: when pristine IS present AND hash matches, the normal + * flow must proceed (not short-circuit to OK_NO_BASELINE). + * A real dropped user line must still be caught. + */ + test('does not short-circuit to OK_NO_BASELINE when pristine exists and hash matches', () => { + resetFixture(); + + const FILE = 'gsd-core/workflows/plan-phase.md'; + const pristineContent = 'stock upstream line long enough to be significant content\n'; + const droppedLine = 'user customisation that was genuinely dropped from the merged output'; + const backupContent = pristineContent + droppedLine + '\n'; + const installedContent = pristineContent; // user line dropped — real failure + + writeBackupMeta({ pristine_hashes: { [FILE]: sha256(pristineContent) } }); + writeFile(path.join(patchesDir, FILE), backupContent); + writeFile(path.join(configDir, FILE), installedContent); + writeFile(path.join(pristineDir, FILE), pristineContent); + + const { status, report } = runVerifier(); + + assert.equal(status, 1, 'real dropped user line must be caught'); + assert.equal(report.failures, 1); + const r0 = report.results[0]; + assert.equal(r0.status, 'fail'); + assert.equal(r0.reason, REASON.FAIL_USER_LINES_MISSING); + assert.notEqual(r0.reason, REASON.OK_NO_BASELINE); + assert.ok(r0.missing.includes(droppedLine)); + }); + + /** + * When --pristine-dir is NOT provided at all (old CLI invocation without the + * flag), the OK_NO_BASELINE path must never fire — there is no pristine dir + * context to consult and the old over-broad behaviour must be preserved. + */ + test('does not return OK_NO_BASELINE when --pristine-dir is not provided', () => { + resetFixture(); + + const FILE = 'gsd-core/workflows/execute-phase.md'; + const backupContent = + 'upstream line removed in newer version but present in backup\n' + + 'model: sonnet — user customisation line in the backup file\n'; + const installedContent = + 'replacement upstream line in the newer release version\n' + + 'model: sonnet — user customisation line in the backup file\n'; + + // Record a hash — but no pristine dir will be passed to the verifier. + writeBackupMeta({ pristine_hashes: { [FILE]: 'sha256:deadbeef00000000000000000000000000000000000000000000000000000001' } }); + writeFile(path.join(patchesDir, FILE), backupContent); + writeFile(path.join(configDir, FILE), installedContent); + + // Run without --pristine-dir flag. + const { status, report } = runVerifier({ pristine: false }); + + // Over-broad mode: every significant backup line is required. + // "upstream line removed in newer version but present in backup" is NOT in + // the installed content → over-broad mode FAILS this file (exit 1). + // OK_NO_BASELINE must NOT fire — there was no pristine dir to consult. + assert.equal(status, 1, `over-broad mode should fail (upstream-removed line absent); got ${status}`); + const r0 = report.results[0]; + assert.equal(r0.status, 'fail', `expected fail status; got ${r0.status}`); + assert.equal(r0.reason, REASON.FAIL_USER_LINES_MISSING, + `expected FAIL_USER_LINES_MISSING from over-broad mode; got ${r0.reason}`); + assert.notEqual(r0.reason, REASON.OK_NO_BASELINE, + `OK_NO_BASELINE must not fire when --pristine-dir is not provided`); + }); +}); diff --git a/tests/bug-936-no-nested-spawner-wrap.test.cjs b/tests/bug-936-no-nested-spawner-wrap.test.cjs new file mode 100644 index 000000000..27bb440bd --- /dev/null +++ b/tests/bug-936-no-nested-spawner-wrap.test.cjs @@ -0,0 +1,207 @@ +'use strict'; +/** + * Structural guard — bug(#936): plan-review-convergence wrapped gsd-plan-phase + * in Agent() at TWO sites (initial planning + replan). On Claude Code, a depth-1 + * Agent has no Agent tool, so plan-phase cannot spawn gsd-planner / gsd-plan-checker + * → the replan loop never works when HIGHs are found. + * + * Fix: run plan-phase INLINE (bare Skill()) from the convergence orchestrator, + * which runs at depth 0 and has Agent available — exactly how autonomous.md, + * manager.md, and discuss-phase-assumptions.md already chain plan-phase. + * + * This guard dynamically derives the set of "spawner" workflows (those containing + * `subagent_type=`) and asserts that NO workflow wraps a spawner inside Agent() + * UNLESS the wrapping block includes a RUNTIME != claude carve-out (the #853 + * pattern already applied to autonomous.md / manager.md). + */ + +// allow-test-rule: source-text-is-the-product +// The workflow markdown IS the runtime instruction — static guards over +// workflow text are the canonical regression-test mechanism (per CONTRIBUTING +// exception matrix and tests/bug-853-bg-dispatch-runtime-gating.test.cjs). + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const WORKFLOWS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows'); + +// ── 1. Derive spawner skill names dynamically ────────────────────────────── +// A "spawner" workflow is one that contains `subagent_type=` — it NEEDS the +// Agent tool to run and therefore cannot safely be wrapped in another Agent() +// on Claude Code (where depth-1 agents have no Agent tool). + +// Recursively collect all *.md files under WORKFLOWS_DIR (covers nested fragments +// like discuss-phase/modes/*.md and execute-phase/steps/*.md). +function collectWorkflowFiles(dir) { + const entries = fs.readdirSync(dir, { withFileTypes: true }); + const results = []; + for (const e of entries) { + const fullPath = path.join(dir, e.name); + if (e.isDirectory()) { + results.push(...collectWorkflowFiles(fullPath)); + } else if (e.name.endsWith('.md')) { + results.push({ + name: path.relative(WORKFLOWS_DIR, fullPath), + path: fullPath, + content: fs.readFileSync(fullPath, 'utf8'), + }); + } + } + return results; +} + +const allWorkflowFiles = collectWorkflowFiles(WORKFLOWS_DIR); + +// Map: base-slug → workflow filename (e.g. "plan-phase" → "plan-phase.md") +// Skill() calls use the "gsd-" convention in all workflow files. +// We build BOTH the bare slug set and the gsd-prefixed skill-name set. +const SPAWNER_BASE_SLUGS = new Set( + allWorkflowFiles + .filter((w) => w.content.includes('subagent_type=')) + .map((w) => w.name.replace(/\.md$/, '')) +); + +// Skill invocations use "gsd-" (e.g. gsd-plan-phase, gsd-execute-phase). +// Build the regex from the prefixed names so it actually matches what workflows write. +const SPAWNER_GSD_NAMES = new Set([...SPAWNER_BASE_SLUGS].map((s) => `gsd-${s}`)); + +// Build a regex that matches Skill(skill='gsd-') or Skill(skill="gsd-") +const spawnerPattern = new RegExp( + `Skill\\(\\s*skill=['"](?:${[...SPAWNER_GSD_NAMES].join('|')})['"]`, + 's' +); + +// ── 2. Helper: extract Agent() blocks from a workflow ───────────────────── +// Each block starts at "Agent(" and ends at the balancing ")". We collect +// the text of each such block together with the surrounding context (a 400 +// char window before the block) so we can check for RUNTIME carve-outs. + +function extractAgentBlocks(content) { + const blocks = []; + let pos = 0; + while (pos < content.length) { + const start = content.indexOf('Agent(', pos); + if (start === -1) break; + // Walk forward to find the balancing closing paren + let depth = 0; + let i = start + 'Agent('.length - 1; // at the '(' + for (; i < content.length; i++) { + if (content[i] === '(') depth++; + else if (content[i] === ')') { + depth--; + if (depth === 0) break; + } + } + const end = i + 1; + const blockText = content.slice(start, end); + // Capture context: 400 chars before the block (for RUNTIME gate detection) + const contextBefore = content.slice(Math.max(0, start - 400), start); + blocks.push({ start, end, blockText, contextBefore }); + pos = end; + } + return blocks; +} + +// ── 3. Helper: does a block have a RUNTIME != claude carve-out nearby? ──── +// The #853 pattern looks like: "RUNTIME is `claude`" in a preceding condition +// that switches to inline Skill() instead of the Agent() block. A block is +// considered guarded when the 400-char context window before it (or the block +// body itself for block-internal guards) contains any of these markers. + +function hasRuntimeCarveout(block) { + const haystack = block.contextBefore + block.blockText; + return ( + /RUNTIME[^`\n]{0,30}(?:!=|≠|is not|!==)\s*[`'"]?claude/i.test(haystack) || + /RUNTIME[^`\n]{0,30}claude[^`\n]{0,30}(?:inline|not.*Agent|do NOT)/i.test(haystack) || + /If `RUNTIME` is `claude`/i.test(haystack) || + /On Claude Code.*inline/is.test(haystack) + ); +} + +// ── 4. The guard: scan every workflow for unguarded Agent→spawner wraps ─── + +describe('bug-936 — no workflow wraps a spawner skill inside Agent() without a RUNTIME carve-out', () => { + test('spawner set is non-empty (self-check: subagent_type= grep must find files)', () => { + assert.ok(SPAWNER_BASE_SLUGS.size > 0, `No spawner workflows found in ${WORKFLOWS_DIR} — SPAWNER_BASE_SLUGS derivation is broken`); + // plan-phase must be a spawner (base slug) + assert.ok(SPAWNER_BASE_SLUGS.has('plan-phase'), 'plan-phase.md must be in the spawner set (contains subagent_type=)'); + // gsd-plan-phase must be in the prefixed set used by the regex + assert.ok(SPAWNER_GSD_NAMES.has('gsd-plan-phase'), 'gsd-plan-phase must be in SPAWNER_GSD_NAMES — the prefixed form used in Skill() calls'); + }); + + for (const wf of allWorkflowFiles) { + // Only scan files that have at least one Agent( call + if (!wf.content.includes('Agent(')) continue; + + test(`${wf.name}: no Agent() block wraps a spawner Skill without a RUNTIME carve-out`, () => { + const blocks = extractAgentBlocks(wf.content); + const violations = blocks.filter((b) => { + const wrapsSpawner = spawnerPattern.test(b.blockText); + if (!wrapsSpawner) return false; + return !hasRuntimeCarveout(b); + }); + + assert.deepStrictEqual( + violations.map((v) => v.blockText.slice(0, 120).replace(/\n/g, '\\n')), + [], + `${wf.name} wraps a spawner Skill inside Agent() without a RUNTIME != claude carve-out.\n` + + `Fix: run the spawner Skill inline (bare Skill() call at depth 0) OR add a RUNTIME gate.\n` + + `See: bug #936, tests/bug-853-bg-dispatch-runtime-gating.test.cjs for the guarded pattern.` + ); + }); + } +}); + +// ── 5. Focused regression: plan-review-convergence never wraps plan-phase ─ + +describe('bug-936 — plan-review-convergence runs plan-phase inline, not inside Agent()', () => { + const CONVERGENCE = fs.readFileSync( + path.join(WORKFLOWS_DIR, 'plan-review-convergence.md'), + 'utf8' + ); + + test('plan-review-convergence does NOT wrap gsd-plan-phase inside Agent()', () => { + // The anti-pattern: Agent( block whose body contains Skill(skill='gsd-plan-phase') + const blocks = extractAgentBlocks(CONVERGENCE); + const wrapping = blocks.filter((b) => + /Skill\(\s*skill=['"]gsd-plan-phase['"]/.test(b.blockText) && + !hasRuntimeCarveout(b) + ); + assert.deepStrictEqual( + wrapping.map((v) => v.blockText.slice(0, 120).replace(/\n/g, '\\n')), + [], + 'plan-review-convergence must NOT wrap gsd-plan-phase inside Agent(). ' + + 'Run it inline (bare Skill() at depth 0) so it can spawn gsd-planner/gsd-plan-checker. ' + + 'See: bug #936' + ); + }); + + test('plan-review-convergence calls gsd-plan-phase inline (bare Skill call outside Agent block)', () => { + // After the fix: at least one bare Skill(skill="gsd-plan-phase") must appear + // outside any Agent( block — that is the inline call from the depth-0 orchestrator. + const blocks = extractAgentBlocks(CONVERGENCE); + // Remove all Agent block ranges from the text + let masked = CONVERGENCE; + // Work from end to start so offsets stay valid + const sorted = [...blocks].sort((a, b) => b.start - a.start); + for (const b of sorted) { + masked = masked.slice(0, b.start) + ' '.repeat(b.end - b.start) + masked.slice(b.end); + } + const hasInlineCall = /Skill\(\s*skill=["']gsd-plan-phase["']/.test(masked); + assert.ok( + hasInlineCall, + 'plan-review-convergence must contain at least one bare Skill(skill="gsd-plan-phase") ' + + 'outside any Agent() block — this is the inline call that lets plan-phase spawn its sub-agents. ' + + 'See: bug #936' + ); + }); + + test('plan-review-convergence still wraps gsd-review inside Agent() (leaf — isolation is correct)', () => { + // gsd-review is a leaf (shells out via Bash, no subagent_type) so the Agent wrap is fine and intentional. + const blocks = extractAgentBlocks(CONVERGENCE); + const reviewWrap = blocks.some((b) => /Skill\(\s*skill=['"]gsd-review['"]/.test(b.blockText)); + assert.ok(reviewWrap, 'gsd-review must still be wrapped in Agent() — it is a Bash leaf and isolation is intentional'); + }); +}); diff --git a/tests/changeset-cli.test.cjs b/tests/changeset-cli.test.cjs index 72b80c91e..831857c13 100644 --- a/tests/changeset-cli.test.cjs +++ b/tests/changeset-cli.test.cjs @@ -332,10 +332,13 @@ describe('changeset cli extract: version-range changelog extraction (#3496)', () test('F1: workflows/update.md contains concrete extract subcommand invocation', (_t) => { const workflowPath = path.join(ROOT, 'gsd-core', 'workflows', 'update.md'); const workflowText = fs.readFileSync(workflowPath, 'utf8'); - // The invocation is: node "$GSD_DIR/gsd-core/scripts/changeset/cli.cjs" extract - // so the literal substring is 'cli.cjs" extract' (quote between script path and subcommand) + // The invocation uses either a direct path or an intermediate variable: + // node "$GSD_DIR/scripts/changeset/cli.cjs" extract + // node "$GSD_CHANGESET_CLI" extract + // Accept either form so future refactors don't immediately trip this anchor. assert.ok( - workflowText.includes('cli.cjs" extract') || workflowText.includes('cli.cjs extract'), + workflowText.includes('cli.cjs" extract') || workflowText.includes('cli.cjs extract') || + (workflowText.includes('GSD_CHANGESET_CLI') && workflowText.includes('" extract')), 'update.md must invoke cli.cjs extract (fix for #3496 BLOCKER 1)', ); assert.ok( @@ -351,6 +354,40 @@ describe('changeset cli extract: version-range changelog extraction (#3496)', () 'update.md must capture exit code or JSON output from extract', ); }); + + // F2: update.md must use the INSTALLED path ($GSD_DIR/scripts/changeset/cli.cjs), + // NOT the old broken path ($GSD_DIR/gsd-core/scripts/changeset/cli.cjs). + // The installer copies scripts/changeset/ into /scripts/changeset/, + // so the runtime path is $GSD_DIR/scripts/changeset/cli.cjs (#935). + // allow-test-rule: reads a product workflow .md file (not CJS source) to verify + // the runtime install path contract; there is no behavioural runtime to invoke. + test('F2: update.md CLI path is $GSD_DIR/scripts/changeset/cli.cjs (not gsd-core/scripts/…) (#935)', (_t) => { + const workflowPath = path.join(ROOT, 'gsd-core', 'workflows', 'update.md'); + const workflowText = fs.readFileSync(workflowPath, 'utf8'); + // The correct installed path must appear somewhere in the update workflow + assert.ok( + workflowText.includes('scripts/changeset/cli.cjs'), + 'update.md must reference scripts/changeset/cli.cjs', + ); + // The old broken path ($GSD_DIR/gsd-core/scripts/changeset/cli.cjs) must not appear + assert.ok( + !workflowText.includes('gsd-core/scripts/changeset/cli.cjs'), + 'update.md must NOT reference the old gsd-core/scripts/changeset/cli.cjs path (fix for #935)', + ); + }); + + // F3: update.md must guard against the CLI being missing (not pure silent-swallow) + // allow-test-rule: reads a product workflow .md file (not CJS source) to verify + // the guard is present; there is no behavioural runtime to invoke. + test('F3: update.md has an explicit guard when changeset CLI is missing (#935)', (_t) => { + const workflowPath = path.join(ROOT, 'gsd-core', 'workflows', 'update.md'); + const workflowText = fs.readFileSync(workflowPath, 'utf8'); + // The workflow must check for CLI existence before invoking it + assert.ok( + workflowText.includes('GSD_CHANGESET_CLI') && workflowText.includes('! -f'), + 'update.md must guard against a missing changeset CLI with [ ! -f "$GSD_CHANGESET_CLI" ] (#935)', + ); + }); }); describe('changeset cli render: file-I/O wrapper (#2975)', () => { diff --git a/tests/install.test.cjs b/tests/install.test.cjs index 42949a22e..91ac51632 100644 --- a/tests/install.test.cjs +++ b/tests/install.test.cjs @@ -684,3 +684,126 @@ describe('Kilo source integration assertions', () => { assert.ok(updateContextSrc.includes('KILO_CONFIG')); }); }); + +// ─── Section N: changeset CLI install regression (#935) ────────────────────── + +describe('install — changeset CLI lands at scripts/changeset/cli.cjs (#935)', () => { + // Regression guard: the changeset CLI must be copied into the runtime config dir + // by the installer so $GSD_DIR/scripts/changeset/cli.cjs resolves at runtime. + // Before this fix, scripts/ was never copied and /gsd-update changelog preview + // silently failed on every real install. + let tmpDir; + let previousCwd; + + beforeEach(() => { + tmpDir = createTempDir('gsd-changeset-install-'); + previousCwd = process.cwd(); + process.chdir(tmpDir); + }); + + afterEach(() => { + process.chdir(previousCwd); + cleanup(tmpDir); + }); + + test('install() copies scripts/changeset/cli.cjs to /scripts/changeset/cli.cjs', () => { + install(false, 'claude'); + const claudeDir = path.join(tmpDir, '.claude'); + const cliPath = path.join(claudeDir, 'scripts', 'changeset', 'cli.cjs'); + assert.ok( + fs.existsSync(cliPath), + `scripts/changeset/cli.cjs must exist at ${path.relative(tmpDir, cliPath)} after install (#935)`, + ); + }); + + test('install() copies scripts/lib/cli-exit.cjs to /scripts/lib/cli-exit.cjs', () => { + install(false, 'claude'); + const claudeDir = path.join(tmpDir, '.claude'); + const cliExitPath = path.join(claudeDir, 'scripts', 'lib', 'cli-exit.cjs'); + assert.ok( + fs.existsSync(cliExitPath), + `scripts/lib/cli-exit.cjs must exist at ${path.relative(tmpDir, cliExitPath)} after install (#935)`, + ); + }); + + test('installed cli.cjs executes without module-resolution errors', () => { + // Smoke test: node can load the installed changeset CLI without crashing. + // This catches path mismatches in require('../lib/cli-exit.cjs') etc. + install(false, 'claude'); + const claudeDir = path.join(tmpDir, '.claude'); + const cliPath = path.join(claudeDir, 'scripts', 'changeset', 'cli.cjs'); + const { spawnSync } = require('node:child_process'); + const result = spawnSync(process.execPath, [cliPath, '--help'], { encoding: 'utf8' }); + // --help exits with code 1 (usage shown), but must NOT throw a MODULE_NOT_FOUND error + assert.ok( + !result.stderr.includes('MODULE_NOT_FOUND'), + `cli.cjs must not produce MODULE_NOT_FOUND errors; stderr=${result.stderr}`, + ); + assert.ok( + !result.stderr.includes('Cannot find module'), + `cli.cjs must resolve all modules; stderr=${result.stderr}`, + ); + }); + + test('installed cli.cjs can run extract subcommand end-to-end (#935)', () => { + // Integration smoke test: the installed CLI's extract path (invoked by update.md) + // must actually work — this catches require() path issues that --help wouldn't surface. + install(false, 'claude'); + const claudeDir = path.join(tmpDir, '.claude'); + const cliPath = path.join(claudeDir, 'scripts', 'changeset', 'cli.cjs'); + // Use the CHANGELOG.md that was installed into gsd-core/ (installed by the installer) + const changelogPath = path.join(claudeDir, 'gsd-core', 'CHANGELOG.md'); + assert.ok(fs.existsSync(changelogPath), 'CHANGELOG.md must be installed under gsd-core/'); + const { spawnSync } = require('node:child_process'); + const result = spawnSync( + process.execPath, + [cliPath, 'extract', '--from', '0.0.0', '--to', '9999.0.0', '--changelog', changelogPath, '--json'], + { encoding: 'utf8' }, + ); + // extract must NOT throw a MODULE_NOT_FOUND or Cannot find module error + assert.ok( + !result.stderr.includes('MODULE_NOT_FOUND') && !result.stderr.includes('Cannot find module'), + `installed cli.cjs extract must resolve all modules; stderr=${result.stderr}`, + ); + // extract exit code 0 (found entries) or 2 (no entries in range) are both valid; + // any other exit code is an error + assert.ok( + result.status === 0 || result.status === 2, + `installed cli.cjs extract must exit 0 or 2; got ${result.status}; stderr=${result.stderr}`, + ); + }); + + test('writeManifest() tracks scripts/changeset/ and scripts/lib/ files', () => { + install(false, 'claude'); + const claudeDir = path.join(tmpDir, '.claude'); + const manifest = writeManifest(claudeDir, 'claude'); + const changesetKeys = Object.keys(manifest.files).filter(k => k.startsWith('scripts/changeset/')); + const libKeys = Object.keys(manifest.files).filter(k => k.startsWith('scripts/lib/')); + assert.ok(changesetKeys.length > 0, 'manifest must track scripts/changeset/ files'); + assert.ok(libKeys.length > 0, 'manifest must track scripts/lib/ files'); + assert.ok( + changesetKeys.includes('scripts/changeset/cli.cjs'), + 'manifest must include scripts/changeset/cli.cjs', + ); + assert.ok( + libKeys.includes('scripts/lib/cli-exit.cjs'), + 'manifest must include scripts/lib/cli-exit.cjs', + ); + }); + + test('uninstall() removes scripts/changeset/ and scripts/lib/', () => { + install(false, 'claude'); + const claudeDir = path.join(tmpDir, '.claude'); + assert.ok(fs.existsSync(path.join(claudeDir, 'scripts', 'changeset', 'cli.cjs')), + 'pre-condition: cli.cjs must be installed before uninstall'); + uninstall(false, 'claude'); + assert.ok( + !fs.existsSync(path.join(claudeDir, 'scripts', 'changeset')), + 'scripts/changeset/ must be removed on uninstall', + ); + assert.ok( + !fs.existsSync(path.join(claudeDir, 'scripts', 'lib')), + 'scripts/lib/ must be removed on uninstall', + ); + }); +}); diff --git a/tests/installer-migration-prune-stale-pristine.test.cjs b/tests/installer-migration-prune-stale-pristine.test.cjs new file mode 100644 index 000000000..173db4349 --- /dev/null +++ b/tests/installer-migration-prune-stale-pristine.test.cjs @@ -0,0 +1,280 @@ +'use strict'; + +/** + * TDD tests for installer migration 004: + * 2026-06-09-prune-stale-pristine-get-shit-done // gsd-allow-legacy-name + * + * Verifies plan() logic for: + * 1. Stale pristine subdir absent -> empty plan (idempotency) + * 2. Stale pristine subdir present -> remove-managed actions emitted for each file + * 3. Stale pristine root is a symlink -> empty plan (symlink safety) + * 4. Symlinked entry inside stale pristine dir is NOT emitted + * 5. Mixed files: all get remove-managed (no user-file classification needed) + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +// Load compiled module (build:lib compiles src/*.cts -> gsd-core/bin/lib/*.cjs) +const migration = require('../gsd-core/bin/lib/installer-migrations/004-prune-stale-pristine-snapshots.cjs'); + +function createTempDir() { + return fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-migration-004-test-')); +} + +function cleanup(dir) { + // eslint-disable-next-line local/no-raw-rmsync-in-tests -- local cleanup in migration test; no helpers import available + fs.rmSync(dir, { recursive: true, force: true }); +} + +function writeFile(root, relPath, content) { + const fullPath = path.join(root, relPath); + fs.mkdirSync(path.dirname(fullPath), { recursive: true }); + fs.writeFileSync(fullPath, content, 'utf8'); +} + +function writeManifest(root, files) { + fs.writeFileSync( + path.join(root, 'gsd-file-manifest.json'), + JSON.stringify({ + version: '1.3.0', + timestamp: '2026-06-01T00:00:00.000Z', + mode: 'full', + files, + }, null, 2), + 'utf8' + ); +} + +// Build a plan context using the real installer-migrations classifyArtifact. +const { + classifyArtifact: realClassifyArtifact, + readInstallManifest, +} = require('../gsd-core/bin/lib/installer-migrations.cjs'); + +function makePlanCtx(configDir) { + const manifest = readInstallManifest(configDir); + return { + configDir, + classifyArtifact: (relPath) => realClassifyArtifact(configDir, relPath, manifest), + }; +} + +// --------------------------------------------------------------------------- +// Metadata +// --------------------------------------------------------------------------- + +describe('migration 004 metadata', () => { + test('exports a single migration object with required fields', () => { + assert.equal(typeof migration, 'object'); + assert.equal(typeof migration.id, 'string'); + assert.ok(migration.id.length > 0, 'id must be non-empty'); + assert.equal(typeof migration.title, 'string'); + assert.equal(typeof migration.description, 'string'); + assert.equal(typeof migration.introducedIn, 'string'); + assert.ok(Array.isArray(migration.scopes), 'scopes must be an array'); + assert.ok(migration.scopes.includes('global'), 'scopes must include global'); + assert.ok(migration.scopes.includes('local'), 'scopes must include local'); + assert.strictEqual(migration.destructive, true); + assert.equal(typeof migration.plan, 'function'); + }); + + test('id contains expected date prefix', () => { + assert.ok(migration.id.startsWith('2026-06-09-'), `id should start with date prefix, got: ${migration.id}`); + }); + + test('id references prune-stale-pristine', () => { + assert.ok( + migration.id.includes('prune-stale-pristine') || migration.id.includes('pristine'), + `id should reference pristine pruning, got: ${migration.id}`, + ); + }); +}); + +// --------------------------------------------------------------------------- +// Case 1: stale pristine subdir absent -> empty plan +// --------------------------------------------------------------------------- + +describe('plan() — stale pristine subdir absent', () => { + test('returns empty array when gsd-pristine/get-shit-done/ does not exist', () => { // gsd-allow-legacy-name + const configDir = createTempDir(); + try { + // Only gsd-pristine/gsd-core/ exists — no legacy subdir. + writeFile(configDir, 'gsd-pristine/gsd-core/workflows/plan.md', 'pristine snapshot\n'); + writeManifest(configDir, {}); + + const actions = migration.plan(makePlanCtx(configDir)); + assert.deepEqual(actions, []); + } finally { + cleanup(configDir); + } + }); + + test('returns empty array when gsd-pristine/ does not exist at all', () => { + const configDir = createTempDir(); + try { + writeManifest(configDir, {}); + const actions = migration.plan(makePlanCtx(configDir)); + assert.deepEqual(actions, []); + } finally { + cleanup(configDir); + } + }); +}); + +// --------------------------------------------------------------------------- +// Case 2: stale pristine subdir present -> remove-managed for each file +// --------------------------------------------------------------------------- + +describe('plan() — stale pristine files present', () => { + test('emits remove-managed for each file under gsd-pristine/get-shit-done/', () => { // gsd-allow-legacy-name + const configDir = createTempDir(); + try { + writeFile(configDir, 'gsd-pristine/get-shit-done/workflows/plan.md', 'old pristine\n'); // gsd-allow-legacy-name + writeFile(configDir, 'gsd-pristine/get-shit-done/skills/gsd-foo/SKILL.md', 'old skill\n'); // gsd-allow-legacy-name + writeManifest(configDir, {}); + + const actions = migration.plan(makePlanCtx(configDir)); + assert.equal(actions.length, 2, `expected 2 actions, got ${actions.length}`); + for (const action of actions) { + assert.equal(action.type, 'remove-managed', `expected remove-managed, got ${action.type}`); + assert.ok( + action.relPath.replace(/\\/g, '/').startsWith('gsd-pristine/get-shit-done/'), // gsd-allow-legacy-name + `relPath should start with gsd-pristine/get-shit-done/, got: ${action.relPath}`, // gsd-allow-legacy-name + ); + assert.equal(typeof action.reason, 'string'); + assert.ok(action.reason.length > 0, 'reason must not be empty'); + assert.equal(typeof action.ownershipEvidence, 'string'); + assert.ok(action.ownershipEvidence.length > 0, 'ownershipEvidence must not be empty'); + } + } finally { + cleanup(configDir); + } + }); + + test('emits exactly one remove-managed per file (correct relPaths)', () => { + const configDir = createTempDir(); + try { + writeFile(configDir, 'gsd-pristine/get-shit-done/workflows/execute-phase.md', 'pristine\n'); // gsd-allow-legacy-name + writeManifest(configDir, {}); + + const actions = migration.plan(makePlanCtx(configDir)); + assert.equal(actions.length, 1); + const relPathNorm = actions[0].relPath.replace(/\\/g, '/'); + assert.equal(relPathNorm, 'gsd-pristine/get-shit-done/workflows/execute-phase.md'); // gsd-allow-legacy-name + } finally { + cleanup(configDir); + } + }); + + test('actions include classification override to managed-pristine', () => { + const configDir = createTempDir(); + try { + writeFile(configDir, 'gsd-pristine/get-shit-done/workflows/plan.md', 'pristine snapshot\n'); // gsd-allow-legacy-name + writeManifest(configDir, {}); + + const actions = migration.plan(makePlanCtx(configDir)); + assert.equal(actions.length, 1); + // The action must carry classification:'managed-pristine' so the framework + // does not downgrade remove-managed to preserve-user (the file is not in + // the manifest so classify() would return 'unknown'). + assert.equal(actions[0].classification, 'managed-pristine', + 'action must carry classification:managed-pristine override'); + } finally { + cleanup(configDir); + } + }); +}); + +// --------------------------------------------------------------------------- +// Case 3: stale pristine root is a symlink -> plan returns [] (symlink safety) +// --------------------------------------------------------------------------- + +describe('plan() — stale pristine root is a symlink', () => { + test('returns empty array when gsd-pristine/get-shit-done/ is a symlink', () => { // gsd-allow-legacy-name + const configDir = createTempDir(); + const externalDir = createTempDir(); + try { + writeFile(externalDir, 'workflows/plan.md', 'pristine content\n'); + // Create gsd-pristine/ as a real dir but make get-shit-done/ a symlink. // gsd-allow-legacy-name + fs.mkdirSync(path.join(configDir, 'gsd-pristine'), { recursive: true }); + const legacyLink = path.join(configDir, 'gsd-pristine', 'get-shit-done'); // gsd-allow-legacy-name + fs.symlinkSync(externalDir, legacyLink); + writeManifest(configDir, {}); + + const actions = migration.plan(makePlanCtx(configDir)); + assert.deepEqual(actions, [], 'plan() must return [] when stale pristine root is a symlink'); + } finally { + cleanup(configDir); + cleanup(externalDir); + } + }); +}); + +// --------------------------------------------------------------------------- +// Case 4: symlinked entry inside stale pristine dir is skipped +// --------------------------------------------------------------------------- + +describe('plan() — symlinked entry inside stale pristine dir is skipped', () => { + test('symlinked file inside stale pristine dir is not included in plan actions', () => { + const configDir = createTempDir(); + const externalTarget = createTempDir(); + try { + // A real file inside gsd-pristine/get-shit-done/ // gsd-allow-legacy-name + writeFile(configDir, 'gsd-pristine/get-shit-done/workflows/plan.md', 'real pristine\n'); // gsd-allow-legacy-name + + // A symlink inside the same dir pointing to external target. + const externalFile = path.join(externalTarget, 'external.md'); + fs.writeFileSync(externalFile, 'external content\n', 'utf8'); + const symlinkPath = path.join(configDir, 'gsd-pristine', 'get-shit-done', 'workflows', 'symlinked.md'); // gsd-allow-legacy-name + fs.symlinkSync(externalFile, symlinkPath); + + writeManifest(configDir, {}); + + const actions = migration.plan(makePlanCtx(configDir)); + + // Only the real file should appear; the symlinked entry must be skipped. + assert.equal(actions.length, 1, `expected 1 action (real file only), got ${actions.length}`); + const hasSymlinked = actions.some((a) => a.relPath.includes('symlinked')); + assert.equal(hasSymlinked, false, 'symlinked entry must not appear in plan actions'); + } finally { + cleanup(configDir); + cleanup(externalTarget); + } + }); +}); + +// --------------------------------------------------------------------------- +// Integration: plan goes through planInstallerMigrations + applyInstallerMigrationPlan +// Files are actually removed from disk. +// --------------------------------------------------------------------------- + +describe('plan() — integration: stale pristine files are removed', () => { + test('stale gsd-pristine/get-shit-done/ file is removed after apply', () => { // gsd-allow-legacy-name + const configDir = createTempDir(); + try { + const fileContent = 'old pristine snapshot content written by gsd installer\n'; + writeFile(configDir, 'gsd-pristine/get-shit-done/workflows/plan.md', fileContent); // gsd-allow-legacy-name + writeManifest(configDir, {}); + + const { planInstallerMigrations, applyInstallerMigrationPlan } = require('../gsd-core/bin/lib/installer-migrations.cjs'); + const plan = planInstallerMigrations({ + configDir, + migrations: [migration], + scope: 'global', + }); + + assert.equal(plan.blocked.length, 0, `expected no blocked actions; got ${JSON.stringify(plan.blocked)}`); + applyInstallerMigrationPlan({ configDir, plan }); + + // File must be gone after apply. + const stillThere = fs.existsSync(path.join(configDir, 'gsd-pristine', 'get-shit-done', 'workflows', 'plan.md')); // gsd-allow-legacy-name + assert.equal(stillThere, false, 'stale pristine file must be removed after apply'); + } finally { + cleanup(configDir); + } + }); +}); diff --git a/tests/installer-migrations.test.cjs b/tests/installer-migrations.test.cjs index 4367f8f15..10c51f6f8 100644 --- a/tests/installer-migrations.test.cjs +++ b/tests/installer-migrations.test.cjs @@ -1465,6 +1465,9 @@ test('shipped installer-migration checksums are locked to a committed baseline ( 'sha256:5ce55294aa02f25758f604a569c899a6d2d060299189f5f447f68d8033157058', '2026-06-02-rename-get-shit-done-to-gsd-core': 'sha256:3a9f1d97f64097fb313203d19c6d93a187a38df61dd299afa5eef73e16124e95', + // Migration 004: prune stale gsd-pristine/get-shit-done/ snapshots (#934) // gsd-allow-legacy-name + '2026-06-09-prune-stale-pristine-get-shit-done': // gsd-allow-legacy-name + 'sha256:6555dd044659276fbc204e81793cd92c5315d54e7316bcdd82d2c98d15a7e9e8', }; const { DEFAULT_MIGRATIONS_DIR, migrationChecksum: computeChecksum } = require('../gsd-core/bin/lib/installer-migrations.cjs'); diff --git a/tests/plan-review-convergence.test.cjs b/tests/plan-review-convergence.test.cjs index cbf880f3b..2f3c08b1a 100644 --- a/tests/plan-review-convergence.test.cjs +++ b/tests/plan-review-convergence.test.cjs @@ -186,10 +186,10 @@ describe('plan-review-convergence workflow: initial planning gate (#2306)', () = ); }); - test('workflow spawns isolated planning agent when no plans exist', () => { + test('workflow runs gsd-plan-phase when no plans exist', () => { assert.ok( workflow.includes('gsd-plan-phase'), - 'workflow must spawn Agent → gsd-plan-phase when no plans exist' + 'workflow must invoke gsd-plan-phase when no plans exist' ); }); @@ -651,3 +651,95 @@ describe('plan-review-convergence local model CONFIGURATION.md documentation (#2 ); }); }); + +// ─── Bug #936: plan-phase must run inline, not inside Agent() ───────────── +// +// Regression guard: inverted from the pre-#936 behavior that locked in the bug. +// On Claude Code a depth-1 Agent has no Agent tool, so gsd-plan-phase wrapped in +// Agent() cannot spawn gsd-planner / gsd-plan-checker → the replan loop breaks. +// Fix: run plan-phase inline (bare Skill()) from the depth-0 convergence orchestrator. +// +// These tests FAIL on pre-fix code and PASS after the fix. + +describe('plan-review-convergence workflow: inline plan-phase dispatch (#936)', () => { + const workflow = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + + // Helper: extract Agent() block bodies from workflow text + function extractAgentBlocks(content) { + const blocks = []; + let pos = 0; + while (pos < content.length) { + const start = content.indexOf('Agent(', pos); + if (start === -1) break; + let depth = 0; + let i = start + 'Agent('.length - 1; + for (; i < content.length; i++) { + if (content[i] === '(') depth++; + else if (content[i] === ')') { depth--; if (depth === 0) break; } + } + blocks.push({ start, end: i + 1, blockText: content.slice(start, i + 1) }); + pos = i + 1; + } + return blocks; + } + + test('initial planning does NOT wrap gsd-plan-phase inside Agent() (#936 fix)', () => { + // Pre-fix: Agent( ... Skill('gsd-plan-phase') ... ) in step 4 + // Post-fix: bare Skill(skill="gsd-plan-phase") at orchestrator level + const blocks = extractAgentBlocks(workflow); + const wrapping = blocks.filter((b) => + /Skill\(\s*skill=['"]gsd-plan-phase['"]/.test(b.blockText) + ); + assert.deepStrictEqual( + wrapping.map((b) => b.blockText.slice(0, 80).replace(/\n/g, '\\n')), + [], + 'Initial planning must NOT wrap gsd-plan-phase inside Agent() — run it inline so ' + + 'it can spawn gsd-planner/gsd-plan-checker at depth 1. See: bug #936' + ); + }); + + test('replan step does NOT wrap gsd-plan-phase inside Agent() (#936 fix)', () => { + // Same check as above; explicitly named for the replan site (step 5d) + const blocks = extractAgentBlocks(workflow); + const wrapping = blocks.filter((b) => + /Skill\(\s*skill=['"]gsd-plan-phase['"]/.test(b.blockText) && + /--reviews/.test(b.blockText) + ); + assert.deepStrictEqual( + wrapping.map((b) => b.blockText.slice(0, 80).replace(/\n/g, '\\n')), + [], + 'Replan step must NOT wrap gsd-plan-phase inside Agent() — the replan loop can ' + + 'never produce a plan on Claude Code when plan-phase is at depth 1. See: bug #936' + ); + }); + + test('workflow calls gsd-plan-phase inline (bare Skill outside Agent block) (#936 fix)', () => { + // After the fix there must be at least one bare Skill(skill="gsd-plan-phase") + // OUTSIDE any Agent() block. + const blocks = extractAgentBlocks(workflow); + let masked = workflow; + const sorted = [...blocks].sort((a, b) => b.start - a.start); + for (const b of sorted) { + masked = masked.slice(0, b.start) + ' '.repeat(b.end - b.start) + masked.slice(b.end); + } + assert.ok( + /Skill\(\s*skill=["']gsd-plan-phase["']/.test(masked), + 'plan-review-convergence must contain at least one bare Skill(skill="gsd-plan-phase") ' + + 'outside any Agent() block — the inline call that preserves depth-0 Agent availability. See: bug #936' + ); + }); + + test('success_criteria describes inline plan-phase, not Agent → Skill (#936 fix)', () => { + const successBlock = workflow.slice(workflow.lastIndexOf('')); + // The broken criterion said "Initial planning via Agent → Skill" + assert.ok( + !successBlock.includes('via Agent → Skill("gsd-plan-phase")'), + 'success_criteria must NOT describe plan-phase as Agent → Skill — that was the broken pattern. See: bug #936' + ); + // The broken criterion said "isolated, not inline" for the replan + assert.ok( + !successBlock.includes('isolated, not inline'), + 'success_criteria must NOT say "isolated, not inline" for plan-phase — the fix makes it inline. See: bug #936' + ); + }); +});