Skip to content

fix: repair orphaned running tool parts; stop a directory 403 from killing the TUI - #1960

Open
wqymi wants to merge 2 commits into
mainfrom
wq/fix-orphan-toolparts-and-403-toast
Open

fix: repair orphaned running tool parts; stop a directory 403 from killing the TUI#1960
wqymi wants to merge 2 commits into
mainfrom
wq/fix-orphan-toolparts-and-403-toast

Conversation

@wqymi

@wqymi wqymi commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Two independent robustness defects. Both are small and both are about surviving an
interruption, so they ride in one PR; the diffs do not overlap and can be reviewed
section by section.

A — orphaned running tool parts are never repaired

A tool part is persisted as running the moment the tool STARTS, deliberately, so the
TUI can stream progress (prompt.ts:1037/1461/1677, processor.ts:496). Only the abort
finalizer in SessionProcessor.cleanup rewrites it, and that finalizer iterated the
in-memory ctx.toolcalls map of the owning process (processor.ts:743-759). Every
exit path that skips it leaves the DB row at running forever: process kill, crash, dev
restart, a second interruption after ctx.toolcalls = {}, a readToolCall miss, or the
250 ms Deferred.await(...).pipe(Effect.timeout("250 millis")) window. There was no
startup or session-load repair anywhere (session/index.ts, storage/db.ts: zero hits).
Live evidence, re-measured across the whole store: **168 orphaned tool parts (126 running

  • 42 pending) across 92 sessions**; in one two-week-old session completed 2474 / error 105 / running 48, of which exactly one was genuinely in flight — i.e. a per-interruption leak.

This is NOT the empty-content provider 400 — proven by wire-level construction

This shape was investigated as a candidate producer of messages.<N>: user messages must have non-empty content and excluded. The exclusion is now a construction, not an
argument: a prompt captured from a real LanguageModelV3.doStream shows that an assistant
message whose ONLY part is an orphaned running/pending tool part converts to

assistant([tool-call])  +  tool([tool-result { type: "error-text",
                                               value: "[Tool execution was interrupted]" }])

yielding zero empty-content messages. message-v2.ts:886-899 (the correct lines; an
earlier revision of this description cited the stale :872-883) matches both
discriminants and substitutes a hard-coded non-empty literal that no data can empty, and
ai@6.0.168's empty-text strip (dist/index.mjs:1424) lives in the role:"user" branch
and only inspects type:"text" parts — it never drops a tool-result from a role:"tool"
block. So no tool part in any state can yield content: [].

The timeline agrees: 74 recorded 400s span 2026-07-14 → 07-28 while the first orphan-only
assistant message in that session is 2026-07-28, so 62 of 74 predate the shape, and 6 of
the 7 sessions carrying the shape have zero 400s. The real producer was a bare-string
continuation turn in ProviderTransform.ensureTrailingUserMessage — see #1948.

So the defect fixed here is data hygiene and UI truthfulness, not provider legality: the
transcript permanently shows tool calls that will never finish. It never touches the
persisted row today.

Fix, two layers:

  • cleanup() gets a second, DB-driven pass over the assistant message's own tool
    parts (MessageV2.parts(ctx.assistantMessage.id)), so a call whose registration raced
    teardown, arrived after the map was cleared, or whose readToolCall lookup missed is
    still finalized. Every tool part of that message belongs to the turn being torn down,
    so any part still pending/running is unfinalized by definition. Idempotent.
  • SessionPrompt.sweepOrphanToolParts repairs rows a previous process left behind,
    at the same recovery point as the existing sweepOrphanAssistants (start of prompt()
    for non-spawn/hook sources).

Safety of the sweep's scoping

A currently executing tool part is also running, so a naive "rewrite every running row"
sweep corrupts live turns. Two guards, both required, and the gate lives inside the
function rather than in the caller contract:

  1. Session status must be idle. busy/retry mean an active runner owns the
    session, and a tool can only execute inside a runner's turn. Same argument the shipped
    sweepOrphanAssistants caller relies on.
  2. Main slice only (sessions.messages default). SessionProcessor publishes status
    for the main slice only (if (isMain) status.set(...)), so a subagent slice can be
    executing tools while the session status reads idle — its parts are out of scope.

Both finalizers now share MessageV2.abortedToolState, so an interrupted part has one
consistent shape (status: "error", error: "Tool execution aborted",
metadata.interrupted: true, original time.start preserved).

B — a rejected directory 403 killed the TUI

server/routes/instance/middleware.ts rejects any directory outside the server cwd
(except the app-owned orchestrator workspace) with a 403. The generated SDK throws the
parsed response body with no status attached, so sync.tsx's bootstrap catch could
not tell a policy rejection from a broken server and ran await exit(e) — destroying the
renderer. A user who picked a non-whitelisted worktree lost their entire session.

