Repository navigation
fix(agent-session): never open a sibling terminal on an unproven create - #18735
Conversation
![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
- Definitive-vs-unknown refusal classification —
launchStructuredCodexSessionnow gates the legacy-terminal fallback onisDefinitiveAgentSessionCreateRefusal, soagent_session_operation_unknownandagent_session_ownership_unknownroute down the existing lost-reply path (verify publication, replay the identical envelope, park as visibility-unknown) instead of opening a sibling terminal. - Error class split — a shared base (
StructuredAgentSessionCreateError) carriescode;StructuredAgentSessionCreateRefusalErrorand the newStructuredAgentSessionCreateUnknownOutcomeErrorextend it soinstanceofis the verdict. All eight consumers keep meaning what they say, and each already has an unknown-outcome branch to route into (verified:structured-agent-session-launch{,-recovery,-callers}.ts,NonGitFolderDialog.tsx,folder-workspace-composer-submit.ts,full-creation-structured-launch.ts,launch-work-item-direct-agent-routing.ts,worktree-creation-structured-session.ts,repo-add-actions.ts). - Focus-intent asymmetry —
abandonStructuredAgentSessionLaunchIntentnow runs only on a definitive refusal, so an unknown outcome keeps its focus intent for recovery to adopt the published tab. - Tests — the new fallback suite drives the real
launchStructuredCodexSessionthroughstartStructuredCodexLaunch(RPC mocked, not the module under test) and counts legacy terminals, with unit coverage pinning the carriedcodeand the focus-intent asymmetry. I ran both changed files: 9/9 green, and the "no sibling" cases are exact assertions that would fail under the old classification.
ℹ️ One pre-commit "proven no" stays off the allowlist
The allowlist is narrower than the set of refusals the host can emit before attach. agentSessionFingerprintConflict returns agent_session_operation_conflict inside resolveUncommittedStructuredCreate — before anything commits — yet it is not allowlisted, so a fingerprint-mismatch create now reconciles rather than falling back to a terminal. That is a defensible reading (a fingerprint mismatch implies the client and host disagree, so "reconcile, don't spawn a sibling" is the safer default, and it is only reachable under version skew), but it is worth confirming this was intended rather than an oversight.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review, this PR added handling for thrown transport-level failures on agentSession.create:
- Thrown
method_not_foundis now definitive —launchStructuredCodexSessionwraps the create RPC in atry/catch; a thrownRuntimeRpcCallErrorwhose code is on the shared allowlist (currently justmethod_not_found) is abandoned and rethrown asStructuredAgentSessionCreateRefusalError, so an older runtime that never registeredagentSession.createfalls back to a single legacy terminal instead of failing silently. - Thrown unknown failures stay unknown — a thrown
runtime_timeout/remote_runtime_unavailable, or any non-RuntimeRpcCallError, is rethrown untouched, preserving the focus intent so the existing lost-reply recovery can still reconcile a session the host may have created. - Tests — unit coverage pins the thrown-path classification (allowlisted code vs. a bare
Error('method_not_found')vs. unknown codes), and the end-to-end fallback suite adds an "older runtime has no create method" case asserting exactly one terminal and a fully-settled (idle) launch state.
I ran both changed files: 14/14 green, and the new method_not_found fallback case is stable across repeated runs.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Visual proof @
|
|
Final review status at
Integration gate: PR #18697 is still open, and its branch moved during this review. The base remains correctly stacked on |
fc29590 to
c3bfc03
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review, this PR was re-expressed onto the advanced base brennanb2025/bug2-refusal-envelope — which renamed the edited module launch-structured-codex-session.ts → launch-structured-agent-session.ts and landed the thrown method_not_found handling the earlier revision added. The delta is a faithful rename plus a narrowing to what the base still lacks:
- Agent-agnostic rename —
launchStructuredCodexSession→launchStructuredAgentSession,startStructuredCodexLaunch→startStructuredAgentLaunch(worktreeId, agent), with nocodex-named references left behind in the module or its callers. - Named unknown-outcome error — the base's anonymous
Object.assign(new Error(message), { code })becomesStructuredAgentSessionCreateUnknownOutcomeError, a sibling ofStructuredAgentSessionCreateRefusalErrorover a shared privateStructuredAgentSessionCreateErrorbase. - Classification short-circuit —
definitiveStructuredAgentSessionCreateErrorCodereturns oninstanceof StructuredAgentSessionCreateErrorbefore the transport-error matcher runs, so a refusal message ending in a definitive token (e.g.Owner check failed: method_not_found) can no longer invert an unknown outcome into a definitive one; the!result.okbranch now gates directly onisDefinitiveAgentSessionCreateRefusal(code). - End-to-end guard test — the new
structured-agent-session-launch-refusal-fallback.test.tsdrives the real classifier throughstartStructuredAgentLaunchand counts legacy terminals.
I ran the two changed test files (22/22) plus the consumer suites (42/42), all green, and confirmed the rename is complete and the classification logic matches the previously reviewed behavior.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
An `agentSession.create` the host could not confirm — it committed the session but could not publish its tab, and answered `agent_session_operation_unknown` — was rejected with a bare `Error` carrying a `code`. Nothing in the type said "unknown", so the verdict lived only in the code string, and the shared transport matcher was still free to re-read that error's *message*: an unknown refusal whose text ends in a definitive token (`Owner check failed: method_not_found`) classified as definitive, which is exactly the answer that permits a legacy sibling terminal. Make the class the verdict. `StructuredAgentSessionCreateUnknownOutcomeError` is a sibling of `StructuredAgentSessionCreateRefusalError`, not a subclass, so the nine existing `instanceof` consumers keep reading "refusal" as "you may fall back" with zero edits, and an unknown outcome flows down the lost-reply path instead — replaying the same envelope, re-publishing the tab the host failed to publish, and parking as visibility-unknown rather than creating anything. Classification now short-circuits on our own classes, so a message we wrote can never invert the verdict we already reached. Adds an end-to-end guard that drives the real classifier through `startStructuredAgentLaunch`: an unknown outcome opens zero legacy terminals, a definitive refusal opens exactly one. Ablating the branch turns that green suite red with `['legacy-terminal']` — the duplicate session the guard exists to prevent.
c3bfc03 to
9855d1d
Compare
|
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; 1 remains after this review. 📝 WalkthroughWalkthroughThe change adds a shared session-create error base and a distinct unknown-outcome error. Launch handling keeps focus for unknown outcomes and abandons it for definitive refusals. Tests cover error classification, refusal messages containing Merge Risk: ⚪ Minimal · up to Structured session creation now avoids opening a sibling terminal when the create result is uncertain, while preserving legacy fallback for confirmed refusals. The covered behavior has no remaining concrete merge-blocking risk. 🚥 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 |
…te (stablyai#18735) An `agentSession.create` the host could not confirm — it committed the session but could not publish its tab, and answered `agent_session_operation_unknown` — was rejected with a bare `Error` carrying a `code`. Nothing in the type said "unknown", so the verdict lived only in the code string, and the shared transport matcher was still free to re-read that error's *message*: an unknown refusal whose text ends in a definitive token (`Owner check failed: method_not_found`) classified as definitive, which is exactly the answer that permits a legacy sibling terminal. Make the class the verdict. `StructuredAgentSessionCreateUnknownOutcomeError` is a sibling of `StructuredAgentSessionCreateRefusalError`, not a subclass, so the nine existing `instanceof` consumers keep reading "refusal" as "you may fall back" with zero edits, and an unknown outcome flows down the lost-reply path instead — replaying the same envelope, re-publishing the tab the host failed to publish, and parking as visibility-unknown rather than creating anything. Classification now short-circuits on our own classes, so a message we wrote can never invert the verdict we already reached. Adds an end-to-end guard that drives the real classifier through `startStructuredAgentLaunch`: an unknown outcome opens zero legacy terminals, a definitive refusal opens exactly one. Ablating the branch turns that green suite red with `['legacy-terminal']` — the duplicate session the guard exists to prevent. Co-authored-by: Merge Sim <sim@local>
#18697 has merged (2026-09-05), so this now targets
maindirectly and was rebased onto it — the stale stacked history is gone and only this PR's own commit remains.What this PR is, precisely: the duplicate-session behaviour is already fixed on
main. This adds the typedStructuredAgentSessionCreateUnknownOutcomeError, closes a latent classification hole, and pins the behaviour with a regression suite so it cannot silently come back. It is hardening, not a behaviour fix — read the before/after below with that in mind.ELI5
You click "start a Codex chat". Orca asks the host to create one. Usually the host says "done" or "no". But sometimes it says "I might have done it — I can't confirm": it really did create the session, then failed on the last step of showing you the tab.
If the desktop files that maybe under no, it helpfully opens a plain terminal for you instead — right next to the chat session that already exists. You asked for one thing and got two, and the extra one is the one you didn't ask for.
The base branch already stopped that from happening. What it did not do is write the difference down anywhere a reader — or the code — can see it: an unconfirmed outcome came back as a plain
Errorwith a code string stapled on. This PR gives that outcome a name, so "I don't know" is a thing the type system knows about rather than a convention every future consumer has to re-derive, and adds the end-to-end test that actually counts the terminals.Before / after
agent_session_operation_unknown)agent_session_ownership_unknown)structured_agent_session_unsupported)The only row that gets a terminal is the one where the host proved it created nothing.
What changed in this PR
Already on the base, not re-added here: the
isDefinitiveAgentSessionCreateRefusalimport,codeonStructuredAgentSessionCreateRefusalError,definitiveStructuredAgentSessionCreateErrorCode, the thrown-error classification, and the mocked-module unit test instructured-agent-session-launch.test.ts.What this PR adds:
StructuredAgentSessionCreateUnknownOutcomeError. The base rejected an unconfirmed create withObject.assign(new Error(message), { code })— an anonymous error whose verdict existed only as a string. It is now a named sibling ofStructuredAgentSessionCreateRefusalError(both extend a privateStructuredAgentSessionCreateErrorthat ownscode), so "the host may already hold this session" is a type, not a convention.Classification short-circuits on our own classes. This closes a real latent hole, not just a naming one.
definitiveStructuredAgentSessionCreateErrorCodefalls through tohasRuntimeRpcErrorCode, which — deliberately, for re-wrapped transport errors — also matches the message. An unknown refusal carrying a host message likeOwner check failed: method_not_foundtherefore classified as definitive on the base:isDefinitiveStructuredAgentSessionCreateErrorreturnedtruefor an outcome that proves nothing. Verified by ablation (below). With the typed class, the verdict is read off the class and a message we wrote can never invert it.The end-to-end guard.
structured-agent-session-launch-refusal-fallback.test.ts(new; absent on the base) drives the real classifier throughstartStructuredAgentLaunchand counts legacy terminals through the fallback every caller uses. The base's coverage of this behaviour mockslaunch-structured-agent-sessionout entirely, so it asserts the caller's contract but never the classification itself.Making the class carry the verdict rather than adding a
definitiveflag was deliberate, and that finding still holds after the rename — the consumer list grew from eight sites to nine (structured-agent-session-launch{,-recovery,-callers}.ts,NonGitFolderDialog.tsx,folder-workspace-composer-submit.ts,full-creation-structured-launch.ts,launch-work-item-direct-agent-routing.ts,worktree-creation-structured-session.ts,repo-add-actions.ts), and because the unknown-outcome error is a sibling rather than a subclass, all nineinstanceofchecks keep meaning what they say with zero edits. A flag would have needed the same nine-site audit and left the original mistake re-makeable.Two consequences worth naming:
agent_session_operation_unknownon the create route meanspublishStructuredAgentSessionTabthrew after the session committed. Replaying the same envelope replays the attach and re-attempts the publication, so recovery doesn't just avoid a duplicate — it usually produces the chat the user asked for.abandonStructuredAgentSessionLaunchIntentruns only on a definitive refusal; abandoning it on a maybe would leave a session that later publishes without focusing.Per
CORRECTIONS.md,agentSessionRefusalOperationStateis deliberately not reused for this decision — it answers "did this durably settle?", for whichstructured_agent_session_unsupportedis correctly pending-admission, the opposite of what the fallback needs.Tests
structured-agent-session-launch-refusal-fallback.test.ts(new, 4 cases) mocks the RPC rather than the module under test:agent_session_operation_unknown⇒ no sibling, visibility-unknown, one error toast;agent_session_ownership_unknown⇒ no sibling;structured_agent_session_unsupported⇒ exactly one terminal and exactly one create (no pointless replay);agentSession.createmethod ⇒ exactly one terminal, one call, launch state back to idle.launch-structured-agent-session.test.tsgains the class assertion on the unknown outcome and the message-sniffing regression case.Ablation (two, both run).
{ ok: false }becomes a refusal — the original defect): 5 tests go red, and with the class assertion relaxed so it can't short-circuit, the "no sibling" cases fail onexpected [ 'legacy-terminal' ] to deeply equal []. The guard catches the duplicate session itself, not just a type name.Object.assign(new Error(message), { code })): exactly 4 tests go red, all on class identity — which is the honest measure of what this PR adds over the base. A targeted probe under that ablation confirmsisDefinitiveStructuredAgentSessionCreateErrorreturnstrueforagent_session_ownership_unknownwith the messageOwner check failed: method_not_found, i.e. hole (2) above is real on the base and closed here.Verification
pnpm tcclean (exit 0, run once) · 8 consumer suites / 66 tests green (renderer launch, launch-recovery, structured-chat guard, work-item routing, worktree-creation ×2, composer-state full-creation, and the new fallback suite) ·pnpm run check:code-quality:changed— 0 new findings ·oxfmton changed paths only ·git status --porcelainempty.Scope
Bug 1 only. Bugs 3 and 4 from the plan are out of scope here; bug 4 in particular is not desktop-inert (it also gates
restoreStructuredTabsIfSupported) and ships separately.