Shape the no-terminal-event turn_failed message for classification

[tino097]

gave the no-terminal-event turn_failed message a type prefix so cowork-server can classify it

Linear: https://linear.app/mindsdb/issue/ENG-1894/an-unexpected-error-occurred-with-no-diagnostic-info-in-mindshub-web

🤖 Generated with Claude Code

[pnewsam]

Code Review

Verdict: COMMENT
Model: claude-opus-4-8[1m]

Reviewed the two-file diff: the no-terminal-event turn_failed string change (anton/cloud_turn/__main__.py:338) plus the new anton-side test. The change is safe (no regression). It is inert if merged alone but correct when landed together with its companion cowork-server#385, which adds the matching TurnInterrupted allowlist branch that surfaces this message to the user. Treat the item below as a merge-coupling requirement, not a code defect.

Validation

• Traced the consumer: cowork-server .../producer.py → remote_turn_error() in cowork/handlers/turn_errors.py. On base cowork-server, an unrecognized type name (TurnInterrupted) falls through to GENERIC_TURN_ERROR_MESSAGE, so the curated string is discarded and the user still sees "An unexpected error occurred."
• cowork-server#385 adds a TurnInterrupted allowlist branch (turn_errors.py:878) that passes the message through — so anton#396 + cowork-server#385 together deliver the message end-to-end.
• The new test's async generator, cancel-before-completion path (reaches the finally terminal emit), and pytest.raises(asyncio.CancelledError) are all sound.

Findings

