Repository navigation
fix(mcpl): answer each way a server's inference ends; a failure is the error, a completion its real result - #380
Conversation
…e error, a completion its real result
The MCPL inference router's streaming path read only `tokens` events from
membrane's yielding stream, then answered `end_turn` with the joined text
and usage 0/0. So a stream that failed (`error`) or stopped early
(`aborted`, such as on a timeout) reached the server as a successful,
truncated completion, and in XML tool mode a model that wrote a tool call
left the request unanswered.
A server now gets a result only for an inference that ended as a plain
completion: a stop MCPL's FinishReason can name (SPEC §11.3 closes it at
end_turn, max_tokens and stop_sequence) and no tool call. Every other
ending gets the JSON-RPC error the non-streamed path already sent for a
failure (-32603, the cause in its message) and an `mcpl:inference_fail`
trace. Nothing is added to the wire.
- streamResponse reads the stream to its terminal: `complete` returns the
response, `error` throws its error, `aborted` throws "The inference
stopped before it finished (<reason>)", and a stream that ends with no
terminal throws too.
- `tool-calls` throws as well. In XML tool mode `</function_calls>` is a
stop sequence whether or not tools are declared, so a model that wrote a
call made the stream yield `tool-calls` and wait for
provideToolResults, which the router never calls. Leaving the
`for await` by the throw calls the stream's return(), which cancels it,
so no stream is left waiting.
- One builder, resultFrom, serves both modes: content from the response's
text blocks, finishReason from its stopReason, usage from its usage. A
streamed answer was always `end_turn` with zero usage, so a max_tokens
cut read as a natural end, and its content held every token, thinking
included. resultFrom throws for a response MCPL can't report as a
completion:
- one that carries tool calls; XML mode's complete() left the call's
markup in the content;
- in XML tool mode, one that ended with a tool-call block still open
(membrane's details.stop.unclosedToolBlock), such as a call cut off at
max_tokens, and one that stopped at `</function_calls>` with no call
membrane could read in the block. inference/request asks for no stop
sequence of its own, so that stop is the one XML tool mode adds.
membrane tells an open block from the text, so a completion that
writes `<function_calls>` in prose and never closes it gets the error
too;
- one whose stop reason FinishReason has no value for, which mapped to
end_turn: `refusal`, `tool_use`, `pause_turn`, `abort`, and membrane's
guard stops `no_progress` and `round_limit`.
- Chunks carry only the tokens membrane marks visible: thinking and the
text of a tool call no longer go out as inference/chunk deltas.
- The traces are shared too. A streamed success trace now carries
inputTokens and outputTokens, and `streamed`, which only that trace
carried, is on every success and failure trace.
test/mcpl-inference-router.test.ts drives the router through a real
Membrane with a scripted provider adapter, in XML tool mode and with
NativeFormatter. membrane's own stops (abort, and its guards' no_progress
and round_limit) and a stream with no terminal event can't be played
through it, so stand-ins play those. test/helpers/mock-membrane.ts's
createMockResponse now sets details.stop, which membrane's
NormalizedResponse type requires and the router now reads.
Read before opening by Gordon-2414.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
[Medium impact] The changes since the previous review appear safe to merge. SummaryThe PR makes streamed and non-streamed
Reviews (2) · Last reviewed commit: "fix(mcpl): answer a provider's finish_re..." · Reviewed by Greptile |
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R1 at 71145a5 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.
Reviewed at 71145a5 against main d5e6feb (the merge-base; main is now e6a0317, which doesn't touch src/mcpl/inference-router.ts or test/helpers/mock-membrane.ts and merges cleanly). Round 1. We have no earlier review on this PR.
Verdict: changes requested (one major, small to fix)
This is a clear improvement. Both modes now go through one result builder, the streamed tool-call hang is gone, and the for await exit really does cancel membrane's stream. One ending still reaches the server as end_turn with partial text: an OpenAI-compatible provider that fails partway through with finish_reason: "error", which is how OpenRouter reports a mid-stream failure. That's the failure this PR and its changelog say are closed. The other items are changelog accuracy and coordination. None of them should hold the merge once R1-1 is handled.
Where each ending lands, at this head with membrane 0.5.87:
| How the inference ends | Streamed | Not streamed |
|---|---|---|
end_turn / max_tokens / stop_sequence, no tool call |
result, real finishReason and usage ✔ |
result ✔ |
refusal, tool_use, pause_turn, abort |
-32603 ✔ | -32603 ✔ |
no_progress, round_limit (XML resumption guards) |
-32603 via resultFrom ✔ (only a complete() stand-in tests it) |
can't happen: complete() makes one provider call |
membrane error event / complete() throws |
-32603 with the cause ✔ | -32603 (unchanged) ✔ |
aborted (e.g. TimeoutAbortError) |
-32603 "stopped before it finished (timeout)" ✔ | complete() throws, -32603 ✔ |
| XML tool call | tool-calls, then -32603, stream cancelled ✔ |
toolCalls, then -32603 ✔ |
| XML open or unreadable tool-call block | -32603 ✔ | -32603 ✔ |
| stream ends with no terminal event | -32603 ✔ (0.5.87 never does this) | n/a |
provider finish_reason: "error" |
end_turn + partial text ✘ (R1-1) |
end_turn ✘ (R1-1) |
retrying |
can't happen: the router passes no refusalRetries |
n/a |
| server cancels | MCPL defines no cancellation for inference/request (SPEC @761cb48) |
— |
host stop() mid-inference |
not cancelled (your Out of scope) | same |
Can an existing MCPL server break? Yes, though probably no live server is exposed. A server that read a streamed answer as always successful now gets an error after its chunks. @animalabs/mcpl-core rejects the pending request with an RpcError (src/connection.ts:325) once the chunk notifications have already been delivered. The error carries no partial text, so a server that salvaged a timed-out answer can't do that any more. GitHub code search for the method name turns up the spec, the protocol libraries and hosts, and no server that sends inference/request. Still, this ships as a .fixed fragment, so it goes out in a patch, and hosts on ^0.22.1 (connectome-host's main) pick it up at their next install. The changelog states accurately what a server now sees. It doesn't say what a streaming server should do with chunks that end in an error (R1-2).
The PR description's claim about the old behaviour holds. On main, handleStreaming read only tokens (src/mcpl/inference-router.ts:285), so error and aborted fell through to finishReason: 'end_turn' with usage 0/0 (:301–302) and the joined tokens, thinking and tool-call markup included. An XML tool call left the request unanswered.
Major
R1-1. A provider that ends with finish_reason: "error" is still answered end_turn with its partial text, streamed and not.
- Evidence: membrane 0.5.87's OpenRouter adapter records
finish_reasonfrom each SSE chunk (src/providers/openrouter.ts:333–336) and never reads the chunk's top-levelerrorobject. ItsmapFinishReasonpasses'error'through unchanged (:741–758;openai.ts:703–720does the same). membrane'smapStopReasonthen turns any unknown token intoend_turn, keeping the raw value only indetails.stop.providerReasonand a one-time log line (src/membrane.ts:2986–3015). OpenRouter's streaming docs give this as the shape of a mid-stream failure: HTTP stays 200, and the final SSE event has anerrorobject andchoices[0].finish_reason: "error".resultFromchecks onlyresponse.stopReason(src/mcpl/inference-router.ts:105). - Reproduced: a scripted adapter that returns
stopReason: 'error', providerStopReason: 'error'after the chunkA summgets{content: 'A summ', finishReason: 'end_turn', usage: …}in all four combinations, native and XML, streamed and not. - Failure: an OpenRouter-routed model's upstream drops partway through a summary. The server gets a successful
end_turnholding the first half. That's the defect class the changelog's first bullet says is fixed. - Fix: in
resultFrom, throw whenresponse.details.stop.providerReason === 'error', e.g. "The provider ended the inference with an error (finish_reason 'error')". membrane threadsproviderReasonthroughcomplete()and both yielding paths. Add one test per mode using the existingScriptedAdapterwithproviderStopReason: 'error'. Don't reject everyproviderReasonthat differs from the normalized reason:stop→end_turndiffers too, and it's fine. If you'd rather leave this to membrane (its adapter should read theerrorobject and throw), add a line to the changelog saying an adapter-levelfinish_reason: "error"still arrives asend_turn.
Minor
R1-2. The changelog fragment doesn't tell a streaming server that its chunks can now end in an error.
changelog.d/mcpl-inference-failures.fixed.md:1says what a server newly receives, and every sentence I checked is accurate. A streaming server's one new obligation isn't there: chunks for a request that ends in an error belong to no result and should be discarded. Your own Not verified section notes that nothing ran against a server's handling of chunks followed by an error.- Fix: open the bullet with the audience, as the README asks of breaking entries: "MCPL server authors: a streamed
inference/requestcan now end in a JSON-RPC error after chunks have arrived; discard them. The error carries no partial text." Whether it should be abreakingfragment (a minor bump, so hosts opt in) is the maintainers' call. I lean towardsfixedwith that sentence. Streamed requests could already get errors onmain: feature-set validation and loop prevention reject before any chunk, and so does astreamYielding()that throws. And I found no server that would need to act.
R1-3. In XML tool mode, chunks, streamed content and non-streamed content can differ on the same output, unlike what the changelog and a code comment say.
These are probes against membrane 0.5.87 through the compiled router, AnthropicXmlFormatter({ toolMode: 'xml' }):
| Model output | chunks (streamed) | content streamed |
content not streamed |
|---|---|---|---|
<thinking>Which memories matter?</thinking>They met twice. (end_turn) |
They met twice. |
They met twice. |
<thinking>Which memories matter?</thinking>They met twice. |
Results arrive in <function_results> blocks. (end_turn) |
Results arrive in |
the whole sentence | the whole sentence |
Use <invoke name="x"> to call. (end_turn) |
Use to call. |
the whole sentence | the whole sentence |
<thinking>Which memo (max_tokens) |
(none) | <thinking>Which memo |
<thinking>Which memo |
- membrane's
complete()parses only tool calls out of XML text (src/membrane.ts:2700–2715). The yielding path re-parses the whole text into blocks (:2853–2866). Its streamingvisibleflag comes from a third parser, which treats a<function_results>or<invoke …>tag written in prose as a block. So the changelog's "Itscontentis the response's text, as a non-streamed result's is, so it no longer includes thinking" doesn't hold in XML mode. Nor does the equivalence in the comment atsrc/mcpl/inference-router.ts:358–359("the result holds only the text blocks, so only visible text is sent"). In row 2, a server that builds its text from chunks loses everything after the tag. - Fix: say in the changelog that chunks are a preview, that the result's
contentis authoritative and can differ from the joined chunks, and that a non-streamed XML result still carries<thinking>markup. Correct the comment. The parsers disagreeing is membrane's to fix. Optionally, pin rows 1 and 2 with tests so a membrane fix shows up here.
R1-4. Coordinate with #378. #378 (draft) edits the same error/aborted arms of the router. Its timeout message is different: "No stream progress within ms (inferenceProgressTimeoutMs)". Its changelog bullet repeats this one ("An MCPL server's streamed inference request whose stream fails or ends on its deadline now gets an error answer…"). git merge-tree of the two heads conflicts in src/mcpl/inference-router.ts. Nothing is wrong in this PR. Whichever lands second should drop the duplicate bullet and settle on one message for aborted: 'timeout'. #378's names the setting to raise.
R1-5 (nit). Native tool blocks bypass the tool-call check. On membrane's native yielding path, response.toolCalls is filled only when the stop is tool_use (src/membrane.ts:4124). A tool_use block cut off at max_tokens would stay in content and pass resultFrom, while complete() would reject it. This can't happen today, because the router declares no tools (src/mcpl/inference-router.ts:231) and a provider won't emit tool blocks without them. If you want it anyway, response.content.some((b) => b.type === 'tool_use') closes it.
R1-6 (nit). The test file's header says "Each case runs in both tool modes a host configures" (test/mcpl-inference-router.test.ts:15). The tool-markup and thinking cases run only in XML mode, and the stop-reason and no-terminal cases use stand-ins. The PR description's Tests section has it right.
Verified fine
- Build and tests:
tscis clean, andtest/mcpl-inference-router.test.ts(37 tests, as the PR says) plusmcpl-awareness-barrierpass at this head on Node 22 with membrane 0.5.87, context-manager 0.14.1 and chronicle 0.4.0. - Dependency: membrane 0.5.87 is npm
latest, so^0.5.87resolves to exactly the version tested. - Event union: the yielding stream's events are exactly
tokens | block | retrying | tool-calls | usage | complete | error | aborted(src/types/yielding-stream.ts:116–124).blockandusageare rightly ignored, andretryingcan't occur withoutrefusalRetries. - Stop union: the stop reasons are exactly the nine in
src/types/response.ts:12–24, andfinishReasonForpasses through exactly the three MCPL names. - Cancellation: leaving the loop calls
return(), which callsdepartConsumer()andcancel()(src/yielding-stream.ts:238–241, 316–321). That aborts the provider signal, rejects the pending tool wait, and drops the extraabortedevent, and the producer's rejection is caught (:335–344). Oncomplete, the state is alreadydone, socancel()does nothing. AsendChunkFnthat throws now cancels the provider stream too. - Stop details:
details.stop.unclosedToolBlockandtriggeredSequenceexist and are always set (src/types/response.ts:126–150;buildStopInfoatsrc/membrane.ts:2908–2918).complete()treats output as XML only when the tool mode isxml(:368), so NativeFormatter can't trip the XML checks. - XML stop sequence: XML tool mode adds
</function_calls>as a stop sequence with no tools declared (src/formatters/anthropic-xml.ts:749). The router's request carried["\nuser:", "</function_calls>"]. - connectome-host: its
mainpinsanthropic-xmltotoolMode: 'xml'(src/recipe-formatter.ts:20) and depends on@animalabs/agent-framework^0.22.1. - No double answers:
answerOnceinframework.ts(:1570–1591) refuses a second answer, so a throw fromrespond()can't send both a result and an error. - Refusals: membrane keeps
refusalas a distinct stop (src/membrane.ts:2981–2985), as the PR says. - Already in the PR's Out of scope:
result.modelis the routed name rather thandetails.model.actual, andstop()doesn't cancel an MCPL inference. Both are present onmainand confirmed as described.
Method: two independent passes, then cross-checked. Codex gpt-5.6-sol at xhigh reviewed a read-only worktree of this head against the merge-base, with the membrane 0.5.87 tarball, the MCPL spec at 761cb48 and connectome-host main available locally. It ran for about 8 minutes, made 178 shell calls and used 6.78M input tokens (6.43M cached) and 37.5k output tokens. It had no network access. Claude (Opus 5.5) did a parallel pass with network: GitHub state for #378 and #172, OpenRouter's streaming docs, GitHub code search, test runs and router probes against real membrane 0.5.87. Claude re-checked every Codex finding before adopting it. R1-1: found by Codex; Claude confirmed it against OpenRouter's docs and reproduced it on all four paths. R1-2: found by both (Codex rated it major and breaking; downgraded for the reasons given there). R1-3: Claude only, from the probes; it corrects a Codex "verified fine" entry that said thinking was excluded consistently. R1-4: Claude only (needed network). R1-5: Codex, rated major there; downgraded because it can't be reached without declared tools. R1-6: Codex. Codex also raised the routed model and stop() cancellation; both are real, present on main, and already listed in the PR's Out of scope, so they aren't counted.
…ool_use block, with the error From the review of this PR (R1-1 to R1-3, R1-5 and R1-6). R1-1. A provider can end an inference with finish_reason 'error' and no error object. membrane 0.5.87's OpenAI-compatible adapters pass the token through as their stop reason, and its stop-reason mapper normalizes it to end_turn, keeping the token as details.stop.providerReason. resultFrom read only the stop reason, so the server got end_turn with the partial text, streamed or not. It now throws on providerReason 'error', and the request is answered with the error. OpenRouter's documented mid-stream error carries an error object as well, and membrane's adapter fails the stream on that, so the first commit already answered it with the error. Other tokens membrane doesn't map still normalize to end_turn: telling one from a mapped token like 'stop' would need membrane to mark it. R1-5. On membrane's native yielding path, toolCalls holds a tool call only when the stop was tool_use, so a tool_use block cut off at max_tokens stayed in the content and the response passed as a completion. resultFrom now refuses a tool_use block in the content as well. The router's request declares no tools. R1-3. The comment on the chunk filter said the chunks match the result. In XML tool mode membrane's stream parser opens a block at a <function_results> or <invoke> tag written in prose, and reads a thinking block cut off before its close tag as thinking, while the result's content keeps such a block as text. So the chunks can leave out text the result's content holds, and the comment now says so. The changelog says it too, and that a result that isn't streamed still carries <thinking> markup, which membrane's complete() keeps in the text. R1-2. The changelog tells MCPL server authors that a streamed request can now end in a JSON-RPC error after chunks have arrived, and that the error carries no partial text. R1-6. The test file's header says which cases run in which tool mode. Tests (mcpl-inference-router): a provider's finish_reason 'error' after the chunk 'A summ', streamed and not, in both tool modes; a native tool_use block cut off at max_tokens, streamed and not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Replying to this review. Round 2 is R1-1. At the
The second row's message comes from membrane reading R1-5. R1-2. The changelog now has a line for MCPL server authors: a streamed R1-3. The changelog and the chunk filter's comment now say what each path gives in XML tool mode:
A probe of your four rows at R1-4. #378 is a draft until a membrane release carries R1-6. The test file's header now says which cases run in which tool mode. At Written by Nigel-2654. — Lottie-2661 |
… helper's response The test "a real inference handler whose result write throws answers with its own error..." gave InferenceRouter a fake membrane whose complete() returned an object of this file's own, with content, stopReason and usage only, cast as never. main's router reads only those three of a response (src/mcpl/inference-router.ts:174-185 on main), so the test passes there. anima-research#380's router also reads response.details.stop and response.toolCalls (src/mcpl/inference-router.ts:89 and :95 at its head 647dd61), and with anima-research#380 merged as well the test fails on Node 20 and 24: the result write it counts never happens (resultWrites 0, expected 1). The fake now returns createMockResponse() from test/helpers/mock-membrane, the shared helper other tests' fakes use. Its response carries toolCalls and details, and anima-research#380 adds to that helper the details.stop its router reads. So the test passes on main, and with anima-research#380 merged as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R2 at 647dd61 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: APPROVE (the formal review event is left to the maintainer). Finding ids R2-n are stable; cite them in fix commits.
Reviewed at 647dd61 against main e670363 (current main; the merge-base is d5e6feb, and this head merges onto e670363 without conflicts). Round 2. Round 1 was at 71145a5.
Verdict: approve
R1-1 is fixed on all four paths: native and XML tool mode, streamed and not. Each path has its own test, and each of those tests fails when the new check is removed. R1-2 to R1-6 are answered, and the change still composes with main: it merges cleanly, typechecks, and the full suite passes on the merge. What's left is three nits, two of them about how comments and tests describe the code, and none should hold the merge.
A correction to our round 1. Your reply is right. We said membrane 0.5.87's OpenRouter adapter "never reads the chunk's top-level error object". It does. throwOnStreamErrorFrame runs on every SSE frame before finish_reason is read (src/providers/openrouter.ts:281, helper at src/providers/utils.ts:111), and openai.ts:319 and openai-compatible.ts:254 do the same. So OpenRouter's documented mid-stream error, an error object plus finish_reason: "error", was already answered with the stream error at 71145a5. Our failure scenario was built on that case, and it was wrong. The case that really reached the server as end_turn was the one we reproduced: a bare finish_reason: "error" with no error object. This round closes that case. We confirmed both points against membrane's real adapters (see R1-1).
Round-1 findings
R1-1. A provider's Fixed at finish_reason: "error" was answered end_turn with partial text.647dd61.
-
resultFromthrows ondetails.stop.providerReason === 'error'(src/mcpl/inference-router.ts:89–91) before it builds a result, so the check coverscomplete()andstreamResponse()alike. membrane 0.5.87 setsproviderReasonin all three of its response builders. Forcomplete()it'ssrc/membrane.ts:2740. For native yielding it's:4104→:4228. For XML yielding it's:3558→:3905, taken from the last round. XML yielding resumes only after a round that stopped at a stop sequence (:3849, and the tool-call path at</function_calls>), so a round the provider ends with'error'instead is always the last one. -
Tests:
test/mcpl-inference-router.test.ts:272–278covers streamed requests and also checks that the chunkA summarrived;:317–322covers requests that aren't streamed. The loop at:212runs both fornativeand forxml. With the check disabled, exactly these four tests fail and the other 39 pass. -
Against real adapters: membrane 0.5.87's
OpenRouterAdapterandOpenAICompatibleAdaptertalked to a local server through the compiled router, in both tool modes, streamed and not. That's eight combinations.Server sends 71145a5647dd61A summ, thenfinish_reason: "error", noerrorobjectend_turn,A summ(all 8)-32603 "The provider ended the inference with an error (finish_reason 'error')" (all 8; streamed ones after the A summchunk)A summ, then a frame with anerrorobject andfinish_reason: "error"(streamed)-32603 "OpenRouter stream error [server_error]: …" / "openai-compatible stream error …" the same
R1-2. The changelog didn't tell streaming servers that chunks can end in an error. Fixed as scoped. changelog.d/mcpl-inference-failures.fixed.md:2 now addresses MCPL server authors: a streamed request can end in a JSON-RPC error after chunks, and the error carries no partial text. The "discard them" half went to anima-research/mcpl#20. That's the right home, since the rule binds every host, not just this one. Optionally, cite anima-research/mcpl#20 in that bullet so a server author who reads only the changelog can find the rule. CHANGELOG.md already cites cross-repo issues this way (anima-research/context-manager#180).
R1-3. XML-mode chunks and content could differ, contrary to the prose. Fixed. The changelog's bullets 3–4 and the comment at src/mcpl/inference-router.ts:369–374 now say what round 1's four probe rows showed. A streamed result drops thinking, except a thinking block still open at the end. A result that isn't streamed keeps <thinking> markup. Chunks can leave out text after a <function_results> or <invoke> tag written in prose.
R1-4. Coordinate with #378. Answered. #378 is still a draft (head 14db078) and still conflicts with this PR in src/mcpl/inference-router.ts. The reply's plan is what we asked for: whichever lands second keeps #378's timeout message, which names the setting to raise, and one changelog bullet. Nothing to change here.
R1-5. A native Fixed. tool_use block cut off by another stop bypassed the tool-call check.src/mcpl/inference-router.ts:94–97 now refuses a tool_use block in the content as well. Without that check, the streamed native test fails. The test of a request that isn't streamed is a control (R2-2).
R1-6. The test-file header overstated which cases run in both modes. Fixed. test/mcpl-inference-router.test.ts:15–20.
Minor (all nits)
R2-1 (nit). The changelog doesn't carry the PR's own exception: other provider stop tokens membrane doesn't map still arrive as end_turn.
- Evidence: the probe above with
finish_reason: "weird_stop"gets a result,end_turnwithA summ, in all eight combinations at647dd61, as your Out of scope says. In membrane 0.5.87 such tokens come from Gemini'sOTHER,LANGUAGEandMALFORMED_FUNCTION_CALL(src/providers/gemini.ts:730), from the Responses API's incomplete reasons other thanmax_output_tokensandcontent_filter(src/providers/openai-responses-api.ts:677), and from any compatible server's own token. - Failure: a server author reads the changelog's lead, "a result only for an inference that completed with a stop MCPL can name", next to the explicit
finish_reason: "error"sentence. They conclude that every abnormal end a provider reports is now an error. A GeminiOTHERstop still arrives as a successfulend_turn. - Fix: add one clause to the first bullet, taken from the PR description: "A provider stop token membrane doesn't map, other than
error, still arrives asend_turn."
R2-2 (nit). The toolCalls comment describes only one of membrane's paths, so the new test of a request that isn't streamed doesn't test the new check.
- Evidence:
src/mcpl/inference-router.ts:92–93andtest/mcpl-inference-router.test.ts:326–328say membrane lists a native call intoolCallsonly when the stop istool_use. That holds for native yielding (membranesrc/membrane.ts:4125).complete()copies everytool_useblock intotoolCallswhatever the stop (:2648–2660). So "answers a tool_use block cut off at max_tokens with the error, not streamed" passes with the content check removed. Your reply says as much. The comment and the test name don't. - Failure: none at runtime. A later reader takes the non-streamed test as a guard on the content check, and it isn't one.
- Fix: say "membrane's native yielding path" in both comments. Then label the non-streamed case as a control, or drop it.
R2-3 (nit). The tests don't pin the order of chunks and the error.
- Evidence: the reply says the streamed
finish_reason: "error"tests check the order, the chunk first and then the error. They check that both happened.askWithrecords chunks (:127) and the answer (:136–137) in separate fields, and the test asserts each one (:275–277). The "failed partway" test (:244–253) and the timeout test (:255–263) script the chunkA summbut never assert that it was sent. - Failure: none today.
streamResponsesends every chunk before it returns or throws, andrespond/respondErrorrun only after that, so the order follows from the code's structure. A refactor that buffers or defers chunks could reorder them, and no test would notice. - Fix: have
askWithpush chunks and the answer onto one ordered list, assert e.g.['chunk:A summ', "error:The provider ended …"], and assert the partial chunk in the failure and timeout cases.
Verified fine
- Composes with
main: the head merges ontoe670363without conflicts, andtscis clean on the merge. On Node 22.21.0 with membrane 0.5.87, context-manager 0.14.1 and chronicle 0.4.0, the full suite passes 2,903 of 2,903 on the merge. An earlier merge ontob96e918, themainwhen this review started, passed 2,773 of 2,773. The router's test file passes 43 of 43, andmcpl-awareness-barrierpasses too.test/helpers/mock-membrane.tsauto-merges withmain's c55eee9 and with #457'sHeldStream, and keeps this PR'sdetails.stop. No commit onmainsince the merge-base touchessrc/mcpl/inference-router.ts. The call site onmain(src/framework.ts, theinference/requesthandler wrapped inanswerOnce) still passesrespond,respondErrorandrequestId, as the router expects. - The tests fail against
main's router: run against it, the head's test file passes only its 6 controls. Of the other 37, 20 fail and 17 are reported cancelled, once the parked XML tool-call test hits its 5 s timeout. - Every built-in adapter supplies
providerStopReason: anthropic, bedrock, gemini, openai, openai-compatible, openai-completions, openrouter and openai-responses-api all do. The images and mock adapters return onlyend_turn. A customProviderAdapterthat reportsstopReason: 'error'withoutproviderStopReasonwould still read asend_turn, because membrane keeps no token in that case (src/membrane.ts:2917). That's membrane's to carry, not this router's. - Responses API failures throw, so no token reaches the router: a streamed
response.failedthrows (src/providers/openai-responses-api.ts:305–314), andcomplete()checks the body (:243). - The match is exact: OpenAI-family adapters pass
'error'through verbatim (openai.ts:703,openrouter.ts:741,openai-compatible.ts:643), and the check compares it exactly. - CI at
647dd61: all four Build & Test legs, the changelog check and the packed-release install pass. - The spec: anima-research/mcpl#20 is open, and §11.4 on MCPL's
mainstill says only "Chunks are followed by the full response."
Method: two independent passes, then cross-checked. Codex gpt-5.6-sol at xhigh reviewed a read-only worktree of this head against the merge-base. It had round 1, your reply, the PR description, mcpl#20, the head merged onto main, the membrane 0.5.87 npm package and tag, MCPL's spec, connectome-host's main and #378's head. It ran for about 7 minutes, made 103 shell calls and used 4.94M input tokens (4.55M cached) and 25.3k output tokens. It had no network access and couldn't run suites that write to disk. Claude (Opus 5.5) did a parallel pass with network. That covered GitHub state for #378, #457, mcpl#20 and CI, the merge onto main and the full suite on it, mutation runs of the two new checks, and the real-adapter probe at both heads. Claude re-checked every Codex finding before adopting it. R1 dispositions came from both passes. Codex rated R1-1 partial because of a custom adapter that omits providerStopReason; we didn't adopt that, since no built-in adapter does so and the router can't see the token there (see Verified fine). Codex marked R1-4 not fixed because #378 still conflicts; we count it as answered, since round 1 asked for coordination, not a change here. Both passes verified the correction to R1-1's evidence: Codex from membrane's source and build, Claude by the probe. R2-1: Claude only, from the probe. R2-2: Codex; Claude reached it independently by mutation. R2-3: Codex, where it was rated minor; downgraded because the order follows from the code's structure.
… lists toolCalls Answers R2-1 to R2-3 of Anarchid's round-2 review of anima-research#380. src/ changes only comments. - R2-3: askWith records what reached the server in one ordered list, `answer.sent`: the text of consecutive chunks joined, then the result or the error. Every streamed test that also checks its chunks, or whose name says the answer comes after them, now asserts that list: a completed stream (control), a provider error and a timeout partway, which now also check that the chunk "A summ" was sent, a provider's finish_reason 'error', the streamed XML tool call, XML thinking left out of the chunks, and the stand-in stream with no terminal event, whose name already said "after the chunks it sent". joined(), left unused, goes. - R2-2: membrane's yielding stream lists a native tool call in toolCalls only when the stop is tool_use, while membrane's complete() lists every tool_use block in toolCalls as well as in the content. The router's comment and the test's comment now say so, and the native tool_use test that isn't streamed is named a control, since either of the router's two checks answers it. - R2-1: the changelog fragment says that an inference the provider ended with a stop token membrane has no stop reason for, other than `error`, such as Gemini's `OTHER`, is still answered `end_turn`. Its note for MCPL server authors points to anima-research/mcpl#20, which asks the spec to say what a server does with chunks sent before an error. - resultFrom's comment on finish_reason 'error' names membrane 0.5.87, since antra-tess/membrane#147, merged after that release, has those adapters fail the call themselves.
|
Replying to this review. Round 3, R2-1. Addressed in R2-2. Addressed in R2-3. Addressed in One more comment changed: the one on At this head, each of the 16 pieces still stops a test when removed alone, against Written by Nigel-2654. — Lottie-2661 |
…pter gives none An adapter can return a stopReason membrane doesn't map, such as 'error', and no providerStopReason (#154). mapStopReason normalizes it to end_turn, and details.stop.providerReason came only from providerStopReason, so the token was dropped and the response read as a clean end of turn. A caller that refuses a failed stop by its providerReason, as anima-research/agent-framework#380 does for 'error', accepted it. providerReasonFor now decides the token for an adapter's result: its providerStopReason when it gives one, otherwise its stopReason when the normalized reason differs from it. Today that is a token mapStopReason doesn't map. A stopReason that is the normalized reason, given alone, still leaves providerReason unset. The five places that read a stop from an adapter's result call it: complete()'s transformResponse, and the XML and native tool loops of stream() and streamYielding(). The docs of StopInfo.providerReason and ProviderResponse.providerStopReason, the default arm's comment and its log line, which said providerReason was present only when the adapter supplied the raw token, now say so. Built-in adapters return providerStopReason beside any token membrane doesn't map, so their responses don't change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…served it MCPL SPEC §11.3 defines the result's `model` as "Actual model used", and §11.5's guidance for hosts includes logging it for verification and cost attribution (anima-research/mcpl 761cb48, SPEC.md:1282, :1316). The router filled it, in both modes, with the model its routing policy resolved: an alias such as `claude-haiku-4-5`, or `default` with no policy. membrane names the model that served the request in `details.model.actual`, which can differ where routing or fallback applies (#414, from Anarchid's review of #380). - resultFrom reports `details.model.actual` as `model`, through actualModelOf, which treats an empty value as none. membrane's type says it always names one, but its anthropic and bedrock adapters pass the provider's `response.model` through as it is (membrane 0.5.87, src/providers/anthropic.ts:852, bedrock.ts:959). When membrane names none, the result gives the routed model, so `model` stays the string MCPL requires. - mcpl:inference_complete keeps `model` as the routed model, as mcpl:inference_start has it, so the two pair on a field whose meaning doesn't shift, and adds `actualModel`, the value the result reports. It leaves `actualModel` out when membrane named none, rather than name the routed model as served. membrane names none only from complete(), through an adapter that passes the provider's absent or empty model through; its streaming paths, and its other adapters for an absent model, name the requested model themselves, and there actualModel is the routed model. mcpl:inference_start (which knows only the routed model) and mcpl:inference_fail (which answers no result) are unchanged. - test/helpers/mock-membrane.ts: createMockResponse sets `details.model`, which membrane's NormalizedResponse type requires and the router now reads, as it already set `details.stop`. No test reads it through the mock today. - test/mcpl-inference-router.test.ts: the scripted adapter takes the model the provider served (`scripted-model` unless given), and ask() takes a routing policy. In both tool modes: a streamed result names the served model, and the start trace still names the routed one; a whole result names it; the completion trace carries it as actualModel, streamed and whole; and a response whose provider names no model, under a policy whose default is `house-summarizer`, gets that routed model with no actualModel in its trace (a control, which a fixed `default` fallback fails). The stand-in for the stop reasons membrane can't play gets `details.model` too. The whole-mode test that pinned `model: 'default'` was a control; it now pins the model that served the request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…is change needs anima-research#643 is main 4e3fea3 merged with anima-research#582's floors and context-manager ^0.18.0. Its membrane ^0.6.0 is the release with progressTimeoutMs, which this change passes on every provider call; on main's ^0.5.87 it can't typecheck. Once anima-research#643 merges, merging main makes this branch's diff its own. Resolved: - src/mcpl/inference-router.ts: main's streamResponse, from anima-research#380, with this change's progressTimeoutMs on both membrane calls. Its aborted arm answers 'timeout' with "No stream progress within <n>ms (inferenceProgressTimeoutMs)" and any other reason in anima-research#380's wording. The router takes its chunks from the responder, so the constructor's sendChunk argument is gone and its options come sixth. - src/agent.ts: both sides' additions, the deadline's default and normalizer beside assertAgentSwitches. - src/framework.ts: both imports; the stream loop maps a deadline's 'timeout' to a retryable error, then main's locus follow reads the event; the router is built with the framework's deadline. Tests and text that main's changes reach: - inference-progress-deadline: the router harness passes chunks through the responder. anima-research#380's own failure answers stay in its tests. An abort that isn't the deadline's is now reason 'error', since a 'user' abort settles as a deliberate cancel on main and is never exhausted; a router test pins that another abort keeps anima-research#380's wording. - mcpl-inference-router: a TimeoutAbortError, which in the router's calls only the progress deadline raises, now answers with the deadline's message. - tool-result-guard: the ephemeral run's abort shape is reason 'error'. A 'timeout' is the deadline's, which the error policy retries rather than ending the run. - The changelog fragment drops its bullet on MCPL error answers, which is anima-research#380's fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TL;DR: an MCPL server's
inference/requestnow gets a result only for an inference that completed with a stop MCPL can name, and the JSON-RPC error otherwise, streamed or not. A failed or timed-out stream was answeredend_turnwith its partial text, and in XML tool mode a model's tool call left a streamed request unanswered.We'll merge this not before 2026-10-17 12:00 UTC, once our pre-merge checks pass on its merge with the then-current
main, unless you ask for changes before then.Problem
InferenceRouter.handleStreaming(src/mcpl/inference-router.ts:271–313onmainatd5e6feb1) read only membrane'stokensevents, then answeredend_turnwith the joined text and usage0/0. So:error), such as on OpenRouter's documented mid-stream error, or stopped early (aborted, such as on a timeout) reached the server as a successful completion of its partial text;anthropic-xmlrecipes (src/recipe-formatter.ts:20),</function_calls>is a stop sequence whether or not tools are declared (membrane 0.5.87,src/formatters/anthropic-xml.ts:749). A model that writes a call makes the yielding stream emittool-callsand wait forprovideToolResults(src/yielding-stream.ts:276–290), which the router never called, so the request was never answered;end_turnwith zero usage, so an answer cut off atmax_tokensread as finished, and its content held every token, thinking included. feat(inference)!: cut an AgentFramework model call that stops making progress, and give a deadline expiry one fresh call #378 lists theend_turnand zero usage under Out of scope.Stop reasons that
FinishReasonhas no value for were answeredend_turnon both paths. The non-streaming path'smapStopReason(:322) sent every stop reason butmax_tokensandstop_sequenceasend_turn, and the streaming path answeredend_turnwhatever the stop. That coversrefusal,tool_use,pause_turn,abort, and membrane's guard stopsno_progressandround_limit. membrane keepsrefusalin its own mapper because defaulting it toend_turnhid refusals (src/membrane.ts:2981–2985). A provider that ended withfinish_reason: "error"and no error object was answeredend_turnon both paths too. membrane's OpenAI-style adapters pass the token through, and membrane normalizes a stop reason it doesn't know toend_turn, keeping the token asdetails.stop.providerReason(src/membrane.ts:2986–3015). And in XML tool mode,complete()leaves a model's tool-call markup in the text, so the non-streaming path returned it as content.Changes
A server now gets a result only for an inference that ended as a plain completion, meaning a stop that MCPL's
FinishReasoncan name and no tool call. SPEC §11.3 closesFinishReasonatend_turn,max_tokensandstop_sequence. Every other ending gets the error the non-streaming path already sent for a failure, code-32603with the cause in its message, and anmcpl:inference_failtrace. Nothing is added to the wire: no field, code or option. Under §4.1 a request expects a response, which JSON-RPC 2.0 makes a result or an error object. §11.4 puts the chunks first: "Chunks are followed by the full response."streamResponsereads the stream to its terminal.completereturns the response.errorthrows its error.abortedthrows "The inference stopped before it finished (<reason>)".tool-callsthrows "The model called a tool (<names>), and inference/request offers none". A stream that ends with no terminal throws "The inference stream ended without a result". Leaving thefor awaitby a throw calls the stream'sreturn(), which cancels it, so no stream is left waiting for tool results.resultFrombuilds the result from membrane's response on both paths:contentfrom its text blocks,finishReasonfrom itsstopReason,usagefrom its usage. It throws instead for:finish_reason: "error"and noerrorobject, which membrane passes on asdetails.stop.providerReason. It gets "The provider ended the inference with an error (finish_reason 'error')". OpenRouter's documented mid-stream error also carries anerrorobject, and membrane's adapter fails the stream on that (src/providers/openrouter.ts:281), so it gets that stream error before this check;tool_useblock in its content. membrane's native yielding path lists a call intoolCallsonly when the stop istool_use(src/membrane.ts:4125), so a block cut off atmax_tokensis only in the content. Itscomplete()lists everytool_useblock intoolCallsas well as in the content (src/membrane.ts:2648–2660);details.stop.unclosedToolBlock), such as a call cut off atmax_tokens, which gets "The model's output ended with a tool-call block still open, and inference/request offers no tools". membrane tells an open block from the text, so a completion that writes<function_calls>in prose and never closes it gets this error too;</function_calls>with no call membrane could read in the block (details.stop.triggeredSequence), which gets "The model wrote tool-call markup, and inference/request offers no tools".inference/requestasks for no stop sequence of its own (§11.2), so that stop is the one XML tool mode adds;FinishReasonhas no value for, which gets "The inference ended with stop reason '<reason>', for which inference/request has no finishReason".ChunkMeta.visible). Thinking and the text of a tool call are no longer sent asinference/chunkdeltas. The result'scontentcomes from membrane's text blocks, which membrane parses separately, so in XML tool mode the chunks can leave out text the content holds. membrane's stream parser opens a block at a<function_results>or<invoke>tag written in prose, and reads a thinking block cut off before its close tag as thinking, which the content keeps as text.mcpl:inference_completetrace now carriesinputTokensandoutputTokens.streamed, which only that trace carried, is on every success and failure trace.What a server newly sees:
finish_reason: "error", or ended with a stopFinishReasoncan't name was answeredend_turn.<function_calls>in prose and never closes it, and where a nativetool_useblock cut off atmax_tokenswas dropped from a completion.finishReasonandusage, and content without thinking, except a thinking block still open when the output ended, such as one cut off atmax_tokens. A result that isn't streamed still carries<thinking>markup in XML tool mode, as before, since membrane'scomplete()keeps it in the text.What each part is for
This change makes
inference/requestanswer each way an inference ends: a result only for a completed inference whose stop MCPL can name, and the JSON-RPC error otherwise, the same in both modes. For each part, what it buys, what it costs to keep, and the simpler option weighed against it:streamResponsereads the stream to its terminal event. A stream that fails, stops early or yields a tool call ends the request with the error, and none is left waiting for tool results. It costs a switch over the stream's events. Reading onlytokens, as before, answered a failed or stopped stream as a completion, and left a tool call's stream waiting with no answer.resultFromfor both modes. A streamed result carries the response's realfinishReasonandusage, and each check below covers both paths. It costs reading membrane'sdetails.stopandtoolCalls, socreateMockResponsenow setsdetails.stopfor the tests that use it. A builder per mode, as before, is how the streamed result came to sayend_turnwith zero usage.finish_reason: "error"(details.stop.providerReason). A provider's bare error token isn't answeredend_turn. It's one comparison. Leaving it to membrane would leave hosts on 0.5.87, whose OpenAI-style adapters pass the token through, answering itend_turn. fix(providers): an error a provider reports inside a 200 fails the call antra-tess/membrane#147, merged after that release, fails the call in membrane instead.tool_useblocks in the content. A tool call isn't returned as content or dropped, sinceinference/requestoffers the model no tools. It takes two conditions: XML tool mode's calls are parsed from the text intotoolCallsalone, and membrane's yielding stream lists a native call cut off by another stop only in the content. As before, a whole result left a native call out of its text.</function_calls>. XML tool mode's markup isn't answered as a completion. It reads two fields ofdetails.stop. As before, the markup was returned as content.FinishReasoncan't name. A stop such asrefusal, or one of membrane's guard stops, isn't reported as a finished turn. It's a switch over the three values §11.3 allows. Mapping them toend_turn, as before, is what hid them.inference/chunkdeltas. The cost is that in XML tool mode the chunks can leave out text the result'scontentholds, as Changes says. Forwarding every token, as before, sent them.streamedon every trace, and token counts on the streamed success trace. As before, that trace saidstreamedbut had no token counts, and the others didn't saystreamed.Membranewith a scripted adapter, so each case meets membrane's own streams. Stand-ins play only what a provider can't, since stand-ins for every case would test the router against what we assume membrane does. They pin the contract the change states: an inference that doesn't complete with a stop MCPL can name, or that calls a tool, gets the error.Left out: anything new on the wire (no field, code or option), partial text in the error, and what's under Out of scope.
Tests
test/mcpl-inference-router.test.ts(new, 43 tests) runs the router on a realMembrane(0.5.87) with a scripted provider adapter, in XML tool mode (AnthropicXmlFormatter({ toolMode: 'xml' }), which connectome-host'srecipeFormatterbuilds foranthropic-xmlrecipes) and withNativeFormatter, its default. Every test also checks that its answer was traced: an error bymcpl:inference_failwith the error's message, a result bymcpl:inference_complete. Every streamed test that checks its chunks checks them and the answer in the order they reached the server.max_tokensstop's finish reason, usage and success trace; a stop at a stop sequence, answeredstop_sequence; a provider error partway, answered with its message after the chunkA summand with a failure trace; aTimeoutAbortErrorpartway, answered with the error after that chunk; a refusal; a provider'sfinish_reason: "error"after the chunkA summ, answered with the error after that chunk.finish_reason: "error".tool_useblock cut off atmax_tokens, streamed, and not streamed as a control: therecomplete()lists the block intoolCallsas well as in the content, so either of the router's two checks answers it.<thinking>are read out of the text: a streamed tool call, answered with the error, with only the text before the call in the chunks and no stream left waiting for tool results; a tool call that isn't streamed; and, streamed and not, a tool call cut off atmax_tokens, an empty tool-call block, one whoseinvokehas no name, and prose that names<function_calls>and ends its turn; thinking left out of the chunks and the content.FinishReasonvalue other thanrefusal(tool_use,pause_turn,abort,no_progress,round_limit), and a stream that ends with no terminal event.test/helpers/mock-membrane.ts:createMockResponsenow setsdetails.stop, which membrane'sNormalizedResponsetype requires (src/types/response.ts:242) and the router now reads. Without it,mcpl-awareness-barrier's two startup tests ofinference/requestfailed.Against
main'ssrc/(d5e6feb1, with these tests), 36 fail and the 6 controls pass. The streamed tool call is cancelled by its 5 s timeout, since there the stream parks.Each of the 16 pieces, removed alone, stops at least one test from passing: the handling of
error,abortedandtool-calls, the visible filter, the provider'sfinish_reason: "error", the response's tool calls, atool_useblock in the content, the open tool-call block, the stop at</function_calls>, the stream with no terminal, the unnamed stop, thestop_sequencearm, the response's usage, the streamed stop reason, and the two traces' new fields. Without thetool-callshandling, the streamed tool call is cancelled by its 5 s timeout; every other removal fails a test outright. Holding every chunk back until after the answer fails 11 tests, and so does sending none: each streamed test that checks its chunks.This head changes only the router's test file, two comments and the changelog fragment since
647dd612. At it,tscis clean, the router's test file passes 43 of 43 on Node 24.22.0 and on Node 20.20.2, and GitHub's CI run passes all four Build & Test legs: Ubuntu and macOS, each on Node 20 and 24.The full suite at
647dd612, whosesrc/differs from this head's only in two comments, with CI's steps, on Linux with membrane 0.5.87, context-manager 0.14.1 and chronicle 0.4.0 installed:tsc7.0.2 is clean.npm test) and on Node 20.20.2. A second forced run on Node 24 counted 2,395:--test-force-exitended it beforetest/mcpl-inference-router.test.tshad reported 3 of its 8 top-level blocks. That file passes 43 of 43 on its own.Not verified
finish_reason: "error", membrane's own OpenRouter, OpenAI and OpenAI-compatible adapters also ran, against a local server playing OpenRouter's documented failures and a barefinish_reason: "error".OpenAIResponsesFormatter, which connectome-host builds for OpenAI Responses providers, wasn't run.OTHERandweird_stopto a realMembrane, as Gemini's adapter returnsOTHER(src/providers/gemini.ts:730–736).Out of scope
inference/lifecyclenotification also ends a failed stream withcompleted: a stream that fails with no retry, a failed attempt that is retried (A retried stream's failed attempt ends as if it had completed: its partial prose is finalized and its lifecycle terminal is completed #358), and a stream that times out. That fix follows in its own pull request once fix: a cancelled stream is a deliberate stop, not a failure; a shutdown is neither (supersedes #134) #172, which changes the samedriveStreamcode, has merged.modelstays the routed model's name on both paths, as before. §11.3 describes it as "Actual model used", which membrane'sdetails.model.actualwould give (src/types/response.ts:161–164). MCPL inference/request reports the routed model alias, not the model that served the request #414 tracks it.stop()isn't cancelled: the router passes membrane no signal, as before. stop() neither cancels nor waits for an in-flight MCPL inference/request #418 tracks it.'error', still reaches the server asend_turn, since membrane normalizes it to that. A probe with the tokensOTHERandweird_stopgotend_turnand the text in both tool modes, streamed and not.errorobject, gets the error, as it did onmain. Its message is membrane'sCannot read properties of undefined (reading '0'), from readingchoices[0](src/providers/openrouter.ts:675), which is membrane's to fix.Written by Nigel-2654 (Claude Opus 5.5). Paloma-2644 found the defect. Ada-1017 made the design calls, the visible-only chunks among them, and Petra-1012 checked the reading of the MCPL spec. Gordon-2414 reviewed the code, after our internal review, and then round 2, which answers Anarchid's first review. Esther-2584 noticed that a comment needed membrane's version, and reviewed round 3, which answers his second.