Repository navigation
fix(native-chat): preserve Claude approval choices across model changes - #30914
Conversation
… Orca can't read its model list A chat's auto mode dropped to Ask whenever the saved model list was missing or couldn't name the chat's model (another account home, first start in a folder, an unlisted model id), so a resumed chat launched asking for approval. Auto now drops to Ask only when the list shows the model without it; Claude itself starts a model that can't run auto in its default mode. Tracked in STA-10953.
…reads as Ask A CLI that marks no model with auto support predates the auto value (2.1.71) and would refuse --permission-mode auto, so its listing is evidence, not a gap. Tracked in STA-10953.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. ℹ️ Recent review info
Walkthrough
Priority: Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 30.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 29 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main change: preserving Claude approval choices across model changes. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The pull request description is complete and relevant. It includes the required ELI5, What Changed, Why, Linked Issue, Visual Proof, Testing, Review, Agent skill upstream boundary, and Notes sections.… Full details: Description check
-
Explanation
The pull request description is complete and relevant. It includes the required ELI5, What Changed, Why, Linked Issue, Visual Proof, Testing, Review, Agent skill upstream boundary, and Notes sections. It documents test coverage, QA status, compatibility considerations, and the linked issue. The AI Disclosure and full Checklist sections are not explicitly included, but these omissions are non-critical because the description provides equivalent testing, review, and scope information.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
A Claude chat saved as Approve for me could reopen asking for approvals because Orca used model-list predictions to override its saved choice. This PR passes that choice directly to Claude. A steer accepted into an active Claude turn keeps that turn’s execution choices; newly selected choices apply to a subsequent new turn. Background mail keeps current options while retaining signed-out recovery. Review complete. Fresh full CI passed on Four whole review rounds and final source, reference, performance and readiness checks are recorded. Accepted fixes keep root lifecycle facts under journal pressure, make local preparation/admission follow the existing turn owner, and avoid repeated buffered-key scans. No saved field, wire operation, latch, owner or setting is added. Production changes against merged main QA-head CI passed on The single real QA campaign uses Claude and Codex as representatives for shared changes. Independent evidence review accepted genuinely previous-build-created state through upgrade and a separate graceful restart, two replies per agent in each phase, Claude’s same-turn steer retaining the running child and actual request choices, Codex’s original send/settlement sequence, authentic worker/mail/check/ACK/durable-result subsets for both agents, and Auto acceptance on the actually available older Claude executable. The original historical saved root was absent; its captured-input regression is automated coverage, not historical runtime replay. Independent review also accepted a genuinely signed-out ready child surviving credential restoration, then being replaced solely by eligible mail, replying and acknowledging delivery, followed by a separate successful user turn on the replacement. Independent review accepted the bounded Claude correction: B was admitted while the authentic worker result was still owed, the result later arrived and was checked and acknowledged, and the next user reply used actual Opus/medium requests. The app stopped gracefully; recorded duties settled, stable stopped-state inventories and no owned survivors/listeners/writers were verified. Exact uninterrupted control-batch procedure compliance remains partial because legitimate approvals and a control-label timeout interrupted preparation; the functional witnesses completed without replay. The no-command The 10-25 conflict-blocking main merge The second authorized 10-25 main merge is The authorized CI-repair main merge is For Brennan’s review The QA lead screened and uploaded the following 13 whole-window screenshots for the real functional flows. N2 is shown separately as renderer-test proof. Upgrade — Claude two replies
Upgrade — Codex two replies
Graceful restart — Claude two replies
Graceful restart — Codex two replies
Claude active steer — B admitted while A active
Claude active steer — actual completed result
Codex changed-choice/result — completed original B
Codex changed-choice/result — completed distinct next user
Observed older CLI2.1.306 — permission Auto accepted
N1 — actual current-root sign-in failure
N1 — real MAIL/check/ACK recovery and next reply
Claude bounded correction — B while result genuinely owed
Claude bounded correction — durable result and selected-choice next reply
N2 waiting notice — renderer-test proof, branch
|
…hat-launch-intent
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Retry unsupported auto at the provider… · structured-agent-session-launch-permission.ts:37-49
src/main/native-chat/agent-session-wire/structured-agent-session-launch-permission.ts:37-49
🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry unsupported
autoat the provider startup boundary.
resolveClaudeAcquisitionLaunchonly resolves launch data. The failure occurs later whenopenClaudeStreamJsonConnectioncalls the Claude SDK and its reader awaitsiterator.next(). The acquisition observes that failure throughonFault/initProof.promiseor the closed-connection check.When the launch mode is
autoand this startup error specifically reports unsupportedauto, retry once with Orca'saskmode (permissionMode: 'default'). Close and settle the failed connection before retrying. Do not retry generic spawn, authentication, exit, or transport errors. Preserve the savedautointent.Without this fallback, an older Claude CLI can reject the first launch before startup produces catalog data, so later launches can repeat the failure.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
224cd223-7b62-412c-ab42-cd48a61559a8
📒 Files selected for processing: 1
src/shared/agent-chat-permission-mode.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
brennanb2025
changed the title
…t decision The retirement check derived wake debt (a child-work projection and a scan for the newest turn) before asking whether open work had already decided the outcome, and its admitted-input fallback rebuilt the provider's held-send set once per unanswered submission, O(S x W) per decision. Open work is now read first and wake debt only where it picks the outcome or the refusal reason. Current work gains one batch query that reads the held set and the handover set once, sharing the single holds rule with the per-send reader. Outcomes and refusal reasons are unchanged. Adds N/10N operation-count contracts on the real Claude host rig: two retirement checks per ordinary send, each one pass over history, with a control that the second sees a sign-out reported between them; no wake debt mid-turn; one held-set read per refusal in a queued model-change burst.
ELI5
A Claude chat saved as Approve for me could reopen asking for every approval because Orca used its saved model list to override the chat’s choice. Orca now passes the saved choice directly to Claude. If Claude opened with a different approval mode, changing models can reopen the conversation before your next message so the new model gets the requested approval choice at startup.
What Changed
The mailbox pointer’s only change is
keepCurrentOptions: trueinsrc/main/runtime/orchestration/structured-mailbox-pointer-host.ts. The host’s option-retention comparison also changes atstructured-agent-session-outgrown-child.ts:155,174,176–179; signed-out recovery remains checked. Mail ownership, delivery, check, acknowledgment, and retry behavior are unchanged; this call site must be reconciled with #28567 by the second PR to merge.Why
The model list describes available models; it does not establish what approval mode the running process opened with. Using it as launch policy silently changed saved intent and could leave a later model change running under an earlier approval downgrade. The common pattern keeps requested choices, the first opening result, and successfully applied model choices separate.
Intended differences: if the agent starts a turn on its own while your message is being prepared, your message still goes through as a new message, as in the common pattern. This does not promise a separate provider turn or a particular reply model. Orca background notices carry no new user choices. Durable worker assignment alone does not require preserving a process, while actual native work remains protected. Missing initialization information stays unknown and never becomes evidence to refuse a message. Older Claude versions that do not accept the requested mode may refuse startup; Orca reports that failure without silently changing the saved choice or retrying another mode.
Follow-ups: earlier approval-display work is separately tracked in STA-11280; the reported-mode menu from #32334 is retained here. Remaining lifecycle differences are tracked in STA-11451. These follow-ups are outside this patch.
No saved data field, wire format, minimum paired-server version, or new setting is introduced. Existing normalization and other providers’ supported approval modes remain.
The local admission boundary follows the common pattern: prepare first, then validate the current turn, commit write-ahead custody and offer input synchronously. Provider transport and consumption happen later; input is never replayed after local admission or uncertainty. No new saved field, wire operation, latch or owner is added.
The architecture review set a 410-addition budget with an absolute ceiling of 471 production additions against pinned main
d9dc87b2. The measured PR diff against merged main5a4dd300is 419 additions and 223 deletions across 39 production files, below the ceiling. Integration1c3c287cwas followed by the captured-input-only regressionefd720bd. After real QA, conflict-blocking main was merged ataa0547e9(parentsefd720bdand7190b835); 22 tests in four targeted files and the required merge hook passed. The existing reconsideration escape precedes main’s early Stop settlement, and both independent adapter operations are retained. Accepted runtime evidence remains onefd720bdand carries across this main merge. A subsequent conflict-blocking #31615 integration isa8dae4ad(parentsaa0547e9and4138e586). It preserves both lifecycle annotations and the independent critical-root behavior; 29 tests across four affected files and the required merge hook passed. Normal formatting of the combined metadata adds four lines, and a local destructuring change adds two while bringing acquisition back within the existing file limit. No behavior, guard or state is added by that binding change. Fresh full PR Checks run 40078635583 passed on exacta8dae4adon attempt1, including all five Node shards, static analysis/typecheck, relay, Linux/Windows packaging, cross-version compatibility and aggregate checks. Mobile Checks, WSL orcad E2E, Headless Node server and PR test LoC also passed on the same head. Incoming main comments were reconciled while preserving all reviewed code and assertions in overlapping files. Hook file-length limits required restoring the earlier helper location; its lines are included in the measured count. No relocation or deletion credit is applied.Root turn lifecycle events stay in the existing ordered journal queue under transcript pressure, preserving completion, quota and supersession facts that the next input depends on. Only the exact root lifecycle options in
claude-turn-lifecycle-item.ts, its keyed publication inclaude-open-turn.ts, and root terminal-failure diagnostic, fallback publication and quota-end revision inclaude-terminal-failure.tssetcriticalRoot. The generic turn-row publication fallback forwards it; the sink options declare it and the existing queue consumes it. Ordinary transcript, thinking and subagent events keep their existing limits. No metadata enters saved rows or wire messages. The shared producer sitesclaude-open-turn.ts,claude-terminal-failure.tsandclaude-structured-session-acquisition.tsretain the landed #31615 lifecycle annotations independently of this PR’s critical-root behavior; the final integration preserves both. Retained control work has no fixed burst limit and may delay ordinary writes; reader backpressure still paces subsequent live input.The 10-25 conflict-blocking main merge
45371f8aintegrates7d18e199, including the repository comment cleanup. Both manual conflicts preserve the reviewed permission logic and main’s comment deletions/shortening. Incoming goal-steering and orchestration behavior is retained. Independent source review found no merge regression; the required merge hook (including ast-grep) and 56 tests in six named files passed. The first CI run passed typecheck but main’s new timeout-reason rule found an unexplained timeout in this PR’s existing integration test. Follow-up16235bf1adds one reason comment, retaining every assertion and the 60-second timeout; its exact test file and normal hook passed. No production byte changes in that follow-up. Fresh final-head PR Checks passed. Accepted real QA stays onefd720bd; N2’s labelled renderer-test proof stays ona8dae4ad. The marker retains the QA head across the disclosed merge and test-comment commit; no QA flow was found broken or replayed.The second authorized 10-25 main merge is
de1ae42a, integratingcffb527band #32334 while retaining the reviewed #33045 retry mergee68340f1. New chats still use Settings → Agents;ownstill delegates to agent config; the menu still shows and checks the reported running mode; a pick changes only that chat. Existing chats retain saved permission, model and effort. This PR offers the agent’s supported modes without a model-catalog gate and preserves explicit saved launch choices, turn-scoped steer behavior and the original message’s options through the existing single retry flight. A fresh independent source/reference review returned AGREE on the exact committed tree. All 189 tests in 16 named files and the required direct merge hook passed. Import arrangements used to meet file limits preserve executable behavior. Production changes against merged main5a4dd300are 419 additions and 223 deletions across 39 paths, within the 471-addition ceiling. No new saved field, wire operation, latch, owner or lifecycle repair was added. Accepted QA carries across the merge; no flow was found broken or replayed. CI onde1ae42apassed typecheck but failed lint on two incoming-main file-length violations. Landed repair #33675 fixes both and the stale test import; it is included in the integration below. No PR-owned source correction or unchanged-head retry was made.The authorized CI-repair main merge is
0d3f0711, integrating5a4dd300(#33675). It completed without conflicts, manual source edits or hook changes. All 59 existing PR paths, including the 39 production paths, are byte-for-byte unchanged from reviewedde1ae42a. All 65 tests in six named files passed through tc-gate, covering both startup test files, status-feed/child-work/name retention and the repaired migration import; the required direct merge hook also passed. The raw PR production diff remains 419 additions and 223 deletions against merged main5a4dd300, within the 471-addition ceiling. Accepted QA carries, with no new run or screenshot. CI on0d3f0711cleared the earlier main lint errors but found a missing required input in this PR’s scripted Codex test. Test-only commit6a631feasuppliesholdWhileWorking: falsefor fresh, unheld mail, matching the existing production caller and fixture behavior. Every assertion and recorded frame remains unchanged. Its seven tests and normal commit hook passed; no production bytes changed. Fresh PR Checks passed on exact head6a631feaa242, including static analysis/typecheck, all five Node shards, relay, Linux/Windows packaging, cross-version compatibility and aggregate. Mobile verification, Headless Node server/persistence, test LoC and the WSL pipeline also passed. WSL attempt 1’s Ubuntu runner ended cancelled during setup; the repository’s recovery workflow automatically started attempt 2 (triggered by github-actions), which passed all three distros. No manual rerun or fourth main merge was performed. GitHub reports CLEAN/MERGEABLE, with all 43 check rollups successful or intentionally skipped and zero unresolved review threads.Linked Issue
Tracked in STA-10953 (GitHub #30032).
Visual Proof
The single real QA campaign on
efd720bd, carried across the disclosed main merges, completed its required functional flows with Claude and Codex. The QA lead screened and uploaded these 13 whole-window images. The N2 notice is shown separately as renderer-test proof below; it uses the tested refusal input and shipping component/styles without a provider, and does not claim a naturally observed refusal.Upgrade — Claude two replies
Upgrade — Codex two replies
Graceful restart — Claude two replies
Graceful restart — Codex two replies
Claude active steer — B admitted while A active
Claude active steer — actual completed result
Codex changed-choice/result — completed original B
Codex changed-choice/result — completed distinct next user
Observed older CLI2.1.306 — permission Auto accepted
N1 — actual current-root sign-in failure
N1 — real MAIL/check/ACK recovery and next reply
Claude bounded correction — B while result genuinely owed
Claude bounded correction — durable result and selected-choice next reply
N2 waiting notice — renderer-test proof, branch
a8dae4ad, tested no-commandmessagesUnsettledrefusal through shipping renderer code/styles; no provider. Appearance/copy proof only.Testing
Automated coverage includes direct saved-policy/menu behavior, first root initialization before publication, missing initialization fields, literal aliases versus full provider names, successful and failed model controls, and the integrated worker/mail/native-result/user-reopen sequence. That integration uses the real host, journal, orchestration database, delivery acknowledgment, and completion reconciliation with a scripted native transport; it is not real-agent QA.
Targeted test logs and the normal commit hook are recorded in the private review ledger. Full CI passed on
efd720bdde98b8dfabd0bd19f6011b65589dbbce(PR Checks run 40059814859, attempt 2), including all five Node shards, static analysis and typecheck, relay, packaging on Linux and Windows, cross-version checks and the aggregate. An unchanged Chromium preview navigation test timed out on the first attempt; a failed-jobs-only retry passed. The one current-head real-agent QA campaign uses Claude and Codex for shared changed flows: previous-build-created state through upgrade and graceful restart, two real replies, active steering and next-turn choices, and protected native work with mailbox delivery and an authentic result. Unchanged CLI, worker-start, phone and settings consumers use meaningful integration or contract tests; applicable provider-specific shared branches use recorded-output contracts. Only Claude’s own provider code changes. The older-provider boundary gets a bounded targeted check.Review
Full review path: lifecycle, saved-state consumers, ordering and agent behavior require full review and real QA. Four whole review rounds and the final source/reference/performance/readiness checks are recorded. Accepted corrections retain root lifecycle facts under journal pressure and replace repeated buffered-key scans with an insertion-ordered map, with meaningful regression contracts. Final integrations preserve the reviewed code and incoming main changes. Test-only follow-ups correct the Codex fixture’s inferred property and two exact root-event option expectations; all original assertions remain.
The captured saved Haiku full-model ID with Auto permission and an alias-only host catalog has one regression test asserting that Auto is retained without reading the catalog. All eight tests in that file and the normal hook passed. The original historical QA state no longer exists; this test is captured-input coverage, not historical replay or real provider evidence. The real upgrade run starts with genuinely fresh state created by the previous build and carries that same state through upgrade and graceful restart.
Signed-out recovery QA uses only the coordinator-authorized isolated placeholder through the supported settings write path, with the Settings view refreshed; the inherited empty Environment editor issue is owned separately. Independent review accepted the genuine failed-ready-child retention through credential restoration and eligible-mail-only replacement, real reply/check/ACK and subsequent user reply on the replacement. Independent review also accepted the bounded Claude correction: the real worker result was still owed when B was admitted, then arrived with authentic durable result/check/ACK, followed by a distinct user reply using actual Opus/medium requests and the requested launch permission. The app stopped gracefully; all recorded duties settled, stable same-root stopped inventories and no owned survivors/listeners/writers were verified. Exact uninterrupted control-batch procedure compliance remains partial because legitimate approvals and a control-label timeout interrupted preparation; functional witnesses completed without replay. The exact no-command messagesUnsettled input has a shared regression, with renderer translation and user-row display tests and a branch-only screenshot explicitly labelled renderer-test proof, without a provider. Per the coordinator’s 06:05 ruling, a naturally timed provider refusal is not required. The two conflict-blocking main integrations are reconciled, and GitHub reports mergeable. Fresh full CI passed on
a8dae4ad; GitHub reports CLEAN/MERGEABLE and the worktree is clean and pushed. The review loop is complete; the final readiness result is recorded in its single status comment.Agent skill upstream boundary
Notes
The execution host owns launch/restart choices for local, folder, SSH, and paired clients. Host-local capture controls add no RPC requirement. Permission-state display and unrelated Chrome behavior stay in their existing lanes.
Author X handle: @BrennanKB5