Repository navigation
fix(acp): start chats for agents that report no model - #26587
Conversation
|
Windows OMP opened its conversation, but Orca invented an empty model name when no model was reported. Saving that invalid value caused “Chat could not be started.” This change represents the missing model as absent in live and closed-chat readers, keeps one shared save guard, and leaves saved-record validation strict. Later model reports still use the existing save points and survive a real store reopen. Intended difference: older paired clients quietly lose thinking-settings/control discovery until a model is reported, preventing a picker crash. Chat creation and sending remain available. Final Windows high-spec proof: the chat starts, reports the real model (Gemini 3.1 Pro Preview), and accepts one prompt. The screenshot does not show an assistant answer. That is a separate, pre-existing message-identity bug, where reasoning and answer chunks share an identifier; it is being fixed separately. No reply-loss change is included here. The test rig hid the user's Fresh adversarial, architecture and narrow reviews passed. Captured-response, Grok/OpenCode, rendered-picker, real-store and mixed-version tests passed, along with the required real Claude tests. After the second authorized main merge, all three queued typechecks, 140 targeted tests in 15 files, and all 22 pre-push checks passed. Main’s #26648 extraction is retained; this branch’s duplicate module is removed. The host and provider-support files match main exactly, and the fresh reviewer confirmed all 20 startup-fix patches are unchanged. Merged current main on 10-09 at Brennan's request (head Deferred follow-up, P2: OMP's useful setup-error details are reduced to “Internal error.” The separate child draft #26666 preserves those details and names the agent while keeping its own next steps; this startup PR leaves error presentation unchanged. The app exited normally; native checks found no remaining or unverifiable owned app processes. The runtime key broker and recorded wrappers/OMP children exited and were verified absent. Real keybindings and the original reproduction database hashes stayed unchanged. Agent-produced runtime writes are recorded locally. The live Windows check ran on |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughSession-option reads now omit an absent model while preserving other reported options. The RPC checks whether a client supports model-less results. Desktop clients advertise that capability. Tests cover provider reports, persistence, picker rendering, and cross-version client behavior. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The change lets chats start for agents that report no model. Older paired clients cannot discover thinking settings until a model is reported, which is an intended tradeoff. No merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed and covers the change, rationale, visual proof, testing, compatibility, and review status. However, the required Linked Issue section does not identify an issue for this PR; it only links to a separate pre-existing bug.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
Resolves two conflicts in the session-options read: keeps both new imports in the RPC method, and keeps this branch's per-agent at-rest test with the journal context stub main's /clear change now needs.
…ain) Rebuilds #26561 on main now that #26416 is squash-merged. Same change as before, re-applied onto what main has since: - Every adapter answers acquire at spawn and reports `started` on protocol readiness with its real handle (Claude initialize, Codex thread, ACP session new/load plus restore, Pi session file plus restore). No bridge, no per-adapter phase or Stop opt-outs. - Picks made while starting are intents applied model then effort before the start is accepted; rewind, goal, rewind recovery and compact wait for a proven start outside the session queue. - One Stop rule: Stop ends the session for every agent (Codex: interrupt, up to 2 s for the turn to end, then close). - Orchestration worker starts share one deadline; a send's answer copies its row; a re-attach during start keeps the start wait. Integrated with main: - ACP: the subagent-stop probe, child-work delivery and new/loaded origin move into the handshake; the sign-in rule (#26544) classifies a failed handshake; acpAgentName stays in acp-structured-agent-definitions. - Codex: the session journal translator and account kind (#26407) reach the start step. - Pi: the start choice (#26821) and the signed-out verdict (#26743) are read in Pi's start, so both exist when `started` is reported; commands follow. - openWithAgent keeps main's pre-agent check and replay rule. - A report with no model drops the saved one, as #26587 decided. - The renderer re-reads a start's sign-in verdict once the start is proven.
ELI5
What broke: some agents open a chat without saying which model they're using. OMP does this when it has no model or provider set up. Orca stored that missing model as an empty name. Its own save check then rejected the empty name, so the user saw "Chat could not be started", even though OMP had opened the conversation.
What's different now: a chat from an agent that reports no model starts normally and shows no blank model control.
What Changed
Both the live agent options reader and the closed-chat reader represent an unreported model as absent. The shared save path also omits absent or empty model names supplied by other agents. Saved-record validation stays strict. Models reported later are saved at the existing session-opening and confirmed-option-change points.
Updated desktop clients advertise support for options without a model. An older paired client receives the existing quiet unsupported response for that options read until a model is reported. Explicit model reports keep their existing behavior.
Why
Fixing the reader removes the invented value before any consumer sees it. One shared save guard handles other agents without accepting invalid records or inventing a placeholder model.
Intended difference: an older paired client cannot discover thinking settings and supported controls while a session has no reported model. This prevents its model picker from crashing. Chat creation and sending messages remain available; discovery resumes when a model is reported.
Linked Issue
Found while checking OMP native chat on Windows. The missing answer in the last screenshot came from a separate, pre-existing bug: the reasoning and the answer can share one message identifier. It was fixed in #26658, which is now merged.
Visual Proof
Before: starting OMP showed “Chat could not be started.”
After, with no reported model: the chat opens with a usable composer and no blank model control. Why there's no model here: the isolated test copy of Orca hides the user's
GEMINI_API_KEY, and OMP gets its model from that key. So OMP genuinely had no model to report, which is exactly the case this PR fixes. A real user whose OMP has no model set up sees the same screen.Final Windows build, with the key given to OMP at runtime: the chat starts and shows the real model, Gemini 3.1 Pro Preview, with High thinking effort.
The same chat accepts one prompt. Its saved turn completes successfully, but the answer is missing because of the separate lost-answer bug that #26658 has since fixed. This screenshot proves startup, model reporting and prompt acceptance. The answer showing up was proven live in #26658's own testing, not re-run here.
Testing
97 tests passed across the original 10 focused files, including the exact scrubbed Windows OMP response through the real reader and save checks, Grok/OpenCode missing-model cases, a real store write and reopen after a later model report, picker rendering, and mixed-version checks in both directions using released code. The real Claude command-line suites and effort-default test passed together: 23 tests in three files. After the second authorized main merge, 140 targeted tests in 15 files, queued Node/renderer/CLI typechecks, and all 22 pre-push checks passed. Every selected CI check completed successfully on final head
ce07804a38af3c4c96aa1096b16d59fe348444d0, including Windows packaging and persistence, all five test shards, and cross-version compatibility. One unchanged Codex connection test timed out in the initial full-suite attempt; its 34-test file passed alone, and a single failed-job retry passed on the same commit. Skipped jobs are not counted as passes.Windows high-spec verification used the real OMP 17.3.8 installation and a hidden app built from final commit
418b82d2df3fe498a8684b670cb1e5fd1f4c01e4. The real UI opened OMP, showed its model, and accepted one prompt; each click had a hit test. Wrapper use and the reported model were verified before sending. The missing answer is the separate pre-existing issue above. Later-model persistence is proved by the real-store write/reopen test.The isolated test environment hides the user's
GEMINI_API_KEY; that was a test-rig difference, not Orca losing the provider key. Direct agent-protocol and command-line checks isolated that difference. A wrapper obtained the user's key at runtime for OMP alone. No key value was written to a file, log, screenshot, or report, or forwarded through Orca's environment. No login or configuration was copied or manually edited.No live macOS run was performed, as requested; regression evidence is the existing macOS OMP recordings and unit tests. No live SSH check is claimed.
Review
Fresh adversarial and independent architecture reviews passed; subsequent changes received fresh narrow reviews. Capacity-limited reviews ran as authorized fresh headless processes. Main fixed its session-host line limit in #26648. The authorized second merge keeps main’s extraction, removes this branch’s duplicate module, and leaves the host and provider-support files identical to main. A fresh reviewer confirmed all 20 startup-fix patches are unchanged. Final-head CI is complete and every selected check passed.
Related fixes, already merged: #26666 keeps an agent's own error details instead of a bare "Internal error", and #26658 fixes the lost answer. This PR changes neither.
Current head
4ee00118a2cis this branch merged with main. On that head, 19 CI checks passed, 15 were skipped and none failed.Agent skill upstream boundary
Notes
No new saved fields, relaxed validation, dependencies, or execution-host assumptions. The capability check uses authenticated client capabilities and covers local and paired desktop readers; mobile does not advertise it. The hidden app exited normally. Fresh native process checks found no remaining or unverifiable owned app processes. The key broker and all recorded wrappers/OMP children exited, and a fresh native table verified their absence. Real Windows keybindings and the original reproduction database retained their original hashes. Agent-produced session/database/cache writes are recorded in the local ledger; evidence stays outside the clean temporary checkout. Never merged.
Checklist