fix: make update promotion crash-safe and trim speculative machinery by aorumbayev · Pull Request #21 · kagan-sh/kagan

aorumbayev · 2026-07-10T08:48:58Z

Summary

F1 crash recovery: cleanupPreparedUpdate restores backup → current when a marker exists but current is missing after an interrupted promotion; stale marker and prepared dirs are removed afterward.
F2/F3 self-healing: orphan backups, stale markers, and leaked prepared dirs no longer wedge prepareUpdate; cleanup failures surface { kind: "broken" } in the board footer instead of silent swallowing.
F4/F5 trim: removed checkForUpdate WeakMap dedup (~43 test lines) and symlink/hardlink/nlink/realpath-escape checks from update-paths.ts; kept layout basename checks and validateWrapper manifest verification.
F6/F7: extracted runAutomaticUpdateLaunch with per-step failure isolation and direct tests; pinned host plugins.add id-dedupe via test/guards/host-update.test.ts.

Deleted update-paths checks (threat-model reasoning)

Removed	Reason
`isSymbolicLink()` on cache dirs/marker	Attacker with write access to the user's plugin cache can edit the plugin code the host executes directly; Node `rename`/`rm` do not follow symlinks at the destination.
`nlink === 1` on marker	Hardlink paranoia against the same writer; also caused permanent update disable (F2).
`realpath` escape check	Same trust boundary; layout basename checks already pin expected cache structure.

Kept: updatePaths layout/basename validation and validateWrapper directory + package.json name/version verification.

Crash recovery test — fails before fix

With restoreCurrentFromBackup disabled, the test throws on missing current:

ENOENT: no such file or directory, lstat '.../kagan@latest'
    at async validateDirectory (src/tui/update-paths.ts:65:25)
    at async removeOrphanBackup (src/tui/update-cleanup.ts:35:9)
    at async cleanupPreparedUpdate (src/tui/update-cleanup.ts:70:9)
(fail) cleanupPreparedUpdate > restores current from backup after interrupted promotion

With fix enabled:

(pass) cleanupPreparedUpdate > restores current from backup after interrupted promotion [3.71ms]

Acceptance verification

$ rg -n "WeakMap" src/tui/updates.ts; echo $?
# (no output, exit 1)

$ bun test test/tui/update-manager.test.ts -t "restores current from backup"
1 pass, 0 fail

$ bun run test
606 pass, 0 fail, 1230 expect() calls

$ bun run verify
verifyx all — pass (format, lint, types, tests, package, knip, skott, jscpd, complexity)

Spec/doc edits

R18.7: local cache cleanup/preparation failures now show persistent footer status.
R18.8: in-process promotion restore unchanged; cross-launch restore via cleanup on next launch.
design.md: automatic-update section matches split cleanup/launch flow and removed symlink checks.
docs/quickstart.md / docs/troubleshooting.md: distinguish registry silence from local updates unavailable footer.

Test plan

Kill-mid-promotion: marker + missing current + valid backup → cleanup restores current, clears marker/backup/prepared
Stale-state variants (orphan backup, stale marker, leaked prepared dir) → cleanup then prepareUpdate succeeds
Launch chain ordering and per-step failure isolation (test/tui/update-launch.test.ts)
Host dedupe guard against vendored runtime.ts
Full gate: bun run verify

Made with Cursor

Restore interrupted promotions on launch, self-heal stale cache state, surface local update failures in the footer, and remove unused concurrency/path checks. Co-authored-by: Cursor <cursoragent@cursor.com>

greptile-apps · 2026-07-10T08:54:29Z

Greptile Summary

This PR changes the automatic update cleanup and launch path. The main changes are:

Crash recovery for interrupted update promotion.
A new runAutomaticUpdateLaunch helper for cleanup, check, prepare, status, and toast flow.
A board footer state for local update cleanup or preparation failures.
Simpler update path validation and removal of in-flight update-check deduplication.
Tests and docs for cleanup recovery, launch ordering, and host plugin id dedupe.