The whitelist is correct policy and is unchanged. Only its recognisability and client
handling change:

  • the 403 body now carries a stable code: "directory_not_allowed" plus the rejected
    directory (new leaf module instance/access.ts owns the code + guard so the TUI does
    not import the server's instance/bootstrap graph);
  • bootstrap classifies that error as recoverable and rethrows instead of exiting;
    everything else is still fatal;
  • the worktree switch restores the previous directory, re-syncs, and toasts
    Cannot switch to <dir>: outside this server's working directory.

Tests

test/session/prompt-orphan-tool-parts.test.ts (6) — an orphaned running part is
repaired when idle; it is not touched while the session is busy, nor while it is
retry; completed parts are untouched; abortedToolState shape.

test/cli/tui/bootstrap-directory-denied.test.tsx (2) — a 403 from the middleware never
reaches the fatal-exit path (exit is never invoked and the error is handed to the
caller); a non-403 bootstrap failure still exits.

Revert probes (each src change reverted alone):

  • remove the idle gate → both safety tests fail with Expected: "running" / Received: "error"
    (exactly the live-turn corruption the gate prevents);
  • neuter the repair → Expected: "error" / Received: "running";
  • restore if (fatal)expect(failure).toBeDefined() fails, Received: undefined
    (bootstrap exited instead of surfacing), while the "still exits" test keeps passing.

bun typecheck exit 0.

Test suites compared against a pristine origin/main worktree with the identical command
(bun test test/session/ test/cron/ test/cli/tui/ --timeout 120000), the two runs
executed side by side:

pass skip fail files
pristine origin/main (60af8f1) 1015 12 10 124
this branch 1023 12 10 126

+8 pass / +2 files is exactly the two new test files. The failure sets are byte
identical (diff of the sorted (fail) lines is empty) — 10 pre-existing failures in the
session/loop busy/cancel/queue group, which flake under concurrent suite load and fail
the same way without this change.

Live-verified against mimo/mimo-v2.5 in an isolated dev home: selecting a worktree
outside the server cwd shows the toast, leaves the transcript intact, and the session
still answers a following turn.

… killing the TUI

Two independent robustness holes.

Orphaned `running` tool parts: a tool part is persisted as `running` when the
tool starts so the TUI can stream progress, and only the abort finalizer in
SessionProcessor.cleanup rewrites it. Any exit path that skips that finalizer
(process kill, crash, dev restart, a registration that raced teardown, a call
that arrived after ctx.toolcalls was cleared) leaves the row `running` forever,
so the transcript permanently shows tool calls that will never finish. Nothing
repaired them: the model-message converter synthesizes an output-error for
pending/running parts so the provider never sees a dangling tool_use, but it
never touches the persisted row.

- cleanup() now takes a second, DB-driven pass over the assistant message's own
  tool parts instead of trusting only the in-memory map.
- New SessionPrompt.sweepOrphanToolParts repairs leftovers from a previous
  process at the next prompt. A currently executing tool part is also `running`,
  so the sweep is gated on session status being idle (no active runner) and on
  the main slice only (SessionProcessor publishes status for the main slice
  only, so a subagent slice can be live while the session reads idle).
- Both finalizers share MessageV2.abortedToolState so interrupted parts get one
  consistent shape.

Directory 403 killing the TUI: the instance middleware correctly rejects a
directory outside the server cwd, but the SDK throws the parsed body with no
status attached, so bootstrap treated it as fatal and exit()ed — a user who
picked a non-whitelisted worktree lost their whole session. The rejection now
carries a stable `code`, bootstrap classifies it as recoverable and rethrows
instead of exiting, and the worktree switch restores the previous directory,
re-syncs and toasts the rejected path. Genuinely fatal bootstrap failures still
exit.
Two bootstrap callers are fire-and-forget: onMount and the
server.instance.disposed handler. bootstrap() rethrows the recoverable
directory-denied rejection so an interactive caller can restore the
previous directory and explain itself, but these two have no such
caller, so a denied directory left the TUI with stale data and no
indication why -- the same silent failure this PR exists to remove, just
on the paths nobody was watching.

The initial directory is not always the server cwd: launching with
MIMO_DEV_CWD records the worktree ROOT as the project sandbox while the
server cwd is a subdirectory, which is exactly the repro this PR's own
403 test uses. So the mount path is reachable, not theoretical.

Uses useToastOptional rather than useToast. Requiring the toast stack
(theme + terminal dimensions + border) would make a data-sync context
untestable for a presentation concern -- SyncProvider's own tests render
without ToastProvider, and adding it pulled in the theme and language
contexts too.

Also documents why the two orphan sweeps at the recovery point cannot
share one message fetch: sweepOrphanAssistants reads every slice
(agentID '*') while sweepOrphanToolParts reads main only, and that
difference is what keeps it from rewriting a live subagent's running
part. A shared fetch would take the wider read and re-filter, which is
where that property would be lost.
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