Fix ADE chat recovery and bound Codex usage scans#796
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughClaude chat lifecycle handling now reconciles unsettled turns, bounds interrupt and stop operations, and refines steering behavior. Codex usage scanning adds bounded, coalesced parsing and lifetime reconciliation, while aggregation excludes lifetime-only entries from period buckets. ChangesClaude chat lifecycle
Local usage ledger scanning
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
191ef0e to
ae1946a
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". |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
29936-29946: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
prepareSendMessagecall for the Claude non-busy steer path.
steer()computespreparedSteerviaprepareSendMessage({sessionId, text: trimmed, ...})(withoutreasoningEffort/executionMode/interactionMode), but for the Claude non-busy branch that result is discarded entirely in favor of a freshsendMessage({...}, { awaitDispatch: true })call using the raw args (this time including those three fields).prepareSendMessagedoes non-trivial work (lane launch-context resolution, per-attachment path validation viaresolvePathWithinRoot), so it runs twice for this path. SincepreparedSteerisn't reused here, consider either building it with the full field set once and passing it through, or deferringprepareSendMessage/preparedSteercomputation until after the branch decision is made.🤖 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 29936 - 29946, Avoid the duplicate prepareSendMessage work in steer() for the Claude non-busy path. Either defer preparedSteer creation until after branch selection, or construct it once with reasoningEffort, executionMode, and interactionMode and reuse it for sendMessage instead of passing raw arguments.
🤖 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 23753-23757: The unconditional reconcileClaudeSessionAfterRestart
call in ensureClaudeSessionRuntime can emit duplicate interrupted status/done
events during Claude model switches. Distinguish genuine host-restart recovery
from non-restart model-switch teardown by skipping the restart sweep for the
latter, or record the terminalized turnId in the existing duplicate-interrupt
guard so the new runtime suppresses it; add coverage for a mid-stream model
switch and verify only one interrupted pair is emitted.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 29936-29946: Avoid the duplicate prepareSendMessage work in
steer() for the Claude non-busy path. Either defer preparedSteer creation until
after branch selection, or construct it once with reasoningEffort,
executionMode, and interactionMode and reuse it for sendMessage instead of
passing raw arguments.
🪄 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: 0deea6d0-cc1c-4221-aec0-d8fda5b08eba
📥 Commits
Reviewing files that changed from the base of the PR and between c76c075 and 191ef0e6e1bf56a98c020f243abefff0adb72316.
⛔ Files ignored due to path filters (4)
docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/usage-tracking.mdis excluded by!docs/**
📒 Files selected for processing (5)
apps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/usage/ledgers/localUsageLedgers.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/main/services/usage/usageTrackingService.ts
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
Fixes the ADE runtime stalls that orphaned Claude turns and left the Stop UI stale. Claude restart, Stop, and idle-steer paths now reconcile missing terminal events and bound SDK control calls. Codex usage history now streams JSONL with hard line, file, byte, and entry limits while preserving exact all-time totals through read-only SQLite reconciliation.\n\nValidation: quality dual review clean; desktop Node 22 typecheck passed; affected CI shards 1 and 8 passed; usage suite 130/130; agent chat suite 562/562; internal docs validated.
Summary by CodeRabbit
Bug Fixes
Usage Tracking
Greptile Summary
This PR improves Claude chat recovery and Codex usage accounting. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
No blocking issues found in the changed code.
What T-Rex did
Important Files Changed
Reviews (2): Last reviewed commit: "ship: iteration 1 — address chat lifecyc..." | Re-trigger Greptile