Confidence Score: 4/5

The changed update cleanup and launch paths need fixes before merging.

Failed preparation can still show a ready-to-restart update; stale marker cleanup can remove a path that was never validated as the prepared wrapper; malformed markers can prevent crash recovery after current has already moved to backup.

src/tui/update-launch.ts and src/tui/update-cleanup.ts

T-Rex Logs

What T-Rex did

Reproduced Ready Status Without Preparation: a focused Bun test against the real update-launch implementation showed prepareUpdate returned false during preparation, but setUpdateStatus reported kind=ready version=0.2.0 and a ready toast was shown.
Verified Stale Marker Deletion: a Bun repro created a stale marker pointing to a sentinel directory outside the prepared wrapper; cleanupPreparedUpdate removed the sentinel and the directory state changed from existing to non-existent, with marker.prepared deleted.
Verified Corrupt Marker Recovery: a Bun repro created a missing current wrapper, a valid backup wrapper, and a malformed update marker; cleanupPreparedUpdate deleted the malformed marker but did not restore the missing current path and left the backup in place, emitting ENOENT during validation.
Captured crash-recovery and stale-state coverage: two logs capture the crash-recovery proof and full update-manager stale-state coverage, and both commands exited with status 0.

_{Ran code and verified through T-Rex}

Important Files Changed

Filename	Overview
src/tui/update-launch.ts	Extracts launch orchestration, but the ready status is still emitted when preparation fails.
src/tui/update-cleanup.ts	Adds crash recovery and stale-state cleanup, but malformed or stale markers can break recovery or widen deletion scope.
src/tui/update-manager.ts	Moves cleanup out of the manager and keeps promotion rollback behavior.
src/tui/update-paths.ts	Simplifies validation by removing realpath, symlink, hardlink, and parent-cache checks while keeping layout and manifest validation.
src/tui/updates.ts	Adds the broken update status and removes speculative in-flight check deduplication.
src/tui/board/board.tsx	Displays the new broken update state as an updates-unavailable footer message.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Plugin launch] --> B[cleanupPreparedUpdate]
  B -->|cleanup ok| C[checkForUpdate]
  B -->|cleanup failed| D[remember cleanup failure]
  D --> C
  C -->|no status or aborted| E[return]
  C -->|ready| F[prepareUpdate]
  F --> G[set footer status]
  C -->|blocked| G
  D -->|after check| H[set broken footer]
  G --> I[optional toast]

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
  A[Plugin launch] --> B[cleanupPreparedUpdate]
  B -->|cleanup ok| C[checkForUpdate]
  B -->|cleanup failed| D[remember cleanup failure]
  D --> C
  C -->|no status or aborted| E[return]
  C -->|ready| F[prepareUpdate]
  F --> G[set footer status]
  C -->|blocked| G
  D -->|after check| H[set broken footer]
  G --> I[optional toast]

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/tui/update-launch.ts:58-62
**Ready Status Without Preparation**

When `prepareUpdate` returns `false` after a host install, validation, marker write, or abort failure, this path still stores and toasts the `ready` status. No promotion callback was registered, so the footer tells the user to restart for an update that cannot be applied.

```suggestion
  if (status.kind === "ready" && !(await prepareUpdate({ api, meta, currentVersion, status, fs }))) return

  setUpdateStatus(status)
```

### Issue 2 of 3
src/tui/update-cleanup.ts:40
**Stale Marker Controls Deletion**

When a marker parses but does not match the current layout, this branch recursively removes `marker.prepared` before validating that it is Kagan’s expected prepared wrapper. A stale marker from another target or a malformed marker can point outside the intended cache state, causing startup cleanup to delete the wrong directory.

### Issue 3 of 3
src/tui/update-cleanup.ts:18-21
**Corrupt Marker Blocks Recovery**

If the process exits after `current` was renamed to `backup` and the marker is left malformed, this catch deletes the only recovery marker and returns `undefined`. The interrupted-promotion branch never runs, `current` stays missing, and each launch can remain stuck in `updates unavailable` instead of restoring from the validated backup.

_{Reviews (1): Last reviewed commit: "fix: make update promotion crash-safe an..." | Re-trigger Greptile}

greptile-apps · 2026-07-10T08:54:32Z

+  if (status.kind === "ready") {
+    await prepareUpdate({ api, meta, currentVersion, status, fs })
+  }
+
+  setUpdateStatus(status)


Ready Status Without Preparation

When prepareUpdate returns false after a host install, validation, marker write, or abort failure, this path still stores and toasts the ready status. No promotion callback was registered, so the footer tells the user to restart for an update that cannot be applied.

Suggested change

if (status.kind === "ready") {

await prepareUpdate({ api, meta, currentVersion, status, fs })

}

setUpdateStatus(status)

if (status.kind === "ready" && !(await prepareUpdate({ api, meta, currentVersion, status, fs }))) return

setUpdateStatus(status)

Context Used: AGENTS.md (source)

Artifacts

Repro: focused Bun test harness for ready status after failed preparation

Contains supporting evidence from the run (text/typescript; charset=utf-8).

Repro: Bun test output showing failed preparation followed by ready status and toast

Keeps the command output available without making the summary code-heavy.

_{Ran code and verified through T-Rex}

Prompt To Fix With AI

