Fix Codex app-server failure recovery#784
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe chat runtime now deduplicates Codex errors and distinguishes retryable failures. The renderer derives turn activity from transcript evidence, displays provider recovery actions, supports retrying failed turns, and can open the model picker programmatically. ChangesProvider failure recovery
Estimated code review effort: 4 (Complex) | ~45 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 |
arul28
changed the title
|
@copilot review but do not make fixes |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
|
@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: 756181db90
ℹ️ 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".
|
Deployment failed with the following error: Learn More: https://vercel.com/arul28s-projects?upgradeToPro=build-rate-limit |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
arul28
force-pushed
the
ade/fix-codex-app-server-failure-recovery-f447906d
branch
from
03f7e9b to
7e5f12f
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/chat/AgentChatPane.tsx (1)
6706-6763: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSurface a rejection state for provider-failure retries
onRetryProviderFailurenow callsresendLastUserMessage(...)directly, butCHAT_AUTH_RETRY_REJECTED_EVENTis only handled byAgentCliAuthCard. If the resend is rejected locally, the provider-failure card stays unchanged and the click reads as a no-op. Surface an inline error or rejection state here 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/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 6706 - 6763, The provider-failure retry can be rejected without updating this pane, leaving the retry card unchanged. Update resendLastUserMessage and its callers to expose a local rejection state or inline error when submitInFlightRef is already set, no user message is found, or the send is rejected as already active; render that state in the provider-failure UI so the retry click visibly communicates the rejection.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/chat/chatTurnState.ts (1)
11-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider dedicated unit tests for these pure helpers.
findUserMessageForTurnandtranscriptLatestTurnIsTerminalencode several non-trivial boundary rules (steer exclusion, differing-turn breaks, reopening signals). They're only exercised indirectly today throughAgentChatPane.test.tsxintegration tests. Direct unit tests would pin down edge cases (e.g., interleaved subagent events, multiple back-to-back turns) more cheaply than debugging through the full pane.Also applies to: 48-68, 70-80
🤖 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/chatTurnState.ts` around lines 11 - 45, Add dedicated unit tests for the pure helpers findUserMessageForTurn and transcriptLatestTurnIsTerminal, covering steer-message exclusion, differing-turn and turn-boundary breaks, interleaved subagent events, reopening signals, and multiple back-to-back turns. Keep these tests focused on helper inputs and outputs rather than relying on AgentChatPane integration tests.
🤖 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/chat/AgentChatPane.tsx`:
- Around line 6713-6728: Update the fallback scan in resendLastUserMessage to
exclude user_message events with a steerId, matching findUserMessageForTurn
behavior. Only select non-steer user messages when locating the original prompt
for CHAT_RETRY_AUTH_TURN_EVENT retries.
- Around line 3043-3046: Reset modelPickerOpenRequestKey in the same
session-switch reset effect that clears other transient per-session UI state,
using setModelPickerOpenRequestKey(undefined), so AgentChatComposer cannot carry
the pending request into the next session.
In `@apps/desktop/src/renderer/components/chat/ProviderFailureRecoveryCard.tsx`:
- Around line 9-30: Update classifyProviderFailure to prioritize the structured
event.errorInfo.category before inspecting freeform text: map the "busy"
category to the existing capacity recovery and "rate_limit" to the existing
usage-limit recovery. Preserve the normalized identity matching as a fallback
for cases without recognized structured categories, including existing
server-overloaded, model-at-capacity, usage-limit, and rate-limit phrases.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 6706-6763: The provider-failure retry can be rejected without
updating this pane, leaving the retry card unchanged. Update
resendLastUserMessage and its callers to expose a local rejection state or
inline error when submitInFlightRef is already set, no user message is found, or
the send is rejected as already active; render that state in the
provider-failure UI so the retry click visibly communicates the rejection.
---
Nitpick comments:
In `@apps/desktop/src/renderer/components/chat/chatTurnState.ts`:
- Around line 11-45: Add dedicated unit tests for the pure helpers
findUserMessageForTurn and transcriptLatestTurnIsTerminal, covering
steer-message exclusion, differing-turn and turn-boundary breaks, interleaved
subagent events, reopening signals, and multiple back-to-back turns. Keep these
tests focused on helper inputs and outputs rather than relying on AgentChatPane
integration tests.
🪄 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
Run ID: e301e61e-ca45-4185-91eb-7b623cb7d940
⛔ Files ignored due to path filters (3)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**
📒 Files selected for processing (11)
apps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.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/ProviderFailureRecoveryCard.tsxapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/chat/chatTranscriptRows.tsapps/desktop/src/renderer/components/chat/chatTurnState.tsapps/desktop/src/renderer/components/shared/ModelPicker/ModelPicker.tsx
| const [modelPickerOpenRequestKey, setModelPickerOpenRequestKey] = useState<number | undefined>(); | ||
| const handleModelPickerOpenRequestHandled = useCallback(() => { | ||
| setModelPickerOpenRequestKey(undefined); | ||
| }, []); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
FILE='apps/desktop/src/renderer/components/chat/AgentChatPane.tsx'
echo '--- outline ---'
ast-grep outline "$FILE" --view expanded | sed -n '1,220p'
echo '--- state/effect/consumer search ---'
rg -n "modelPickerOpenRequestKey|onChooseProviderFailureModel|selectedSessionId|laneId|AgentChatComposer" "$FILE"
echo '--- relevant slices ---'
sed -n '3020,3095p' "$FILE"
echo '--- around composer usage ---'
sed -n '10990,11220p' "$FILE"Repository: arul28/ADE
Length of output: 50366
🏁 Script executed:
set -euo pipefail
FILE='apps/desktop/src/renderer/components/chat/AgentChatPane.tsx'
echo '--- session-change reset effect ---'
sed -n '5838,5868p' "$FILE"
echo '--- composer invocation ---'
sed -n '10220,10248p' "$FILE"
echo '--- handleModelPickerOpenRequestHandled usage ---'
rg -n "handleModelPickerOpenRequestHandled|modelPickerOpenRequestKey" "$FILE"Repository: arul28/ADE
Length of output: 3108
🏁 Script executed:
set -euo pipefail
FILE='apps/desktop/src/renderer/components/chat/AgentChatPane.tsx'
echo '--- composer wrapper around invocation ---'
sed -n '10224,10264p' "$FILE"
echo '--- AgentChatComposer definition and model-picker handling ---'
COMPOSER_FILE='apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx'
ast-grep outline "$COMPOSER_FILE" --view expanded | sed -n '1,220p'
echo '--- direct search in composer ---'
rg -n "modelPickerOpenRequestKey|onModelPickerOpenRequestHandled|Choose model|model picker|request key|key=" "$COMPOSER_FILE"Repository: arul28/ADE
Length of output: 8365
🏁 Script executed:
set -euo pipefail
P='apps/desktop/src/renderer/components/chat/AgentChatPane.tsx'
echo '--- composer key search ---'
rg -n "AgentChatComposer|key=\{.*selectedSessionId|key=\{.*laneId|key=\{.*sessionId|key=" "$P"
echo '--- request setter around onChooseProviderFailureModel ---'
sed -n '11136,11168p' "$P"
echo '--- composer internal open-request handling ---'
C='apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx'
sed -n '3838,3870p' "$C"Repository: arul28/ADE
Length of output: 4821
Reset modelPickerOpenRequestKey on session switch
The transient UI reset clears other per-session state, but this request can still survive a session change and open the model picker for the next session because AgentChatComposer stays mounted. Clear it in the same reset effect.
🤖 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.tsx` around lines
3043 - 3046, Reset modelPickerOpenRequestKey in the same session-switch reset
effect that clears other transient per-session UI state, using
setModelPickerOpenRequestKey(undefined), so AgentChatComposer cannot carry the
pending request into the next session.
| const resendLastUserMessage = useCallback(async (sessionId: string, failedTurnId?: string | null) => { | ||
| if (submitInFlightRef.current) { | ||
| rejectAuthRetry(sessionId); | ||
| return; | ||
| } | ||
| const events = selectedEventsForDisplayRef.current; | ||
| let userEvent: Extract<AgentChatEvent, { type: "user_message" }> | null = null; | ||
| for (let index = events.length - 1; index >= 0; index -= 1) { | ||
| const evt = events[index]?.event; | ||
| if (evt?.type === "user_message" && typeof evt.text === "string" && evt.text.trim().length > 0) { | ||
| userEvent = evt; | ||
| break; | ||
| let userEvent = failedTurnId ? findUserMessageForTurn(events, failedTurnId) : null; | ||
| if (!failedTurnId) { | ||
| for (let index = events.length - 1; index >= 0; index -= 1) { | ||
| const evt = events[index]?.event; | ||
| if (evt?.type === "user_message" && typeof evt.text === "string" && evt.text.trim().length > 0) { | ||
| userEvent = evt; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "findUserMessageForTurn|CHAT_RETRY_AUTH_TURN_EVENT|resendLastUserMessage|steerId" apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
echo '--- outline ---'
ast-grep outline apps/desktop/src/renderer/components/chat/AgentChatPane.tsx --view expandedRepository: arul28/ADE
Length of output: 18160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant region around resendLastUserMessage and the helper definition if present.
sed -n '6660,6785p' apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
echo '--- findUserMessageForTurn occurrences ---'
rg -n -C 4 "function findUserMessageForTurn|const findUserMessageForTurn|findUserMessageForTurn\(" apps/desktop/src/renderer/components/chat/AgentChatPane.tsxRepository: arul28/ADE
Length of output: 6426
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper implementation and nearby types.
sed -n '1,220p' apps/desktop/src/renderer/components/chat/chatTurnState.ts
echo '--- references to steerId and turn mapping in chatTurnState.ts ---'
rg -n -C 3 "steerId|user_message|failedTurnId|turn" apps/desktop/src/renderer/components/chat/chatTurnState.tsRepository: arul28/ADE
Length of output: 5263
Skip steer messages in the fallback retry scan The CHAT_RETRY_AUTH_TURN_EVENT path should mirror findUserMessageForTurn and ignore user_message events with steerId; otherwise a retry can resend a mid-turn steer instead of the turn’s original prompt.
🤖 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.tsx` around lines
6713 - 6728, Update the fallback scan in resendLastUserMessage to exclude
user_message events with a steerId, matching findUserMessageForTurn behavior.
Only select non-steer user messages when locating the original prompt for
CHAT_RETRY_AUTH_TURN_EVENT retries.
| export function classifyProviderFailure( | ||
| event: Extract<AgentChatEvent, { type: "error" }>, | ||
| ): ProviderFailureRecovery | null { | ||
| const identity = `${ | ||
| typeof event.errorInfo === "string" ? event.errorInfo : event.errorInfo?.category ?? "" | ||
| } ${event.message}`.toLowerCase().replace(/[^a-z0-9]+/g, ""); | ||
| if (identity.includes("serveroverloaded") || identity.includes("modelisatcapacity")) { | ||
| return { | ||
| kind: "capacity", | ||
| label: "Provider capacity", | ||
| guidance: "The provider ended this turn because the selected model is at capacity. This thread is still safe to continue.", | ||
| }; | ||
| } | ||
| if (identity.includes("usagelimitexceeded") || identity.includes("ratelimit")) { | ||
| return { | ||
| kind: "rate_limit", | ||
| label: "Usage limit", | ||
| guidance: "The provider ended this turn at a usage limit. Retry after the limit resets or choose another available model.", | ||
| }; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B2 -A6 '"busy"|"budget"' apps/desktop/src/main/services/chat apps/desktop/src/shared/types --type=tsRepository: arul28/ADE
Length of output: 8914
Classify provider failures from errorInfo.category first
This heuristic is still brittle: event.errorInfo.category already carries structured values like "busy" and "rate_limit", but the card only matches a few hardcoded message phrases. That means a "busy" capacity error such as “already running this agent” will fall through to null unless the freeform text happens to contain serveroverloaded or modelisatcapacity. Use the structured category for the primary branch and keep text matching only as a 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/desktop/src/renderer/components/chat/ProviderFailureRecoveryCard.tsx`
around lines 9 - 30, Update classifyProviderFailure to prioritize the structured
event.errorInfo.category before inspecting freeform text: map the "busy"
category to the existing capacity recovery and "rate_limit" to the existing
usage-limit recovery. Preserve the normalized identity matching as a fallback
for cases without recognized structured categories, including existing
server-overloaded, model-at-capacity, usage-limit, and rate-limit phrases.
Source: Coding guidelines
1 participant
Summary
errornotification and failedturn/completed, while preserving distinct failuresProtocol basis
Official Codex app-server semantics allow a top-level error notification before terminal
turn/completed;willRetry: truemeans the app-server is retrying and the turn remains active. ADE now distinguishes that retry notice from a terminal provider failure.Verification
/quality: no remaining Blocker/High findings after two re-review passesSeparate verified follow-ups
These symptoms were investigated and are not caused by the chat lifecycle bug, so this PR does not mix speculative search/CLI changes into the recovery fix:
error/errorInfopayloads, soserverOverloadedcannot be found from the original error eventade chat list --lane … --textandade lanes list --textboth return data; the earlier blank-output report did not reproduce on current main/runtimeGreptile Summary
This PR fixes Codex app-server failure recovery and related chat lifecycle handling. The main changes are:
errornotifications and failedturn/completedevents.willRetry: trueCodex errors as non-terminal provider-health notices.Confidence Score: 5/5
Safe to merge with minimal risk.
The changes are focused on Codex failure lifecycle handling and renderer recovery UX. Added tests cover duplicate terminal errors, non-terminal retry notices, stale active summaries, retry actions, and cross-provider terminal-state behavior. No blocking correctness or security issues were identified in the changed paths.
No files require special attention.
What T-Rex did
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Codex as Codex app-server participant Main as agentChatService participant Transcript as Persisted transcript participant Pane as AgentChatPane participant User as User Codex->>Main: "error(willRetry=true)" Main->>Transcript: system_notice(provider_health) Main-->>Pane: turn remains active Codex->>Main: "error(willRetry=false)" Main->>Transcript: error(deduped by turn + payload) Codex->>Main: "turn/completed(status=failed)" Main->>Transcript: status failed + done failed Pane->>Pane: resolveTurnActive(transcript outranks stale summary) Pane-->>User: show Send composer + recovery card User->>Pane: Retry turn / Choose model Pane->>Main: resend failed turn text or open ModelPicker%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Codex as Codex app-server participant Main as agentChatService participant Transcript as Persisted transcript participant Pane as AgentChatPane participant User as User Codex->>Main: "error(willRetry=true)" Main->>Transcript: system_notice(provider_health) Main-->>Pane: turn remains active Codex->>Main: "error(willRetry=false)" Main->>Transcript: error(deduped by turn + payload) Codex->>Main: "turn/completed(status=failed)" Main->>Transcript: status failed + done failed Pane->>Pane: resolveTurnActive(transcript outranks stale summary) Pane-->>User: show Send composer + recovery card User->>Pane: Retry turn / Choose model Pane->>Main: resend failed turn text or open ModelPickerReviews (3): Last reviewed commit: "ship: iteration 3 — disable stale recove..." | Re-trigger Greptile
Summary by CodeRabbit
New Features
Bug Fixes