Skip to content

fix(session): never let arg recovery rewrite a route into a new child - #1962

Closed
wqymi wants to merge 1 commit into
mainfrom
fix/session-recover-no-silent-create
Closed

fix(session): never let arg recovery rewrite a route into a new child#1962
wqymi wants to merge 1 commit into
mainfrom
fix/session-recover-no-silent-create

Conversation

@wqymi

@wqymi wqymi commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

The defect

recoverSessionArgs (src/tool/session.ts:338 at origin/main) is the shell-mode fallback for a malformed session tool call. Its last branch synthesizes a create from any payload that carries a task string:

if (typeof obj.task === "string") {
  const op: Record<string, unknown> = { action: "create", task: obj.task }
  ...

mimo-v2.5 repeatedly emits a FLATTENED shape instead of the nested one:

{"operation":"send","sessionID":"ses_0582701b9ffenkU394MLPdIi6d","task":""}

strictObject rejects that with "Invalid arguments" today, so the recovery path is not reached in practice (the call errors loudly first and the model retries correctly — verified in live runs). But if it ever were, a correct route-to-existing-child send would be silently converted into a NEW child create — the exact route-first violation #1741 exists to prevent, and invisible: no error surfaces, you just get a duplicate child session doing the work the standing child should have picked up.

The same hazard applies to {"action":"send","sessionID":…,"task":…}, and to any {task, sessionID} pair with no verb at all.

The fix, and why this shape

Two changes, both in recoverSessionArgs:

  1. Reconstruct the flattened shape properly rather than merely erroring. The top-level discriminator — a bare operation verb string that survived the JSON.parse attempt, or action — is re-nested with its sibling operands and validated against the real parameters union. On success the model's actual intent runs (send stays a send); on failure we return undefined.

    Justification for recovering rather than only erroring: this is a real, repeatedly observed model output, the intent is unambiguous (the verb is stated explicitly), and the reconstruction is checked — so it cannot misroute. Validation is load-bearing here, not decoration: shell-wrap.ts:44-47 hands a recovered value straight to def.execute without re-validating it against parameters (the comment there claiming otherwise is wrong), so an unvalidated re-nest could have pushed a half-built op into the send branch. A loud error remains the outcome for anything that does not validate.

  2. Never synthesize a create when the payload carries routing evidence. If any of sessionID / session_id / sessionIDs / question / target is present, the model named an existing session (or an ask/grant target) and cannot have meant "spawn a new child" — recovery declines and returns undefined, so the call errors loudly and the model self-corrects. A loud error that makes the model retry is acceptable; silent misrouting is not.

A bare {task} with no routing evidence still recovers into a create, unchanged.

Tests (test/tool/session-tool.test.ts)

  • NEVER yields a create when the payload carries a routing field — a battery of malformed payloads (sessionID, session_id, sessionIDs, question, target, and one with create-ish decorations alongside a sessionID) each assert action !== "create" and that recovery declines.
  • reconstructs the FLATTENED send that mimo-v2.5 actually emits — both the operation:"send" and action:"send" variants become {operation:{action:"send",sessionID,task}}.
  • reconstructs other flattened operationsstatus, cancel, ask (with its snake_case session_id), list, and a flattened create.
  • a flattened operation that does not validate fails loudly instead of degrading to a createsend missing task, send missing sessionID, and unknown verbs all return undefined rather than falling through to the create branch.
  • a bare {task} with no routing evidence still creates (unchanged) plus all seven pre-existing recoverSessionArgs tests, untouched and passing.

Revert-probed: with the function body restored to its origin/main form, exactly these 4 new tests fail, the central one verbatim as

error: expect(received).not.toBe(expected)
Expected: not "create"
(fail) recoverSessionArgs > NEVER yields a create when the payload carries a routing field

bun typecheck exits 0; test/tool/session-tool.test.ts is 56 pass / 1 skip / 0 fail.

recoverSessionArgs' bare-{task} fallback synthesized {action:"create"} from ANY
payload carrying a `task` string. mimo-v2.5 repeatedly emits a FLATTENED shape
({"operation":"send","sessionID":"ses_…","task":"…"}) which strictObject rejects,
so had that recovery path been reached a correct route-to-existing-child `send`
would have been silently converted into a NEW child `create` — the route-first
violation #1741 exists to prevent, and invisible: no error, just a duplicate
child. (Today the strict-parse error fires first, so this is latent.)

Recovery now:
- reconstructs the flattened shape properly — the top-level discriminator
  (`operation` verb string or `action`) plus its sibling operands are re-nested
  and validated against the real union, so the model's actual intent runs. This
  shape is a real, repeatedly-observed model output, and validating here is what
  makes it safe: shell-wrap hands a recovered value to def.execute WITHOUT
  re-validating against `parameters`.
- refuses to synthesize a `create` when the payload carries routing evidence
  (sessionID / session_id / sessionIDs / question / target), and returns
  undefined so the call errors loudly and the model self-corrects.
@wqymi

wqymi commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Folded into #1741 as 1dabcc3 (cherry-picked cleanly, no conflicts; both sides' tests pass: 71 pass / 1 skip / 0 fail on test/tool/session-tool.test.ts + test/session/orchestrator-active-sessions.test.ts, typecheck exit 0).

Reason: this is the same defect as #1741's, not a neighbouring one. recoverSessionArgs silently rewriting a send (route) into a create (new child) IS the route-first failure at the tool layer — the orchestrator intends to route and a new child appears instead. It also touches exactly the two files #1741 already modifies (src/tool/session.ts, test/tool/session-tool.test.ts), so shipping it separately would have guaranteed a merge conflict for no benefit.

Branch retained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant