refactor: simplify verifyx gate - #19
Conversation
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. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Greptile SummaryThis PR simplifies the verify and automatic-update paths. The main changes are:
Confidence Score: 4/5The automatic update rollback path can still leave The changed restore branch hides the failure that matters for recovering the active wrapper. Startup cleanup can show updates as unavailable, but it does not recreate the missing wrapper from backup.
What T-Rex did
|
| Filename | Overview |
|---|---|
| src/tui/update-manager.ts | The promotion rollback path still drops a failed restore after moving the active wrapper to backup. |
| src/tui/update-cleanup.ts | The new cleanup helper removes stale update state and reports broken cleanup through the launch flow. |
| src/tui/update-launch.ts | The new launch helper runs cleanup before checking and preparing automatic updates. |
| src/tui/update-paths.ts | The path helper now owns wrapper target derivation and the simplified update marker shape. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/tui/update-manager.ts:28-29
**Restore Failure Is Dropped**
When `current` has already moved to `backup`, a failed `prepared -> current` rename depends on this restore to recreate `kagan@latest`. If the restore rename also fails, this branch drops that failure and rethrows only the promotion error, leaving the active wrapper missing while startup cleanup can only mark updates unavailable instead of recovering the loadable wrapper.
Reviews (4): Last reviewed commit: "test: make merge-dialog command tests de..." | Re-trigger Greptile
* 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. ---------
renderListEditorRows only forwarded its five arguments to <ListEditorRows> and had a single caller; render it directly and drop the now-unused JSX import.
| } catch (error) { | ||
| try { | ||
| await fs.rename(paths.backup, paths.current) | ||
| } catch (restoreError) { | ||
| throw new AggregateError([error, restoreError], "Kagan update promotion and restore failed") | ||
| } | ||
| await fs.rename(paths.backup, paths.current).catch(() => {}) | ||
| throw error | ||
| } |
There was a problem hiding this comment.
Restore Failure Gets Hidden
When promotion fails after current has already moved to backup, this restore is the only path that recreates the active kagan@latest wrapper. Swallowing a failed restore reports only the promotion error and can leave the next OpenCode launch without a loadable Kagan wrapper.
| } catch (error) { | |
| try { | |
| await fs.rename(paths.backup, paths.current) | |
| } catch (restoreError) { | |
| throw new AggregateError([error, restoreError], "Kagan update promotion and restore failed") | |
| } | |
| await fs.rename(paths.backup, paths.current).catch(() => {}) | |
| throw error | |
| } | |
| } catch (error) { | |
| try { | |
| await fs.rename(paths.backup, paths.current) | |
| } catch (restoreError) { | |
| throw new AggregateError([error, restoreError], "Kagan update promotion and restore failed") | |
| } | |
| throw error | |
| } |
Artifacts
Repro: focused Bun test harness for hidden rollback failure
- Contains supporting evidence from the run (text/typescript; charset=utf-8).
Repro: Bun test output showing hidden restore failure and missing current wrapper
- 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-manager.ts
Line: 27-30
Comment:
**Restore Failure Gets Hidden**
When promotion fails after `current` has already moved to `backup`, this restore is the only path that recreates the active `kagan@latest` wrapper. Swallowing a failed restore reports only the promotion error and can leave the next OpenCode launch without a loadable Kagan wrapper.
```suggestion
} catch (error) {
try {
await fs.rename(paths.backup, paths.current)
} catch (restoreError) {
throw new AggregateError([error, restoreError], "Kagan update promotion and restore failed")
}
throw error
}
```
How can I resolve this? If you propose a fix, please make it concise.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.
bba1419 to
0b9b330
Compare
| await fs.rename(paths.backup, paths.current).catch(() => {}) | ||
| throw error |
There was a problem hiding this comment.
Restore Failure Is Dropped
When current has already moved to backup, a failed prepared -> current rename depends on this restore to recreate kagan@latest. If the restore rename also fails, this branch drops that failure and rethrows only the promotion error, leaving the active wrapper missing while startup cleanup can only mark updates unavailable instead of recovering the loadable wrapper.
Context Used: AGENTS.md (source)
Artifacts
Repro: generated Bun test exercising the restore failure rollback path
- Contains supporting evidence from the run (text/typescript; charset=utf-8).
Repro: verbose Bun test output showing swallowed restore error and missing current wrapper
- 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-manager.ts
Line: 28-29
Comment:
**Restore Failure Is Dropped**
When `current` has already moved to `backup`, a failed `prepared -> current` rename depends on this restore to recreate `kagan@latest`. If the restore rename also fails, this branch drops that failure and rethrows only the promotion error, leaving the active wrapper missing while startup cleanup can only mark updates unavailable instead of recovering the loadable wrapper.
**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.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).
|
🎉 This PR is included in version 0.3.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Changes
Verification
bun run verifybun run test(604 passing)bunx verifyx lintbunx verifyx check-typesbunx verifyx duplicate-codebun run plugin:installManual Smoke
Restart OpenCode, then open the board, settings route, and create-task dialog to confirm rendering and submission.