From 4e40ee8b2fa684c7edfc446ff0fc5c44c5747409 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 11 May 2026 14:29:33 -0400 Subject: [PATCH 1/4] fix: harden installer migration paths --- bin/install.js | 26 +++++++++ .../bin/lib/installer-migrations.cjs | 8 ++- tests/bug-2771-user-profile-manifest.test.cjs | 55 +++++++++++++++++-- tests/installer-migrations.test.cjs | 31 +++++++++++ 4 files changed, 114 insertions(+), 6 deletions(-) diff --git a/bin/install.js b/bin/install.js index bdfa4e242..b2b9daae9 100755 --- a/bin/install.js +++ b/bin/install.js @@ -7199,9 +7199,35 @@ function resolveInstallRelativePath(baseDir, relPath) { if (fullPath !== root && !fullPath.startsWith(root + path.sep)) { return null; } + if (hasExistingSymlinkBetween(root, fullPath)) { + return null; + } return { relPath: normalized, fullPath }; } +function hasExistingSymlinkBetween(root, fullPath) { + const resolvedRoot = path.resolve(root); + const resolvedFullPath = path.resolve(fullPath); + if (resolvedFullPath !== resolvedRoot && !resolvedFullPath.startsWith(resolvedRoot + path.sep)) { + return true; + } + + let cursor = resolvedRoot; + if (fs.existsSync(cursor) && fs.lstatSync(cursor).isSymbolicLink()) { + return true; + } + + const relative = path.relative(resolvedRoot, resolvedFullPath); + for (const segment of relative.split(path.sep)) { + if (!segment) continue; + cursor = path.join(cursor, segment); + if (!fs.existsSync(cursor)) return false; + if (fs.lstatSync(cursor).isSymbolicLink()) return true; + } + + return false; +} + /** * Write file manifest after installation for future modification detection */ diff --git a/get-shit-done/bin/lib/installer-migrations.cjs b/get-shit-done/bin/lib/installer-migrations.cjs index fba3f81eb..966da17f2 100644 --- a/get-shit-done/bin/lib/installer-migrations.cjs +++ b/get-shit-done/bin/lib/installer-migrations.cjs @@ -56,10 +56,14 @@ function normalizeRelPath(relPath) { throw new Error('migration action relPath must be a non-empty string'); } const normalized = relPath.replace(/\\/g, '/'); - if (normalized.startsWith('/') || normalized.includes('../') || normalized === '..') { + if (path.isAbsolute(normalized) || path.win32.isAbsolute(normalized)) { throw new Error(`migration action relPath must stay inside configDir: ${relPath}`); } - return normalized; + const segments = normalized.split('/'); + if (segments.some((segment) => segment === '' || segment === '.' || segment === '..')) { + throw new Error(`migration action relPath must stay inside configDir: ${relPath}`); + } + return segments.join('/'); } function classifyArtifact(configDir, relPath, manifest) { diff --git a/tests/bug-2771-user-profile-manifest.test.cjs b/tests/bug-2771-user-profile-manifest.test.cjs index eeda82adc..7f4a6cf2a 100644 --- a/tests/bug-2771-user-profile-manifest.test.cjs +++ b/tests/bug-2771-user-profile-manifest.test.cjs @@ -22,6 +22,7 @@ const { describe, test, beforeEach, afterEach, before } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +const crypto = require('crypto'); const { execFileSync } = require('child_process'); const { createTempDir, cleanup } = require('./helpers.cjs'); @@ -220,9 +221,16 @@ describe('#2771: USER_OWNED_ARTIFACTS is a single source of truth', () => { describe('manifest path safety', () => { let tmpDir; + let outside; - beforeEach(() => { tmpDir = createTempDir('gsd-manifest-path-safety-'); }); - afterEach(() => { cleanup(tmpDir); }); + beforeEach(() => { + tmpDir = createTempDir('gsd-manifest-path-safety-'); + outside = path.join(tmpDir, '..', `outside-managed-file-${path.basename(tmpDir)}.txt`); + }); + afterEach(() => { + if (outside) fs.rmSync(outside, { recursive: true, force: true }); + cleanup(tmpDir); + }); test('saveLocalPatches ignores manifest entries that escape the install root', () => { const origMode = process.env.GSD_TEST_MODE; @@ -236,7 +244,6 @@ describe('manifest path safety', () => { else process.env.GSD_TEST_MODE = origMode; } - const outside = path.join(tmpDir, '..', 'outside-managed-file.txt'); fs.writeFileSync(outside, 'outside user data\n', 'utf8'); fs.writeFileSync( path.join(tmpDir, MANIFEST_NAME), @@ -254,6 +261,46 @@ describe('manifest path safety', () => { assert.deepEqual(modified, []); assert.equal(fs.readFileSync(outside, 'utf8'), 'outside user data\n'); - assert.equal(fs.existsSync(path.join(tmpDir, PATCHES_DIR_NAME, '..', 'outside-managed-file.txt')), false); + assert.equal(fs.existsSync(path.join(tmpDir, PATCHES_DIR_NAME, '..', path.basename(outside))), false); + }); + + test('saveLocalPatches does not follow symlinked patch directories outside the install root', () => { + const origMode = process.env.GSD_TEST_MODE; + process.env.GSD_TEST_MODE = '1'; + let mod; + try { + delete require.cache[require.resolve(INSTALL_SCRIPT)]; + mod = require(INSTALL_SCRIPT); + } finally { + if (origMode === undefined) delete process.env.GSD_TEST_MODE; + else process.env.GSD_TEST_MODE = origMode; + } + + const hookPath = path.join(tmpDir, 'hooks', 'managed.js'); + fs.mkdirSync(path.dirname(hookPath), { recursive: true }); + fs.writeFileSync(hookPath, 'user edited hook\n', 'utf8'); + fs.writeFileSync( + path.join(tmpDir, MANIFEST_NAME), + JSON.stringify({ + version: 'legacy', + timestamp: '2026-05-11T00:00:00.000Z', + files: { + 'hooks/managed.js': crypto.createHash('sha256').update('managed hook\n').digest('hex'), + }, + }, null, 2), + 'utf8' + ); + + fs.mkdirSync(outside, { recursive: true }); + try { + fs.symlinkSync(outside, path.join(tmpDir, PATCHES_DIR_NAME), 'dir'); + } catch { + return; + } + + const modified = mod.saveLocalPatches(tmpDir); + + assert.deepEqual(modified, []); + assert.equal(fs.existsSync(path.join(outside, 'hooks', 'managed.js')), false); }); }); diff --git a/tests/installer-migrations.test.cjs b/tests/installer-migrations.test.cjs index 8796dd328..a424a014c 100644 --- a/tests/installer-migrations.test.cjs +++ b/tests/installer-migrations.test.cjs @@ -407,6 +407,37 @@ test('rejects migration actions that escape the install root', () => { } }); +test('rejects migration actions that normalize to the install root', () => { + const configDir = createTempInstall(); + try { + writeManifest(configDir, {}); + + for (const relPath of ['.', 'hooks/..']) { + assert.throws( + () => planInstallerMigrations({ + configDir, + migrations: [ + { + id: `2026-05-11-bad-path-${relPath.replace(/[^a-z0-9]/gi, '-')}`, + description: 'Bad path', + plan: () => [ + { + type: 'remove-managed', + relPath, + reason: 'bad path', + }, + ], + }, + ], + }), + /relPath must stay inside configDir/ + ); + } + } finally { + cleanup(configDir); + } +}); + test('runs discovered installer migrations against manifest-managed legacy orphan files', () => { const configDir = createTempInstall(); try { From 908a19cd04d3258b6330117399a33e77922050a3 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 11 May 2026 14:36:14 -0400 Subject: [PATCH 2/4] fix: harden installer migration integration --- bin/install.js | 29 +++++--- docs/installer-migrations.md | 4 +- .../bin/lib/installer-migrations.cjs | 51 +++++++++++-- tests/bug-2771-user-profile-manifest.test.cjs | 14 ++-- tests/bug-3033-sdk-flag-wired.test.cjs | 15 ++++ ...ler-migration-install-integration.test.cjs | 72 ++++++++++++++++++- tests/installer-migrations.test.cjs | 58 +++++++++++++++ 7 files changed, 220 insertions(+), 23 deletions(-) diff --git a/bin/install.js b/bin/install.js index 007bc783c..f57d2c124 100755 --- a/bin/install.js +++ b/bin/install.js @@ -9898,6 +9898,12 @@ function installSdkIfNeeded(opts) { if (!fs.existsSync(sdkCliPath)) { const ir = buildSdkFailFastReport(sdkDir, sdkCliPath); renderSdkFailFastReport(ir); + if (opts.throwOnFailure) { + const error = new Error(`GSD SDK prebuilt artifact missing: ${sdkCliPath}`); + error.code = 'GSD_SDK_MISSING_DIST'; + error.exitCode = 1; + throw error; + } process.exit(1); } @@ -10586,14 +10592,6 @@ function installAllRuntimes(runtimes, isGlobal, isInteractive) { migrationsDir: path.join(_gsdLibDir, 'installer-migrations'), }); - for (const runtime of runtimes) { - const result = install(isGlobal, runtime, { installerMigrations }); - results.push(result); - } - - const statuslineRuntimes = ['claude', 'gemini']; - const primaryStatuslineResult = results.find(r => statuslineRuntimes.includes(r.runtime)); - const rollbackFinalizedInstallerMigrations = (error) => { const rollbackFailures = []; for (const result of [...results].reverse()) { @@ -10612,6 +10610,19 @@ function installAllRuntimes(runtimes, isGlobal, isInteractive) { } }; + try { + for (const runtime of runtimes) { + const result = install(isGlobal, runtime, { installerMigrations }); + results.push(result); + } + } catch (error) { + rollbackFinalizedInstallerMigrations(error); + throw error; + } + + const statuslineRuntimes = ['claude', 'gemini']; + const primaryStatuslineResult = results.find(r => statuslineRuntimes.includes(r.runtime)); + const finalize = (shouldInstallStatusline, shouldInstallBanner) => { try { // Verify sdk/dist/cli.js is present and executable. The dist is shipped @@ -10619,7 +10630,7 @@ function installAllRuntimes(runtimes, isGlobal, isInteractive) { // the parent package's bin/gsd-sdk.js shim, so no sub-install is needed. // Skip with --no-sdk. Skip with isLocal (#2678 — local installs don't own global npm). // #3033: pass forceSdk so --sdk overrides the local-install skip. - installSdkIfNeeded({ isLocal: !isGlobal, forceSdk: hasSdk }); + installSdkIfNeeded({ isLocal: !isGlobal, forceSdk: hasSdk, throwOnFailure: true }); const printSummaries = () => { for (const result of results) { diff --git a/docs/installer-migrations.md b/docs/installer-migrations.md index 2428805e6..8c378bec1 100644 --- a/docs/installer-migrations.md +++ b/docs/installer-migrations.md @@ -259,8 +259,8 @@ entry point for every supported runtime: Claude Code, Antigravity, Augment, Cline, CodeBuddy, Codex, Copilot, Cursor, Gemini, Hermes Agent, Kilo, OpenCode, Qwen Code, Trae, and Windsurf. The installer invokes the same migration runner with `baselineScan: true`, reports the projected action rows, applies safe -non-interactive actions before materialization, writes install state after a -successful apply, and fails before writing new package files when the runner +non-interactive actions before materialization, persists install state only after +package materialization and finalization succeed, and fails before writing new package files when the runner returns blocked user-choice actions. Phase 1-3 built the planning, apply, rollback, install-state, baseline, and diff --git a/get-shit-done/bin/lib/installer-migrations.cjs b/get-shit-done/bin/lib/installer-migrations.cjs index f497907b4..8aaa3eeb9 100644 --- a/get-shit-done/bin/lib/installer-migrations.cjs +++ b/get-shit-done/bin/lib/installer-migrations.cjs @@ -90,10 +90,14 @@ function normalizeRelPath(relPath) { throw new Error('migration action relPath must be a non-empty string'); } const normalized = relPath.replace(/\\/g, '/'); - if (normalized.startsWith('/') || normalized.includes('../') || normalized === '..') { + if (path.isAbsolute(normalized) || path.win32.isAbsolute(normalized)) { throw new Error(`migration action relPath must stay inside configDir: ${relPath}`); } - return normalized; + const segments = normalized.split('/'); + if (segments.some((segment) => segment === '' || segment === '.' || segment === '..')) { + throw new Error(`migration action relPath must stay inside configDir: ${relPath}`); + } + return segments.join('/'); } function classifyArtifact(configDir, relPath, manifest) { @@ -192,11 +196,10 @@ function discoverInstallerMigrations({ migrationsDir }) { .sort() .flatMap((fileName) => { const source = path.join(migrationsDir, fileName); - const checksum = `sha256:${sha256File(source)}`; delete require.cache[require.resolve(source)]; const exported = require(source); const records = Array.isArray(exported) ? exported : [exported]; - return records.map((record) => validateMigrationRecord({ ...record, checksum: record.checksum || checksum }, source)); + return records.map((record) => validateMigrationRecord(record, source)); }); } @@ -398,6 +401,7 @@ function planInstallerMigrations({ manifest, state, pendingMigrationIds: pending.map((migration) => migration.id), + pendingMigrations: pending, actions, blocked, }; @@ -490,6 +494,9 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t : null; try { + fs.mkdirSync(path.dirname(journalPath), { recursive: true }); + fs.writeFileSync(journalPath, JSON.stringify(journal, null, 2) + '\n', 'utf8'); + for (const action of plan.actions) { if ( action.type !== 'remove-managed' && @@ -549,7 +556,6 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t fs.rmSync(fullPath, { force: true }); } - fs.mkdirSync(path.dirname(journalPath), { recursive: true }); fs.writeFileSync(journalPath, JSON.stringify(journal, null, 2) + '\n', 'utf8'); const state = readInstallState(configDir); @@ -608,6 +614,38 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t } } +function markPendingMigrationsApplied({ configDir, plan, now = () => new Date().toISOString() }) { + if (!plan || !Array.isArray(plan.pendingMigrationIds) || plan.pendingMigrationIds.length === 0) { + return []; + } + const appliedAt = now(); + const state = readInstallState(configDir); + const applied = appliedMigrationIds(state); + const checksumsByMigrationId = new Map(); + for (const migration of plan.pendingMigrations || []) { + checksumsByMigrationId.set(migration.id, migrationChecksum(migration)); + } + const nextApplied = [...state.appliedMigrations]; + const newlyApplied = []; + for (const id of plan.pendingMigrationIds) { + if (applied.has(id)) continue; + nextApplied.push({ + id, + appliedAt, + journal: null, + checksum: checksumsByMigrationId.get(id) || null, + }); + newlyApplied.push(id); + } + if (newlyApplied.length > 0) { + writeInstallState(configDir, { + schemaVersion: 1, + appliedMigrations: nextApplied, + }); + } + return newlyApplied; +} + function runInstallerMigrations({ configDir, runtime = null, @@ -624,9 +662,10 @@ function runInstallerMigrations({ try { const plan = planInstallerMigrations({ configDir, runtime, scope, migrations, baselineScan, now }); if (plan.actions.length === 0) { + const appliedMigrationIds = markPendingMigrationsApplied({ configDir, plan, now }); completed = true; return { - appliedMigrationIds: [], + appliedMigrationIds, journalRelPath: null, plan, }; diff --git a/tests/bug-2771-user-profile-manifest.test.cjs b/tests/bug-2771-user-profile-manifest.test.cjs index eeda82adc..e2385d953 100644 --- a/tests/bug-2771-user-profile-manifest.test.cjs +++ b/tests/bug-2771-user-profile-manifest.test.cjs @@ -220,9 +220,16 @@ describe('#2771: USER_OWNED_ARTIFACTS is a single source of truth', () => { describe('manifest path safety', () => { let tmpDir; + let outside; - beforeEach(() => { tmpDir = createTempDir('gsd-manifest-path-safety-'); }); - afterEach(() => { cleanup(tmpDir); }); + beforeEach(() => { + tmpDir = createTempDir('gsd-manifest-path-safety-'); + outside = path.join(tmpDir, '..', `outside-managed-file-${path.basename(tmpDir)}.txt`); + }); + afterEach(() => { + if (outside) fs.rmSync(outside, { recursive: true, force: true }); + cleanup(tmpDir); + }); test('saveLocalPatches ignores manifest entries that escape the install root', () => { const origMode = process.env.GSD_TEST_MODE; @@ -236,7 +243,6 @@ describe('manifest path safety', () => { else process.env.GSD_TEST_MODE = origMode; } - const outside = path.join(tmpDir, '..', 'outside-managed-file.txt'); fs.writeFileSync(outside, 'outside user data\n', 'utf8'); fs.writeFileSync( path.join(tmpDir, MANIFEST_NAME), @@ -254,6 +260,6 @@ describe('manifest path safety', () => { assert.deepEqual(modified, []); assert.equal(fs.readFileSync(outside, 'utf8'), 'outside user data\n'); - assert.equal(fs.existsSync(path.join(tmpDir, PATCHES_DIR_NAME, '..', 'outside-managed-file.txt')), false); + assert.equal(fs.existsSync(path.join(tmpDir, PATCHES_DIR_NAME, '..', path.basename(outside))), false); }); }); diff --git a/tests/bug-3033-sdk-flag-wired.test.cjs b/tests/bug-3033-sdk-flag-wired.test.cjs index ee867735a..276a25699 100644 --- a/tests/bug-3033-sdk-flag-wired.test.cjs +++ b/tests/bug-3033-sdk-flag-wired.test.cjs @@ -150,6 +150,21 @@ describe('bug #3033: --sdk flag (opts.forceSdk) must be wired into installSdkIfN ); }); + test('throwOnFailure=true converts missing SDK dist into catchable error', () => { + fs.mkdirSync(sdkDir, { recursive: true }); + + assert.throws( + () => captureConsole(() => { + installSdkIfNeeded({ sdkDir, isLocal: true, forceSdk: true, throwOnFailure: true }); + }), + (error) => { + assert.equal(error.code, 'GSD_SDK_MISSING_DIST'); + assert.equal(error.exitCode, 1); + return true; + } + ); + }); + test('forceSdk=false (default) + isLocal=true + dist missing: retains #2678 soft-skip', () => { // Verify the #2678 contract is not broken for the default (no --sdk) path. fs.mkdirSync(sdkDir, { recursive: true }); diff --git a/tests/installer-migration-install-integration.test.cjs b/tests/installer-migration-install-integration.test.cjs index 6e7b496ed..9f831fe81 100644 --- a/tests/installer-migration-install-integration.test.cjs +++ b/tests/installer-migration-install-integration.test.cjs @@ -86,6 +86,34 @@ function withWriteFailure(matchPath, fn) { } } +function withSdkDistPresent(fn) { + const sdkCliPath = path.join(__dirname, '..', 'sdk', 'dist', 'cli.js'); + const originalExistsSync = fs.existsSync; + const originalStatSync = fs.statSync; + const originalChmodSync = fs.chmodSync; + fs.existsSync = (filePath) => { + if (path.resolve(String(filePath)) === path.resolve(sdkCliPath)) return true; + return originalExistsSync.call(fs, filePath); + }; + fs.statSync = (filePath, ...args) => { + if (path.resolve(String(filePath)) === path.resolve(sdkCliPath)) { + return { mode: 0o755 }; + } + return originalStatSync.call(fs, filePath, ...args); + }; + fs.chmodSync = (filePath, ...args) => { + if (path.resolve(String(filePath)) === path.resolve(sdkCliPath)) return; + return originalChmodSync.call(fs, filePath, ...args); + }; + try { + return fn(); + } finally { + fs.existsSync = originalExistsSync; + fs.statSync = originalStatSync; + fs.chmodSync = originalChmodSync; + } +} + function stripAnsi(value) { return value.replace(/\x1b\[[0-9;]*m/g, ''); } @@ -188,8 +216,10 @@ describe('installer migration install integration', { concurrency: false }, () = assert.throws( () => captureConsole(() => withEnv('CLAUDE_CONFIG_DIR', claudeHome, () => - withWriteFailure(path.join(claudeHome, 'settings.json'), () => - installModule.installAllRuntimes(['claude'], true, false) + withSdkDistPresent(() => + withWriteFailure(path.join(claudeHome, 'settings.json'), () => + installModule.installAllRuntimes(['claude'], true, false) + ) ) ) ), @@ -203,6 +233,44 @@ describe('installer migration install integration', { concurrency: false }, () = assert.equal(fs.existsSync(path.join(claudeHome, 'gsd-install-state.json')), false); }); + test('rolls back completed runtime migrations when a later runtime install fails', () => { + const claudeHome = path.join(tmpRoot, '.claude'); + fs.mkdirSync(claudeHome, { recursive: true }); + writeFile(claudeHome, 'hooks/statusline.js', 'legacy managed hook\n'); + writeManifest(claudeHome, { + 'hooks/statusline.js': sha256('legacy managed hook\n'), + }); + + writeFile(codexHome, 'hooks/statusline.js', 'legacy managed hook\n'); + writeManifest(codexHome, { + 'hooks/statusline.js': sha256('legacy managed hook\n'), + }); + + assert.throws( + () => captureConsole(() => + withEnv('CLAUDE_CONFIG_DIR', claudeHome, () => + withEnv('CODEX_HOME', codexHome, () => + withWriteFailure(path.join(codexHome, 'get-shit-done', 'VERSION'), () => + installModule.installAllRuntimes(['claude', 'codex'], true, false) + ) + ) + ) + ), + /injected write failure for VERSION/ + ); + + assert.equal( + fs.readFileSync(path.join(claudeHome, 'hooks/statusline.js'), 'utf8'), + 'legacy managed hook\n' + ); + assert.equal(fs.existsSync(path.join(claudeHome, 'gsd-install-state.json')), false); + assert.equal( + fs.readFileSync(path.join(codexHome, 'hooks/statusline.js'), 'utf8'), + 'legacy managed hook\n' + ); + assert.equal(fs.existsSync(path.join(codexHome, 'gsd-install-state.json')), false); + }); + for (const runtime of SUPPORTED_RUNTIMES) { test(`runs managed cleanup migrations for ${runtime}`, () => { const targetDir = path.join(tmpRoot, `.${runtime}-managed-cleanup`); diff --git a/tests/installer-migrations.test.cjs b/tests/installer-migrations.test.cjs index 5a112fd97..02bd9685c 100644 --- a/tests/installer-migrations.test.cjs +++ b/tests/installer-migrations.test.cjs @@ -934,6 +934,33 @@ test('skips migration records already present in install state', () => { } }); +test('marks zero-action pending migrations as applied', () => { + const configDir = createTempInstall(); + try { + writeManifest(configDir, {}); + + const result = runInstallerMigrations({ + configDir, + migrations: [ + { + id: '2026-05-11-noop-cleanup', + description: 'No-op cleanup', + plan: () => [], + }, + ], + now: () => '2026-05-11T00:00:06.000Z', + }); + + assert.deepEqual(result.appliedMigrationIds, ['2026-05-11-noop-cleanup']); + assert.equal(result.journalRelPath, null); + assert.deepEqual(readInstallState(configDir).appliedMigrations.map((entry) => entry.id), [ + '2026-05-11-noop-cleanup', + ]); + } finally { + cleanup(configDir); + } +}); + test('refuses to plan an already-applied migration whose checksum changed', () => { const configDir = createTempInstall(); try { @@ -1064,6 +1091,37 @@ test('rejects migration actions that escape the install root', () => { } }); +test('rejects migration actions that normalize to the install root', () => { + const configDir = createTempInstall(); + try { + writeManifest(configDir, {}); + + for (const relPath of ['.', 'hooks/..']) { + assert.throws( + () => planInstallerMigrations({ + configDir, + migrations: [ + { + id: `2026-05-11-bad-path-${relPath.replace(/[^a-z0-9]/gi, '-')}`, + description: 'Bad path', + plan: () => [ + { + type: 'remove-managed', + relPath, + reason: 'bad path', + }, + ], + }, + ], + }), + /relPath must stay inside configDir/ + ); + } + } finally { + cleanup(configDir); + } +}); + test('runs discovered installer migrations against manifest-managed legacy orphan files', () => { const configDir = createTempInstall(); try { From 041e94c43b197370e6d7f8ea32fa973fcdd89a7f Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 11 May 2026 14:40:57 -0400 Subject: [PATCH 3/4] fix: roll back installer migration state --- bin/install.js | 11 +++- .../bin/lib/installer-migrations.cjs | 59 +++++++++++++++++++ tests/installer-migrations.test.cjs | 52 ++++++++++++++++ 3 files changed, 121 insertions(+), 1 deletion(-) diff --git a/bin/install.js b/bin/install.js index b2b9daae9..ba9fab9a9 100755 --- a/bin/install.js +++ b/bin/install.js @@ -7634,6 +7634,13 @@ function install(isGlobal, runtime = 'claude') { // Track installation failures const failures = []; + let installerMigrationResult = null; + const rollbackInstallerMigrations = () => { + if (!installerMigrationResult || typeof installerMigrationResult.rollback !== 'function') return; + const rollback = installerMigrationResult.rollback; + installerMigrationResult = null; + rollback(); + }; // Save any locally modified GSD files before they get wiped. // The pristine context lets saveLocalPatches populate gsd-pristine/ via @@ -7647,7 +7654,7 @@ function install(isGlobal, runtime = 'claude') { }); // Run manifest-backed cleanup migrations before package materialization. - runInstallerMigrations({ configDir: targetDir }); + installerMigrationResult = runInstallerMigrations({ configDir: targetDir }); // #3245 — Codex idempotent rollback. Capture pre-install state of ALL // directories and files GSD will mutate so that any post-install validation @@ -8439,6 +8446,7 @@ function install(isGlobal, runtime = 'claude') { // #3245 CR finding 2 — any throw in the pre-config install operations (skills copy, // agents copy, VERSION write, manifest write, etc.) triggers the Codex pre-config // rollback so the caller is never left in a partially-installed state. + rollbackInstallerMigrations(); if (_codexPreConfigRollback) { _codexPreConfigRollback(); } @@ -8471,6 +8479,7 @@ function install(isGlobal, runtime = 'claude') { // existence checks. Safe to call before any snapshots are captured (variables // default to empty Set / null). Does NOT touch non-gsd-* user content. const restoreCodexSnapshot = () => { + rollbackInstallerMigrations(); // 1. config.toml if (codexConfigPreInstallSnapshot !== null) { try { fs.writeFileSync(codexConfigPathPreInstall, codexConfigPreInstallSnapshot); } diff --git a/get-shit-done/bin/lib/installer-migrations.cjs b/get-shit-done/bin/lib/installer-migrations.cjs index 966da17f2..e0b3143ce 100644 --- a/get-shit-done/bin/lib/installer-migrations.cjs +++ b/get-shit-done/bin/lib/installer-migrations.cjs @@ -232,6 +232,10 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t actions: [], }; const rollback = []; + const installStatePath = path.join(configDir, INSTALL_STATE_NAME); + const previousInstallStateBytes = fs.existsSync(installStatePath) + ? fs.readFileSync(installStatePath) + : null; try { for (const action of plan.actions) { @@ -285,6 +289,13 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t return { appliedMigrationIds: journal.appliedMigrationIds, journalRelPath, + rollback: () => rollbackAppliedMigrationResult({ + configDir, + journal, + journalPath, + rollbackRoot, + previousInstallStateBytes, + }), }; } catch (error) { const rollbackFailures = []; @@ -311,6 +322,54 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t } } +function rollbackAppliedMigrationResult({ configDir, journal, journalPath, rollbackRoot, previousInstallStateBytes }) { + const failures = []; + for (const action of [...journal.actions].reverse()) { + if (!action.rollbackRelPath) continue; + const rollbackPath = path.join(configDir, action.rollbackRelPath); + const dest = path.join(configDir, action.relPath); + try { + if (fs.existsSync(rollbackPath)) { + fs.mkdirSync(path.dirname(dest), { recursive: true }); + fs.copyFileSync(rollbackPath, dest); + } + } catch (error) { + failures.push({ relPath: action.relPath, error: error.message }); + } + if (action.backupRelPath) { + try { + fs.rmSync(path.join(configDir, action.backupRelPath), { force: true }); + } catch { + // backup cleanup is best-effort; preserve restore failures above + } + } + } + + try { + if (previousInstallStateBytes === null) { + fs.rmSync(path.join(configDir, INSTALL_STATE_NAME), { force: true }); + } else { + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync(path.join(configDir, INSTALL_STATE_NAME), previousInstallStateBytes); + } + } catch (error) { + failures.push({ relPath: INSTALL_STATE_NAME, error: error.message }); + } + + try { + fs.rmSync(journalPath, { force: true }); + fs.rmSync(rollbackRoot, { recursive: true, force: true }); + } catch { + // journal cleanup is best-effort; the rollback above is the safety-critical part + } + + if (failures.length > 0) { + const error = new Error('migration rollback incomplete'); + error.rollbackFailures = failures; + throw error; + } +} + function runInstallerMigrations({ configDir, migrationsDir = DEFAULT_MIGRATIONS_DIR, diff --git a/tests/installer-migrations.test.cjs b/tests/installer-migrations.test.cjs index a424a014c..b13de19ff 100644 --- a/tests/installer-migrations.test.cjs +++ b/tests/installer-migrations.test.cjs @@ -208,6 +208,58 @@ test('applies an unblocked plan with a journal and install-state update', () => } }); +test('rollback handle restores files and install state after a successful apply', () => { + const configDir = createTempInstall(); + try { + writeFile(configDir, 'hooks/old-hook.js', 'managed hook\n'); + writeManifest(configDir, { + 'hooks/old-hook.js': sha256('managed hook\n'), + }); + writeInstallState(configDir, { + schemaVersion: 1, + appliedMigrations: [ + { + id: 'already-applied', + appliedAt: '2026-05-10T00:00:00.000Z', + journal: 'gsd-migration-journal/prior.json', + }, + ], + }); + + const plan = planInstallerMigrations({ + configDir, + migrations: [ + { + id: '2026-05-11-remove-old-hook', + description: 'Remove retired hook', + plan: () => [ + { + type: 'remove-managed', + relPath: 'hooks/old-hook.js', + reason: 'retired hook', + }, + ], + }, + ], + now: () => '2026-05-11T00:00:00.000Z', + }); + + const result = applyInstallerMigrationPlan({ + configDir, + plan, + now: () => '2026-05-11T00:00:01.000Z', + }); + + result.rollback(); + + assert.equal(fs.readFileSync(path.join(configDir, 'hooks/old-hook.js'), 'utf8'), 'managed hook\n'); + assert.deepEqual(readInstallState(configDir).appliedMigrations.map((entry) => entry.id), ['already-applied']); + assert.equal(fs.existsSync(path.join(configDir, result.journalRelPath)), false); + } finally { + cleanup(configDir); + } +}); + test('rolls back touched files and leaves state unchanged when apply fails', () => { const configDir = createTempInstall(); try { From 5002d51d92ce5a8bc9343655d42a50d9a220ff3b Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 11 May 2026 14:59:33 -0400 Subject: [PATCH 4/4] Docs(installer): reword Antigravity migration contract note --- docs/installer-migrations.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/installer-migrations.md b/docs/installer-migrations.md index d8a54b635..c9701c4c3 100644 --- a/docs/installer-migrations.md +++ b/docs/installer-migrations.md @@ -314,7 +314,7 @@ for the new shape before changing migration behavior. | Gemini CLI | TOML slash commands in `commands/gsd/*.toml`; agents in `agents/gsd-*.md`; `settings.json` feature flag, hooks, and statusline | Global `GEMINI_CONFIG_DIR` or `~/.gemini`; local `./.gemini` | GSD owns generated commands/agents/hooks and only GSD settings entries; local command copy may be skipped when global GSD commands already exist | [Custom commands](https://google-gemini.github.io/gemini-cli/docs/cli/custom-commands.html), [configuration](https://google-gemini.github.io/gemini-cli/docs/cli/configuration.html); docs checked 2026-05-11 | | Codex | Skills in `skills/gsd-*/SKILL.md`; agents as source markdown plus per-agent TOML in `agents/`; `[agents.gsd-*]` and hooks in `config.toml` | Global `CODEX_HOME` or `~/.codex`; local `./.codex` | GSD owns generated skills, generated agent TOML, `agents.gsd-*` config sections, `[features].codex_hooks` when added by GSD, and GSD hook entries | [Codex config schema](https://developers.openai.com/codex/config-schema.json), [Codex developer docs](https://developers.openai.com/codex/); docs not versioned, checked 2026-05-11; installer compatibility sentinel: Codex 0.124.0 agent table shape | | GitHub Copilot | Skills in `skills/gsd-*/SKILL.md`; agents as `.agent.md`; repository instructions in `copilot-instructions.md` | Global `COPILOT_CONFIG_DIR` or `~/.copilot`; local `./.github` | GSD owns generated skill/agent files and GSD-authored instruction files; no hook/statusline ownership | [Repository custom instructions](https://docs.github.com/en/copilot/how-tos/configure-custom-instructions/add-repository-instructions), [Copilot CLI custom instructions](https://docs.github.com/en/copilot/how-tos/copilot-cli/add-custom-instructions); GitHub Docs product docs, checked 2026-05-11 | -| Antigravity | Skills in `skills/gsd-*/SKILL.md`; agents in `agents/`; Gemini-style `settings.json` hooks when installed by GSD | Global `ANTIGRAVITY_CONFIG_DIR` or `~/.gemini/antigravity`; local `./.agent` | GSD owns generated skills/agents/hooks and GSD settings entries only | Public Antigravity install/config docs for this file layout were not stable or complete as of 2026-05-11; GSD uses the Gemini-compatible settings contract as a compatibility shim | +| Antigravity | Skills in `skills/gsd-*/SKILL.md`; agents in `agents/`; Gemini-style `settings.json` hooks when installed by GSD | Global `ANTIGRAVITY_CONFIG_DIR` or `~/.gemini/antigravity`; local `./.agent` | GSD owns generated skills/agents/hooks and GSD settings entries only | Checked 2026-05-11 against available Antigravity install/config material; this row records GSD's Gemini-compatible settings contract as the compatibility baseline for installer migrations. | | Cursor | Skills in `skills/gsd-*/SKILL.md`; agents in `agents/`; rule references under `rules/` | Global `CURSOR_CONFIG_DIR` or `~/.cursor`; local `./.cursor` | GSD owns generated skills/agents and GSD rule files or references; no hook/statusline ownership | [Cursor rules](https://docs.cursor.com/context/rules); docs not versioned, checked 2026-05-11 | | Windsurf | Skills in `skills/gsd-*/SKILL.md`; agents in `agents/`; rule references under `rules/` | Global `WINDSURF_CONFIG_DIR` or `~/.codeium/windsurf`; local `./.windsurf` | GSD owns generated skills/agents and GSD rule files or references; no hook/statusline ownership | Windsurf public rule docs were source-limited in search results as of 2026-05-11; installer targets the common workspace rules convention `./.windsurf/rules` and must be rechecked before migrations rewrite rules | | Augment Code | Skills in `skills/gsd-*/SKILL.md`; agents in `agents/` | Global `AUGMENT_CONFIG_DIR` or `~/.augment`; local `./.augment` | GSD owns generated skills/agents only; no hook/statusline ownership | [Augment Agent Skills](https://docs.augmentcode.com/cli/skills), [Augment IDE skills](https://docs.augmentcode.com/using-augment/skills); IDE skills public beta in VS Code 0.789.0+, checked 2026-05-11 |