This is a comment left during a code review. Path: src/tui/update-launch.ts Line: 58-62 Comment: **Ready Status Without Preparation** When `prepareUpdate` returns `false` after a host install, validation, marker write, or abort failure, this path still stores and toasts the `ready` status. No promotion callback was registered, so the footer tells the user to restart for an update that cannot be applied. ```suggestion if (status.kind === "ready" && !(await prepareUpdate({ api, meta, currentVersion, status, fs }))) return setUpdateStatus(status) ``` **Context Used:** AGENTS.md ([source](https://app.greptile.com/kagan/github/kagan-sh/kagan/-/custom-context?memory=88a14340-9a15-4297-925d-3656d144ad2a)) How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-07-10T08:54:33Z

+}
+
+async function removeStaleMarker(fs: FileSystem, markerPath: string, marker: UpdateMarker) {
+  if (marker.prepared) await fs.rm(marker.prepared, { recursive: true, force: true }).catch(() => {})


Stale Marker Controls Deletion

When a marker parses but does not match the current layout, this branch recursively removes marker.prepared before validating that it is Kagan’s expected prepared wrapper. A stale marker from another target or a malformed marker can point outside the intended cache state, causing startup cleanup to delete the wrong directory.

Context Used: AGENTS.md (source)

Artifacts

Repro: focused Bun script creating a stale marker that points prepared at an outside sentinel directory

Contains supporting evidence from the run (text/typescript; charset=utf-8).

Repro: execution log showing before and after filesystem evidence for sentinel deletion

Keeps the command output available without making the summary code-heavy.

_{Ran code and verified through T-Rex}

Prompt To Fix With AI

This is a comment left during a code review. Path: src/tui/update-cleanup.ts Line: 40 Comment: **Stale Marker Controls Deletion** When a marker parses but does not match the current layout, this branch recursively removes `marker.prepared` before validating that it is Kagan’s expected prepared wrapper. A stale marker from another target or a malformed marker can point outside the intended cache state, causing startup cleanup to delete the wrong directory. **Context Used:** AGENTS.md ([source](https://app.greptile.com/kagan/github/kagan-sh/kagan/-/custom-context?memory=88a14340-9a15-4297-925d-3656d144ad2a)) How can I resolve this? If you propose a fix, please make it concise.

greptile-apps · 2026-07-10T08:54:34Z

+  try {
+    return JSON.parse(await fs.readFile(markerPath, "utf8")) as UpdateMarker
+  } catch {
+    await fs.rm(markerPath, { force: true })


Corrupt Marker Blocks Recovery

If the process exits after current was renamed to backup and the marker is left malformed, this catch deletes the only recovery marker and returns undefined. The interrupted-promotion branch never runs, current stays missing, and each launch can remain stuck in updates unavailable instead of restoring from the validated backup.

Context Used: AGENTS.md (source)

Artifacts

Repro: focused Bun harness for corrupt marker crash-recovery state

Contains supporting evidence from the run (text/typescript; charset=utf-8).

Repro: command output showing marker deletion with current still missing and backup still present

Keeps the command output available without making the summary code-heavy.

_{Ran code and verified through T-Rex}

Prompt To Fix With AI

This is a comment left during a code review. Path: src/tui/update-cleanup.ts Line: 18-21 Comment: **Corrupt Marker Blocks Recovery** If the process exits after `current` was renamed to `backup` and the marker is left malformed, this catch deletes the only recovery marker and returns `undefined`. The interrupted-promotion branch never runs, `current` stays missing, and each launch can remain stuck in `updates unavailable` instead of restoring from the validated backup. **Context Used:** AGENTS.md ([source](https://app.greptile.com/kagan/github/kagan-sh/kagan/-/custom-context?memory=88a14340-9a15-4297-925d-3656d144ad2a)) How can I resolve this? If you propose a fix, please make it concise.

Finding 1: removeStaleMarker only deletes marker.prepared when it is a kagan@<x.y.z> sibling of the current wrapper in the same scope cache; a corrupted marker naming any other path now leaves that path alone. Finding 2: removed dead restoreCurrentFromBackup / interruptedPromotion branch (the host re-downloads latest before the plugin loads, so it can never run), aligned R18.8 and design.md to the real contract, and closed the prepared-dir leak in the matched-marker cleanup path. Finding 3: prepare failure on a ready check now sets broken status and suppresses the ready toast instead of promising a restart that applies nothing; a failed cleanup now returns broken before the network check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

aorumbayev · 2026-07-10T09:04:04Z

Addressed the three review findings.

Finding 1 (HIGH) — unvalidated recursive delete. removeStaleMarker now deletes marker.prepared only when it is contained in the expected location: dirname(marker.prepared) strictly equals the scope-cache dir (dirname(paths.current)) AND basename is kagan@<x.y.z> (validated with parseRelease). Otherwise only the marker file is removed and the named path is left alone.
Pinned by test/tui/update-manager.test.ts → "leaves an out-of-cache prepared path untouched when clearing a stale marker": a stale marker whose prepared points to a sibling temp dir with a sentinel → cleanup runs, sentinel survives, marker gone.
Fails-without-guard demo (containment check reverted locally):

(fail) cleanupPreparedUpdate > leaves an out-of-cache prepared path untouched when clearing a stale marker
219 | expect(await readFile(sentinel, "utf8")).toBe("keep")
ENOENT: no such file or directory, open '.../kagan-update-Xb2fOm/outside/keep.txt'

Finding 2 (MED) — dead restore path, dishonest spec, prepared-dir leak.

Removed restoreCurrentFromBackup and the interruptedPromotion branch entirely. Verified against the vendored host (references/opencode/packages/core/src/npm.ts): add re-reifies latest when node_modules/@kagan-sh/kagan is missing, so online the host re-creates current before the plugin loads and offline the plugin never loads — the restore was unreachable by construction. Removed its test.
Rewrote R18.8 in requirements.md and the matching passage in design.md: an interrupted promotion recovers via the host re-downloading latest on next launch (requires network); cleanup then removes the leftover backup, marker, and prepared directory. docs/troubleshooting.md and docs/quickstart.md made no restore claim, so no change.
Closed the leak: the matched-marker cleanup block now also deletes paths.prepared (self-computed, no containment check needed). Pinned by "removes the leftover prepared directory when clearing a matched marker's backup": matched marker + prepared + backup → all three gone, current untouched.

Finding 3 (MED) — prepare failure showed a false promise. runAutomaticUpdateLaunch now uses the prepareUpdate result: on failure it sets { kind: "broken" } (footer renders "· updates unavailable", toast suppressed) and skips the ready toast. Also reordered so a failed cleanup sets broken and returns before the network check. quickstart.md already stated "the ready message appears only after preparation succeeds" — now the code honors it.
Pinned in test/tui/update-launch.test.ts: prepare-fails → status broken, no toast; cleanup-fails → broken, no version check; prepare-succeeds → ready + toast (the success test's api mock was missing onDispose, which had been masking a silent prepare failure — added it so prepare genuinely succeeds).

bun run verify green; bun run test green.

references/opencode is git-ignored, so CI checkouts never have it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* test: make git hermeticity structural via preload * test: rename hermeticity wording to git isolation * test: wait for the merge-conflict notice instead of asserting notice order * test: allow slow CI git spawns to deliver the merge-conflict notice * fix: make update promotion crash-safe and trim speculative machinery (#21) * fix: make update promotion crash-safe and trim speculative machinery Restore interrupted promotions on launch, self-heal stale cache state, surface local update failures in the footer, and remove unused concurrency/path checks. * fix: harden update cleanup against unvalidated deletes and false ready Finding 1: removeStaleMarker only deletes marker.prepared when it is a kagan@<x.y.z> sibling of the current wrapper in the same scope cache; a corrupted marker naming any other path now leaves that path alone. Finding 2: removed dead restoreCurrentFromBackup / interruptedPromotion branch (the host re-downloads latest before the plugin loads, so it can never run), aligned R18.8 and design.md to the real contract, and closed the prepared-dir leak in the matched-marker cleanup path. Finding 3: prepare failure on a ready check now sets broken status and suppresses the ready toast instead of promising a restart that applies nothing; a failed cleanup now returns broken before the network check. * test: skip the host-dedupe pin when the vendored source is absent references/opencode is git-ignored, so CI checkouts never have it. ---------

* fix: enforce all verifyx checks and check-only pre-commit Install knip, skott, and jscpd so verifyx all runs every built-in gate, dedupe status resolution and break git/domain layering cycles flagged by the new checks, and make the pre-commit hook check-only so staged content is never rewritten. * test: make git isolation structural via test preload (#20) * test: make git hermeticity structural via preload * test: rename hermeticity wording to git isolation * test: wait for the merge-conflict notice instead of asserting notice order * test: allow slow CI git spawns to deliver the merge-conflict notice * fix: make update promotion crash-safe and trim speculative machinery (#21) * fix: make update promotion crash-safe and trim speculative machinery Restore interrupted promotions on launch, self-heal stale cache state, surface local update failures in the footer, and remove unused concurrency/path checks. * fix: harden update cleanup against unvalidated deletes and false ready Finding 1: removeStaleMarker only deletes marker.prepared when it is a kagan@<x.y.z> sibling of the current wrapper in the same scope cache; a corrupted marker naming any other path now leaves that path alone. Finding 2: removed dead restoreCurrentFromBackup / interruptedPromotion branch (the host re-downloads latest before the plugin loads, so it can never run), aligned R18.8 and design.md to the real contract, and closed the prepared-dir leak in the matched-marker cleanup path. Finding 3: prepare failure on a ready check now sets broken status and suppresses the ready toast instead of promising a restart that applies nothing; a failed cleanup now returns broken before the network check. * test: skip the host-dedupe pin when the vendored source is absent references/opencode is git-ignored, so CI checkouts never have it. --------- * refactor: simplify update marker and cleanup machinery * refactor: merge single-importer modules * refactor: extract duplicated TUI blocks * chore: drop redundant verify overrides * ci: run check workflow on pull requests only * refactor: inline single-use list-editor row wrapper renderListEditorRows only forwarded its five arguments to <ListEditorRows> and had a single caller; render it directly and drop the now-unused JSX import. * test: make merge-dialog command tests deterministic across file order The two "approving …" tests in test/tui/board/commands.test.ts flaked only on CI. Root cause: bun's mock.module is process-global and persistent, and test/tui/session/tasks.test.ts mocks src/git/runner (currentBranch -> "kagan/x") and src/git/merge (mergeTaskBranch -> ok:true). ESM imports hoist above mock.module, so whichever test file loads first wins; on the Linux runner tasks.test.ts loaded first, so the merge-dialog tests saw the leaked mocks (wrong branch, no conflict) and failed. macOS load order hid it locally. - commands.test.ts now declares its own git/runner + git/merge mocks and drives the three merge-dialog tests through reset-per-test vars (currentBranchValue, localBranches, mergeResult), so its values win for its own tests regardless of which file ran first. - tasks.test.ts's git/runner mock was incomplete (missing listLocalBranches and baseBranchFreshness); stub them so binding those exports elsewhere while the mock is active no longer throws "Export not found". - The merge dialog's onSelect returns its handler promise so tests await the real work instead of racing a render/timer. Reproduced the exact leak order locally (bun test tasks.test.ts commands.test.ts) and via a Linux-container full-suite run; green there, commands-only, and reverse. * fix: surface both promotion and restore errors on update rollback promotePreparedUpdate runs in api.lifecycle.onDispose, whose thrown errors the host logs via console.error rather than discarding. Swallowing a failed restore therefore hid it: a lost kagan@latest wrapper logged only as a promotion failure. Rethrow an AggregateError carrying both, and add a test covering the double-failure path (the single-failure restore was already tested).

fix: make update promotion crash-safe and trim speculative machinery

4fa884a

Restore interrupted promotions on launch, self-heal stale cache state, surface local update failures in the footer, and remove unused concurrency/path checks. Co-authored-by: Cursor <cursoragent@cursor.com>

greptile-apps Bot reviewed Jul 10, 2026

View reviewed changes

aorumbayev and others added 3 commits July 10, 2026 11:12

merge: pick up merge-notice race fix from base branch

9335921

test: skip the host-dedupe pin when the vendored source is absent

55ffd7e

references/opencode is git-ignored, so CI checkouts never have it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

merge: pick up CI merge-notice timeout fix

2ac586d

aorumbayev merged commit 4d9bdad into fix/test-hermeticity-structural Jul 10, 2026
2 of 3 checks passed

aorumbayev deleted the fix/update-manager-crash-safety branch July 10, 2026 10:09

Navigation Menu

Uh oh!

fix: make update promotion crash-safe and trim speculative machinery - #21

fix: make update promotion crash-safe and trim speculative machinery#21
aorumbayev merged 5 commits into
fix/test-hermeticity-structuralfrom
fix/update-manager-crash-safety

aorumbayev commented Jul 10, 2026

Uh oh!

greptile-apps Bot commented Jul 10, 2026

T-Rex Logs

Important Files Changed

Uh oh!

greptile-apps Bot Jul 10, 2026

Uh oh!

greptile-apps Bot Jul 10, 2026

Uh oh!

greptile-apps Bot Jul 10, 2026

Uh oh!

aorumbayev commented Jul 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

aorumbayev commented Jul 10, 2026

Summary

Deleted update-paths checks (threat-model reasoning)

Crash recovery test — fails before fix

Acceptance verification

Spec/doc edits

Test plan

Uh oh!

greptile-apps Bot commented Jul 10, 2026

Greptile Summary

Confidence Score: 4/5

T-Rex Logs

Important Files Changed

Flowchart

Uh oh!

greptile-apps Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

aorumbayev commented Jul 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant