Harden chat delivery and machine connections#904
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
arul28
changed the title
📝 WalkthroughWalkthroughThe change adds provider-neutral chat recovery, durable unprocessed-message resolution, expanded diagnostics and lifecycle rendering, correlation IDs for directory and relay flows, improved route ordering and failure reporting, preserved direct trust after sign-out, and updated search across desktop, iOS, CLI, and relay clients. ChangesCore runtime behavior
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/services/search/searchService.ts (1)
205-233: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSuppress queued steers from chat FTS.
user_messagecan also be emitted withdeliveryState: "queued"when a steer is staged, sochatEventSearchText()should also returnnullfor queued events. Otherwise the queued text creates a separate FTS row instead of being hidden until the steer is accepted/processed/unprocessed.🤖 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/search/searchService.ts` around lines 205 - 233, Update chatEventSearchText for user_message events to return null when deliveryState is "queued", alongside the existing processed and unprocessed states. Preserve searchable text extraction for accepted messages and all other event types.apps/desktop/src/main/services/ipc/registerIpc.ts (1)
6762-6784: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate project/session ownership in the recovery IPC path.
ensureManagedSession()validates that the session exists and is an agent chat session, but it does not enforce that the requesting project/runtime owns that session. These handlers currently pass renderer-suppliedsessionId,turnId, andsteerIdthrough to recovery methods that mutate chat state and dispatch turns; add IPC/runtime-level ownership checks for malformed inputs and cross-session/chat requests, plus IPC tests that cover them.🤖 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/ipc/registerIpc.ts` around lines 6762 - 6784, The recovery IPC handlers agentChatRecoverTurn, agentChatRecoverCodexTurn, and agentChatResolveUnprocessedMessage must validate renderer-supplied sessionId, turnId, and steerId before invoking the service. Reuse the runtime’s project/session ownership validation, reject malformed identifiers and cross-project or cross-chat requests, and preserve valid recovery behavior; add IPC tests covering malformed, cross-session, and authorized requests.Source: Path instructions
🧹 Nitpick comments (10)
apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx (1)
1182-1186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a named query over bare
getByRole("button").This throws as soon as any other button renders in the diagnostics row;
getByRole("button", { name: /Turn details/ })keeps the intent explicit and the failure message useful.🤖 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/chat/AgentChatMessageList.test.tsx` around lines 1182 - 1186, Update the button lookup in the test around the Turn details assertions to use a named role query matching /Turn details/ instead of bare getByRole("button"), so the click targets the intended diagnostics control.apps/ios/ADE/Services/SyncService.swift (1)
11506-11528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDirect-route failure is discarded when the relay fallback also fails.
The caught
errorfrom the direct race is only used for the guard; if the relay race then throws, users see the relay error and the (usually more diagnostic) direct failure disappears from both the thrown error and the log. Consider logging the direct error alongside the fallback notice.♻️ Log the direct failure before falling back
syncConnectLog.notice( - "ADE_SYNC_TRACE direct routes exhausted; beginning authenticated relay fallback" + "ADE_SYNC_TRACE direct routes exhausted (\(error.localizedDescription, privacy: .public)); beginning authenticated relay fallback" )🤖 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/ios/ADE/Services/SyncService.swift` around lines 11506 - 11528, In the catch block around raceAuthenticatedConnectionCandidates for directRacePlan, log the caught direct-route error before starting the authenticated relay fallback, alongside the existing fallback notice. Preserve the current guard and relay behavior while ensuring the direct failure details remain available if the relay race also throws.apps/desktop/src/renderer/webclient/sync/connection.ts (1)
1019-1031: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the
rankmap out of the comparator.
rankis re-allocated on every comparison. Move it to module scope (or just above the sort) — the same table already exists inendpoints.tsLine 170, so exporting one shared constant would also keep the two orderings in sync.🤖 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/webclient/sync/connection.ts` around lines 1019 - 1031, Move the `rank` ordering map out of the sort comparator in the candidate-sorting flow, preferably reusing a shared exported constant from `endpoints.ts` if appropriate. Update the comparator to reference that constant while preserving the existing lan, tailnet, and relay ordering.apps/desktop/src/main/services/remoteRuntime/pairedRuntimeBootstrap.ts (1)
128-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
attemptsandrouteErrorsshare one budget with different fill rates.
recordAttemptcounts"skipped"relay-proof entries, whileaddRouteErroronly counts dial failures. On a machine with many saved endpoints the attempt list can be exhausted by skips before the route that actually failed is recorded, leaving the diagnostic without the interesting entry. Consider reserving the last slot (or tracking anomittedAttemptCountalongsideomittedRouteErrorCount) so truncation is visible to the consumer.🤖 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/remoteRuntime/pairedRuntimeBootstrap.ts` around lines 128 - 133, Update recordAttempt and the related routeErrors truncation accounting so skipped relay-proof entries cannot consume all diagnostic capacity before dial failures are recorded. Reserve capacity for meaningful route errors or track omitted attempts alongside omittedRouteErrorCount, and expose the resulting truncation through the existing diagnostic output.apps/ade-cli/src/tuiClient/pairedRemoteConnector.ts (1)
171-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate of the desktop bootstrap's attempt-tracking scaffolding.
openPairedCandidatenow mirrorsbootstrapPairedRuntime(correlation ID, boundedrecordAttempt, non-relay-before-relay ordering) with a hardcoded8instead of theMAX_ROUTE_ATTEMPTSconstant. Since this file already imports from../../../desktop/src/main/services/remoteRuntime/pairedRuntimeRoutes, consider exportingMAX_ROUTE_ATTEMPTSplus a smallcreateRouteAttemptRecorder()/orderPairedCandidates()helper there and reusing them, so the bound and ordering policy can't drift between the CLI and desktop.🤖 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/tuiClient/pairedRemoteConnector.ts` around lines 171 - 181, Extract the shared attempt-tracking and candidate-ordering logic from the duplicated scaffolding in openPairedCandidate and bootstrapPairedRuntime into pairedRuntimeRoutes. Export MAX_ROUTE_ATTEMPTS plus focused createRouteAttemptRecorder and orderPairedCandidates helpers, then update both callers to reuse them instead of hardcoding 8 or maintaining separate non-relay-before-relay ordering logic.apps/ade-cli/src/cli.ts (1)
2712-2767: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated legacy-recovery mapping and unsupported-error predicate.
LEGACY_CODEX_RECOVERY_ACTION_BY_NEUTRAL/isUnsupportedChatRecoveryActionErrorhere duplicateLEGACY_RECOVERY_ACTION_BY_NEUTRAL/isUnsupportedRecoveryActionErrorinapps/ade-cli/src/tuiClient/adeApi.ts. Two copies of the fallback heuristic will drift, andadeApi.test.tsalready pins specific message shapes only for its copy. Consider exporting one shared helper module and having both call sites use 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 `@apps/ade-cli/src/cli.ts` around lines 2712 - 2767, The legacy recovery action mapping and unsupported-error predicate are duplicated between this CLI code and the helpers in adeApi.ts. Export the shared mapping and isUnsupportedRecoveryActionError helper from adeApi.ts (or a common module), remove LEGACY_CODEX_RECOVERY_ACTION_BY_NEUTRAL and isUnsupportedChatRecoveryActionError here, and update this call site to use the shared symbols while preserving the existing behavior.apps/ade-cli/src/services/sync/syncRemoteCommandService.ts (1)
2315-2330: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse the shared recovery action enum for runtime validation.
apps/ade-cli/src/services/sync/syncRemoteCommandService.tscurrently maintains its own literal list at line 2318, whileAgentChatTurnRecoveryActionis defined separately inapps/desktop/src/shared/types/chat.ts. Reuse the shared literals or an exported guard from the shared types module so a future change to the recovery union is caught as a type error instead of silently rejecting valid remote calls.🤖 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/syncRemoteCommandService.ts` around lines 2315 - 2330, Update parseAgentChatRecoverTurnArgs to validate action using the shared AgentChatTurnRecoveryAction literals or exported guard from the shared chat types module instead of maintaining a local string list. Keep the existing unsupported-action error behavior and ensure future changes to the shared recovery union are reflected automatically.apps/ade-cli/src/tuiClient/app.tsx (1)
1753-1757: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the identical
sourceSessionIdternary arms.Both
turn_healthandcodex_turn_stalledresolve the target session the same way, so the branch at Line 1753-1757 and the one at Line 1779-1781 evaluate to identical expressions. A singlematchingEnvelope?.event.sourceSessionId?.trim() || matchingEnvelope?.sessionId || args.sessionIdreads clearer and avoids future drift.♻️ Proposed simplification (explicit-turn branch)
- const targetSessionId = matchingEnvelope?.event.type === "codex_turn_stalled" - ? matchingEnvelope.event.sourceSessionId?.trim() || matchingEnvelope.sessionId - : matchingEnvelope?.event.type === "turn_health" - ? matchingEnvelope.event.sourceSessionId?.trim() || matchingEnvelope.sessionId - : args.sessionId; + const matchedSourceSessionId = matchingEnvelope + && (matchingEnvelope.event.type === "turn_health" || matchingEnvelope.event.type === "codex_turn_stalled") + ? matchingEnvelope.event.sourceSessionId?.trim() || matchingEnvelope.sessionId + : null; + const targetSessionId = matchedSourceSessionId ?? args.sessionId;Also applies to: 1779-1781
🤖 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/tuiClient/app.tsx` around lines 1753 - 1757, Update the target session ID selection around matchingEnvelope so both turn_health and codex_turn_stalled use one shared fallback expression: trimmed sourceSessionId, then matchingEnvelope.sessionId, then args.sessionId. Remove the duplicate event-type ternary branches while preserving the existing explicit-turn fallback behavior.apps/desktop/src/main/services/chat/agentChatService.ts (1)
25827-25832: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider not blocking Codex runtime startup on the transcript scan.
hydrateAcceptedCodexSteersreads up to 8 MB of transcript history synchronously with respect tostartCodexRuntime, adding latency to every cold start even for sessions with no outstanding steers. If the ordering guarantee (hydrate before the first steer can be accepted) can be expressed as a promise the steer path awaits, the runtime handshake could return sooner.🤖 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/chat/agentChatService.ts` around lines 25827 - 25832, Decouple hydrateAcceptedCodexSteers from the synchronous startCodexRuntime startup path so the runtime handshake returns without waiting for the transcript scan. Expose or reuse a promise representing hydration completion, and make the Codex steer acceptance path await it before accepting the first steer, preserving the hydrate-before-accept ordering guarantee.apps/desktop/src/renderer/components/chat/AgentChatPane.test.tsx (1)
495-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
resolveUnprocessedMessageecho the requested action.The mock always resolves
action: "run_next", so the dismiss test would still pass if the component mishandled the result. Echoing the input keeps the assertions honest.♻️ Proposed change
- const resolveUnprocessedMessage = vi.fn().mockResolvedValue({ - steerId: "steer-unprocessed", - action: "run_next", - status: "completed", - replacementMessageId: "message-next", - }); + const resolveUnprocessedMessage = vi.fn().mockImplementation( + async ({ steerId, action }: { steerId: string; action: "run_next" | "dismiss" }) => ({ + steerId, + action, + status: "completed", + ...(action === "run_next" ? { replacementMessageId: "message-next" } : {}), + }), + );🤖 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/chat/AgentChatPane.test.tsx` around lines 495 - 500, Update the resolveUnprocessedMessage mock in AgentChatPane tests to return the action provided in its request rather than always returning "run_next". Preserve the existing response fields while ensuring dismiss requests resolve with their requested action so assertions validate component behavior.
🤖 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/account-directory/src/directory.ts`:
- Around line 583-608: Update the OPTIONS handling around the directory request
route to validate the preflight method based on the matched route: retain GET
for list routes and permit DELETE for the machine-removal route, then emit the
corresponding route-aware access-control-allow-methods value. Add a
hosted-origin DELETE preflight test in
apps/account-directory/test/directory.test.ts covering authorization and
x-ade-correlation-id; update apps/account-directory/src/directory.ts
accordingly.
In `@apps/ade-cli/src/cli.ts`:
- Around line 1814-1817: Update the Actions help text in the CLI usage output to
include the accepted wait action, alongside nudge, retry, and resume, matching
normalizeChatRecoveryCliAction and its validation message.
In `@apps/ade-cli/src/tuiClient/format.ts`:
- Around line 842-856: Update the event pre-scan and rendering logic in the
formatter to track latestHealthIndexByTurn for turn_health events, matching the
existing latest-index maps for turn_diagnostics, turn_recovery, and
codex_turn_recovery. Render a turn_health block only when its index is the
latest recorded for that turn, while preserving the recovered-state skip
behavior.
In `@apps/ade-cli/src/tuiClient/pairedRemoteConnector.ts`:
- Around line 231-241: Update the PairedRuntimeRelayAuthRequiredError handling
in the paired connection attempt flow to record outcome "failed" instead of
"skipped", matching pairedRuntimeBootstrap.ts for post-dial authentication
rejections; preserve the existing authentication failure field and retry control
flow.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 24246-24252: Clear the codexAutomaticRecoveryAttemptedBySession
entry during session disposal and deletion, alongside the existing per-session
cleanup in deleteSession and forceDisposeManagedSession. Ensure both normal and
forced disposal remove the managed session ID so recreated sessions can perform
automatic recovery and the map does not grow indefinitely.
- Around line 24507-24514: Update the auto-resolution path in
autoApproveCodexRuntimeApproval so that after emitPendingInputResolved resolves
a Codex approval, it also resumes the suspended turn watchdog using the existing
resumeCodexTurnWatchdogAfterInput flow. Preserve the current behavior for
approvals requiring user input and ensure the watchdog is re-armed when
auto-approval leaves no pending local inputs.
In `@apps/desktop/src/main/services/remoteRuntime/pairedRuntimeBootstrap.test.ts`:
- Around line 340-358: The paired runtime bootstrap test does not verify each
attempt’s classified failure. Update the `unavailable.diagnostic.attempts`
expectation for the LAN, tailnet, and relay entries to assert the expected
`failure` value, exposing the current authentication-versus-unreachable
classification mismatch from `classifyPairedRuntimeFailure`.
In `@apps/desktop/src/main/services/remoteRuntime/pairedRuntimeRoutes.ts`:
- Around line 138-159: In
apps/desktop/src/main/services/remoteRuntime/pairedRuntimeRoutes.ts lines
138-159, update classifyPairedRuntimeFailure so the transport/unreachable
pattern is evaluated before authentication keywords, and add word boundaries for
auth, token, and proof. In
apps/desktop/src/main/services/remoteRuntime/pairedRuntimeBootstrap.test.ts
lines 340-358, include failure: "unreachable" in each expected attempt object so
the classification contract is asserted.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 241-288: Update the primary recovery buttons, secondary recovery
buttons, and the “More” toggle in the AgentChatMessageList action controls to
include the same visible focus-visible outline/ring styling used by nearby
interactive elements. Preserve their existing hover, disabled, and layout
classes while ensuring keyboard focus is clearly indicated on all three button
types.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 9609-9613: Centralize the legacy Codex recovery detection by
extracting LEGACY_CODEX_RECOVERY_ACTION_BY_NEUTRAL and
isUnsupportedChatRecoveryActionError from apps/ade-cli/src/cli.ts (lines
2712-2767) into a shared module, then import and use them from AgentChatPane.tsx
(lines 9609-9613) and apps/ade-cli/src/tuiClient/adeApi.ts. Ensure the shared
predicate recognizes recoverTurn-specific unsupported-action errors and the
legacy message forms such as “Action not supported” and “Unknown action,” while
not treating generic service-unavailable errors as legacy recovery failures.
- Around line 9651-9656: Update handleEditUnprocessedMessage to preserve the
existing composer draft by appending the edited message text using the same
behavior as the onEditSteer handler. Reuse the established draft-combination
logic and keep the existing displayText/text fallback.
In `@apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts`:
- Around line 1412-1429: Update the turn_recovery translation in the event
handling branch to preserve the neutral event’s actual recovery action when
constructing legacyRecovery, rather than hard-coding "restart_resume_thread". If
turn_recovery does not expose an action, omit the legacy action field instead of
inventing a value.
- Around line 1357-1379: The user_message_resolution branch currently drops
resolutions when no matching row exists. Update the surrounding transcript
context and user_message append/merge path to stash unmatched resolutions keyed
by steerId, then apply the stored resolution metadata when the matching row is
later created; preserve the existing immediate-update behavior for rows found by
userMessageRowIndexBySteer.
- Around line 1290-1302: Update the metadata merge in the existing-row update
logic to preserve existing metadata fields while applying later event metadata,
especially existing.event.metadata.unprocessedMessageResolution. Ensure
subsequent user_message envelopes with unrelated metadata such as scheduledWake
retain the durable resolution marker used by the user_message_resolution branch
and AgentChatMessageList state.
In `@apps/ios/ADE/Models/RemoteModels.swift`:
- Around line 2717-2751: Update the turnHealth and turnRecovery decoding
branches in the event initializer to use decodeIfPresent for non-essential
fields that may be omitted by other clients. Default recoveryCount to 0,
supportedActions to an empty array, and automatic to false, while preserving
strict decoding for required event fields.
In `@apps/ios/ADE/Views/Work/WorkChatHeaderAndMessageViews.swift`:
- Around line 497-505: Update the unprocessed-message action rendering around
WorkUnprocessedMessageActions to require a non-empty steerId before showing Run
next and Dismiss actions, while keeping Edit available for messages without
steerId. Preserve the existing deliveryState == "unprocessed" condition and
local edit behavior.
In `@apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift`:
- Around line 1865-1888: Update the turnDiagnostics and codexTurnRecovery cases
in the merge-key helper to use only their turnId as the per-turn identity, while
retaining each event’s distinct type prefix and any required provider
distinction. Remove moderationChecks, failure details, receipt.state, and
receipt.at from the merge keys so updated payloads overwrite the existing
diagnostics or recovery row instead of creating additional timeline rows.
- Around line 38-58: Replace the sortedWorkChatEnvelopes(transcript) iteration
used to build resolutionBySteerId with a single pass over transcript. Track the
newest resolution per normalized steerId using the event’s timestamp and
sequence as tie-breakers, so the latest (timestamp, sequence) entry wins; leave
the separate assembly loop’s ordering behavior unchanged.
In `@apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift`:
- Around line 2545-2561: Update normalizedWorkIntegrationFailures to preserve
the first non-empty message for each integration when deduplicating failures.
When a later failure has no message, retain the existing stored message;
continue normalizing integration names and sorting the resulting values as
currently implemented.
In `@apps/ios/ADE/Views/Work/WorkTranscriptParser.swift`:
- Around line 546-558: Update the default action in the turn_recovery case when
constructing WorkCodexRecoveryReceipt so omitted action values use the
normalized "restart_resume_thread" identifier, matching the legacy
codex_turn_recovery behavior; leave the other receipt defaults unchanged.
---
Outside diff comments:
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 6762-6784: The recovery IPC handlers agentChatRecoverTurn,
agentChatRecoverCodexTurn, and agentChatResolveUnprocessedMessage must validate
renderer-supplied sessionId, turnId, and steerId before invoking the service.
Reuse the runtime’s project/session ownership validation, reject malformed
identifiers and cross-project or cross-chat requests, and preserve valid
recovery behavior; add IPC tests covering malformed, cross-session, and
authorized requests.
In `@apps/desktop/src/main/services/search/searchService.ts`:
- Around line 205-233: Update chatEventSearchText for user_message events to
return null when deliveryState is "queued", alongside the existing processed and
unprocessed states. Preserve searchable text extraction for accepted messages
and all other event types.
---
Nitpick comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 2712-2767: The legacy recovery action mapping and
unsupported-error predicate are duplicated between this CLI code and the helpers
in adeApi.ts. Export the shared mapping and isUnsupportedRecoveryActionError
helper from adeApi.ts (or a common module), remove
LEGACY_CODEX_RECOVERY_ACTION_BY_NEUTRAL and isUnsupportedChatRecoveryActionError
here, and update this call site to use the shared symbols while preserving the
existing behavior.
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 2315-2330: Update parseAgentChatRecoverTurnArgs to validate action
using the shared AgentChatTurnRecoveryAction literals or exported guard from the
shared chat types module instead of maintaining a local string list. Keep the
existing unsupported-action error behavior and ensure future changes to the
shared recovery union are reflected automatically.
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 1753-1757: Update the target session ID selection around
matchingEnvelope so both turn_health and codex_turn_stalled use one shared
fallback expression: trimmed sourceSessionId, then matchingEnvelope.sessionId,
then args.sessionId. Remove the duplicate event-type ternary branches while
preserving the existing explicit-turn fallback behavior.
In `@apps/ade-cli/src/tuiClient/pairedRemoteConnector.ts`:
- Around line 171-181: Extract the shared attempt-tracking and
candidate-ordering logic from the duplicated scaffolding in openPairedCandidate
and bootstrapPairedRuntime into pairedRuntimeRoutes. Export MAX_ROUTE_ATTEMPTS
plus focused createRouteAttemptRecorder and orderPairedCandidates helpers, then
update both callers to reuse them instead of hardcoding 8 or maintaining
separate non-relay-before-relay ordering logic.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 25827-25832: Decouple hydrateAcceptedCodexSteers from the
synchronous startCodexRuntime startup path so the runtime handshake returns
without waiting for the transcript scan. Expose or reuse a promise representing
hydration completion, and make the Codex steer acceptance path await it before
accepting the first steer, preserving the hydrate-before-accept ordering
guarantee.
In `@apps/desktop/src/main/services/remoteRuntime/pairedRuntimeBootstrap.ts`:
- Around line 128-133: Update recordAttempt and the related routeErrors
truncation accounting so skipped relay-proof entries cannot consume all
diagnostic capacity before dial failures are recorded. Reserve capacity for
meaningful route errors or track omitted attempts alongside
omittedRouteErrorCount, and expose the resulting truncation through the existing
diagnostic output.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsx`:
- Around line 1182-1186: Update the button lookup in the test around the Turn
details assertions to use a named role query matching /Turn details/ instead of
bare getByRole("button"), so the click targets the intended diagnostics control.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.test.tsx`:
- Around line 495-500: Update the resolveUnprocessedMessage mock in
AgentChatPane tests to return the action provided in its request rather than
always returning "run_next". Preserve the existing response fields while
ensuring dismiss requests resolve with their requested action so assertions
validate component behavior.
In `@apps/desktop/src/renderer/webclient/sync/connection.ts`:
- Around line 1019-1031: Move the `rank` ordering map out of the sort comparator
in the candidate-sorting flow, preferably reusing a shared exported constant
from `endpoints.ts` if appropriate. Update the comparator to reference that
constant while preserving the existing lan, tailnet, and relay ordering.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 11506-11528: In the catch block around
raceAuthenticatedConnectionCandidates for directRacePlan, log the caught
direct-route error before starting the authenticated relay fallback, alongside
the existing fallback notice. Preserve the current guard and relay behavior
while ensuring the direct failure details remain available if the relay race
also throws.
🪄 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: 932e435c-d114-49a4-9873-678e52702db2
⛔ Files ignored due to path filters (7)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/search/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/ios-companion.mdis excluded by!docs/**docs/features/sync-and-multi-device/remote-commands.mdis excluded by!docs/**docs/features/web-client/README.mdis excluded by!docs/**
📒 Files selected for processing (93)
apps/account-directory/src/directory.tsapps/account-directory/test/directory.test.tsapps/ade-cli/README.mdapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.test.tsapps/ade-cli/src/services/account/accountMachineDirectoryService.tsapps/ade-cli/src/services/account/accountMachinePublisherService.test.tsapps/ade-cli/src/services/account/accountMachinePublisherService.tsapps/ade-cli/src/services/personalChats/personalChatScope.test.tsapps/ade-cli/src/services/personalChats/personalChatScope.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/ChatView.test.tsxapps/ade-cli/src/tuiClient/__tests__/adeApi.test.tsapps/ade-cli/src/tuiClient/__tests__/aggregate.test.tsapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/commands.test.tsapps/ade-cli/src/tuiClient/__tests__/format.test.tsapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/aggregate.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/commands.tsapps/ade-cli/src/tuiClient/format.tsapps/ade-cli/src/tuiClient/pairedRemoteConnector.tsapps/ade-cli/src/tuiClient/remoteLauncher.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeBootstrap.test.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeBootstrap.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeErrors.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeRoutes.test.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeRoutes.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionService.test.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionService.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.test.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.tsapps/desktop/src/main/services/remoteRuntime/syncRuntimeTransport.test.tsapps/desktop/src/main/services/remoteRuntime/syncRuntimeTransport.tsapps/desktop/src/main/services/search/searchService.test.tsapps/desktop/src/main/services/search/searchService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/chat/chatTranscriptRows.tsapps/desktop/src/renderer/components/remoteTargets/RemoteTargetList.test.tsxapps/desktop/src/renderer/components/remoteTargets/SavedMachineRow.tsxapps/desktop/src/renderer/webclient/account/client.tsapps/desktop/src/renderer/webclient/account/leaseMonitor.tsapps/desktop/src/renderer/webclient/shell/MachinePicker.tsxapps/desktop/src/renderer/webclient/shell/WebClientRoot.tsxapps/desktop/src/renderer/webclient/shell/__tests__/MachinePicker.test.tsxapps/desktop/src/renderer/webclient/sync/__tests__/sync.test.tsapps/desktop/src/renderer/webclient/sync/connection.tsapps/desktop/src/renderer/webclient/sync/endpoints.tsapps/desktop/src/renderer/webclient/sync/envStore.test.tsapps/desktop/src/renderer/webclient/sync/envStore.tsapps/desktop/src/shared/accountDirectory.test.tsapps/desktop/src/shared/accountDirectory.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/pairedRuntime.tsapps/desktop/src/shared/types/personalChats.tsapps/desktop/src/shared/types/remoteRuntime.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/AccountDirectory.swiftapps/ios/ADE/Services/SyncConnectionRace.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkActivityIndicator.swiftapps/ios/ADE/Views/Work/WorkChatHeaderAndMessageViews.swiftapps/ios/ADE/Views/Work/WorkChatRichCardViews.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+Timeline.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkEventMapping.swiftapps/ios/ADE/Views/Work/WorkModels.swiftapps/ios/ADE/Views/Work/WorkRootScreen.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView+Actions.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADE/Views/Work/WorkTranscriptParser.swiftapps/ios/ADETests/ADETests.swiftapps/tunnel-relay/src/tunnelDo.tsapps/tunnel-relay/test/relay.test.ts
| nudge Ask the current provider turn for a short status update. | ||
| retry Interrupt, then retry on the same provider runtime. | ||
| resume Restart the provider runtime, resume its thread, then retry. | ||
| `, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Help omits the wait action that the validator accepts.
normalizeChatRecoveryCliAction accepts wait and its error message lists it ("must be wait, nudge, retry, or resume"), but the Actions block documents only nudge/retry/resume.
📝 Proposed fix
+ wait Keep waiting for the current provider turn.
nudge Ask the current provider turn for a short status update.
retry Interrupt, then retry on the same provider runtime.
resume Restart the provider runtime, resume its thread, then retry.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| nudge Ask the current provider turn for a short status update. | |
| retry Interrupt, then retry on the same provider runtime. | |
| resume Restart the provider runtime, resume its thread, then retry. | |
| `, | |
| wait Keep waiting for the current provider turn. | |
| nudge Ask the current provider turn for a short status update. | |
| retry Interrupt, then retry on the same provider runtime. | |
| resume Restart the provider runtime, resume its thread, then retry. | |
| `, |
🤖 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/cli.ts` around lines 1814 - 1817, Update the Actions help
text in the CLI usage output to include the accepted wait action, alongside
nudge, retry, and resume, matching normalizeChatRecoveryCliAction and its
validation message.
|
@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: 2400b8b539
ℹ️ 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 (!dispatchReceiptWritten) { | ||
| emitChatEvent(managed, { | ||
| type: "user_message_resolution", | ||
| steerId, | ||
| action: "run_next", | ||
| state: "completed", | ||
| resolvedAt: nowIso(), | ||
| replacementMessageId: dispatchedReplacementMessageId, |
There was a problem hiding this comment.
When disk pressure blocks chat_turn, sendMessage returns normally before creating or invoking the backend-dispatch callback. This fallback then writes a terminal run_next resolution anyway, so the original unprocessed message disappears as completed even though no replacement turn was sent, and subsequent retries return already_completed. Only persist the resolution after the callback confirms dispatch; a no-dispatch return must remain retryable.
Useful? React with 👍 / 👎.
| if ( | ||
| pendingReplacementMessageId | ||
| && event.type === "status" | ||
| && event.turnStatus === "started" | ||
| && event.turnId?.trim() | ||
| ) { | ||
| replacementMessageId = pendingReplacementMessageId; | ||
| pendingReplacementMessageId = undefined; |
There was a problem hiding this comment.
After a crash during Run next for Cursor or Droid, this scan can falsely mark the replay as dispatched: sendMessage persists the replay-tagged user message and a turn-id-bearing status: started optimistically before executePreparedSendMessage reaches the provider. If the process stops in that window, the next retry treats these local events as the commit point, writes an already_completed resolution, and never sends the message. Recovery should rely on a durable backend-dispatch acknowledgement rather than the optimistic started status.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
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/chat/agentChatService.ts (1)
23239-23261: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDisambiguate
turn/steercorrelation instead of using arbitrary fallback matching.
markAcceptedCodexSteerProcessedmatches the CodexuserMessageitem only by text/suffix and falls back toaccepted.values().next().valuewhen no candidate matches. A Codex request can send a combinedinputarray (context +submitSteertext), while each accepted steer is keyed by itsvisibleText. With multiple accepted steers on the same turn, the fallback can mark the wrong steer"processed"/delete it and then havemarkAcceptedCodexSteersUnprocessedemit"unprocessed"for the steer that was actually processed. Track the mapped text marker or strict send/ack order instead of using an arbitrary insertion-order match.🤖 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/chat/agentChatService.ts` around lines 23239 - 23261, Update markAcceptedCodexSteerProcessed to correlate each Codex userMessage with the accepted steer’s mapped text marker or strict send/ack order, rather than matching loosely by text and falling back to accepted.values().next().value. Ensure combined input arrays containing context and submitSteer text identify the correct steer, and only remove/emit “processed” for that matched steer so later markAcceptedCodexSteersUnprocessed calls cannot target the wrong entry.
🧹 Nitpick comments (1)
apps/ade-cli/src/tuiClient/pairedRemoteConnector.ts (1)
236-249: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRelay-auth failures lose the new attempt diagnostics.
When
relayAuthErrortriggers the final throw (Line 265), the collectedcorrelationId/attempts/omittedAttemptCountare discarded sincePairedRuntimeRelayAuthRequiredErroronly carries(message, cause). This is likely the most common failure mode (user not signed in) yet it's the one case where the new diagnostic enrichment added in this PR doesn't apply.Consider threading the diagnostic object into
PairedRuntimeRelayAuthRequiredError(or wrapping it) so recovery/debug tooling gets consistent visibility regardless of failure type.Also applies to: 265-265
🤖 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/tuiClient/pairedRemoteConnector.ts` around lines 236 - 249, Update the PairedRuntimeRelayAuthRequiredError flow in the connector’s catch/final-throw path so the collected correlationId, attempts, and omittedAttemptCount diagnostics are attached to the propagated authentication error. Extend the error construction or wrap it while preserving its existing cause and authentication semantics, ensuring the final relayAuthError throw exposes the same diagnostics as other failure paths.
🤖 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.
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 23239-23261: Update markAcceptedCodexSteerProcessed to correlate
each Codex userMessage with the accepted steer’s mapped text marker or strict
send/ack order, rather than matching loosely by text and falling back to
accepted.values().next().value. Ensure combined input arrays containing context
and submitSteer text identify the correct steer, and only remove/emit
“processed” for that matched steer so later markAcceptedCodexSteersUnprocessed
calls cannot target the wrong entry.
---
Nitpick comments:
In `@apps/ade-cli/src/tuiClient/pairedRemoteConnector.ts`:
- Around line 236-249: Update the PairedRuntimeRelayAuthRequiredError flow in
the connector’s catch/final-throw path so the collected correlationId, attempts,
and omittedAttemptCount diagnostics are attached to the propagated
authentication error. Extend the error construction or wrap it while preserving
its existing cause and authentication semantics, ensuring the final
relayAuthError throw exposes the same diagnostics as other failure paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 357f917a-58db-42b3-a995-a84420d9ddd4
📒 Files selected for processing (37)
apps/account-directory/src/directory.tsapps/account-directory/test/directory.test.tsapps/ade-cli/src/chatRecovery.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/format.test.tsapps/ade-cli/src/tuiClient/adeApi.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/format.tsapps/ade-cli/src/tuiClient/pairedRemoteConnector.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/ipc/runtimeBridge.test.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeBootstrap.test.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeBootstrap.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeErrors.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeRoutes.test.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeRoutes.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionService.test.tsapps/desktop/src/main/services/remoteRuntime/remoteConnectionService.tsapps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.tsapps/desktop/src/main/services/search/searchService.test.tsapps/desktop/src/main/services/search/searchService.tsapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/chat/chatTranscriptRows.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/remoteRuntime.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Views/Work/WorkChatHeaderAndMessageViews.swiftapps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADE/Views/Work/WorkTranscriptParser.swiftapps/ios/ADETests/ADETests.swift
🚧 Files skipped from review as they are similar to previous changes (29)
- apps/desktop/src/main/services/remoteRuntime/pairedRuntimeErrors.ts
- apps/ade-cli/src/tuiClient/tests/format.test.ts
- apps/desktop/src/main/services/search/searchService.test.ts
- apps/account-directory/test/directory.test.ts
- apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/desktop/src/main/services/search/searchService.ts
- apps/desktop/src/shared/types/remoteRuntime.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.test.tsx
- apps/ade-cli/src/tuiClient/adeApi.ts
- apps/desktop/src/shared/types/chat.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/desktop/src/main/services/remoteRuntime/syncPairedMachineStore.ts
- apps/ios/ADE/Views/Work/WorkErrorAndMessageHelpers.swift
- apps/desktop/src/main/services/remoteRuntime/remoteConnectionService.ts
- apps/desktop/src/main/services/remoteRuntime/pairedRuntimeBootstrap.test.ts
- apps/ade-cli/src/tuiClient/format.ts
- apps/desktop/src/main/services/remoteRuntime/remoteConnectionService.test.ts
- apps/ios/ADE/Views/Work/WorkTranscriptParser.swift
- apps/desktop/src/main/services/remoteRuntime/pairedRuntimeRoutes.ts
- apps/desktop/src/main/services/remoteRuntime/pairedRuntimeBootstrap.ts
- apps/ios/ADE/Views/Work/WorkChatHeaderAndMessageViews.swift
- apps/ios/ADE/Views/Work/WorkTimelineHelpers.swift
- apps/ade-cli/src/cli.ts
- apps/ios/ADE/Models/RemoteModels.swift
- apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts
- apps/ade-cli/src/tuiClient/app.tsx
- apps/desktop/src/main/services/chat/agentChatService.test.ts
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19c1d09c00
ℹ️ 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".
| event.type === "user_message_resolution" | ||
| && event.steerId === steerId | ||
| ) { | ||
| alreadyCompleted = event; |
There was a problem hiding this comment.
If ADE exits after the replay's backend turn/start succeeds but before the buffered user_message_resolution is flushed, the transcript contains the original unprocessed steer and its optimistic replacement but no resolution. This scan recognizes only the missing resolution event, so a retry after restart dispatches the same message as another turn, potentially repeating commands or file edits. Persist a distinct backend-dispatch commit before acknowledging success and reconstruct the resolution from it on retry.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e39441d974
ℹ️ 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".
| const managedSessions = new Map<string, ManagedChatSession>(); | ||
| const lastPersistedPointerFingerprints = new Map<string, string>(); | ||
| const codexRecoveryInFlight = new Set<string>(); | ||
| const codexAutomaticRecoveryAttemptedBySession = new Set<string>(); |
There was a problem hiding this comment.
Store this marker in the session's persisted state rather than only in this process-local Set. If ADE exits or crashes after automatically restarting a silent turn and the same active turn is resumed on launch, this set is empty, so the watchdog can perform another automatic interrupt/restart despite the UI promising only one automatic attempt; repeated restarts can discard ongoing provider work each time the app is relaunched.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25d1d6e7b4
ℹ️ 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 ( | ||
| event.deliveryState === "queued" | ||
| || event.deliveryState === "processed" | ||
| || event.deliveryState === "unprocessed" | ||
| ) { | ||
| return null; |
There was a problem hiding this comment.
When a follow-up is queued for Claude, Cursor, Droid, or OpenCode, the only transcript row until dispatch has deliveryState: "queued"; if it remains waiting or is later cancelled, no accepted row is ever emitted. Returning null here therefore makes visible queued/cancelled chat text impossible to find with ade search, rather than merely deduplicating lifecycle snapshots. Deduplicate repeated rows by steerId while retaining the initial queued message.
AGENTS.md reference: AGENTS.md:L37-L37
Useful? React with 👍 / 👎.
| const history = await readTranscriptEnvelopesForSessionIdAsync( | ||
| sessionId, | ||
| 8 * 1024 * 1024, | ||
| ); |
There was a problem hiding this comment.
For chats whose transcript exceeds 8 MiB, an unresolved message loaded through older-history pagination can fall outside this tail read. Clicking Run next or Dismiss then reports that the message is not owned/no longer waiting even though the UI is displaying the durable unprocessed row. The ownership check and resolution lookup need a targeted paged/full scan by steerId rather than only the latest 8 MiB.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/main/services/chat/agentChatService.ts`:
- Around line 11520-11532: Update persistUnprocessedMessageResolutionReceipt to
snapshot the prior receipt for receipt.steerId, then restore that prior map
state when persistChatState returns false before throwing. If no prior receipt
existed, remove the newly inserted entry; otherwise reinstate the previous
receipt, while preserving the existing eviction and successful persistence
behavior.
🪄 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: 882cb288-96e2-4d89-8e95-5bb4b41c41f7
📒 Files selected for processing (5)
apps/ade-cli/src/tuiClient/__tests__/remoteLauncher.test.tsapps/ade-cli/src/tuiClient/pairedRemoteConnector.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/remoteRuntime/pairedRuntimeErrors.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ade-cli/src/tuiClient/pairedRemoteConnector.ts
- apps/desktop/src/main/services/chat/agentChatService.test.ts
| const persistUnprocessedMessageResolutionReceipt = ( | ||
| managed: ManagedChatSession, | ||
| receipt: PersistedUnprocessedMessageResolutionReceipt, | ||
| ): void => { | ||
| managed.unprocessedMessageResolutionReceipts.delete(receipt.steerId); | ||
| managed.unprocessedMessageResolutionReceipts.set(receipt.steerId, receipt); | ||
| evictOldestEntries(managed.unprocessedMessageResolutionReceipts, 64); | ||
| if (!persistChatState(managed, { fsync: true })) { | ||
| throw new Error( | ||
| "The provider accepted this message, but ADE could not save its delivery receipt. Retry without closing this chat.", | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
persistUnprocessedMessageResolutionReceipt doesn't roll back the in-memory receipt when the fsync fails, unlike its sibling recovery-guard functions.
const persistUnprocessedMessageResolutionReceipt = (
managed: ManagedChatSession,
receipt: PersistedUnprocessedMessageResolutionReceipt,
): void => {
managed.unprocessedMessageResolutionReceipts.delete(receipt.steerId);
managed.unprocessedMessageResolutionReceipts.set(receipt.steerId, receipt);
evictOldestEntries(managed.unprocessedMessageResolutionReceipts, 64);
if (!persistChatState(managed, { fsync: true })) {
throw new Error(
"The provider accepted this message, but ADE could not save its delivery receipt. Retry without closing this chat.",
);
}
};persistCodexAutomaticRecoveryAttempt/clearCodexAutomaticRecoveryAttempt (a few hundred lines below, same file) explicitly restore the previous flag value when persistChatState returns false. This function mutates the map unconditionally and never rolls it back on failure.
Consequence: after a failed fsync, resolveUnprocessedMessage still has the (unpersisted) receipt in managed.unprocessedMessageResolutionReceipts. A caller that retries after seeing the thrown error will hit the existingResolution short-circuit path and get back status: "already_completed" even though nothing was durably written — for the dismiss action in particular, no side effect occurred at all. If the process exits before any later full-state persist happens to flush this entry, the resolution is silently lost even though the retry reported success. This defeats the crash-safety guarantee this PR is explicitly adding ("close the crash window between provider dispatch and the async transcript append").
🛠️ Suggested fix — mirror the rollback pattern used elsewhere in this file
const persistUnprocessedMessageResolutionReceipt = (
managed: ManagedChatSession,
receipt: PersistedUnprocessedMessageResolutionReceipt,
): void => {
+ const previous = managed.unprocessedMessageResolutionReceipts.get(receipt.steerId);
managed.unprocessedMessageResolutionReceipts.delete(receipt.steerId);
managed.unprocessedMessageResolutionReceipts.set(receipt.steerId, receipt);
evictOldestEntries(managed.unprocessedMessageResolutionReceipts, 64);
if (!persistChatState(managed, { fsync: true })) {
+ if (previous) {
+ managed.unprocessedMessageResolutionReceipts.set(receipt.steerId, previous);
+ } else {
+ managed.unprocessedMessageResolutionReceipts.delete(receipt.steerId);
+ }
throw new Error(
"The provider accepted this message, but ADE could not save its delivery receipt. Retry without closing this chat.",
);
}
};🧰 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 { spawn, type ChildProcessWithoutNullStreams } 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 `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 11520 -
11532, Update persistUnprocessedMessageResolutionReceipt to snapshot the prior
receipt for receipt.steerId, then restore that prior map state when
persistChatState returns false before throwing. If no prior receipt existed,
remove the newly inserted entry; otherwise reinstate the previous receipt, while
preserving the existing eviction and successful persistence behavior.
1 participant
Summary
Validation
Open in ADE: https://ade-app.dev/open?type=pr&repo=arul28%2FADE&number=904
Summary by CodeRabbit
Greptile Summary
This PR hardens cross-client chat recovery and machine connectivity.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failures remain.
What T-Rex did
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client as Desktop / CLI / TUI / iOS participant Brain as ADE Brain participant Provider as Chat Provider participant Directory as Account Directory participant Relay as Tunnel Relay Client->>Brain: Send or recover chat message Brain->>Provider: Dispatch provider-neutral action Provider-->>Brain: Dispatch acknowledgement Brain-->>Client: Durable delivery/recovery state Client->>Directory: Resolve saved machine routes Directory-->>Client: LAN, Tailscale, Relay endpoints Client->>Brain: Attempt LAN, then Tailscale alt Direct routes unavailable Client->>Relay: Connect with same-account proof Relay->>Brain: Establish paired tunnel endReviews (6): Last reviewed commit: "ship: iteration 5 — persist recovery att..." | Re-trigger Greptile