Feat(installer): Phase 4 install/update integration#3402
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR implements Phases 3–4 of the installer migrations feature, integrating baseline scanning and install-flow wiring. It introduces migration planning and execution with transactional rollback, reporting, and integration into the multi-runtime installer to run migrations before package materialization. ChangesInstaller Migrations Phase 3–4: Baseline Scanning and Install Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
trek-e
changed the title
trek-e
changed the base branch from
codex/installer-migrations-phase-three
to
main
trek-e
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/installer-migration-install-integration.test.cjs (1)
60-72: 💤 Low value
captureConsoleonly interceptsconsole.log/console.warn.If
bin/install.jsever emits the “Installer migrations” banner or row labels throughprocess.stdout.write,console.info, orconsole.error, the in-process tests (lines 127-138, 145-153) will silently see an empty/partial output and theassert.matchcould fail in confusing ways or pass for the wrong reason. The CLI-mode tests (line 215+) are unaffected because they usespawnSyncand read combined stdout/stderr.Worth either widening the patch to cover
console.info/console.error(and optionallyprocess.stdout.write) or documenting the assumption.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/installer-migration-install-integration.test.cjs` around lines 60 - 72, The captureConsole helper currently only overrides console.log and console.warn; update captureConsole to also intercept console.info and console.error and optionally process.stdout.write so tests capture all banner output from bin/install.js: save originals (console.info, console.error, and process.stdout.write), override them to push their stringified output into the existing lines array (keeping the same join behavior), call fn(), then restore all originals in the finally block; reference the captureConsole function to make these changes so in-process tests correctly assert on installer output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/install.js`:
- Around line 10615-10623: The finalize() catch/rollback path is bypassed
because installSdkIfNeeded() calls process.exit(1) on failure; change
installSdkIfNeeded() to surface failures by throwing an Error or returning a
rejected Promise instead of calling process.exit(1) so finalize() can catch it
and call rollbackFinalizedInstallerMigrations(); update any callers that expect
exit behavior to propagate the error (e.g., in the same module where finalize()
invokes installSdkIfNeeded()) and remove direct process.exit calls from
installSdkIfNeeded() so abort logic is centralized in finalize().
- Around line 10589-10591: The loop over runtimes that calls install(isGlobal,
runtime, { installerMigrations }) must be wrapped in a try/catch so that if any
install() throws, you iterate the previously pushed results array in reverse and
invoke their rollback/cleanup method (the rollback handle returned by install())
before rethrowing the original error; use the runtimes, results and install
identifiers to locate the code and ensure rollback calls are awaited/handled in
reverse order and that the caught error is rethrown after cleanup.
In `@get-shit-done/bin/lib/installer-migrations.cjs`:
- Around line 625-633: The zero-action branch returns early without persisting
that pending migrations were evaluated; update the branch that checks
plan.actions.length === 0 so it records plan.pendingMigrationIds into the
install state (the same place where successful runs persist
appliedMigrationIds/journalRelPath in gsd-install-state.json), set completed =
true and return the object with appliedMigrationIds: plan.pendingMigrationIds
and journalRelPath: null (keeping the plan), ensuring the state write path used
elsewhere in this module (the code that updates gsd-install-state.json after
normal runs) is invoked or duplicated so pendingMigrationIds are saved.
- Around line 492-553: The journal is only written after performing filesystem
mutations; persist a prewritten journal before any destructive action and update
it after each action so recovery can resume. Specifically, before iterating
plan.actions in the function that uses journal, journalPath, plan.actions,
journalAction, rollback, rollbackRootRelPath and backupRootRelPath, create the
journal directory and write an initial journal entry (e.g., status "in-progress"
or actions marked "pending") to journalPath; then after each successful action
commit (where the code currently pushes journal.actions and does
fs.copyFileSync/fs.rmSync/writeFileAtomicSync), immediately fs.writeFileSync (or
writeFileAtomicSync) the updated journal to journalPath to flush the new state;
ensure you create the directory once (fs.mkdirSync(path.dirname(journalPath), {
recursive: true })) before the loop and use durable writes so a crash still
leaves a recoverable journal.
- Around line 193-199: The current flatMap logic sets checksum =
`sha256File(source)` and applies it to every exported record, coupling siblings
to one file checksum; change the handling in the flatMap/export processing so
that if exported is an array you either (a) reject array exports with an
explicit error (i.e., disallow multi-exports) or (b) derive a unique per-record
checksum instead of the file hash — for example call sha256 on a stable
serialization of each record (or combine the file hash with a record-specific
identifier/index) and pass record.checksum || derivedChecksum into
validateMigrationRecord; update the code around exported, records, sha256File
and validateMigrationRecord to implement the chosen approach.
- Around line 88-97: normalizeRelPath currently allows inputs like "." or
"hooks/.." which normalize to the config root and later cause EISDIR; update
normalizeRelPath to run a posix-safe normalization (e.g., path.posix.normalize
after replacing backslashes), then reject any result that is empty, equal to "."
or "..", starts with "/" or with "..", or contains a ".." path segment (e.g.,
normalized.split('/').includes('..')); this ensures values that would resolve to
the configDir root are rejected before ensureInsideConfig and file operations.
In `@tests/bug-2771-user-profile-manifest.test.cjs`:
- Around line 239-258: The test leaks a file because `outside` is created as a
sibling of `tmpDir` in the system temp root and only `cleanup(tmpDir)` is
called; fix by isolating the external file into a second temp directory you
control (e.g., create a new temp dir variable for the "outside" file) or ensure
the test removes it by unlinking `outside` in the test teardown (e.g., in
`afterEach`), updating references to `outside` and the manifest content where
`MANIFEST_NAME` and `mod.saveLocalPatches(tmpDir)` are used so the cleanup
removes all created temp artifacts.
---
Nitpick comments:
In `@tests/installer-migration-install-integration.test.cjs`:
- Around line 60-72: The captureConsole helper currently only overrides
console.log and console.warn; update captureConsole to also intercept
console.info and console.error and optionally process.stdout.write so tests
capture all banner output from bin/install.js: save originals (console.info,
console.error, and process.stdout.write), override them to push their
stringified output into the existing lines array (keeping the same join
behavior), call fn(), then restore all originals in the finally block; reference
the captureConsole function to make these changes so in-process tests correctly
assert on installer output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ddf992a4-ae6b-48af-81a5-c1a26c0e5c0b
📒 Files selected for processing (22)
.changeset/gentle-jays-zip.md.changeset/merry-quails-roam.md.changeset/steady-yaks-caper.md.changeset/vivid-foxes-romp.mdbin/install.jsdocs/ARCHITECTURE.mddocs/INVENTORY-MANIFEST.jsondocs/INVENTORY.mddocs/README.mddocs/adr/0008-installer-migration-module.mddocs/adr/README.mddocs/installer-migrations.mdget-shit-done/bin/lib/installer-migration-report.cjsget-shit-done/bin/lib/installer-migrations.cjsget-shit-done/bin/lib/installer-migrations/000-first-time-baseline.cjsget-shit-done/bin/lib/installer-migrations/001-legacy-orphan-files.cjsget-shit-done/bin/lib/installer-migrations/002-codex-legacy-hooks-json.cjstests/bug-2771-user-profile-manifest.test.cjstests/bug-3357-codex-legacy-hooks-json-migration.test.cjstests/installer-migration-install-integration.test.cjstests/installer-migration-report.test.cjstests/installer-migrations.test.cjs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/installer-migrations.md (1)
272-275: ⚡ Quick winDocument failure-time cleanup of migration artifacts explicitly.
This rollback section should also state that run-scoped journal/rollback/backup/lock artifacts are cleaned on failure, matching the Phase 4 behavior and preventing ambiguity for maintainers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/installer-migrations.md` around lines 272 - 275, Update the rollback section to explicitly document failure-time cleanup: state that when an apply step fails the executor uses the journal to restore modified paths and also removes any run-scoped artifacts (journal, rollback metadata, backups, and locks) created for that installer run, matching the Phase 4 cleanup behavior so maintainers know these files are cleaned on failure and not left behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/installer-migrations.md`:
- Around line 261-264: Update the sentence that currently begins "with
`baselineScan: true`, reports the projected action rows..." to explicitly state
that the install-state is persisted only after package
materialization/finalization completes successfully; reference the Phase 4
success semantics and the earlier execution order (the lines describing applying
actions and materialization) and change wording so it cannot be read as "written
after migration apply"—make clear that if materialization/finalization fails or
runner returns blocked user-choice actions, install-state is not written.
---
Nitpick comments:
In `@docs/installer-migrations.md`:
- Around line 272-275: Update the rollback section to explicitly document
failure-time cleanup: state that when an apply step fails the executor uses the
journal to restore modified paths and also removes any run-scoped artifacts
(journal, rollback metadata, backups, and locks) created for that installer run,
matching the Phase 4 cleanup behavior so maintainers know these files are
cleaned on failure and not left behind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ecb08f94-12b4-4108-8307-da45cb1bd6ea
📒 Files selected for processing (1)
docs/installer-migrations.md
33 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
get-shit-done/bin/lib/installer-migrations.cjs (1)
497-559:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFlush the journal after each committed action.
The journal is pre-created now, but it is still only rewritten once after the loop. If the process dies after a remove/rewrite and before Line 559, the rollback snapshots exist but the durable journal does not describe what was already applied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@get-shit-done/bin/lib/installer-migrations.cjs` around lines 497 - 559, The journal is only written once at the end (fs.writeFileSync(journalPath...)), so if the process dies mid-loop applied actions won’t be durable; after each committed action update (every place you push into journal.actions inside the loop — e.g., where journalAction is added for 'missing', 'recorded'/'preserved', 'removed', 'rewritten') immediately flush the journal to disk by calling the same write used elsewhere (fs.writeFileSync(journalPath, JSON.stringify(journal, null, 2) + '\n', 'utf8') or an atomic equivalent) so the journal reflects each applied action as it happens.
🧹 Nitpick comments (2)
tests/bug-3033-sdk-flag-wired.test.cjs (2)
161-162: ⚡ Quick winUse
assert.strictEqualfor consistency.The rest of the test file uses
assert.strictEqual(e.g., line 189-192), which is generally preferred as it enforces strict equality (===) rather than coercive equality (==).♻️ Proposed fix
- assert.equal(error.code, 'GSD_SDK_MISSING_DIST'); - assert.equal(error.exitCode, 1); + assert.strictEqual(error.code, 'GSD_SDK_MISSING_DIST'); + assert.strictEqual(error.exitCode, 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/bug-3033-sdk-flag-wired.test.cjs` around lines 161 - 162, Replace the two coercive equality assertions that use assert.equal for error.code and error.exitCode with strict equality assertions: change the calls referencing error.code and error.exitCode (the assert.equal lines in the test block) to use assert.strictEqual so the test enforces === semantics and matches the rest of the file.
160-164: 💤 Low valueConsider simplifying the validator function.
The current validator uses
assert.equalcalls that throw on failure, which can produce confusing error messages when the assertion fails inside the validator. A cleaner pattern would be to check conditions and return a boolean, or pass an error object/class directly toassert.throws.♻️ Alternative patterns
Option 1: Return boolean
(error) => { - assert.equal(error.code, 'GSD_SDK_MISSING_DIST'); - assert.equal(error.exitCode, 1); - return true; + return error.code === 'GSD_SDK_MISSING_DIST' && error.exitCode === 1; }Option 2: Assert outside validator
+ let thrownError; 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; - } + (error) => { thrownError = error; return true; } ); + assert.strictEqual(thrownError.code, 'GSD_SDK_MISSING_DIST'); + assert.strictEqual(thrownError.exitCode, 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/bug-3033-sdk-flag-wired.test.cjs` around lines 160 - 164, The inline validator arrow function currently uses assert.equal calls which throw inside the validator; replace it with a pure boolean check that returns true when error.code === 'GSD_SDK_MISSING_DIST' and error.exitCode === 1 (e.g., (error) => error.code === 'GSD_SDK_MISSING_DIST' && error.exitCode === 1) so the test framework receives a simple true/false result instead of catching assertion exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/installer-migrations.md`:
- Around line 102-126: The example install-state JSON in docs is incorrect:
update the example to match the actual shape written by the runner/installer (as
implemented in installer-migrations.cjs) by using the keys schemaVersion and
appliedMigrations (an array of entries containing appliedAt, journal and
checksum) instead of schema, runtime, scope, installed_version, install_mode,
and applied_migrations; ensure the example object and the prose describe the
checksum being computed from the migration definition and that checksum changes
trigger a warning/refusal, and remove or clearly separate any fields not
produced by installer-migrations.cjs so the docs match the code.
- Around line 148-160: The docs' plan(ctx) authoring contract advertises helper
predicates and actions that the runner doesn't currently provide; update the
section around the `plan(ctx)` description (and the repeated block at lines
182-201) to reflect the actual runtime surface: list only the properties the
runner passes (`configDir`, `manifest`, `state`, `baselineScan`, `now`,
`classifyArtifact`, `readJson`) and the supported action types, and either
remove references to helpers like `isManaged`, `isUserOwned`, `exists`,
`readToml` and actions like `move-managed`/`rewrite-config` or mark them
explicitly as "future work / not yet supported" so contributors don't author
incompatible migrations.
In `@get-shit-done/bin/lib/installer-migrations.cjs`:
- Around line 59-67: readInstallState currently treats any JSON (even with the
wrong shape) as absent; instead validate the structure returned by
readJsonIfPresent and reject it if it doesn't match expectations. In the
readInstallState function (which reads INSTALL_STATE_NAME via readJsonIfPresent
with STRICT_JSON), check that state is a plain object, that state.schemaVersion
is a finite number, and that state.appliedMigrations is an array (and optionally
that its items are the expected type); if the shape is invalid, throw an Error
(or otherwise fail the install) rather than returning a default/empty state so
migrations/checksums cannot be skipped.
- Around line 191-203: discoverInstallerMigrations currently returns validated
records but doesn't check for duplicate migration ids; update
discoverInstallerMigrations to collect all validateMigrationRecord outputs (from
function discoverInstallerMigrations) into a single array and then scan that
array for duplicate id values, throwing an error (including the duplicated id
and involved source filenames) if any duplicates are found so discovery fails
fast; reference validateMigrationRecord and the local variable/array produced by
the .flatMap/.map so you detect duplicates after validation and before returning
the records.
- Around line 688-699: The finally block for releaseLock() currently throws
releaseError (in the catch) which can override an earlier exception; refactor to
capture the operation result/return in a local variable instead of returning
early inside the try, ensure primaryError is set rather than throwing inside the
finally catch, and after the finally block re-throw primaryError if present;
specifically update the block around releaseLock(), primaryError, and completed
so the try assigns a result variable and any releaseError only augments
primaryError (e.g., push to primaryError.suppressed or set primaryError =
releaseError when none exists) and do not throw inside finally, then after
finally if primaryError is set throw it.
---
Duplicate comments:
In `@get-shit-done/bin/lib/installer-migrations.cjs`:
- Around line 497-559: The journal is only written once at the end
(fs.writeFileSync(journalPath...)), so if the process dies mid-loop applied
actions won’t be durable; after each committed action update (every place you
push into journal.actions inside the loop — e.g., where journalAction is added
for 'missing', 'recorded'/'preserved', 'removed', 'rewritten') immediately flush
the journal to disk by calling the same write used elsewhere
(fs.writeFileSync(journalPath, JSON.stringify(journal, null, 2) + '\n', 'utf8')
or an atomic equivalent) so the journal reflects each applied action as it
happens.
---
Nitpick comments:
In `@tests/bug-3033-sdk-flag-wired.test.cjs`:
- Around line 161-162: Replace the two coercive equality assertions that use
assert.equal for error.code and error.exitCode with strict equality assertions:
change the calls referencing error.code and error.exitCode (the assert.equal
lines in the test block) to use assert.strictEqual so the test enforces ===
semantics and matches the rest of the file.
- Around line 160-164: The inline validator arrow function currently uses
assert.equal calls which throw inside the validator; replace it with a pure
boolean check that returns true when error.code === 'GSD_SDK_MISSING_DIST' and
error.exitCode === 1 (e.g., (error) => error.code === 'GSD_SDK_MISSING_DIST' &&
error.exitCode === 1) so the test framework receives a simple true/false result
instead of catching assertion exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f833becc-8dfd-448c-8f2b-7c62961f420c
📒 Files selected for processing (7)
bin/install.jsdocs/installer-migrations.mdget-shit-done/bin/lib/installer-migrations.cjstests/bug-2771-user-profile-manifest.test.cjstests/bug-3033-sdk-flag-wired.test.cjstests/installer-migration-install-integration.test.cjstests/installer-migrations.test.cjs
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/bug-2771-user-profile-manifest.test.cjs
- tests/installer-migration-install-integration.test.cjs
- bin/install.js
- tests/installer-migrations.test.cjs
Comment on lines
+102
to
+126
| The installer writes an install-state file next to the manifest. | ||
|
|
||
| Required fields: | ||
|
|
||
| ```json | ||
| { | ||
| "schema": 1, | ||
| "runtime": "codex", | ||
| "scope": "global", | ||
| "installed_version": "1.50.0", | ||
| "install_mode": "full", | ||
| "applied_migrations": [ | ||
| { | ||
| "id": "2026-05-11-codex-hooks-layout", | ||
| "package_version": "1.50.0", | ||
| "checksum": "sha256:...", | ||
| "applied_at": "2026-05-11T00:00:00.000Z" | ||
| } | ||
| ] | ||
| } | ||
| ``` | ||
|
|
||
| The checksum is calculated from the migration definition. If an applied | ||
| migration's checksum changes, the installer must warn and refuse to silently | ||
| re-run it. Fix-forward migrations should use a new migration id. |
Contributor
There was a problem hiding this comment.
Align the install-state example with the schema the runner actually writes.
The implementation persists schemaVersion / appliedMigrations with appliedAt, journal, and checksum; it does not write schema, runtime, scope, installed_version, install_mode, or applied_migrations. As written, this section documents a different file shape than installer-migrations.cjs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/installer-migrations.md` around lines 102 - 126, The example
install-state JSON in docs is incorrect: update the example to match the actual
shape written by the runner/installer (as implemented in
installer-migrations.cjs) by using the keys schemaVersion and appliedMigrations
(an array of entries containing appliedAt, journal and checksum) instead of
schema, runtime, scope, installed_version, install_mode, and applied_migrations;
ensure the example object and the prose describe the checksum being computed
from the migration definition and that checksum changes trigger a
warning/refusal, and remove or clearly separate any fields not produced by
installer-migrations.cjs so the docs match the code.
Comment on lines
+148
to
+160
| The `plan(ctx)` function receives an install context with runtime, scope, | ||
| target directory, previous manifest, install state, package manifest, and | ||
| filesystem helpers. It returns actions. It must not mutate disk. | ||
|
|
||
| Migrations may use helper predicates such as: | ||
|
|
||
| - `isManaged(relPath)` | ||
| - `isUserOwned(relPath)` | ||
| - `hashMatchesManifest(relPath)` | ||
| - `exists(relPath)` | ||
| - `readJson(relPath)` | ||
| - `readToml(relPath)` | ||
|
|
Contributor
There was a problem hiding this comment.
Trim this authoring contract to the currently supported surface.
This section advertises helpers like isManaged, exists, readToml and actions like move-managed / rewrite-config, but the runner currently passes configDir, manifest, state, baselineScan, now, classifyArtifact, and readJson, and the executor rejects unsupported action types. Please either mark the extra helpers/actions as future work or document the current surface so contributors do not author migrations that cannot run.
Also applies to: 182-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/installer-migrations.md` around lines 148 - 160, The docs' plan(ctx)
authoring contract advertises helper predicates and actions that the runner
doesn't currently provide; update the section around the `plan(ctx)` description
(and the repeated block at lines 182-201) to reflect the actual runtime surface:
list only the properties the runner passes (`configDir`, `manifest`, `state`,
`baselineScan`, `now`, `classifyArtifact`, `readJson`) and the supported action
types, and either remove references to helpers like `isManaged`, `isUserOwned`,
`exists`, `readToml` and actions like `move-managed`/`rewrite-config` or mark
them explicitly as "future work / not yet supported" so contributors don't
author incompatible migrations.
Comment on lines
+59
to
+67
| function readInstallState(configDir) { | ||
| const state = readJsonIfPresent(path.join(configDir, INSTALL_STATE_NAME), STRICT_JSON); | ||
| if (!state || typeof state !== 'object') { | ||
| return { schemaVersion: 1, appliedMigrations: [] }; | ||
| } | ||
| return { | ||
| schemaVersion: state.schemaVersion || 1, | ||
| appliedMigrations: Array.isArray(state.appliedMigrations) ? state.appliedMigrations : [], | ||
| }; |
Contributor
There was a problem hiding this comment.
Reject structurally invalid install-state, not just invalid JSON.
STRICT_JSON only catches parse failures. Valid JSON with the wrong shape currently falls back to an empty state, which can re-run old migrations and skip checksum-drift enforcement instead of failing the install as Phase 4 intends.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@get-shit-done/bin/lib/installer-migrations.cjs` around lines 59 - 67,
readInstallState currently treats any JSON (even with the wrong shape) as
absent; instead validate the structure returned by readJsonIfPresent and reject
it if it doesn't match expectations. In the readInstallState function (which
reads INSTALL_STATE_NAME via readJsonIfPresent with STRICT_JSON), check that
state is a plain object, that state.schemaVersion is a finite number, and that
state.appliedMigrations is an array (and optionally that its items are the
expected type); if the shape is invalid, throw an Error (or otherwise fail the
install) rather than returning a default/empty state so migrations/checksums
cannot be skipped.
Comment on lines
+191
to
+203
| function discoverInstallerMigrations({ migrationsDir }) { | ||
| if (!migrationsDir || !fs.existsSync(migrationsDir)) return []; | ||
| return fs.readdirSync(migrationsDir, { withFileTypes: true }) | ||
| .filter((entry) => entry.isFile() && entry.name.endsWith('.cjs')) | ||
| .map((entry) => entry.name) | ||
| .sort() | ||
| .flatMap((fileName) => { | ||
| const source = path.join(migrationsDir, fileName); | ||
| delete require.cache[require.resolve(source)]; | ||
| const exported = require(source); | ||
| const records = Array.isArray(exported) ? exported : [exported]; | ||
| return records.map((record) => validateMigrationRecord(record, source)); | ||
| }); |
Contributor
There was a problem hiding this comment.
Fail fast on duplicate migration IDs.
Discovery validates each record individually, but it never rejects two records with the same id. That lets both plans execute on first run while only one applied-state entry is persisted, which makes follow-up checksum/state handling ambiguous.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@get-shit-done/bin/lib/installer-migrations.cjs` around lines 191 - 203,
discoverInstallerMigrations currently returns validated records but doesn't
check for duplicate migration ids; update discoverInstallerMigrations to collect
all validateMigrationRecord outputs (from function discoverInstallerMigrations)
into a single array and then scan that array for duplicate id values, throwing
an error (including the duplicated id and involved source filenames) if any
duplicates are found so discovery fails fast; reference validateMigrationRecord
and the local variable/array produced by the .flatMap/.map so you detect
duplicates after validation and before returning the records.
Comment on lines
+688
to
+699
| } finally { | ||
| try { | ||
| releaseLock(); | ||
| } catch (releaseError) { | ||
| if (primaryError) { | ||
| primaryError.suppressed = [...(primaryError.suppressed || []), releaseError]; | ||
| } else if (completed) { | ||
| throw releaseError; | ||
| } else { | ||
| throw releaseError; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd get-shit-done && find . -name "installer-migrations.cjs" -type fRepository: gsd-build/get-shit-done
Length of output: 102
🏁 Script executed:
cat -n ./bin/lib/installer-migrations.cjs | sed -n '680,710p'Repository: gsd-build/get-shit-done
Length of output: 134
🏁 Script executed:
find . -name "installer-migrations.cjs" -type f 2>/dev/nullRepository: gsd-build/get-shit-done
Length of output: 116
🏁 Script executed:
cat -n ./get-shit-done/bin/lib/installer-migrations.cjs | sed -n '680,710p'Repository: gsd-build/get-shit-done
Length of output: 995
🏁 Script executed:
cat -n ./get-shit-done/bin/lib/installer-migrations.cjs | sed -n '650,710p'Repository: gsd-build/get-shit-done
Length of output: 2166
🏁 Script executed:
cat -n ./get-shit-done/bin/lib/installer-migrations.cjs | sed -n '645,660p'Repository: gsd-build/get-shit-done
Length of output: 631
Move lock-release error handling out of finally block.
The current code has a control flow issue: if releaseLock() throws an error while handling an already-caught exception or during a normal return, the exception from finally (lines 695 and 697) overrides the original error. This is the anti-pattern Biome flags.
The fix is to avoid throwing from finally. Instead:
- Remove early
returnstatements from thetryblock and assign results to a variable instead - In the
finallyblock'scatch, don't throw—only setprimaryErrorwhen appropriate - After
finallycompletes, throwprimaryErrorif it exists
Suggested control-flow shape
- 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,
- journalRelPath: null,
- plan,
- };
- }
- if (plan.blocked.length > 0) {
- completed = true;
- return {
- appliedMigrationIds: [],
- journalRelPath: null,
- plan,
- blocked: plan.blocked,
- };
- }
- const result = applyInstallerMigrationPlan({ configDir, plan, now });
- completed = true;
- return { ...result, plan };
- } catch (error) {
- primaryError = error;
- throw error;
- } finally {
- try {
- releaseLock();
- } catch (releaseError) {
- if (primaryError) {
- primaryError.suppressed = [...(primaryError.suppressed || []), releaseError];
- } else if (completed) {
- throw releaseError;
- } else {
- throw releaseError;
- }
- }
- }
+ let result;
+ try {
+ const plan = planInstallerMigrations({ configDir, runtime, scope, migrations, baselineScan, now });
+ if (plan.actions.length === 0) {
+ const appliedMigrationIds = markPendingMigrationsApplied({ configDir, plan, now });
+ completed = true;
+ result = {
+ appliedMigrationIds,
+ journalRelPath: null,
+ plan,
+ };
+ } else if (plan.blocked.length > 0) {
+ completed = true;
+ result = {
+ appliedMigrationIds: [],
+ journalRelPath: null,
+ plan,
+ blocked: plan.blocked,
+ };
+ } else {
+ result = { ...applyInstallerMigrationPlan({ configDir, plan, now }), plan };
+ completed = true;
+ }
+ } catch (error) {
+ primaryError = error;
+ throw error;
+ } finally {
+ try {
+ releaseLock();
+ } catch (releaseError) {
+ if (primaryError) {
+ primaryError.suppressed = [...(primaryError.suppressed || []), releaseError];
+ } else {
+ primaryError = releaseError;
+ }
+ }
+ }
+ if (primaryError) throw primaryError;
+ return result;🧰 Tools
🪛 Biome (2.4.15)
[error] 695-695: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
[error] 697-697: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@get-shit-done/bin/lib/installer-migrations.cjs` around lines 688 - 699, The
finally block for releaseLock() currently throws releaseError (in the catch)
which can override an earlier exception; refactor to capture the operation
result/return in a local variable instead of returning early inside the try,
ensure primaryError is set rather than throwing inside the finally catch, and
after the finally block re-throw primaryError if present; specifically update
the block around releaseLock(), primaryError, and completed so the try assigns a
result variable and any releaseError only augments primaryError (e.g., push to
primaryError.suppressed or set primaryError = releaseError when none exists) and
do not throw inside finally, then after finally if primaryError is set throw it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature PR
Linked Issue
Closes #3395
Linked issue labels: approved-feature, in-progress
Approved Issue Description
What to build
Build a first-class Installer Migration Module that replaces ad hoc install cleanup with a planned, journaled, reviewable migration system.
This should future-proof GSD updates when features move, rename, replace, or retire installed files. The migration tool must clean up stale GSD-owned artifacts, preserve user-owned data, back up locally modified managed files, support dry-run planning, and avoid leaving hybrid installs after failures.
Design references:
docs/installer-migrations.mddocs/adr/0008-installer-migration-module.mdPhases
Phase 1: Foundational migration runner
Create the core migration infrastructure in one coherent pass:
gsd-file-manifest.jsonPhase 2: Port existing cleanup behavior
Move one or more existing installer cleanup paths into the migration runner:
Phase 3: First-time baseline scanner
Add a baseline migration for existing installs that predate migration tracking:
Phase 4: Install/update integration
Wire the migration runner into normal install/update flow:
Phase 5: Authoring guardrails
Add contributor-facing guardrails:
Acceptance criteria
Blocked by
None - can start immediately
Standards Followed
CONTEXT.md: contributor gate order, changeset PR-number rule, and domain vocabulary requirements.CONTRIBUTING.md: issue-first process, typed PR template, closing keyword, one-concern scope, changeset fragment, and test requirements.docs/contributor-standards.md: CONTEXT/ADR hierarchy, isolated worktree expectation, AI-assisted PR body requirements, and architecture-aware testing guidance.docs/adr/0008-installer-migration-module.md: Installer Migration Module decision, runtime contract decision, and migration authoring contract.docs/installer-migrations.md: migration architecture, runtime configuration contract registry, ownership boundaries, source snapshots, and Phase 4 requirements.Feature Summary
This PR implements Phase 4: Install/update integration of the approved first-class Installer Migration Module feature.
What Changed
Spec Compliance For This Phase
Testing
TMPDIR=/private/tmp node --test tests/installer-migrations.test.cjs tests/installer-migration-install-integration.test.cjs tests/installer-migration-report.test.cjs tests/bug-3357-codex-legacy-hooks-json-migration.test.cjspassed with 69 tests and 0 failures.TMPDIR=/private/tmp npm run lint:testspassed.TMPDIR=/private/tmp npm run lint:descriptionspassed.TMPDIR=/private/tmp npm run lint:changesetpassed.git diff --checkpassed.TMPDIR=/private/tmp npm testpassed with 8,795 tests and 0 failures.Platforms Tested
Runtimes Tested
Scope Confirmation
Checklist
Closes #3395.approved-featurelabel.Feat(installer):..changeset/fragment added:.changeset/vivid-foxes-romp.md.Breaking Changes
None.
Phase 4: Install/update integration
This final section is intentionally appended after the approved issue description so reviewers can compare the whole approved feature request above against the concrete phase delivered by this PR.
Summary by CodeRabbit
New Features
Bug Fixes
throwOnFailureis enabled.Tests