diff --git a/.changeset/silly-goats-fly.md b/.changeset/silly-goats-fly.md new file mode 100644 index 000000000..888ba9f90 --- /dev/null +++ b/.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. diff --git a/src/roadmap-upgrade.cts b/src/roadmap-upgrade.cts index ba5969f79..76632d293 100644 --- a/src/roadmap-upgrade.cts +++ b/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(); + 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 }; diff --git a/tests/bug-685-windowshide-spawn.test.cjs b/tests/bug-685-windowshide-spawn.test.cjs index d1c936083..7d8fc683c 100644 --- a/tests/bug-685-windowshide-spawn.test.cjs +++ b/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')}`); }); diff --git a/tests/roadmap-upgrade.test.cjs b/tests/roadmap-upgrade.test.cjs new file mode 100644 index 000000000..1a892962f --- /dev/null +++ b/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'); + }); +});