• Major (merge coupling, not a code fix): Do not merge anton#396 alone — on base cowork-server the TurnInterrupted: string still resolves to the generic message, so ENG-1894 stays unfixed for the user. Land it together with (or after) cowork-server#385. Consider noting the dependency in the PR description.
• Security cross-check for #385: cowork-server#385 passes this message to the client/DB verbatim, trusting it to be a static, self-authored literal. This diff sets it to a fixed "TurnInterrupted: ..." literal today — please keep it a fixed literal (no provider/runtime/path interpolation), otherwise #385 would leak that text to clients.
• Minor: The new test asserts only that anton emits the TurnInterrupted: prefix; it never exercises remote_turn_error, so it passes while the actual user-facing outcome depends entirely on the companion PR. Consider a cross-repo contract test (or a comment pointing at #385) so a future edit to the classifier can't silently re-break this.

[alecantu7]

Reviewed as part of a pass over all four ENG-1894 PRs together, since the fix only works if the whole chain connects. Nothing here is unsafe to merge — the notes are about whether the line lands where the ticket needs it.

The branch this shapes is close to unreachable in the pod

anton/cloud_turn/__main__.py — the finally branch is guarded by terminal_emitted, which is set both on normal completion and in the except Exception handler. So the new string is emitted only when a BaseException that is not an Exception escapes.

Probed executably in a worktree at this head: a session raising asyncio.CancelledError / SystemExit / KeyboardInterrupt reaches it (all three emit TurnInterrupted: ...), while an ordinary RuntimeError emits _scrub(exc) → RuntimeError: boom and never touches the new line.

In the deployed k8s path nothing inside the pod produces those BaseExceptions: main() does a bare asyncio.run(stream_turn(raw_line, emit)) with no signal handler, no timeout and no cancellation, and anton's own internal cancels are caught locally (core/dispatch/local_runtime.py:119,295, core/backends/local.py:926, core/backends/scratchpad_boot.py:507).

The failures that actually look like "the turn ended unexpectedly" are synthesized by the controller, not the pod — scratchpad-controller src/scratchpad_controller/main.py:825-832 publishes turn_failed {'error': 'pod stream ended without a terminal event[; stderr tail: …]'} for the OOM-kill / exec-drop case, and by then it has already breaked out of the read loop, so a late TurnInterrupted from the pod would not be read anyway.

Worth keeping as defence-in-depth. I'd just not count it as the fix — the shaping needs to happen where the strings are minted (see the cowork-server#385 note).

The cross-repo literal is pinned by nothing

The whole point of this line is that cowork-server's remote_turn_error recognises the TurnInterrupted: prefix. Those are two repos with no shared constant, no shared schema, and no test that spans them — neither CI run exercises the pair. If either side is respelled, the failure is silent and looks exactly like the bug being fixed.

I ran the actual pairing by hand and it does work today: feeding this PR's exact string into cowork-server#385's exact classifier yields ('anton_error', 'The turn ended unexpectedly. Please try again.'), i.e. the curated copy survives. Worth pinning that with a test on one side or the other, with a comment naming the other repo, so it stays true.

Merging this changes nothing until someone bumps the pod image

Flagging because the merge order on the ticket doesn't mention it. The pod that runs every web turn is SCRATCHPAD_CONTROLLER__SCRATCHPAD_IMAGE, and in all three environments that is an immutable anton commit SHA, never a branch or moving tag (scratchpad-controller deployment/scratchpad-controller/values.yaml:106; staging and dev leave their overrides commented out, so they inherit it).

Nothing in anton builds that image on a push — git grep -n build-push-ecr origin/main -- .github returns exactly one hit, scratchpad-dev-build.yml:36, whose trigger is pull_request only. And anton is baked into the image (COPY . /app + uv sync --frozen, CMD ["python","-m","anton.cloud_turn"]), so the pinned SHA fully determines the pod's anton code.

So after this merges, the chain is: anton staging advances → the standing staging→main release PR builds minds-anton-scratchpad:development-head-<S> → a human opens a scratchpad-controller PR bumping values.yaml:106 → deploy. That third step is a fifth PR in a fifth repo.

Sharper rather than softer: the image carrying this fix already exists — scratchpad-dev-build.yml ran successfully on this PR's head (7e1b8efc…, 2026-08-25T14:31Z), so development-head-7e1b8efc107335a9dfc1127c41d8d721a3e67e2e is in ECR right now. Nothing points at it. That image could be pinned in a dev environment to prove the end-to-end classification before merge.

For QA: Step 0 has to be "confirm the running pod image tag matches the anton SHA carrying this fix" —
kubectl -n <ns> get deploy scratchpad-controller -o jsonpath='{..env[?(@.name=="SCRATCHPAD_CONTROLLER__SCRATCHPAD_IMAGE")].value}' — never inferred from a green anton CI run.

Smaller notes

• TurnInterrupted is not an exception type anywhere in anton — it's a string invented to satisfy a parser in another repo. Fine as a choice, but the branch also fires on SystemExit and on a crash whose own error reporting failed, so "interrupted" isn't always accurate, and that name propagates into what the user reads.
• The new test pins less than it looks like. It asserts .startswith("TurnInterrupted:"), which is right, but the test's docstring describes a "pod torn down mid-turn" scenario that the deployed path can't actually produce (see above) — worth adjusting the docstring so the next reader doesn't take it as evidence the production case is covered.

Verdict: fine to merge, low risk, no objection. Just don't let it close ENG-1894 on its own — on the deployed path the banner rate won't move.

[tino097]

Findings addressed since review:

• Test docstring overstated what's covered (pnewsam minor, alecantu7): fixed. It no longer implies the deployed k8s path can reach this fallback via a real pod OOM-kill/exec-drop — only a BaseException escaping does, which the pod doesn't raise today. Module + class docstrings now say so explicitly, and point at scratchpad-controller's own literal as the real detector for that case.
• Security cross-check (pnewsam): confirmed — the TurnInterrupted: ... string is a fixed literal, no interpolation of exception detail, provider text, or runtime state.

Not changed, by design:

• Merge coupling with cowork-server#385 (pnewsam major, alecantu7): both PRs are meant to land together — noted, not a code change here.
• The pod-image-bump step to actually deploy this (alecantu7): a real deployment step, out of scope for this PR's diff.
• Cross-repo literal has no shared test (pnewsam minor, alecantu7): cowork-server#385's tests document the expected string with a comment naming this file; no shared schema exists between the repos to pin further.