Repository navigation
fix(agent-session): refuse a pre-commit structured create with an envelope - #18697
Conversation
…elope The create route refused by throwing, which reaches a client as a generic transport error indistinguishable from a lost answer — so desktop parked the launch as visibility-unknown with no chat and no terminal. Convert the whole pre-commit span, everything before `attach`, into a refusal envelope carrying a code, and name the definitive-refusal allowlist the fallback decision needs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds definitive refusal classification for agent-session creation. It converts pre-attachment preparation failures into refusal envelopes with wire codes. The Merge Risk: ⚪ Minimal · up to Pre-attach structured-session failures now return a refusal that can safely fall back to a Codex terminal, while ambiguous outcomes continue to avoid duplicate-session fallback. The covered behavior is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
structured-agent-session-precommit-refusal.ts— newresolveUncommittedStructuredCreate()wrapper that turns any pre-attachthrow into a{ ok: false, refusal }envelope; code-carrying throws keep their code, code-less ones becomestructured_agent_session_unsupportedwith the cause preserved and logged.structured-agent-session.ts—agentSession.createnow runs the fingerprint check throughensureHostInstalled/requireHostinside the wrapper, leavingattachand tab publication outside it.agent-session-definitive-refusal.ts— newisDefinitiveAgentSessionCreateRefusal()allowlist (structured_agent_session_unsupported+ RPCmethod_not_found), exported but deliberately unconsumed until bug 1 wires it in.
Verified the refactor is behavior-preserving (both branches still call ensureHostInstalled before requireHost, in the same order; the post-attach agent_session_operation_unknown path is untouched), that structured_agent_session_unsupported is absent from AGENT_SESSION_RPC_ERROR_CODES so the old thrown path genuinely flattened to runtime_error, and that the boundary tests assert attach is uncalled on refusal and stays unknown on attach/publish failure. All 15 new tests and the 24 existing structured-agent-session.test.ts tests pass locally.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Visual proof — refused structured launch now falls back to a terminalDriven through the real UI on an isolated dev instance (own CDP port, own user-data dir, own scratch repo), Playwright over CDP. Tested at PR head How the pre-commit failure was triggered without touching product code: placed a regular file at Flow in both runs: New tab → Codex, structured native chat enabled, local execution host. BEFORE — fix ablated (the
|
# Conflicts: # src/main/runtime/rpc/methods/structured-agent-session.ts
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- De-branded refusal copy —
precommitRefusalnow emits "structured agent chat" rather than "Codex", with test assertions pinning the agent-agnostic wording. - Restored the
supportsCreategate on attach-shaped entries — extractedresolveClientSuppliedAttachand routed bothagentSession.create(client-supplied location) andagentSession.ensurethrough it, re-applying the host capability check the prior commit dropped;createnow answers an unsupported client location with a definitive refusal envelope whileensurekeeps its top-level error. - Restored per-agent tab publication — the worktree branch publishes the tab with
resolved.agentagain, dropping the hardcoded'codex'agent the prior commit introduced. - Gated the terminal fallback on definitive refusals —
launchStructuredAgentSessionnow throwsStructuredAgentSessionCreateRefusalError(carryingcode) only for definitive refusals; unknown refusals and unclassified transport failures throw plain errors thatlaunchAndReconcileroutes through recover/retry instead of a sibling terminal, and old-hostmethod_not_found/structured_agent_session_unsupportedtransport errors are reclassified as definitive.
Verified all 78 tests across the five touched suites pass locally, and that the net structured-agent-session.ts change faithfully restores main's supportsCreate and per-agent tab behavior rather than regressing it.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Visual proof — structured create refusal fallback (HEAD
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Mobile refusal classification aligned with desktop —
createMobileStructuredCodexSessionnow routes both the top-level error and envelope-refusal paths through a newclassifyCreateRefusalhelper backed by the sharedisDefinitiveAgentSessionCreateRefusalallowlist, so onlystructured_agent_session_unsupported/method_not_foundfall back to a legacy terminal and every other outcome (agent_session_ownership_unknown,agent_session_operation_unknown,runtime_error, unknown future codes) stays unknown with no sibling session. - Tests pin the definitive-vs-unknown boundary — added mobile tests covering top-level and envelope classification, updated the fallback test to a genuinely definitive code, and added a no-sibling assertion for unknown top-level outcomes.
Verified the mobile change closes the same duplicate-session gap the desktop renderer already fixed: agent_session_ownership_unknown previously classified as failed (fall back to terminal), now correctly unknown. Ran the three touched suites — 23 mobile tests plus 6 shared allowlist tests — all pass.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Engineering review complete at
From the code-review and validation side, this PR is ready to merge. |
The rebase onto #18697 brought its definitive-refusal tests in cleanly, but they call the pre-rename createMobileStructuredCodexSession, and mobile tsc excludes test files so nothing caught it. Retarget them and give the agent-copy test a code that is actually in the definitive allowlist - agent_session_refused now correctly stays unknown, so it never reached the failure copy it asserted.
The rebase onto #18697 brought its definitive-refusal tests in cleanly, but they call the pre-rename createMobileStructuredCodexSession, and mobile tsc excludes test files so nothing caught it. Retarget them and give the agent-copy test a code that is actually in the definitive allowlist - agent_session_refused now correctly stays unknown, so it never reached the failure copy it asserted.
* feat(mobile): structured native Claude chat Mobile already spoke the structured agent-session protocol for Codex, and the host already had a Claude capability gate — mobile just never advertised it, so `projectAgentSessionTabsOut` stripped every Claude tab before it left the desktop. The structured lane in mobile/ turned out to be agent-agnostic already (shared reducer, message projection, option catalog, prompt tokens), so this opens the gate rather than building a second lane: - advertise `agent-session.structured.claude.v1` - resolve any structured provider in `resolveMobileNativeChat` via the shared `isAgentSessionHandleProvider`, instead of a `'codex'` literal - widen the `agent-session` route type off `'codex'` - route bare Claude launches through `agentSession.createSupport` like Codex, which still degrades to a terminal when the host refuses (remote, WSL, win32, managed-account mismatch, or structured chat switched off) Deduplicate the create envelope. Renderer and mobile each assembled the `agentSession.create` params by hand; the fingerprint has to be computed over the same fields the host recomputes, so both now build it in one shared `structuredAgentSessionCreateParams`. Mobile's Codex-only launcher becomes `createMobileStructuredAgentSession(client, worktreeId, agent)` and reuses the shared display-name map; two copies of a random-UUID fallback collapse into one. Answer grouped Claude questions. A Claude AskUserQuestion carrying more than one question — or one multi-select question — is emitted with the real content in `body.questions` and the flat `options` left EMPTY, so mobile rendered a card with nothing to tap and the turn stalled with no way out. Codex never emits this shape. The phone has room for one question at a time, so the group is answered as steps and submitted once, reusing the shared `encodeAgentSessionQuestionAnswers` / `isValidAgentSessionQuestionAnswers` rather than a second encoding. Prompt responses move into `useMobileStructuredPromptResponses` because grouped questions carry a multi-step draft the rest of the session does not touch, and the session hook was at the 300-line cap. Pin the mobile capability list against the host's parser bounds: it fails closed to NO capabilities when the array exceeds 64 entries, which would look exactly like an old client. Re-pin mobile-session-route-parity: the create-actions edit drops one runtime string literal and changes one nested function body. Ablated to confirm that file is the sole cause. * fix(mobile): derive the grouped-question draft instead of clearing it in an effect The React Doctor gate flagged the session-change reset as a state adjustment after a prop change, which renders the stale draft for a frame. Store the session the answers were collected in alongside them and check it on read, so a session switch drops the draft during render with no effect at all. * test(mobile): pin that grouped steps key apart when the questions read identically Claude can ask the same text twice in one group (once per file, say). The view keys the question card by its projected content, so identical wording must still key apart or step 1's checkboxes would be submitted as step 2's answer. * fix(mobile): harden grouped Claude question answers * fix(mobile): retry transient structured support probes * fix(mobile): preserve grouped prompt response compatibility * fix(mobile): preserve tokenless duplicate choice identity * fix(mobile): point the launch tests at the generalized create API The rebase onto #18697 brought its definitive-refusal tests in cleanly, but they call the pre-rename createMobileStructuredCodexSession, and mobile tsc excludes test files so nothing caught it. Retarget them and give the agent-copy test a code that is actually in the definitive allowlist - agent_session_refused now correctly stays unknown, so it never reached the failure copy it asserted. * test(mobile): re-pin route parity after the rebase onto main Main moved its own runtime-string pin to 547; this branch drops the 'codex' literal from the create-actions gate. Ablated against main's pins to confirm that file is the sole cause before re-deriving. --------- Co-authored-by: Merge Sim <sim@local>
…elope (stablyai#18697) * fix(agent-session): refuse a pre-commit structured create with an envelope The create route refused by throwing, which reaches a client as a generic transport error indistinguishable from a lost answer — so desktop parked the launch as visibility-unknown with no chat and no terminal. Convert the whole pre-commit span, everything before `attach`, into a refusal envelope carrying a code, and name the definitive-refusal allowlist the fallback decision needs. * fix(agent-session): gate legacy fallback on definitive refusals * fix(mobile): preserve unknown structured create outcomes --------- Co-authored-by: Merge Sim <sim@local>
* feat(mobile): structured native Claude chat Mobile already spoke the structured agent-session protocol for Codex, and the host already had a Claude capability gate — mobile just never advertised it, so `projectAgentSessionTabsOut` stripped every Claude tab before it left the desktop. The structured lane in mobile/ turned out to be agent-agnostic already (shared reducer, message projection, option catalog, prompt tokens), so this opens the gate rather than building a second lane: - advertise `agent-session.structured.claude.v1` - resolve any structured provider in `resolveMobileNativeChat` via the shared `isAgentSessionHandleProvider`, instead of a `'codex'` literal - widen the `agent-session` route type off `'codex'` - route bare Claude launches through `agentSession.createSupport` like Codex, which still degrades to a terminal when the host refuses (remote, WSL, win32, managed-account mismatch, or structured chat switched off) Deduplicate the create envelope. Renderer and mobile each assembled the `agentSession.create` params by hand; the fingerprint has to be computed over the same fields the host recomputes, so both now build it in one shared `structuredAgentSessionCreateParams`. Mobile's Codex-only launcher becomes `createMobileStructuredAgentSession(client, worktreeId, agent)` and reuses the shared display-name map; two copies of a random-UUID fallback collapse into one. Answer grouped Claude questions. A Claude AskUserQuestion carrying more than one question — or one multi-select question — is emitted with the real content in `body.questions` and the flat `options` left EMPTY, so mobile rendered a card with nothing to tap and the turn stalled with no way out. Codex never emits this shape. The phone has room for one question at a time, so the group is answered as steps and submitted once, reusing the shared `encodeAgentSessionQuestionAnswers` / `isValidAgentSessionQuestionAnswers` rather than a second encoding. Prompt responses move into `useMobileStructuredPromptResponses` because grouped questions carry a multi-step draft the rest of the session does not touch, and the session hook was at the 300-line cap. Pin the mobile capability list against the host's parser bounds: it fails closed to NO capabilities when the array exceeds 64 entries, which would look exactly like an old client. Re-pin mobile-session-route-parity: the create-actions edit drops one runtime string literal and changes one nested function body. Ablated to confirm that file is the sole cause. * fix(mobile): derive the grouped-question draft instead of clearing it in an effect The React Doctor gate flagged the session-change reset as a state adjustment after a prop change, which renders the stale draft for a frame. Store the session the answers were collected in alongside them and check it on read, so a session switch drops the draft during render with no effect at all. * test(mobile): pin that grouped steps key apart when the questions read identically Claude can ask the same text twice in one group (once per file, say). The view keys the question card by its projected content, so identical wording must still key apart or step 1's checkboxes would be submitted as step 2's answer. * fix(mobile): harden grouped Claude question answers * fix(mobile): retry transient structured support probes * fix(mobile): preserve grouped prompt response compatibility * fix(mobile): preserve tokenless duplicate choice identity * fix(mobile): point the launch tests at the generalized create API The rebase onto stablyai#18697 brought its definitive-refusal tests in cleanly, but they call the pre-rename createMobileStructuredCodexSession, and mobile tsc excludes test files so nothing caught it. Retarget them and give the agent-copy test a code that is actually in the definitive allowlist - agent_session_refused now correctly stays unknown, so it never reached the failure copy it asserted. * test(mobile): re-pin route parity after the rebase onto main Main moved its own runtime-string pin to 547; this branch drops the 'codex' literal from the create-actions gate. Ablated against main's pins to confirm that file is the sole cause before re-deriving. --------- Co-authored-by: Merge Sim <sim@local>
ELI5
When Orca can't start a Codex chat in a workspace, the host used to fail in a way the app couldn't recognise as a refusal — it looked identical to "the connection broke."
So the app did nothing. You clicked New tab → Codex and got neither a chat nor a terminal. No error, no toast, nothing.
Now the host says "no, and here's the reason code" in a form the app understands, so it falls back and gives you a terminal.
Before / after
Visual proof
Corrected — this PR does change user-visible behaviour, and an earlier draft of this description wrongly said
N/A. Captured as a runtime differential on an isolated dev instance:[agent-session] create refused before it committed anything.The failure was triggered without editing product code — a regular file placed where the session store expects a directory makes the store's
mkdirfail. That is a genuine code-less pre-commit failure, exactly the class this PR is about.What changed
The whole side-effect-free pre-attach span of
agentSession.createnow returns a refusal envelope carrying a code, instead of throwing. Converting the span rather than a list of throw sites matters: alongside the code-carrying throws there is a code-less class (unknown worktree, store/host-install failure) that per-site conversion misses entirely.Also ships
src/shared/agent-session-definitive-refusal.ts— the definitive-vs-unknown allowlist, exported for the follow-up fix and deliberately not consumed here.Deliberately left outside the span: the capability gate (it must keep hiding the surface from capability-less clients, per its own contract) and the fence throw (a programming-error passthrough). Both verified correct in review.
Cross-version
Safe. The envelope shape has been on the create wire since the capability shipped — no new opcode, no new field. Shipped mobile lacks the capability entirely; desktop is same-build. Anything not on the allowlist stays unknown, so no client creates a duplicate.
Evidence
Ablation red for the right reason: reverting only the changed file fails exactly the 4 refusal-conversion tests, while all 5 boundary tests stay green. 15 new tests; independent review returned CLEAN.