[ZuyiZhou]

Summary

Two halves of one incident, on 2026-09-20. A Raven-Design run (an ACP sub-agent) stalled on its model stream; the parent conversation was told turn_failed (code -32099) and nothing else, diagnosed it as an npm crash and restarted the run. Behind that, the same run had written 58,675 agent_thought_chunk frames, one per reasoning token, averaging 5.7 characters. Every frame is a synchronous write plus flush on the client's pipe, so when the client (the gateway, frozen by a synchronous find, PR #566) stopped reading, the server blocked on that write too.

Thought chunks are coalesced. UpdateTranslator (the ACP server's outbound sink, raven/acp/updates.py) now holds consecutive thinking.delta updates per session and sends them as one agent_thought_chunk when:

• a 200ms window closes (a loop timer, THOUGHT_COALESCE_WINDOW_S), or
• the held text reaches 512 characters (THOUGHT_COALESCE_MAX_CHARS), or
• anything that has to follow them arrives: a chunk of the answer, a tool call, a latch or an ending event, the session's release, the turn slot being dropped, or the connection closing.

Order on the wire is kept: held text always precedes whatever released it, and a delegated agent's thought (a different _meta target) never joins the main agent's held text. The thresholds are measured, not chosen: replaying the incident's frame log, 200ms/512 gives 4,333 frames (13.5x fewer); a 100ms window gives 6,210; a 1,024-character cap changes nothing (4,331), so the window is what does the work. The median gap between consecutive thought frames in that log was 0.34ms, the p90 92ms. agent_message_chunk (the answer itself) is not coalesced: a reader watches it stream, and its frame count was 79.

A failed turn names what failed. (Second round: the class is prefixed only when the message does not already open with it, so an SDK error that names itself does not stutter; and StreamIdleTimeoutError is classified by type, as stream_idle_timeout, before any substring branch can read the budget in its message as a 429 or a 500.) Lane._run_turn built TurnFailed(error=str(exc)), and str() of asyncio's bare TimeoutError is empty, so the ACP translator's _error had only the wire message to show. describe_failure(exc) now carries the exception class and its message when it has one (ValueError: boom, TimeoutError). And the stall itself is named at the source: the per-chunk idle wait_for in LiteLLMProvider.chat_stream raises StreamIdleTimeoutError(idle=...), a TimeoutError subclass (so every except TimeoutError arm and retry verdict keeps applying, the same argument FirstByteTimeoutError makes) whose message reads the model stream sent nothing for 180s (bound stream_idle_timeout=180s).

Not changed: the frame writer is still a blocking write on the loop. Fewer frames make the stall far less likely, they do not make it impossible; moving the write off the loop would change the ordering guarantee UpdateTranslator's docstring states and is not attempted here. The other providers' idle watchdogs (the Responses and Anthropic SSE paths) still raise a bare TimeoutError; they now surface as TimeoutError through describe_failure rather than as nothing, and giving them the same message is a follow-up.

Type

• Fix
• Feature
• Docs
• CI / tooling
• Refactor
• Other

Verification

At head b6097ea:

uv run pytest tests/test_acp_updates.py tests/test_spine_scheduler_lane.py tests/test_litellm_provider_timeout.py -q -n 0
# 233 passed
uv run pytest <24 test files across acp, spine, providers, rpc spine, subagent acp> -q
# 1048 passed (before the last test was added)
uv run pytest -q --ignore=tests/integration
# 23791 passed, 110 skipped at head b6097ead, in a throwaway worktree with COLORTERM=truecolor
make lint-python lint-imports lint-types lint-deps          # all pass
make check-source-language check-large-files                # exit 0 (COMMIT_RANGE=origin/main..HEAD)

Revert-to-red, each production file (pair) reverted to origin/main on its own with the tests kept:

raven/acp/updates.py reverted                              6 failed
raven/spine/scheduler.py reverted                          2 failed
raven/providers/{first_byte,litellm_provider}.py reverted  1 failed

Mutation, each mutant applied alone, control restored between runs:

no flush before an update that must follow the thoughts   3 failed
window timer never armed                                  2 failed
512-character cap disabled                                1 failed
different speakers merged into one chunk                  1 failed
release_session keeps the held text                       1 failed
close keeps the held text                                 1 failed
end_turn keeps the held text                              1 failed  (0 before the second commit added its test)
describe_failure drops the class name                     2 failed
idle stall re-raised as a bare TimeoutError               1 failed
control (nothing mutated)                                 230 passed

Second round (267 tests across test_acp_updates.py, test_spine_scheduler_lane.py, test_error_classification.py):

the three files reverted to the first-round head          4 failed
idle stall classified by substring                        3 failed
describe_failure always prefixes                          1 failed
settle_turn keeps the held text                           1 failed
sliding window (timer re-armed per chunk)                 1 failed
control                                                   267 passed

The two guard clauses the first round's mutation pass showed could not decide the flush (or result.latch, or _ends_the_stream(event)) were dropped rather than tested; the comment says why.

One existing test changed: test_reasoning_arrives_as_a_thought_chunk asserted a lone thought went out immediately; it now sends two thoughts and an answer chunk and asserts one thought frame precedes the answer, which is the ordering rule this PR adds.

• Relevant tests pass locally
• Relevant lint / type checks pass locally
• User-facing docs or screenshots are updated when needed (CHANGELOG Unreleased / Fixed, two entries)

Risk

User-visible behaviour changes:

• An ACP client sees reasoning text arrive in bursts of up to 200ms or 512 characters instead of per token, so the first thought token of a turn arrives up to 200ms later than before. Answer text, tool calls and endings are unchanged and never reordered relative to the thoughts that preceded them.
• Every failure event (error on the wire, turn_failed in ACP) now starts with the exception class, e.g. ValueError: boom where it read boom. A client matching the old text exactly would need to match the suffix.
• The mid-stream idle stall in the LiteLLM path raises StreamIdleTimeoutError instead of a bare TimeoutError. It is a TimeoutError, so except TimeoutError behaves as before; classify_error names it stream_idle_timeout (retryable, fallback) where it used to say network, and _NON_AUTH_HINTS has no entry for it, so the CLI renders its error line alone, as it does for first_byte_timeout.

Rollback: revert the squash commit. No stored data or wire schema changes; the coalescer is process-local state.

• Security impact considered (held text is the model's own reasoning, already on this channel; redact is applied where it was before, in the failure text)
• Backward compatibility considered (frame shapes unchanged and schema-validated in the tests; exception hierarchy preserved)
• Rollback path is clear for risky changes

Related Issues

N/A

[gloryfromca]

No blockers; this can merge as far as I am concerned.

Reviewed the full diff and relevant ACP/session callers, provider timeout propagation, scheduler failure handling, and the branch history. I also checked the repository rules and Runtime terminology/architecture guidance, backward compatibility, and the test changes for weakened coverage. The new coalescer preserves ordering at the subscription and prompt-response boundaries, separates sessions and speakers, and flushes on every lifecycle path that can finish or release a turn. The typed idle timeout remains a TimeoutError, while scheduler failures now retain both class and message.

Verification: uv run pytest tests/test_acp_updates.py tests/test_spine_scheduler_lane.py tests/test_litellm_provider_timeout.py -q -n 0 -> 230 passed in 11.11s. git diff --check github/main...HEAD also passed. The documented limitation that writes remain synchronous is an explicit scope boundary; this patch still materially reduces frame pressure without changing the wire schema.

[0xKT]

Not a blocker. All three changes do what they say, the frozen-tier cost is
inside its budget (spine 1,395 -> 1,408 lines against a 2,000 ceiling), and the
new tests hold most of what they should. Five observations. Read on head
1d89ce1 against base 0cb8b79.

1. The new error's message is fed to the classifier's substring matcher, and a configured idle value can flip the bucket

classify_error lowercases str(exc) into msg at providers/base.py:416 and
runs every substring branch against it. The rate-limit branch tests has("429")
at :456-466 and the server branch tests has("500","502","503","504") at
:478-494 -- both before the isinstance(exc, TimeoutError) arm at :498.

StreamIdleTimeoutError's message embeds the configured value twice, so with
streamIdleTimeout: 500 a stalled stream classifies as server, and with 429
as rate_limit. The default 180 is safe, which is why this is an observation
and not a blocker.

The declared sibling is immune, and the reason is the thing to copy:
base.py:427 short-circuits on isinstance(exc, FirstByteTimeoutError) before
any substring runs, and its comment says exactly why -- "it is a TimeoutError
subclass, so without this branch it would still land on the network bucket".
The new class got no such branch.

The smoking gun is the comment on the arm this used to reach, base.py:498-500:

isinstance covers the builtin TimeoutError raised by asyncio.wait_for (its
class name "timeouterror" and empty str() match neither the name set nor
the substrings below).

That parenthesis is the premise, and this PR is what makes it untrue. Nothing
about retry changes -- all three buckets are retryable=True, should_fallback=True -- but the diagnosis does: Error calling LLM (server@...) plus "Hint: provider-side error; retry later or switch models" for
a stall that is neither, which is the same class of wrong-cause reporting the PR
exists to remove. Two lines fix it: an isinstance(exc, StreamIdleTimeoutError)
branch beside :427.

2. describe_failure re-decides a rule providers/base.py already owns, in the opposite direction

providers/base.py:67 is _EXC_NAME_PREFIX_RE, and format_llm_error's
docstring states its job: "the detail drops duplicated exception-name
prefixes". The new function in the frozen kernel adds one, unconditionally.
On an exception whose own str() already begins with its class name -- which
several provider SDK errors do -- the result stutters:

describe_failure(APIError("APIError: rate limited"))
  -> "APIError: APIError: rate limited"

The empty-message case the PR is built for is untouched by this
(describe_failure(TimeoutError()) -> "TimeoutError", correct). The narrower
form -- prefix only when str(exc) does not already start with the class name,
or reuse the regex that already owns the question -- gets the fix without the
second answer. For a kernel-tier addition, "this rule already has an owner
outside the frozen set" seems worth a sentence in the PR itself.

3. Two things the comments assert that the code does not support

THOUGHT_COALESCE_MAX_CHARS: the comment concludes "a 1,024-character cap
changes nothing, so the window is what does the work". Driven against the real
translator, the 512 cap does bind, on the same stream shape the comment
measures -- so the constant is load-bearing, not inert, and the sentence reads
as though it were not. Worth correcting, because the next person to tune the
window will read that line and leave the cap alone.

Latency: the timer is armed from the first held chunk (not sliding), so the
bound is honest -- but the first thought token of a turn now reaches an ACP
client about 220ms later than before (200ms window plus the emitter's 16ms
loop). That is a real user-visible change and the CHANGELOG does not mention it.

4. Only one of the two wires got the fix

The 58,675-frame cost the comment quantifies is paid on the ACP wire and on the
RPC wire. This PR coalesces the first. On the second,
raven/rpc/subscriptions.py's outgoing merge
(_merge_consecutive_token_deltas) tests event.get("type") == "token.delta"
and nothing else, so a run of thought tokens is still one frame per token for
the TUI and ui-web. _MERGEABLE at :40 does name thinking.delta, but that
tuple feeds the replay buffer at :196, not the frames that go out.

Not a defect in what this PR does, and arguably the right scope for one change.
Recording it because the CHANGELOG's number is a whole-system figure and only
half of it is now addressed.

5. Coverage, with controls

Four mutants on the new code leave the suite green:

remove `or result.latch` from the _deliver guard      230 passed
remove `or _ends_the_stream(event)` from it           230 passed
remove the flush from settle_turn                     192 passed
arm-from-first -> a sliding timer                     192 passed

The controls say the suite reaches both sites: deleting the whole guard fails 3,
and dropping _hold_thought's _meta split fails 1. The first two look like
dead weight rather than a hole -- neither arm can be the deciding clause, since
a latch and a stream-ending event both arrive with a non-thought update that the
any(...) already catches -- in which case they can be dropped rather than
tested. The other two are genuine holes: settle_turn's flush and the
arm-from-first property are both load-bearing (the second is what makes the
200ms bound a bound at all) and nothing pins either.

One small thing in the frozen file: scheduler.py:474-476 still says "The event
carries only str(exc) to the front-end", fourteen lines above the line this PR
changed to describe_failure(exc).

[gloryfromca]

No blockers; suggestions only, and they are marked inline.

This new evidence changes my prior clean stance to nonblocking. I checked all five observations:

1. The classification collision holds. I reproduced StreamIdleTimeoutError(idle=429) as rate_limit and idle=500 as server, while idle=180 remains network; retry and fallback stay true in every case. This is a narrow diagnostic error for those configured values, not a merge blocker.
2. The duplicated exception-name symptom also holds: describe_failure(APIError("APIError: rate limited")) produces APIError: APIError: rate limited. I would not reuse the provider-private regex from spine, because that would point the kernel at a provider implementation detail; local de-duplication can address the presentation issue without crossing that boundary.
3. I do not think the threshold or latency wording establishes another defect. The PR already records 4,333 versus 4,331 frames when doubling the cap, so saying the cap change has no material effect is compatible with the 512-character guard occasionally binding. The new incremental delay is bounded by the 200ms thought window; the emitters 16ms batching existed on the old path too, and the CHANGELOG already says thoughts are held for 200ms or 512 characters.
4. The RPC-wire observation is accurate scope context, but the implementation, CHANGELOG, and PR description explicitly limit this fix to ACP. I do not treat the unchanged TUI/web path as a defect in this revision.
5. The latch and stream-ending clauses are redundant under the current translation shapes, so their surviving mutants do not expose a behavior gap. Dedicated tests for settle_turn flushing and fixed-from-first timer semantics would strengthen regression coverage, and the stale str(exc) scheduler comment should be corrected; both are nonblocking follow-ups because the implemented behavior is correct.

Verification rerun: uv run pytest tests/test_acp_updates.py tests/test_spine_scheduler_lane.py tests/test_litellm_provider_timeout.py -q -n 0 -> 230 passed in 7.40s.

No new inline findings from me; the actionable suggestions are already captured in the numbered comment above.

[0xKT]

Not a blocker -- a correction to my own note above, on two points the
reviewer is right about.

Point 2's second option is wrong and should not be taken. I wrote "or reuse
the regex that already owns the question". That regex lives in
raven/providers/base.py, and describe_failure lives in raven/spine/ --
reusing it would make the frozen kernel import a provider implementation
detail, which is the kernel stands alone import-linter contract. Only the
first option stands: prefix when str(exc) does not already begin with the
class name, decided locally inside spine.

Point 3 overstated the cap. The comment's claim is that a 1,024-character
cap changes nothing, and the PR's own figures (4,333 against 4,331) support
exactly that -- which is compatible with the 512 guard binding sometimes. I read
it as "the cap never binds" and it does not say that. Withdrawn; there is
nothing to fix in that sentence.

The other three stand as written.

[ZuyiZhou]

Thanks, both. All five checked and acted on in b6097ea (same branch, pushed).

1. Classifier collision -- reproduced: classify_error(StreamIdleTimeoutError(idle=429)) gave rate_limit, idle=500 gave server, idle=180 gave network. Fixed as 0xKT suggested: an isinstance(exc, StreamIdleTimeoutError) arm beside the first-byte one, before any substring runs, returning stream_idle_timeout with the same retryable/fallback verdict. The comment on the generic TimeoutError arm no longer claims an empty str() for everything that reaches it. Test: test_a_stream_idle_timeout_is_named_whatever_budget_its_message_carries[180|429|500]. _NON_AUTH_HINTS in agent_commands.py has no entry for first_byte_timeout either, so the new category renders its error line alone like that one; I left the map as it is.

2. describe_failure stutter -- reproduced (APIError: APIError: rate limited). Took the first option, decided locally in spine on the text alone: the class is prefixed only when str(exc) does not already open with it. The provider layer's _EXC_NAME_PREFIX_RE stays where it is; the kernel does not import providers. Test: test_a_message_that_already_names_its_class_is_not_prefixed_twice covers the stutter, the plain message, the empty message and the unrelated class.

3. Cap wording and latency -- the cap sentence stands as gloryfromca read it (4,333 vs 4,331 is what it says). The latency is now in the CHANGELOG: "the first thought token of a turn reaches an ACP client up to 200ms later than before; the answer's own chunks are not held."

4. RPC wire -- agreed it is scope context: subscriptions.py's outgoing merge coalesces token.delta only, and this PR touches the ACP sink only. Not changed here; the PR body says so.

5. Coverage -- the two redundant clauses (or result.latch, or _ends_the_stream(event)) are dropped rather than tested: a latch and a stream-ending error both arrive on an update the first test already catches, and the comment now says so. The two real holes each have a test: test_settling_the_turn_from_outside_the_stream_sends_what_is_held (a settle_turn from a teardown or cancel) and test_the_window_is_measured_from_the_first_held_chunk_not_the_last (chunks every 20ms for 120ms against a 50ms window; a sliding timer never closes it). The stale str(exc) comment in the scheduler is corrected.

Mutation at b6097ea, each alone, control restored between runs (267 tests across test_acp_updates.py, test_spine_scheduler_lane.py, test_error_classification.py):

the three files reverted to 1d89ce1a                 4 failed
idle stall classified by substring                   3 failed
describe_failure always prefixes                     1 failed
settle_turn keeps the held text                      1 failed
sliding window (timer re-armed per chunk)            1 failed
control                                              267 passed

Focused suites at this head: test_acp_updates.py test_spine_scheduler_lane.py test_litellm_provider_timeout.py test_error_classification.py test_agent_loop_stream.py test_cli_agent_commands.py -> 347 passed. The PR body is being updated with the full-suite run on this head.

[gloryfromca]

No blockers; this can merge as far as I am concerned.

Reviewed the delta from 1d89ce1aff05 and the affected classifier, scheduler, ACP lifecycle paths, callers, tests, history, and author response. I also rechecked the repository rules, Runtime terminology and architecture boundary, backward compatibility, and whether the new tests merely preserved green status. The classifier now identifies the idle timeout by type before substring matching, failure descriptions avoid duplicate class prefixes without importing the provider layer into the kernel, and the simplified thought-flush guard remains equivalent for every current latch and terminal event shape. The previously missing settle and fixed-window behaviors now have direct regression tests; the RPC-wire behavior remains explicitly outside this ACP-scoped change.

Verification on Python 3.12: uv run --frozen --python 3.12 pytest tests/test_acp_updates.py tests/test_spine_scheduler_lane.py tests/test_litellm_provider_timeout.py tests/test_error_classification.py tests/test_agent_loop_stream.py tests/test_cli_agent_commands.py -q -n 0 -> 347 passed in 15.64s. uv run --frozen --python 3.12 lint-imports -> 10 contracts kept, 0 broken. git diff --check github/main...HEAD passed.