Restore API-backed Exposé and finish sidecar restart repair - #358
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR removes the installed-shim restart fast-path in favor of a local-bootstrap ChangesRestart Orchestration Overhaul
Estimated code review effort: 5 (Critical) | ~120 minutes tmux Exposé Feature
Estimated code review effort: 5 (Critical) | ~150 minutes Session Fresh-Relaunch and Restore Semantics
Estimated code review effort: 4 (Complex) | ~90 minutes Dashboard Startup Priming and Mutation Settlement
Estimated code review effort: 4 (Complex) | ~75 minutes Dashboard Window Replace-When-Ready & UI State
Estimated code review effort: 3 (Moderate) | ~45 minutes Managed Launch Env & Worktree Persistence
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as core-cli
participant Client as restartControlPlaneFromCli
participant Restart as restartAimuxControlPlane
participant Supervisor as daemon-supervisor
participant Dashboard as dashboard-control
CLI->>Client: restartControlPlaneFromCli(projectRoot)
Client->>Restart: restartAimuxControlPlane({ ensureDaemonRunning, stopDaemon })
Restart->>Dashboard: stopPreRestartDashboardRepairWindows
Restart->>Supervisor: terminateDaemonOnDefaultPort / ensureDaemonRunning
Supervisor-->>Restart: daemon health/PID
Restart-->>Client: RuntimeRestartResult
Client-->>CLI: { restart, text, source: "local-bootstrap" }
sequenceDiagram
participant Tmux as tmux-control.sh
participant Popup as expose.ts (runTmuxExpose)
participant Model as expose-model.ts
participant Service as project-service/daemon
participant Runtime as TmuxRuntimeManager
Tmux->>Popup: display-popup aimux expose --project-root ...
Popup->>Model: loadExposeScopeItems(scope, context)
Model->>Service: GET /controls/switchableAgents or /core/expose/items
Service-->>Model: items[]
Model-->>Popup: ExposeScopeView
Popup->>Runtime: capture pane previews / draw tiles
Popup->>Model: focusExposeItem(item)
Model->>Service: POST focus/expose-focus route
Service-->>Popup: { ok: true }
Popup->>Runtime: switch to focused window
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/multiplexer/runtime-state.ts (1)
522-541: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope offline resume to the host project root.
findTopologySession(sessionId, ["offline"])is unscoped, andreconcileBackendSessionIdForSession(session, getRepoRoot())can read/write the wrong project’s state on project-service hosts; useprojectRootFor(host)for both.🤖 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 `@src/multiplexer/runtime-state.ts` around lines 522 - 541, Scope the offline resume flow to the current host project root instead of the global repo root. In the runtime-state path where `offlineEntry` is built and `upsertTopologySession` is called, make sure the lookup and reconciliation logic use `projectRootFor(host)` consistently, including the `findTopologySession(sessionId, ["offline"])` and `reconcileBackendSessionIdForSession(...)` calls, so project-service hosts only read and write state for their own project.src/dashboard/ui-state-store.ts (1)
149-172: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the restore flag alive while a preferred selection is still pending
Switching away from"sessions"before the preferred entry appears can clearselectionNeedsRestorehere even thoughpendingPreferredSelectionis still true, so the optimistic selection is never retried unless some other path marks the store dirty again. Preserve the restore state until the preferred entry is resolved or explicitly abandoned.🤖 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 `@src/dashboard/ui-state-store.ts` around lines 149 - 172, The restore flow in consumeSelectionRestore is clearing selectionNeedsRestore too early when a preferred selection is still pending. Update the logic around preferredSelection, pendingPreferredSelection, and selectionNeedsRestore so that leaving the "sessions" level does not disable restore until the preferred entry is actually found or explicitly dropped; keep the restore flag set whenever pendingPreferredSelection remains true, and only clear it after the pending selection is resolved.src/runtime-restart.ts (1)
633-675: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t swallow aborts inside verification catches.
Abort checks at Lines 637, 659, and 662 can be caught by the generic
catchblocks and converted intolatestError, especially on the final attempt where no later sleep rethrows. Re-throw when the signal is aborted.Proposed fix
try { await input.ensureProjectService(projectRoot); throwIfRestartAborted(input.abortSignal); } catch (error) { + throwIfRestartAborted(input.abortSignal); latestError = `post-restart service repair failed for ${projectRoot}: ${errorMessage(error)}`; } } } } catch (error) { + throwIfRestartAborted(input.abortSignal); latestError = errorMessage(error); }🤖 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 `@src/runtime-restart.ts` around lines 633 - 675, In the restart verification flow in runtime-restart.ts, the generic catch blocks in the attempt loop can swallow abort-related exceptions from throwIfRestartAborted and turn them into latestError. Update the catch handling around buildRuntimeCoherenceReport and ensureProjectService so that if input.abortSignal is aborted, the abort is re-thrown immediately instead of being converted to an error string, preserving cancellation semantics across all attempts.src/launcher-env.ts (1)
40-56: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove the forced
process.exit(code)in the core path.process.exitCodeis enough;process.exit()can cut off piped stdout/stderr before JSON or other machine-readable output fully flushes.🤖 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 `@src/launcher-env.ts` around lines 40 - 56, In runRoutedCli, the core branch is still forcing termination with process.exit(code) after runCoreCli returns, which can truncate buffered output. Update the core-path logic in runRoutedCli so it only sets process.exitCode from the returned code and lets the process end naturally; keep the existing run.catch handling unchanged.docs/core-sidecar-north-star.md (1)
58-65: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winOwnership Matrix row 59 now contradicts the restart architecture this PR ships.
Row 59 states restart should be daemon-owned repair-on-request ("CLI/TUI should request the daemon to repair, not perform repair independently"). But this same PR reclassifies
aimux restart/aimux daemon restartasBOOTSTRAP(command-ownership-inventory.md row 70) and rewritesrestartControlPlaneFromClito always perform local repair orchestration from the launcher (src/control-plane-restart-client.ts), never asking a running daemon to repair itself. Since this is the "north star" doc the sidecar migration is measured against, leaving this stale guidance risks steering future work toward the wrong pattern.📝 Proposed doc fix
-| daemon status, restart, repair orchestration | daemon | CLI/TUI should request the daemon to repair, not perform repair independently. | +| daemon status, repair orchestration | daemon | CLI/TUI should request the daemon to repair, not perform repair independently. | +| restart (bootstrap recovery) | launcher + daemon + tmux | Launcher performs local repair orchestration to recover a stale or wedged daemon; see command-ownership-inventory.md. |🤖 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/core-sidecar-north-star.md` around lines 58 - 65, Update the Ownership Matrix entry in the north-star doc so it matches the restart flow now implemented by restartControlPlaneFromCli and the BOOTSTRAP ownership in command-ownership-inventory.md. Replace the stale “CLI/TUI should request the daemon to repair” guidance with wording that reflects local launcher-owned restart orchestration for aimux restart and aimux daemon restart, while keeping the row aligned with the current daemon/project-service split.
🧹 Nitpick comments (6)
src/multiplexer/persistence-methods.test.ts (1)
917-971: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for list/create; consider extending to graveyard/delete/remove paths.
These tests validate
projectRootthreading forlistDesktopWorktreesandcreateDesktopWorktree, butgraveyardDesktopWorktree,deleteGraveyardWorktree,removeDesktopWorktree, andsortDesktopWorktreesalso gainedprojectRoot-scopedfindMainRepo/getWorktreeBaseDircalls inpersistence-methods.tswithout matching new assertions here.🤖 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 `@src/multiplexer/persistence-methods.test.ts` around lines 917 - 971, Extend the existing persistence-methods tests to cover the other projectRoot-scoped paths added in persistenceMethods, specifically graveyardDesktopWorktree, deleteGraveyardWorktree, removeDesktopWorktree, and sortDesktopWorktrees. Add assertions that these methods call findMainRepo and getWorktreeBaseDir with the host projectRoot and that their behavior is rooted under the host repo, similar to the current listDesktopWorktrees and createDesktopWorktree coverage.src/dashboard/ui-state-store.ts (1)
137-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo fallback/timeout if the optimistic entry never appears.
pendingPreferredSelectionstaystrueindefinitely if the requested entry never materializes (e.g., the underlying create/action fails). This keepsselectionNeedsRestoreperpetuallytrue(per the early return at line 168), causing the restore logic to re-run every reconcile cycle without ever settling. Consider a way to give up after N attempts or a timeout so a failed optimistic action doesn't leave selection restore permanently "in flight."🤖 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 `@src/dashboard/ui-state-store.ts` around lines 137 - 143, The optimistic selection flow in preferEntrySelection can remain stuck forever if the requested entry never appears, because pendingPreferredSelection and selectionNeedsRestore are never cleared. Update the restore path in ui-state-store to track either a bounded retry count or a timeout for the preferredSelection attempt, and clear pendingPreferredSelection/selectionNeedsRestore once the limit is exceeded. Make sure the early-return logic that currently re-enters restore on each reconcile cycle respects this fallback so failed optimistic actions eventually settle instead of staying in-flight.src/expose-control.test.ts (1)
14-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't verify
sessionNamesscoping is threaded through.The mock
listItemsFnignores thesessionNamesargument, and no assertion checks it was called with the project-scoped session list. This is the core correctness guarantee oflistAllProjectsExposeItems(see companion comment onsrc/expose-control.ts); a regression that drops or ignoressessionNameswould pass this test silently.✅ Suggested assertion addition
const items = listAllProjectsExposeItems({ tmux: tmux as never, listProjectsFn: () => [...], listItemsFn: listItemsFn as never, }); expect(items.map((item) => [item.projectName, item.projectRoot])).toEqual([...]); expect(listItemsFn).toHaveBeenCalledTimes(2); + expect(listItemsFn).toHaveBeenCalledWith( + expect.objectContaining({ projectRoot: "/repo/two", sessionNames: ["aimux-two"] }), + tmux, + { scope: "all" }, + );Also applies to: 36-41
🤖 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 `@src/expose-control.test.ts` around lines 14 - 24, The test for listAllProjectsExposeItems is not validating that sessionNames is passed through, so a regression could ignore project-scoped sessions unnoticed. Update the listItemsFn mock and its expectations in expose-control.test to accept the sessionNames argument and assert it is called with the project-specific session list when listAllProjectsExposeItems runs. Use the existing listAllProjectsExposeItems and listItemsFn symbols to keep the assertion tied to the correct call path.src/tmux/expose.ts (1)
372-406: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExposé's tile list never refreshes except on manual zoom — dead sessions can linger as zombie tiles.
scheduleRefreshonly callsrefreshCaptures()(pane content) on each tick;itemsis only re-fetched viareload()when the user pressesg. If an agent session exits mid-popup,refreshCaptures(Line 381-383) silently reuses the last-known capture on failure, so the tile keeps showing stale content indefinitely, and selecting it will fail server-side (daemon/project-service 404 on focus) with no feedback to the user (selectTilejust exits with code 1).Consider periodically re-running
reload()(e.g., every few refresh ticks) or detectingcaptureTargetfailures and pruning the corresponding item fromitems.Also applies to: 486-497, 577-602
🤖 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 `@src/tmux/expose.ts` around lines 372 - 406, The tile list is only refreshing pane captures, so dead sessions can remain visible as stale tiles until a manual reload. Update the refresh flow around refreshCaptures(), scheduleRefresh(), and reload() so the periodic tick occasionally re-runs reload() or otherwise removes items whose captureTarget() fails, instead of silently keeping the last capture. Also make selectTile() handle missing/dead targets more gracefully by syncing with the refreshed items list before attempting focus, so stale entries do not persist or fail without feedback.src/launcher-env.ts (1)
42-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
cliEntryFor(process.argv)call.
cliEntryFor(process.argv)is computed twice for the ternary chain. Hoisting it into a local avoids recomputation and reads slightly cleaner.♻️ Suggested refactor
export function runRoutedCli(): void { + const entry = cliEntryFor(process.argv); const run = - cliEntryFor(process.argv) === "core" + entry === "core" ? import("./core-cli.js").then(async ({ runCoreCli }) => { const code = await runCoreCli(process.argv.slice(2)); process.exitCode = code; }) - : cliEntryFor(process.argv) === "expose" + : entry === "expose" ? import("./popup-expose.js").then((m) => m.runExpose()) : import("./main.js").then(() => undefined);🤖 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 `@src/launcher-env.ts` around lines 42 - 50, The ternary chain in launcher-env.ts calls cliEntryFor(process.argv) twice, so hoist that result into a local variable and reuse it for the core/expose/main branch selection. Update the logic around cliEntryFor and the import("./core-cli.js"), import("./popup-expose.js"), and import("./main.js") branches so the CLI entry is computed once and the control flow stays the same.src/multiplexer/runtime-guard.ts (1)
19-21: 🩺 Stability & Availability | 🔵 TrivialNote the detection-latency tradeoff.
Raising the health-probe timeout to 10s reduces false "disconnected" classifications during restore/start mutations, but since
refreshRuntimeGuardserializes probes viaruntimeGuardProbing, a genuinely dead service can now take up to 10s (vs 2.5s) to be detected and trigger repair. Worth keeping an eye on repair-latency metrics/UX after this change, given no other change compensates for it.🤖 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 `@src/multiplexer/runtime-guard.ts` around lines 19 - 21, The health probe timeout change in runtime-guard.ts increases detection latency for genuinely dead services because refreshRuntimeGuard serializes probes through runtimeGuardProbing. Keep the 10s bound if needed for restore/start stability, but update the surrounding logic in runtimeGuardProbing/refreshRuntimeGuard or related timeout handling so repair latency is not significantly worse, and make the tradeoff explicit in the runtime guard behavior or metrics/telemetry.
🤖 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 `@scripts/tmux-control.sh`:
- Around line 519-542: The retry loop in the Exposé popup flow can spin forever
when display-popup or the Exposé command keeps returning status 75. Update the
while loop in tmux-control.sh to cap retries with a counter or similar guard
around the display-popup/popup_status handling, and stop retrying after a small
maximum. Keep the existing 75-only retry behavior in the
expose_cmd/display-popup path, but ensure the loop exits cleanly once the limit
is reached.
In `@src/core-project-actor.ts`:
- Around line 43-52: The already-started branch in CoreProjectActor.start()
currently calls ensureEndpointPublished() directly, so a transient publish
failure can reject an otherwise healthy fast path. Update the started-path in
start() to treat republishing as best-effort by catching errors from
ensureEndpointPublished(), logging them, and still returning getState(); keep
the change localized to CoreProjectActor.start() and ensureEndpointPublished().
In `@src/daemon.ts`:
- Around line 610-632: The request handler in daemon.ts is trimming windowId and
projectRoot before validating their types, so non-string JSON values can throw
instead of returning a 400. Update the validation around the payload parsing in
the openTargetForClient flow to explicitly check that windowId and projectRoot
(and any similar trimmed fields) are strings before calling trim, and return a
400 with a clear error when they are not. Keep the check close to the existing
windowId/projectRoot handling so the listAllProjectsExposeItems lookup only runs
on validated string input.
- Around line 3273-3279: The exposed route handling in the daemon request
dispatcher still allows relay-authenticated owners to reach the `/core/expose/*`
endpoints, so add a local-only access check before calling `exposeItemsRoute()`
and `exposeFocusRoute()` in `src/daemon.ts`. Update the routing logic around the
`CORE_API_ROUTES.exposeItems` and `CORE_API_ROUTES.exposeFocus` branches to
explicitly reject remote requests, even when `assertRemoteAccessAllowed()` would
otherwise return ok for `actor.role === "owner"`, so these routes remain
local-only.
In `@src/dashboard/targets.ts`:
- Around line 119-124: The dashboard replacement flow clears the ready stamp too
early, so a failed or timed-out swap leaves the existing window undiscoverable
by findLiveDashboardTarget. In targets.ts, keep the current
TMUX_DASHBOARD_READY_OPTION value on the live dashboardTarget until
replaceWindowWhenReady succeeds, then update the option only after the new
window is confirmed ready. Use the existing tmux.setWindowOption and
tmux.replaceWindowWhenReady flow to preserve the original stamp on failure.
In `@src/multiplexer/dashboard-model.ts`:
- Around line 263-265: The topology readiness check in the dashboard model is
reading offline session state without scoping it to the current project, which
can return the wrong restoreBlockedReason. Update the waitForMetadataCondition
logic in dashboard-model.ts to pass projectRootFor(host) into
listTopologySessionStates, keeping the lookup for session.id the same so the
check only uses topology state from this dashboard’s project root.
- Around line 237-245: In metadataSessionCanReceiveInput, the explicit
semantic.runtime.canReceiveInput flag is only honored when true, which lets a
false value fall through to isMetadataSessionRunning and incorrectly mark the
session ready. Update the function to treat runtime?.canReceiveInput === false
as an immediate false result before checking isMetadataSessionRunning, while
keeping the existing true and isAlive checks intact.
In `@src/tmux/runtime-manager.ts`:
- Around line 725-734: The ready-path commit in the window swap flow is not
rollback-safe, so if renameWindow or exec("swap-window", ...) fails the
replacement can be left detached and the original window renamed to "-old".
Update the commit block in the logic around getWindowOption, renameWindow, exec,
and killWindow to run inside a try, and on failure restore the original window
name/state and dispose of the replacement before rethrowing so the operation is
atomic.
---
Outside diff comments:
In `@docs/core-sidecar-north-star.md`:
- Around line 58-65: Update the Ownership Matrix entry in the north-star doc so
it matches the restart flow now implemented by restartControlPlaneFromCli and
the BOOTSTRAP ownership in command-ownership-inventory.md. Replace the stale
“CLI/TUI should request the daemon to repair” guidance with wording that
reflects local launcher-owned restart orchestration for aimux restart and aimux
daemon restart, while keeping the row aligned with the current
daemon/project-service split.
In `@src/dashboard/ui-state-store.ts`:
- Around line 149-172: The restore flow in consumeSelectionRestore is clearing
selectionNeedsRestore too early when a preferred selection is still pending.
Update the logic around preferredSelection, pendingPreferredSelection, and
selectionNeedsRestore so that leaving the "sessions" level does not disable
restore until the preferred entry is actually found or explicitly dropped; keep
the restore flag set whenever pendingPreferredSelection remains true, and only
clear it after the pending selection is resolved.
In `@src/launcher-env.ts`:
- Around line 40-56: In runRoutedCli, the core branch is still forcing
termination with process.exit(code) after runCoreCli returns, which can truncate
buffered output. Update the core-path logic in runRoutedCli so it only sets
process.exitCode from the returned code and lets the process end naturally; keep
the existing run.catch handling unchanged.
In `@src/multiplexer/runtime-state.ts`:
- Around line 522-541: Scope the offline resume flow to the current host project
root instead of the global repo root. In the runtime-state path where
`offlineEntry` is built and `upsertTopologySession` is called, make sure the
lookup and reconciliation logic use `projectRootFor(host)` consistently,
including the `findTopologySession(sessionId, ["offline"])` and
`reconcileBackendSessionIdForSession(...)` calls, so project-service hosts only
read and write state for their own project.
In `@src/runtime-restart.ts`:
- Around line 633-675: In the restart verification flow in runtime-restart.ts,
the generic catch blocks in the attempt loop can swallow abort-related
exceptions from throwIfRestartAborted and turn them into latestError. Update the
catch handling around buildRuntimeCoherenceReport and ensureProjectService so
that if input.abortSignal is aborted, the abort is re-thrown immediately instead
of being converted to an error string, preserving cancellation semantics across
all attempts.
---
Nitpick comments:
In `@src/dashboard/ui-state-store.ts`:
- Around line 137-143: The optimistic selection flow in preferEntrySelection can
remain stuck forever if the requested entry never appears, because
pendingPreferredSelection and selectionNeedsRestore are never cleared. Update
the restore path in ui-state-store to track either a bounded retry count or a
timeout for the preferredSelection attempt, and clear
pendingPreferredSelection/selectionNeedsRestore once the limit is exceeded. Make
sure the early-return logic that currently re-enters restore on each reconcile
cycle respects this fallback so failed optimistic actions eventually settle
instead of staying in-flight.
In `@src/expose-control.test.ts`:
- Around line 14-24: The test for listAllProjectsExposeItems is not validating
that sessionNames is passed through, so a regression could ignore project-scoped
sessions unnoticed. Update the listItemsFn mock and its expectations in
expose-control.test to accept the sessionNames argument and assert it is called
with the project-specific session list when listAllProjectsExposeItems runs. Use
the existing listAllProjectsExposeItems and listItemsFn symbols to keep the
assertion tied to the correct call path.
In `@src/launcher-env.ts`:
- Around line 42-50: The ternary chain in launcher-env.ts calls
cliEntryFor(process.argv) twice, so hoist that result into a local variable and
reuse it for the core/expose/main branch selection. Update the logic around
cliEntryFor and the import("./core-cli.js"), import("./popup-expose.js"), and
import("./main.js") branches so the CLI entry is computed once and the control
flow stays the same.
In `@src/multiplexer/persistence-methods.test.ts`:
- Around line 917-971: Extend the existing persistence-methods tests to cover
the other projectRoot-scoped paths added in persistenceMethods, specifically
graveyardDesktopWorktree, deleteGraveyardWorktree, removeDesktopWorktree, and
sortDesktopWorktrees. Add assertions that these methods call findMainRepo and
getWorktreeBaseDir with the host projectRoot and that their behavior is rooted
under the host repo, similar to the current listDesktopWorktrees and
createDesktopWorktree coverage.
In `@src/multiplexer/runtime-guard.ts`:
- Around line 19-21: The health probe timeout change in runtime-guard.ts
increases detection latency for genuinely dead services because
refreshRuntimeGuard serializes probes through runtimeGuardProbing. Keep the 10s
bound if needed for restore/start stability, but update the surrounding logic in
runtimeGuardProbing/refreshRuntimeGuard or related timeout handling so repair
latency is not significantly worse, and make the tradeoff explicit in the
runtime guard behavior or metrics/telemetry.
In `@src/tmux/expose.ts`:
- Around line 372-406: The tile list is only refreshing pane captures, so dead
sessions can remain visible as stale tiles until a manual reload. Update the
refresh flow around refreshCaptures(), scheduleRefresh(), and reload() so the
periodic tick occasionally re-runs reload() or otherwise removes items whose
captureTarget() fails, instead of silently keeping the last capture. Also make
selectTile() handle missing/dead targets more gracefully by syncing with the
refreshed items list before attempting focus, so stale entries do not persist or
fail without feedback.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b527b607-66c0-406d-86b1-4908b84eca87
📒 Files selected for processing (70)
docs/command-ownership-inventory.mddocs/core-sidecar-north-star.mdscripts/installed-aimux-shim.shscripts/tmux-control.shsrc/backend-session-discovery.test.tssrc/backend-session-discovery.tssrc/control-plane-restart-client.test.tssrc/control-plane-restart-client.tssrc/core-cli.test.tssrc/core-cli.tssrc/core-command-contract.tssrc/core-command-ownership.test.tssrc/core-project-actor.test.tssrc/core-project-actor.tssrc/daemon-supervisor.tssrc/daemon.test.tssrc/daemon.tssrc/dashboard/command-spec.test.tssrc/dashboard/command-spec.tssrc/dashboard/targets.test.tssrc/dashboard/targets.tssrc/dashboard/ui-state-store.test.tssrc/dashboard/ui-state-store.tssrc/expose-control.test.tssrc/expose-control.tssrc/installed-shim.test.tssrc/launcher-env.test.tssrc/launcher-env.tssrc/main.tssrc/managed-launch-env.test.tssrc/managed-launch-env.tssrc/metadata-server.test.tssrc/metadata-server.tssrc/multiplexer/dashboard-control.test.tssrc/multiplexer/dashboard-control.tssrc/multiplexer/dashboard-model.tssrc/multiplexer/dashboard-ops.test.tssrc/multiplexer/dashboard-ops.tssrc/multiplexer/dashboard-tail-methods.test.tssrc/multiplexer/dashboard-tail-methods.tssrc/multiplexer/index.tssrc/multiplexer/persistence-methods.test.tssrc/multiplexer/persistence-methods.tssrc/multiplexer/runtime-guard.test.tssrc/multiplexer/runtime-guard.tssrc/multiplexer/runtime-state.test.tssrc/multiplexer/runtime-state.tssrc/multiplexer/session-launch.test.tssrc/multiplexer/session-launch.tssrc/multiplexer/session-runtime-core.test.tssrc/multiplexer/session-runtime-core.tssrc/one-shot-node-inventory.test.tssrc/popup-expose.test.tssrc/popup-expose.tssrc/runtime-core/topology-sessions.tssrc/runtime-core/topology-store.tssrc/runtime-restart.test.tssrc/runtime-restart.tssrc/session-fresh-relaunch.tssrc/session-restorability.test.tssrc/session-restorability.tssrc/tmux/control-script.test.tssrc/tmux/doctor.test.tssrc/tmux/expose-layout.test.tssrc/tmux/expose-model.test.tssrc/tmux/expose-model.tssrc/tmux/expose-tile.test.tssrc/tmux/expose.tssrc/tmux/runtime-manager.test.tssrc/tmux/runtime-manager.ts
💤 Files with no reviewable changes (1)
- scripts/installed-aimux-shim.sh
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime-core/topology-sessions.ts (1)
159-160: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear
freshRelaunchAllowedon non-offline upserts
upsertTopologySession(..., "running"/"idle", ...)still carriesfreshRelaunchAllowedfrom an offline snapshot, so a live row can keep an offline-only flag and feed it back intoreconcileRuntimeTopologySessions. Delete it withrestoreBlockedReasonwhenstatus !== "offline".🐛 Proposed fix
const nextSession = { ...sessionToTopologySession(session, node.id, now), status }; if (status !== "offline") { delete nextSession.restoreBlockedReason; + delete nextSession.freshRelaunchAllowed; }🤖 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 `@src/runtime-core/topology-sessions.ts` around lines 159 - 160, The upsert path in upsertTopologySession is preserving the offline-only freshRelaunchAllowed flag for running/idle sessions, so clear it the same way restoreBlockedReason is cleared whenever session.lifecycle is not "offline". Update the session object construction in topology-sessions.ts so both fields are only populated from the offline snapshot and are undefined for non-offline upserts, preventing stale flags from being fed back into reconcileRuntimeTopologySessions.
🧹 Nitpick comments (4)
src/tmux/runtime-manager.test.ts (1)
1854-1883: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRollback test doesn't verify call ordering.
expect(calls).toEqual(expect.arrayContaining([...]))only checks that the listed calls occurred somewhere, not their relative order. For a rollback path, ordering is exactly what matters (e.g., renaming@1back to "dashboard" must happen only after the swap failure is detected, and before/after the@2rename-and-kill sequence). The earlier success test (lines 1756-1806, per diff summary) asserts an exact ordered sequence — this rollback test would benefit from the same rigor to catch a rollback executed out of order (which could transiently leave two windows named "dashboard" or kill the wrong window first).♻️ Suggested approach using indices (similar to metadata-server.test.ts's clearReadyCallIndex pattern)
- const calls = exec.mock.calls.map((call) => call[0].join(" ")); - expect(calls).toEqual( - expect.arrayContaining([ - "rename-window -t `@1` dashboard-old", - "rename-window -t `@2` dashboard", - "swap-window -d -s `@2` -t `@1`", - "rename-window -t `@1` dashboard", - "kill-window -t `@2`", - ]), - ); + const calls = exec.mock.calls.map((call) => call[0].join(" ")); + const idx = (needle: string) => calls.indexOf(needle); + expect(idx("swap-window -d -s `@2` -t `@1`")).toBeGreaterThan(idx("rename-window -t `@1` dashboard-old")); + expect(idx("rename-window -t `@1` dashboard")).toBeGreaterThan(idx("swap-window -d -s `@2` -t `@1`")); + expect(idx("kill-window -t `@2`")).toBeGreaterThan(idx("rename-window -t `@1` dashboard"));🤖 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 `@src/tmux/runtime-manager.test.ts` around lines 1854 - 1883, The rollback test for manager.replaceWindowWhenReady only checks that the tmux commands happened somewhere, not that they happened in the correct order. Replace the loose expect.arrayContaining assertion on exec.mock.calls with an ordered sequence check (or explicit index assertions, similar to a clearReadyCallIndex-style pattern) so the rollback flow verifies rename-window, swap-window, recovery rename, and kill-window happen in the intended order after the swap failure.src/dashboard/ui-state-store.test.ts (1)
185-217: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis test can't actually detect a stuck
pendingPreferredSelection.Per the upstream contract in
src/dashboard/ui-state-store.ts(consumeSelectionRestore), oncestate.level === "sessions"the early-return gate onpendingPreferredSelectiononly fires inside the "preferred entry not found" branch, after clamping. Since "gone-agent" never appears in eitherworktreeEntriesupdate in this test, both a correct implementation (pending cleared after first call) and a buggy one (pending stuck forever) yieldsessionIndex === 0— the assertions can't distinguish them.To actually validate "does not retain it as pending," reintroduce the missing entry in a follow-up round and assert
sessionIndexdoesn't jump to it (which it would if pending were incorrectly retained).🧪 Suggested strengthening of the assertion
state.worktreeEntries = [ { kind: "session", id: "later-agent" }, { kind: "session", id: "remaining-agent" }, ]; store.consumeSelectionRestore(state, [], true, 0, () => undefined); expect(state.sessionIndex).toBe(0); + + // If pendingPreferredSelection were incorrectly retained, reintroducing the + // originally-preferred (but stale) entry would cause a selection jump here. + state.worktreeEntries = [ + { kind: "session", id: "gone-agent" }, + { kind: "session", id: "remaining-agent" }, + ]; + store.consumeSelectionRestore(state, [], true, 0, () => undefined); + + expect(state.sessionIndex).toBe(0); });🤖 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 `@src/dashboard/ui-state-store.test.ts` around lines 185 - 217, The test in DashboardUiStateStore is not proving that pendingPreferredSelection is cleared because the selected entry never reappears, so both correct and buggy behavior look the same. Update the scenario around DashboardUiStateStore.consumeSelectionRestore and DashboardUiStateStore.loadInto to first keep the preferred entry missing, then bring it back in a later worktreeEntries update and assert the sessionIndex does not jump to it if the pending state was correctly cleared. Use the existing symbols DashboardUiStateStore, consumeSelectionRestore, and sessionIndex to anchor the stronger assertion.src/runtime-restart.ts (2)
199-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
readRuntimeRestartLockPidname doesn't reflect its generic reuse.The function is now also used to read the dashboard-repair lock's owner (Line 211), not just the runtime-restart lock. Consider a more generic name (e.g.,
readLockOwnerPid) for clarity.♻️ Suggested rename
-function readRuntimeRestartLockPid(lockPath: string): number | null { +function readLockOwnerPid(lockPath: string): number | null {(update call sites accordingly)
Also applies to: 208-215
🤖 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 `@src/runtime-restart.ts` around lines 199 - 206, The helper name readRuntimeRestartLockPid is too specific for its broader use in reading any lock owner PID, including the dashboard-repair lock. Rename the function to a generic symbol such as readLockOwnerPid, and update all call sites in runtime-restart.ts that use it so the name matches its shared purpose and remains clear if reused elsewhere.
611-680: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAbort is only checked before/after
sleep, not during it.
throwIfRestartAbortedis called immediately before and afterinput.sleep(intervalMs)(Lines 669-671), butsleepitself doesn't take theabortSignal, so an abort fired mid-sleep isn't observed until the fullintervalMselapses. For larger intervals this adds unnecessary latency to cancellation. Consider threading the abort signal intosleep(e.g.,Promise.racewith an abort-aware rejection) so cancellation is immediate.🤖 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 `@src/runtime-restart.ts` around lines 611 - 680, Abort handling in verifyPostRestartCoherence is delayed because input.sleep(intervalMs) is not abort-aware, so cancellation only takes effect before or after the full wait. Update verifyPostRestartCoherence to pass input.abortSignal into the waiting path (or wrap sleep with an abort-aware race) so a mid-sleep abort rejects immediately, while preserving the existing throwIfRestartAborted checks around buildRuntimeCoherenceReport and ensureProjectService.
🤖 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 `@src/tmux/expose.ts`:
- Around line 602-608: The timer-driven refresh in the expose flow is resetting
the user’s in-flight selection and can also close the popup on transient reload
failures. Update the `reload()` path in `src/tmux/expose.ts` so periodic
refreshes preserve the current selection instead of always re-seeking to
`options.currentWindowId`, and handle errors from the reload logic locally
rather than letting the outer `catch` call `finish(1)`. Use the existing
`refreshTick`, `ITEM_RELOAD_EVERY_TICKS`, `reload`, and `finish` flow to keep
auto-reload non-destructive.
---
Outside diff comments:
In `@src/runtime-core/topology-sessions.ts`:
- Around line 159-160: The upsert path in upsertTopologySession is preserving
the offline-only freshRelaunchAllowed flag for running/idle sessions, so clear
it the same way restoreBlockedReason is cleared whenever session.lifecycle is
not "offline". Update the session object construction in topology-sessions.ts so
both fields are only populated from the offline snapshot and are undefined for
non-offline upserts, preventing stale flags from being fed back into
reconcileRuntimeTopologySessions.
---
Nitpick comments:
In `@src/dashboard/ui-state-store.test.ts`:
- Around line 185-217: The test in DashboardUiStateStore is not proving that
pendingPreferredSelection is cleared because the selected entry never reappears,
so both correct and buggy behavior look the same. Update the scenario around
DashboardUiStateStore.consumeSelectionRestore and DashboardUiStateStore.loadInto
to first keep the preferred entry missing, then bring it back in a later
worktreeEntries update and assert the sessionIndex does not jump to it if the
pending state was correctly cleared. Use the existing symbols
DashboardUiStateStore, consumeSelectionRestore, and sessionIndex to anchor the
stronger assertion.
In `@src/runtime-restart.ts`:
- Around line 199-206: The helper name readRuntimeRestartLockPid is too specific
for its broader use in reading any lock owner PID, including the
dashboard-repair lock. Rename the function to a generic symbol such as
readLockOwnerPid, and update all call sites in runtime-restart.ts that use it so
the name matches its shared purpose and remains clear if reused elsewhere.
- Around line 611-680: Abort handling in verifyPostRestartCoherence is delayed
because input.sleep(intervalMs) is not abort-aware, so cancellation only takes
effect before or after the full wait. Update verifyPostRestartCoherence to pass
input.abortSignal into the waiting path (or wrap sleep with an abort-aware race)
so a mid-sleep abort rejects immediately, while preserving the existing
throwIfRestartAborted checks around buildRuntimeCoherenceReport and
ensureProjectService.
In `@src/tmux/runtime-manager.test.ts`:
- Around line 1854-1883: The rollback test for manager.replaceWindowWhenReady
only checks that the tmux commands happened somewhere, not that they happened in
the correct order. Replace the loose expect.arrayContaining assertion on
exec.mock.calls with an ordered sequence check (or explicit index assertions,
similar to a clearReadyCallIndex-style pattern) so the rollback flow verifies
rename-window, swap-window, recovery rename, and kill-window happen in the
intended order after the swap failure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9796d902-cf3c-4154-bb7f-2ebb31f0b606
📒 Files selected for processing (25)
docs/core-sidecar-north-star.mdscripts/tmux-control.shsrc/core-project-actor.test.tssrc/core-project-actor.tssrc/daemon.test.tssrc/daemon.tssrc/dashboard/targets.tssrc/dashboard/ui-state-store.test.tssrc/dashboard/ui-state-store.tssrc/expose-control.test.tssrc/launcher-env.tssrc/managed-launch-env.test.tssrc/metadata-server.test.tssrc/multiplexer/dashboard-model.test.tssrc/multiplexer/dashboard-model.tssrc/multiplexer/runtime-state.test.tssrc/multiplexer/runtime-state.tssrc/runtime-core/backend-session-ids.tssrc/runtime-core/topology-sessions.tssrc/runtime-restart.test.tssrc/runtime-restart.tssrc/tmux/control-script.test.tssrc/tmux/expose.tssrc/tmux/runtime-manager.test.tssrc/tmux/runtime-manager.ts
💤 Files with no reviewable changes (1)
- src/dashboard/targets.ts
✅ Files skipped from review due to trivial changes (1)
- docs/core-sidecar-north-star.md
🚧 Files skipped from review as they are similar to previous changes (9)
- src/expose-control.test.ts
- src/core-project-actor.ts
- scripts/tmux-control.sh
- src/tmux/control-script.test.ts
- src/multiplexer/runtime-state.test.ts
- src/dashboard/ui-state-store.ts
- src/daemon.ts
- src/daemon.test.ts
- src/multiplexer/dashboard-model.ts
| refreshTick += 1; | ||
| const reloadedItems = refreshTick >= ITEM_RELOAD_EVERY_TICKS; | ||
| if (reloadedItems) { | ||
| refreshTick = 0; | ||
| await reload(); | ||
| } | ||
| const captureChanged = reloadedItems || refreshCaptures(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '470,640p' src/tmux/expose.tsRepository: TraderSamwise/aimux
Length of output: 5009
Preserve selection on auto-reload, and don’t exit the popup on reload errors
reload() always re-seeks to options.currentWindowId, so the timer-driven refresh can silently reset a user’s in-flight selection before Enter. The same refresh path is also wrapped by the outer catch that calls finish(1), so a transient loadExposeScopeItems failure closes the exposé entirely. Keep the current selection across periodic reloads and handle reload failures locally.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@src/tmux/expose.ts` around lines 602 - 608, The timer-driven refresh in the
expose flow is resetting the user’s in-flight selection and can also close the
popup on transient reload failures. Update the `reload()` path in
`src/tmux/expose.ts` so periodic refreshes preserve the current selection
instead of always re-seeking to `options.currentWindowId`, and handle errors
from the reload logic locally rather than letting the outer `catch` call
`finish(1)`. Use the existing `refreshTick`, `ITEM_RELOAD_EVERY_TICKS`,
`reload`, and `finish` flow to keep auto-reload non-destructive.
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes