Reliability: guaranteed brain updates, wedge-proof event loop, verified handover, AEAD account-connect fix#889
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughChangesRuntime diagnostics and reliability
Desktop update and presentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3da5e5e2b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runningHash: | ||
| process.env.ADE_RUNTIME_BUILD_HASH?.trim() | ||
| || preparedServiceCommand.buildHash, | ||
| isIdle: () => !hasActiveHeadlessConnections(states), |
There was a problem hiding this comment.
When the desktop is open, its long-lived brain RPC socket keeps activeConnections nonempty even while all agents and work sessions are idle, so every binary freshness mismatch waits the full 30-minute maximum; once that deadline expires, the monitor restarts even if an agent has since become active. This both delays brain updates during genuinely idle periods and can terminate live work at the forced deadline; use the runtime activity summary rather than connection count for this desktop socket-backed path.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| patchSnapshot({ autoApplyPending: null }); | ||
| void quitAndInstall().then((started) => { |
There was a problem hiding this comment.
If an agent turn or work session starts after the last periodic activity poll but before this countdown callback runs, the callback installs immediately without obtaining a fresh activity summary. With the defaults there is a roughly five-second blind window, and the verification/preparation steps can extend it further, allowing an automatic update to quit ADE while newly started work is active.
Useful? React with 👍 / 👎.
| this.activeClient = client; | ||
| this.activeRuntimePid = runtimeInfo.pid; | ||
| this.activeRuntimeSyncPort = runtimeInfo.syncPort; | ||
| this.activeRuntimePublishHealth = runtimeInfo.publishHealth; |
There was a problem hiding this comment.
runtimeInfo.publishHealth is captured only when the socket connects, while the account publisher changes state repeatedly without reconnecting. Consequently the Machines panel's 30-second app.getInfo() polling keeps receiving this frozen initial value, so sustained failures and later recoveries are never displayed unless the brain connection happens to restart; refresh this diagnostic through the underlying runtime status path rather than polling a cached field in the renderer.
AGENTS.md reference: AGENTS.md:L33-L34
Useful? React with 👍 / 👎.
| useEffect(() => { | ||
| let cancelled = false; | ||
| const infoPromise = window.ade.app?.getInfo?.(); |
There was a problem hiding this comment.
This effect reads lastWedge only once. In the primary scenario the notice is meant to report, the brain wedges after ADE is already open, restarts, and the connection pool learns the new breadcrumb on reconnect, but the mounted app shell never requests it again; users therefore see no recovery notice until a later full remount or app restart.
Useful? React with 👍 / 👎.
| const hasStagedUpdate = updateSnapshot.status === "ready" || Boolean(updateSnapshot.parked); | ||
| const runningVersion = info.appVersion; | ||
| const installedVersion = hasStagedUpdate && updateSnapshot.version | ||
| ? updateSnapshot.version | ||
| : info.appVersion; |
There was a problem hiding this comment.
When the updater reaches ready, the new artifact is only downloaded in the updater cache; the application bundle on disk still has info.appVersion until quitAndInstall succeeds. Assigning the staged version to installedVersion therefore makes About report a version that is not installed, including after a restart attempt aborts; label it as downloaded/staged or keep Installed tied to the actual bundle version.
AGENTS.md reference: AGENTS.md:L59-L60
Useful? React with 👍 / 👎.
…unt-connect Electron's BoringSSL lacks chacha20-poly1305 in createCipheriv, so v1.2.35's sealed account adoption failed with "Unknown cipher" on every route. Clients now offer supportedAeads in account_challenge; hosts intersect, echo the chosen aead (covered by the challenge signature when negotiated), and both sides seal/unseal with it. Clients without the field get the byte-identical legacy chacha path, keeping iOS and older desktops untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ver, publish health, truthful version UI Brain lifecycle: freshness self-monitor (stat-gated, idle-drained self-restart), wedge watchdog (worker-thread loop monitor, breadcrumbed SIGKILL recovery), responsiveness-probed launchd repair with verified PID handover, sync-listener zombie reaping with PID-reuse guards, port-drift republish, and a protocol compatibility window so newer brains connect instead of being quarantined. Updates: transactional install (rollback + parked state on abort, 10s quit escalation after consent), idle auto-apply with cancelable countdown, unified latest-version source, and truthful Running/Installed/Latest UI for both the app and the brain. Diagnostics: per-leg publish budgets with honest token/http timeout classification, cancellable Clerk refresh, rotating timestamped brain JSONL logs, ade doctor, machine-card publish-health line, wedge recovery notice, and reliability telemetry events. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r module split Applies the dual-review synthesis: shared parseRuntimePublishHealth/LastWedge replace two drifting hand-rolled parsers; observesAbort moves onto handler registrations (set-equality test guards the 48-action invariant); shared runWithAbortSignal; doctor orchestration moves out of cli.ts; dead duplicate state clear removed; pure version helpers extracted from autoUpdateService. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…idation Documents the six reliability/update PostHog events (taxonomy + bounded volumes), adds the Reliability-incidents insight to the dashboard spec (validated, provisioner tests green), updates six internal docs for the reliability release, rewrites the stale README doctor section, and merges the two remoteMachineModel test files into one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ests Extracts the electron-free release feed out of githubService so ade doctor's online check can't drag electron into the CLI bundle; the update snapshot hook tolerates hosts without the update IPC surface; the pool tests wait for ade/initialize readiness instead of bare socket accept. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3da5e5e to
d9635d5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/settings/AboutSection.tsx (1)
178-178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReconcile the two update sources before showing them side by side.
In packaged mode,
getLatestRelease()and the updater share the samelatestKnownVersion, but dev mode can return a GitHub release while the updater still carries an older cachedlatestKnownVersion. Since this component prefers the updater feed for the "Latest" text but useslatest.updateAvailablefor the pill, the UI can show "Update available" next to a lower/older latest version during release propagation. Derive both from one normalized latest value, or document/enforce that the feeds cannot diverge.🤖 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 `@apps/desktop/src/renderer/components/settings/AboutSection.tsx` at line 178, Update the AboutSection update-state logic around updateAvailable so the “Latest” version text and availability pill use one normalized latest-version source. Reconcile or normalize getLatestRelease() and the updater’s latestKnownVersion before deriving either value, ensuring the UI cannot pair an older displayed version with an “Update available” indicator; preserve the existing dev-mode behavior where appropriate.
🧹 Nitpick comments (4)
apps/ade-cli/src/commands/doctor.ts (1)
531-537: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
DEFAULT_SYNC_HOST_PORTinstead of the literal8787.
syncPortRowhardcodes8787(comparison and copy) whilerunDoctorCommanduses the importedDEFAULT_SYNC_HOST_PORT. If that constant ever changes, this row silently flags the correct port as a mismatch.♻️ Reference the shared constant
- if (input.syncPort === 8787) { + if (input.syncPort === DEFAULT_SYNC_HOST_PORT) { return { key: "sync_port", label: "Sync port", status: "ok", - detail: "bound on 8787", + detail: `bound on ${DEFAULT_SYNC_HOST_PORT}`, }; }The
bound on ${input.syncPort} instead of 8787string at Line 548 should likewise interpolateDEFAULT_SYNC_HOST_PORT.🤖 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 `@apps/ade-cli/src/commands/doctor.ts` around lines 531 - 537, Update syncPortRow to use the imported DEFAULT_SYNC_HOST_PORT for the port comparison and the “bound on” detail text, replacing both hardcoded 8787 references while preserving the existing status behavior.apps/ade-cli/src/serviceManager/installLaunchd.ts (1)
36-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the local
probeResponsivenessto avoid clashing withdeps.probeResponsiveness.
deps.probeResponsiveness(boolean opt-out) and the localconst probeResponsiveness(the probe function, line 286) share an identical base name and are used one line apart (298 vs. 299). Both are new in this diff, so renaming now is cheap and removes an easy-to-mistype ambiguity for future edits.♻️ Suggested rename
- const probeResponsiveness = deps.responsivenessProbe ?? defaultResponsivenessProbe; + const probeSocketResponsiveness = deps.responsivenessProbe ?? defaultResponsivenessProbe; ... if (!forceRestart && plistUnchanged && loaded?.running === true) { if ( deps.probeResponsiveness === false - || probeResponsiveness({ socketPath, timeoutMs: 1_500, command }) + || probeSocketResponsiveness({ socketPath, timeoutMs: 1_500, command }) ) {(and similarly at the handover-loop call site around line 395.)
Also applies to: 276-309
🤖 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 `@apps/ade-cli/src/serviceManager/installLaunchd.ts` around lines 36 - 46, Rename the local probe function currently declared as probeResponsiveness in the install flow to a distinct name, and update both its initial invocation and handover-loop call site. Keep deps.probeResponsiveness unchanged as the boolean opt-out configuration.apps/ade-cli/src/services/sync/sharedSyncListener.ts (1)
588-617: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the zombie-reap termination against a throwing
terminatePid.
terminatePidis injectable (options.terminatePid), and today's default swallows kill errors, but if a future/custom implementation throws, the exception escapes thiscatchblock uncaught and aborts the entirebindOnce()/ensureListening()chain instead of falling through to the normal bind-failure logging/retry path a few lines below.🛡️ Suggested guard
if ( staleHolder.startTime != null && confirmedHolder?.pid === staleHolder.pid && confirmedHolder.startTime === staleHolder.startTime ) { - zombieReapedPorts.add(attemptedPort); - await terminatePid(staleHolder.pid); - logger.info?.("sync_listener.zombie_reaped", { - port: attemptedPort, - pid: staleHolder.pid, - }); - // Non-preferred candidates occur only once in the normal plan. - // Insert exactly one immediate retry for the newly-freed port. - attemptPlan.splice(attemptIndex + 1, 0, attemptedPort); + zombieReapedPorts.add(attemptedPort); + try { + await terminatePid(staleHolder.pid); + logger.info?.("sync_listener.zombie_reaped", { + port: attemptedPort, + pid: staleHolder.pid, + }); + // Non-preferred candidates occur only once in the normal plan. + // Insert exactly one immediate retry for the newly-freed port. + attemptPlan.splice(attemptIndex + 1, 0, attemptedPort); + } catch (terminateError) { + logger.warn?.("sync_listener.zombie_reap_failed", { + port: attemptedPort, + pid: staleHolder.pid, + error: terminateError instanceof Error ? terminateError.message : String(terminateError), + }); + } }🤖 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 `@apps/ade-cli/src/services/sync/sharedSyncListener.ts` around lines 588 - 617, Guard the await terminatePid call in the stale-holder branch of the sync listener’s bind failure handling so an injected implementation that throws does not escape the surrounding catch flow. Preserve the existing zombie-reaped bookkeeping, logging, and retry behavior only when termination succeeds, and allow termination failures to continue through the normal bind-failure logging/retry path.apps/ade-cli/src/serviceManager/common.ts (1)
228-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
terminatePidGracefullyAsyncduplicatesterminatePidGracefully.Both functions run the identical SIGTERM → poll-until-dead → SIGKILL sequence, differing only in how they wait (
sleepSyncvs. an awaitedsetTimeout). Consider factoring the shared logic into one internal helper parameterized by the wait strategy, so a future fix to the termination sequence doesn't need to be applied in two places.♻️ Suggested consolidation
+async function waitForPidDeath( + pid: number, + pidAlive: (pid: number) => boolean, + deadline: number, + wait: (ms: number) => void | Promise<void>, +): Promise<boolean> { + while (Date.now() < deadline) { + if (!pidAlive(pid)) return true; + await wait(50); + } + return false; +}Both
terminatePidGracefullyandterminatePidGracefullyAsynccould then callwaitForPidDeathwithsleepSync/an awaited-setTimeoutwait function respectively, and share the kill/SIGKILL fallback logic too.🤖 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 `@apps/ade-cli/src/serviceManager/common.ts` around lines 228 - 274, Factor the duplicated termination sequence from terminatePidGracefully and terminatePidGracefullyAsync into one internal helper that accepts the wait strategy. Keep the existing validation, SIGTERM handling, polling deadline, and SIGKILL fallback shared, while having the synchronous function use sleepSync and the async function await its unref’d timer-based wait.
🤖 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 `@apps/ade-cli/src/commands/doctor.ts`:
- Around line 246-253: Update the health-read block around the
Promise.allSettled call in doctor() so a doctorTimeout rejection is converted
into settled rejected results rather than propagating to the outer catch.
Preserve the existing per-call result handling, and ensure an exhausted
remaining deadline reports the initialized brain as reachable but unhealthy
instead of brain.running false or “not responding”.
In `@apps/ade-cli/src/services/account/accountMachinePublisherService.ts`:
- Around line 449-457: Reset publishFailureAnalyticsEmitted whenever the health
failure window is cleared, not only when state is "published". Update the state
handling around failureLegForState so any outcome that leaves
health.failingSinceMs null clears the flag, while preserving the existing
threshold-based emission for active failure windows.
In `@apps/ade-cli/src/services/runtime/brainLoopWatchdog.ts`:
- Around line 195-222: Unify the worker logic in buildWorkerSource() with the
tested evaluateBrainLoopWatchdog() algorithm instead of maintaining a duplicate
slept/blocked calculation. Ensure startBrainLoopWatchdog() passes
checkIntervalMs into workerData, or consistently provide the same default, so
the worker’s sleep-gap threshold and check cadence use the tested configuration.
In `@apps/desktop/src/renderer/components/app/AutoUpdateBanner.tsx`:
- Around line 78-109: Prevent the countdown effect from re-showing the
auto-apply toast while cancellation is in flight. Update handleCancelAutoApply
and the pending countdown effect using a cancellation-in-flight guard, set it
before dismissing the toast, and have renderToast skip showToast while that
guard is active; clear the guard when autoApplyPending becomes false so the
snapshot-confirmed cancellation can complete normally.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/settings/AboutSection.tsx`:
- Line 178: Update the AboutSection update-state logic around updateAvailable so
the “Latest” version text and availability pill use one normalized
latest-version source. Reconcile or normalize getLatestRelease() and the
updater’s latestKnownVersion before deriving either value, ensuring the UI
cannot pair an older displayed version with an “Update available” indicator;
preserve the existing dev-mode behavior where appropriate.
---
Nitpick comments:
In `@apps/ade-cli/src/commands/doctor.ts`:
- Around line 531-537: Update syncPortRow to use the imported
DEFAULT_SYNC_HOST_PORT for the port comparison and the “bound on” detail text,
replacing both hardcoded 8787 references while preserving the existing status
behavior.
In `@apps/ade-cli/src/serviceManager/common.ts`:
- Around line 228-274: Factor the duplicated termination sequence from
terminatePidGracefully and terminatePidGracefullyAsync into one internal helper
that accepts the wait strategy. Keep the existing validation, SIGTERM handling,
polling deadline, and SIGKILL fallback shared, while having the synchronous
function use sleepSync and the async function await its unref’d timer-based
wait.
In `@apps/ade-cli/src/serviceManager/installLaunchd.ts`:
- Around line 36-46: Rename the local probe function currently declared as
probeResponsiveness in the install flow to a distinct name, and update both its
initial invocation and handover-loop call site. Keep deps.probeResponsiveness
unchanged as the boolean opt-out configuration.
In `@apps/ade-cli/src/services/sync/sharedSyncListener.ts`:
- Around line 588-617: Guard the await terminatePid call in the stale-holder
branch of the sync listener’s bind failure handling so an injected
implementation that throws does not escape the surrounding catch flow. Preserve
the existing zombie-reaped bookkeeping, logging, and retry behavior only when
termination succeeds, and allow termination failures to continue through the
normal bind-failure logging/retry path.
🪄 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: 57b18207-016c-4995-b1c5-2ea4f1fbf60e
⛔ Files ignored due to path filters (7)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/desktop-auto-update.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/storage-and-recovery/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/logging.mdis excluded by!docs/**
📒 Files selected for processing (80)
apps/account-directory/wrangler.jsoncapps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/commands/brainUpdate.tsapps/ade-cli/src/commands/doctor.test.tsapps/ade-cli/src/commands/doctor.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/serviceManager/common.test.tsapps/ade-cli/src/serviceManager/common.tsapps/ade-cli/src/serviceManager/index.tsapps/ade-cli/src/serviceManager/installLaunchd.tsapps/ade-cli/src/services/account/accountAuthService.test.tsapps/ade-cli/src/services/account/accountAuthService.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/personalChats/personalChatScope.tsapps/ade-cli/src/services/projects/projectScope.tsapps/ade-cli/src/services/runtime/brainFreshnessMonitor.test.tsapps/ade-cli/src/services/runtime/brainFreshnessMonitor.tsapps/ade-cli/src/services/runtime/brainLogger.test.tsapps/ade-cli/src/services/runtime/brainLogger.tsapps/ade-cli/src/services/runtime/brainLoopWatchdog.test.tsapps/ade-cli/src/services/runtime/brainLoopWatchdog.tsapps/ade-cli/src/services/runtime/runtimeBuildIdentity.tsapps/ade-cli/src/services/sync/abortSignal.tsapps/ade-cli/src/services/sync/sharedSyncListener.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncHostSingleton.tsapps/ade-cli/src/services/sync/syncLoopbackCollision.test.tsapps/ade-cli/src/services/sync/syncPairingConnectInfo.tsapps/ade-cli/src/services/sync/syncProtocol.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/analytics/productAnalyticsPolicy.tsapps/desktop/src/main/services/analytics/productAnalyticsService.test.tsapps/desktop/src/main/services/github/adeReleaseFeed.tsapps/desktop/src/main/services/github/githubService.test.tsapps/desktop/src/main/services/github/githubService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/logging/logger.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.test.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.tsapps/desktop/src/main/services/updates/autoUpdateService.test.tsapps/desktop/src/main/services/updates/autoUpdateService.tsapps/desktop/src/main/services/updates/autoUpdateVersions.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/app/AutoUpdateBanner.test.tsxapps/desktop/src/renderer/components/app/AutoUpdateBanner.tsxapps/desktop/src/renderer/components/app/AutoUpdateControl.test.tsxapps/desktop/src/renderer/components/app/AutoUpdateControl.tsxapps/desktop/src/renderer/components/app/BrainRecoveryNotice.test.tsapps/desktop/src/renderer/components/app/BrainRecoveryNotice.tsxapps/desktop/src/renderer/components/app/useAutoUpdateSnapshot.tsapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.tsxapps/desktop/src/renderer/components/remoteTargets/remoteMachineModel.test.tsapps/desktop/src/renderer/components/remoteTargets/remoteMachineModel.tsapps/desktop/src/renderer/components/settings/AboutSection.tsxapps/desktop/src/renderer/webclient/adapter/app.tsapps/desktop/src/renderer/webclient/adapter/index.tsapps/desktop/src/shared/adeRuntimeProtocol.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/sync/adoptChannelCrypto.test.tsapps/desktop/src/shared/sync/adoptChannelCrypto.tsapps/desktop/src/shared/types/core.tsapps/desktop/src/shared/types/productAnalytics.tsapps/desktop/src/shared/types/sync.tsscripts/posthog/dashboard-spec.mjsscripts/posthog/provision.test.mjs
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…th, review fixes Pins ADE_DEFAULT_ROLE and injects service status in the newer-brain pool tests (Linux CI inherited no role and rejected the fake brain); re-checks activity at the auto-apply deadline; refreshes publishHealth/lastWedge on getInfo instead of caching initialize data; recovery notice subscribes before reading; About separates Downloaded from Installed; doctor degrades slow health reads without reporting the brain down; watchdog SIGKILLs from finally; cancel suppresses countdown ticks until confirmed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
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)
apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts (1)
1686-1694: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNotify status listeners after teardown state is cleared.
Both paths clear
activeClientbefore closing it, so theonDisconnectguard at Line 2214 returns early and subscribers retain stale connected/runtime-health state.
apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts#L1686-L1694: emit a runtime-status update after clearing the current connection.apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts#L2265-L2277: emit the same update after clearing owned-child state.Proposed fix
private clearConnectionIfCurrent(entry: LocalRuntimeConnection): boolean { // ... this.activeRuntimeLastWedge = null; this.projectsByRoot.clear(); + this.emitRuntimeStatusChange(); return true; } const clearCurrentChildState = (): void => { // ... this.projectsByRoot.clear(); if (client) closeRuntimeClient(client); + this.emitRuntimeStatusChange(); };🤖 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 `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts` around lines 1686 - 1694, Emit the runtime-status update after teardown state is cleared in clearConnectionIfCurrent, ensuring subscribers see the disconnected state even when onDisconnect returns early. Apply the same status update after clearing owned-child state in apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts lines 2265-2277; both sites require the change.
🤖 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 `@apps/desktop/src/renderer/components/app/AutoUpdateBanner.tsx`:
- Around line 79-85: Update handleCancelAutoApply to inspect the resolved result
of updateCancelAutoApply() and reset cancelRequestedRef.current when
cancellation is not accepted (false), while preserving the existing reset on
rejection and toast dismissal behavior.
---
Outside diff comments:
In `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts`:
- Around line 1686-1694: Emit the runtime-status update after teardown state is
cleared in clearConnectionIfCurrent, ensuring subscribers see the disconnected
state even when onDisconnect returns early. Apply the same status update after
clearing owned-child state in
apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts lines
2265-2277; both sites require the change.
🪄 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: 42175c97-25bf-498e-8b92-c571fda7f941
📒 Files selected for processing (27)
apps/ade-cli/src/cli.tsapps/ade-cli/src/commands/doctor.test.tsapps/ade-cli/src/commands/doctor.tsapps/ade-cli/src/multiProjectRpcServer.test.tsapps/ade-cli/src/multiProjectRpcServer.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/runtime/brainFreshnessMonitor.test.tsapps/ade-cli/src/services/runtime/brainFreshnessMonitor.tsapps/ade-cli/src/services/runtime/brainLoopWatchdog.test.tsapps/ade-cli/src/services/runtime/brainLoopWatchdog.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/updates/autoUpdateService.test.tsapps/desktop/src/main/services/updates/autoUpdateService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/AutoUpdateBanner.test.tsxapps/desktop/src/renderer/components/app/AutoUpdateBanner.tsxapps/desktop/src/renderer/components/app/BrainRecoveryNotice.test.tsapps/desktop/src/renderer/components/app/BrainRecoveryNotice.tsxapps/desktop/src/renderer/components/settings/AboutSection.test.tsapps/desktop/src/renderer/components/settings/AboutSection.tsxapps/desktop/src/shared/ipc.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- apps/desktop/src/renderer/browserMock.ts
- apps/desktop/src/preload/global.d.ts
- apps/desktop/src/renderer/components/app/BrainRecoveryNotice.test.ts
- apps/desktop/src/renderer/components/app/BrainRecoveryNotice.tsx
- apps/ade-cli/src/services/runtime/brainFreshnessMonitor.test.ts
- apps/ade-cli/src/multiProjectRpcServer.test.ts
- apps/desktop/src/renderer/components/app/AutoUpdateBanner.test.tsx
- apps/ade-cli/src/commands/doctor.test.ts
- apps/ade-cli/src/services/runtime/brainFreshnessMonitor.ts
- apps/desktop/src/renderer/components/settings/AboutSection.tsx
- apps/desktop/src/main/main.ts
- apps/ade-cli/src/services/runtime/brainLoopWatchdog.ts
- apps/ade-cli/src/services/account/accountMachinePublisherService.test.ts
- apps/ade-cli/src/multiProjectRpcServer.ts
- apps/ade-cli/src/cli.ts
- apps/ade-cli/src/services/account/accountMachinePublisherService.ts
- apps/desktop/src/main/services/updates/autoUpdateService.test.ts
- apps/desktop/src/main/services/updates/autoUpdateService.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a05b4e508
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } finally { | ||
| if (timeout) clearTimeout(timeout); |
There was a problem hiding this comment.
When the directory returns response headers but then stalls its body, this finally clears the only request timer before the later readAccountDirectoryHttpReason(response) calls finish. The publisher's inFlight promise then remains pending, so the heartbeat and relay-state schedulers keep coalescing onto it and the machine ages out of the directory's online window until the brain is restarted. Keep the controller's timeout active through body consumption, or independently bound and abort those reads.
Useful? React with 👍 / 👎.
| token = await postTokenForm({ | ||
| fetchImpl, | ||
| tokenUrl: `${config.issuer}/oauth/token`, | ||
| signal, | ||
| body: { |
There was a problem hiding this comment.
When two callers overlap and the first caller has a short timeout or abort signal, this signal is captured by the process-wide refreshInFlight operation that every caller awaits. Aborting the publisher's request therefore cancels the shared OAuth exchange and rejects unrelated desktop or CLI token requests that never asked to cancel; the environment-token refresh path has the same coupling. Run the coalesced refresh under service-owned cancellation and race each caller against its own signal instead.
Useful? React with 👍 / 👎.
…olate shared token refresh BrainRecoveryNotice tolerates hosts without onRuntimeStatusChanged; a declined auto-apply cancel restores the countdown toast; publish response bodies are read under a 5s bound with stream cancel so a stalled body can't pin the heartbeat; both coalesced token refreshes run under service-owned cancellation so one caller's abort no longer rejects every other caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05cd6fcad0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!pidAlive(pid)) return; | ||
| await new Promise<void>((resolve) => { | ||
| const timer = setTimeout(resolve, 50); | ||
| timer.unref?.(); |
There was a problem hiding this comment.
When ade serve --install-service runs as a standalone CLI and the old brain survives SIGTERM, this awaited delay can be the only remaining referenced work; because the timer is unreferenced, Node may exit immediately before the grace loop reaches SIGKILL and before installLaunchdService reaches launchctl load. This can leave the launch agent unloaded precisely while repairing a wedged brain, so awaited lifecycle timers—including the handover polling delay—must remain referenced.
AGENTS.md reference: AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
| "The account token request was aborted.", | ||
| ); | ||
| } finally { | ||
| if (envRefreshInFlight === refreshPromise) envRefreshInFlight = null; |
There was a problem hiding this comment.
When a caller times out or is disposed while an ADE_ACCOUNT_TOKEN refresh is still running, this caller-level finally clears envRefreshInFlight before the shared exchange settles. A subsequent caller can then launch a second OAuth exchange with the same rotating refresh token, causing one request to consume the token while the other fails or discards its result; clear the field only from the shared refresh promise's own settlement handler.
Useful? React with 👍 / 👎.
| token = await postTokenForm({ | ||
| fetchImpl, | ||
| tokenUrl: `${config.issuer}/oauth/token`, | ||
| signal, |
There was a problem hiding this comment.
When the first participant in a coalesced stored-session refresh times out or aborts while another caller is waiting, passing that caller's signal into postTokenForm aborts the common HTTP request despite the service-owned sharedSignal. All joiners then lose the refresh, and a token endpoint taking longer than the publisher's 10-second budget can never benefit from the intended 30-second shared window; use sharedSignal for the exchange.
Useful? React with 👍 / 👎.
…change, durable env single-flight Follow-ups to #889 from the final review round: awaited termination/handover timers stay referenced so a standalone CLI can't exit mid-repair; the session token exchange runs on the service-owned shared signal as intended; the env refresh single-flight is released by the shared exchange settling, not by an aborting caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…change, durable env single-flight Follow-ups to #889 from the final review round: awaited termination/handover timers stay referenced so a standalone CLI can't exit mid-repair; the session token exchange runs on the service-owned shared signal as intended; the env refresh single-flight is released by the shared exchange settling, not by an aborting caller. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…change, durable env single-flight (#891) Follow-ups to #889 from the final review round: awaited termination/handover timers stay referenced so a standalone CLI can't exit mid-repair; the session token exchange runs on the service-owned shared signal as intended; the env refresh single-flight is released by the shared exchange settling, not by an aborting caller. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary by CodeRabbit
ade doctorwith structured output, table-style health rows, and clearer local vs--onlinebehavior.ade doctordocumentation with the new--online --textexample and check list.Greptile Summary
This large reliability PR adds four major improvements: a worker-thread event-loop watchdog that kills the process on confirmed wedge, a brain-freshness monitor that restarts stale builds, per-leg publish telemetry with timeout isolation for the account-directory publisher, and a negotiated AEAD cipher for the account-adoption channel. It also introduces idle-based auto-apply for desktop updates and propagates AbortSignal through all read-heavy sync remote commands.
process.kill(SIGKILL)in afinallyblock, so the kill fires even if the breadcrumb write fails.adoptChannelCryptoadds AES-256-GCM as a runtime-detected fallback; backward-compat is preserved by only includingaeadin the signature input when negotiated.postTokenForminside the sharedrefreshInFlightclosure receives the first caller'ssignalinstead of the service-ownedsharedSignal, breaking abort isolation for all co-waiting callers.Confidence Score: 4/5
Safe to merge with one fix in accountAuthService.ts where the shared token-refresh HTTP request can be cancelled by a single caller's abort signal.
The watchdog, AEAD, and abort-propagation work are well-structured and correctly isolated. One concrete defect: the first caller's AbortSignal is passed to postTokenForm inside the shared refreshInFlight closure, so if that caller times out the HTTP exchange is cancelled for every co-waiting caller.
apps/ade-cli/src/services/account/accountAuthService.ts — the postTokenForm call inside refreshInFlight should use sharedSignal, not the caller-captured signal variable.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Publisher as AccountMachinePublisher participant Auth as AccountAuthService participant Dir as AccountDirectory Publisher->>Auth: getAccessToken(signal, timeoutMs) Note over Auth: shared refreshInFlight coalesces callers Auth-->>Publisher: accessToken Publisher->>Dir: sendRegistration(token) [http leg] Dir-->>Publisher: 401 Unauthorized Publisher->>Auth: getAccessToken(forceRefresh, signal) Auth-->>Publisher: refreshed token Publisher->>Dir: sendRegistration(refreshedToken) Dir-->>Publisher: 200 OK Publisher->>Publisher: recordOutcome(published, legDurations)Comments Outside Diff (1)
apps/ade-cli/src/services/account/accountAuthService.ts, line 1879-1888 (link)postTokenFormhere receivessignal, the lexical variable captured from the first caller's invocation ofgetAccessTokenWithSignal. The comment directly above states "a single caller's abort must not cancel it for the rest. It runs under service-owned cancellation." But if the first caller's signal is aborted (e.g., because the publisher's per-call timeout fires), the HTTP request is cancelled and therefreshInFlightpromise rejects for every other caller waiting on it.The env-refresh path correctly uses
envSharedSignalin the equivalent call; this branch should usesharedSignalfor the same reason.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "ship: guard runtime-status subscription,..." | Re-trigger Greptile