fix(#1542): roadmap upgrade rollback restores .planning regardless of git tracking (#1543)

* fix(core): roadmap upgrade rollback must restore .planning regardless of git tracking (#1542)

applyMigration rolled back a failed migration with git reset --hard + git clean
-fd .planning/phases/. For a commit_docs:false project (.planning gitignored —
the default) that restores NOTHING (reset ignores untracked, clean without -x
skips ignored), yet it threw 'Migration failed (rolled back to <sha>)' — a false
claim leaving .planning half-migrated. git reset --hard is also a whole-repo op.

Replace it with a surgical, git-independent rollback: record the exact renames
performed and snapshot each file before rewriting it, then on failure reverse the
renames and restore the snapshots (deleting files that did not previously exist).
Correct whether .planning is tracked or ignored; touches only what it changed.

Claude-Session: https://claude.ai/code/session_01R88n7Q54bAaVHFkDbbH1yz

* chore(changeset): Fixed fragment for #1543 (roadmap upgrade surgical rollback)

Claude-Session: https://claude.ai/code/session_01R88n7Q54bAaVHFkDbbH1yz

* test(core): update bug-685 execSync count floor after surgical rollback (#1542)

The #1542 surgical, git-independent rollback removed the rev-parse/reset/clean
git execSync calls from roadmap-upgrade.cts, leaving only the git status
precondition. bug-685 asserted calls.length >= 4; lower the floor to >= 1 — the
durable guard (every remaining git execSync sets windowsHide:true) is unchanged.

Claude-Session: https://claude.ai/code/session_01R88n7Q54bAaVHFkDbbH1yz

---------

Co-authored-by: Tom Boucher <trekkie@nomorestars.com>

.changeset/silly-goats-fly.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 1543
+---
+A failed `roadmap upgrade --apply` now actually rolls back .planning/ even when it is gitignored (commit_docs:false), instead of reporting a successful rollback while leaving the workspace half-migrated. Rollback is surgical and no longer runs a whole-repo git reset --hard.

src/roadmap-upgrade.cts

@@ -492,14 +492,6 @@ function applyMigration(cwd: string, plan: MigrationPlan, options: { dryRun?: bo
     throw new Error('Working tree is dirty. Commit or stash changes before migrating.');
   }
 
-  // Capture HEAD sha for rollback
-  let headSha: string;
-  try {
-    headSha = execSync('git rev-parse HEAD', { cwd, encoding: 'utf8', windowsHide: true }).trim();
-  } catch (err) {
-    throw new Error(`git rev-parse HEAD failed: ${(err as Error).message}`);
-  }
-
   const pDir = planningDir(cwd);
   const phasesDir = path.join(pDir, 'phases');
   const roadmapPath = path.join(pDir, 'ROADMAP.md');
@@ -508,6 +500,23 @@ function applyMigration(cwd: string, plan: MigrationPlan, options: { dryRun?: bo
   const renamedDirs: string[] = [];
   const editedFiles: string[] = [];
 
+  // Surgical, git-independent rollback state (#1542). A `git reset --hard` +
+  // `git clean` rollback restores NOTHING for a gitignored `.planning/`
+  // (commit_docs:false — the default) and is a whole-repo operation besides.
+  // Instead, record the exact renames performed and snapshot each file before
+  // rewriting it, then undo precisely those on failure — correct whether
+  // `.planning/` is git-tracked or ignored.
+  const performedRenames: Array<{ oldPath: string; newPath: string }> = [];
+  const fileBackups = new Map<string, { existed: boolean; content: string }>();
+  const snapshotFile = (filePath: string): void => {
+    if (fileBackups.has(filePath)) return;
+    try {
+      fileBackups.set(filePath, { existed: true, content: fs.readFileSync(filePath, 'utf8') });
+    } catch {
+      fileBackups.set(filePath, { existed: false, content: '' });
+    }
+  };
+
   try {
     // 1. Rename phase directories
     for (const phaseEntry of plan.phases) {
@@ -515,6 +524,7 @@ function applyMigration(cwd: string, plan: MigrationPlan, options: { dryRun?: bo
       const newPath = path.join(phasesDir, phaseEntry.newDir);
       if (fs.existsSync(oldPath)) {
         fs.renameSync(oldPath, newPath);
+        performedRenames.push({ oldPath, newPath });
         renamedDirs.push(`${phaseEntry.oldDir} → ${phaseEntry.newDir}`);
       }
     }
@@ -532,6 +542,7 @@ function applyMigration(cwd: string, plan: MigrationPlan, options: { dryRun?: bo
         }
       }
 
+      snapshotFile(roadmapPath);
       fs.writeFileSync(roadmapPath, lines.join('\n'), 'utf8');
       editedFiles.push('ROADMAP.md');
     }
@@ -561,6 +572,7 @@ function applyMigration(cwd: string, plan: MigrationPlan, options: { dryRun?: bo
       }
 
       if (changed) {
+        snapshotFile(filePath);
         fs.writeFileSync(filePath, content, 'utf8');
         editedFiles.push(fileName);
       }
@@ -573,18 +585,28 @@ function applyMigration(cwd: string, plan: MigrationPlan, options: { dryRun?: bo
     } catch { /* config may not exist yet */ }
 
     configData['phase_id_convention'] = 'milestone-prefixed';
+    snapshotFile(configPath);
     fs.writeFileSync(configPath, JSON.stringify(configData, null, 2) + '\n', 'utf8');
     editedFiles.push('config.json');
 
   } catch (err) {
-    // Rollback via git reset --hard + git clean
-    try {
-      execSync(`git reset --hard ${headSha}`, { cwd, stdio: 'pipe', windowsHide: true });
-      execSync('git clean -fd .planning/phases/', { cwd, stdio: 'pipe', windowsHide: true });
-    } catch {
-      // Swallow rollback errors — surface original error
+    // Surgical rollback: reverse the renames (newest first) and restore every
+    // file we snapshotted (deleting files that did not previously exist). This
+    // actually restores `.planning/` regardless of git tracking — so the
+    // "rolled back" claim is truthful — and never touches anything else.
+    for (let i = performedRenames.length - 1; i >= 0; i--) {
+      const { oldPath, newPath } = performedRenames[i];
+      try {
+        if (fs.existsSync(newPath)) fs.renameSync(newPath, oldPath);
+      } catch { /* best-effort */ }
     }
-    throw new Error(`Migration failed (rolled back to ${headSha}): ${(err as Error).message}`);
+    for (const [filePath, backup] of fileBackups) {
+      try {
+        if (backup.existed) fs.writeFileSync(filePath, backup.content, 'utf8');
+        else if (fs.existsSync(filePath)) fs.unlinkSync(filePath);
+      } catch { /* best-effort */ }
+    }
+    throw new Error(`Migration failed and rolled back: ${(err as Error).message}`);
   }
 
   return { applied: true, renamedDirs, editedFiles };

tests/bug-685-windowshide-spawn.test.cjs

@@ -60,7 +60,10 @@ describe('bug #685: Windows spawns must set windowsHide:true (no console-window
   test('roadmap-upgrade execSync git calls all set windowsHide', () => {
     const src = read('src/roadmap-upgrade.cts');
     const calls = src.match(/execSync\([^)]*\)/g) || [];
-    assert.ok(calls.length >= 4, 'expected the roadmap-upgrade git execSync calls to be present');
+    // #1542 made rollback git-independent (surgical fs restore), so the only
+    // remaining git execSync is the `git status --porcelain` precondition. The
+    // durable guard is that EVERY git execSync still present sets windowsHide.
+    assert.ok(calls.length >= 1, 'expected at least the roadmap-upgrade git status execSync call to be present');
     const missing = calls.filter((c) => !/windowsHide:\s*true/.test(c));
     assert.deepEqual(missing, [], `execSync without windowsHide:\n${missing.join('\n')}`);
   });

tests/roadmap-upgrade.test.cjs

@@ -0,0 +1,103 @@
+'use strict';
+
+const { test, describe, mock } = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
+const { execSync } = require('node:child_process');
+const { createTempDir, cleanup } = require('./helpers.cjs');
+
+const { computeMigrationPlan, applyMigration } = require('../gsd-core/bin/lib/roadmap-upgrade.cjs');
+
+/**
+ * Build a git project whose `.planning/` is GITIGNORED (commit_docs:false) —
+ * the condition under which the old `git reset --hard` + `git clean -fd`
+ * rollback restored nothing yet still reported "rolled back". #1542.
+ */
+function makeGitignoredPlanningProject() {
+  const dir = createTempDir('m3-rollback-');
+  fs.writeFileSync(path.join(dir, '.gitignore'), '.planning/\n');
+  fs.writeFileSync(path.join(dir, 'README.md'), '# tracked\n');
+  const git = (c) => execSync(c, { cwd: dir, stdio: 'pipe' });
+  git('git init');
+  git('git config user.email t@t.t');
+  git('git config user.name t');
+  git('git config commit.gpgsign false');
+  git('git add -A');
+  git('git commit -m initial');
+
+  // .planning created AFTER the commit → untracked + gitignored.
+  const planning = path.join(dir, '.planning');
+  fs.mkdirSync(path.join(planning, 'phases', '01-foo'), { recursive: true });
+  fs.mkdirSync(path.join(planning, 'phases', '02-bar'), { recursive: true });
+  fs.writeFileSync(path.join(planning, 'phases', '01-foo', 'PLAN.md'), 'foo plan\n');
+  fs.writeFileSync(path.join(planning, 'phases', '02-bar', 'PLAN.md'), 'bar plan\n');
+  fs.writeFileSync(
+    path.join(planning, 'ROADMAP.md'),
+    ['## v1.0: First Milestone', '', '### Phase 1: Foo', '', '### Phase 2: Bar', ''].join('\n'),
+  );
+  return dir;
+}
+
+function snapshotPlanning(dir) {
+  const planning = path.join(dir, '.planning');
+  return {
+    phases: fs.readdirSync(path.join(planning, 'phases')).sort(),
+    roadmap: fs.readFileSync(path.join(planning, 'ROADMAP.md'), 'utf8'),
+    hasConfig: fs.existsSync(path.join(planning, 'config.json')),
+  };
+}
+
+describe('roadmap upgrade rollback (#1542)', () => {
+  test('a mid-migration failure restores .planning even when it is gitignored', (t) => {
+    const dir = makeGitignoredPlanningProject();
+    t.after(() => cleanup(dir));
+
+    const plan = computeMigrationPlan(dir);
+    assert.equal(plan.alreadyMigrated, false);
+    assert.ok(plan.phases.length >= 1, 'fixture must produce phase renames');
+    assert.ok(plan.roadmapEdits.length >= 1, 'fixture must produce roadmap edits');
+
+    const before = snapshotPlanning(dir);
+    assert.equal(before.hasConfig, false, 'precondition: no config.json yet');
+
+    // Inject a failure on the LAST mutation step (the config.json write) so the
+    // phase renames AND the ROADMAP rewrite have already happened when rollback
+    // fires — exactly the half-migrated state the old git rollback could not undo.
+    const realWrite = fs.writeFileSync;
+    const writeMock = mock.method(fs, 'writeFileSync', function (target, data, opts) {
+      if (String(target).endsWith('config.json')) {
+        const err = new Error('EIO: simulated write failure');
+        err.code = 'EIO';
+        throw err;
+      }
+      return realWrite.call(fs, target, data, opts);
+    });
+    t.after(() => writeMock.mock.restore());
+
+    assert.throws(() => applyMigration(dir, plan, { dryRun: false }), /Migration failed/);
+
+    // The rollback must have actually restored the workspace — not just claimed to.
+    const after = snapshotPlanning(dir);
+    assert.deepEqual(after.phases, before.phases, 'phase dirs must be restored to their original names');
+    assert.equal(after.roadmap, before.roadmap, 'ROADMAP.md must be restored to its original content');
+    assert.equal(after.hasConfig, false, 'config.json created during migration must be removed on rollback');
+  });
+
+  test('a successful migration still applies (renames + roadmap rewrite + config), no rollback', (t) => {
+    const dir = makeGitignoredPlanningProject();
+    t.after(() => cleanup(dir));
+
+    const plan = computeMigrationPlan(dir);
+    const before = snapshotPlanning(dir);
+
+    const result = applyMigration(dir, plan, { dryRun: false });
+
+    assert.equal(result.applied, true);
+    const after = snapshotPlanning(dir);
+    assert.notDeepEqual(after.phases, before.phases, 'phase dirs renamed on success');
+    assert.equal(after.hasConfig, true, 'config.json written on success');
+    const config = JSON.parse(fs.readFileSync(path.join(dir, '.planning', 'config.json'), 'utf8'));
+    assert.equal(config.phase_id_convention, 'milestone-prefixed');
+  });
+});