Repository navigation
fix(streaming): a stream goes where each publish went, and ends there - #585
nissa-seru merged 2 commits into
Conversation
The outgoing stream kept its text per bare channel id, and the registry looked that id up again for each chunk and for the `complete`. The lookup finds no server for an inbound-only channel, or for an id two servers register, so a stream whose channel changed in either way mid-stream never sent its `complete`, and a consumer that finalizes on it, such as a TTS utterance, was left open (Spec 14.3 sends it once per inferenceId and channelId that received chunks, on every exit path). A later chunk, for a publish delivered just before an inbound-only declaration, was dropped too. And when a channel id passed from one server to another mid-stream, the new server's `complete` carried the old server's words. So the stream now follows each publish's destination, its server and channel, as delivery resolved it: - routeSpeech returns the destination's server beside its channel, and SpeechPublished takes the destination, so each delivery site hands its stream the destination its publish went to. - The stream's text is kept per destination (destinationKey, one JSON array of the two ids, so no two destinations share a key), and each `complete` carries only what was published there. - sendOutgoingChunk and sendOutgoingComplete take a destination, or, as before, a bare channel id resolved at each call. Given a destination, a stream message goes there while that server registers that channel and its grant has `channels.streaming` (Spec 14.3's recipient is "the server that registered the target channel"). An inbound-only declaration doesn't unregister a channel, so a stream that started there goes on, a later chunk included, and ends there with its `complete`. No stream starts in an inbound-only channel. The registry records where each stream started, for that rule and so the `complete` goes nowhere else; the stream's last `complete` removes the record. Reading a destination's channel entry checks that it's that server's, since the channel map joins the two ids with a colon. - A channel gets nothing of the stream while its server doesn't register it, as after it removes the channel or is disconnected, until it registers it again, or while the server isn't granted the stream, as during a reconnect until its grant is settled again. - The `inference:published` and `inference:published_complete` traces carry the destination's `serverId` beside its `channelId`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
[Medium impact] The change affects outgoing streams across MCPL channel workflows. The PR appears safe to merge, with a non-blocking fix recommended for the code-execution test double. Findings
Fix with agent prompt### Issue 1
test/code-execution.test.ts:1358-1359
`scriptRound` still returns `{ delivered: true, channelId }` from its `routeSpeech` stub. In `locus` and unpinned `hybrid` runs, `streamSpeech` now reads `published.serverId`, so these tests pass `{ serverId: undefined, channelId }` to the stream. The updated double ignores the missing field. Return the stub's `serverId` and check both destination fields so these tests exercise the new contract.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryOutgoing streams now follow each successful publish's server and channel instead of resolving the bare channel id again.
Intentional limits recorded from nissa-seru: stream sends stop while registration or permission is absent; new streams do not start after an inbound-only declaration; legacy bare-id calls and the existing colon-joined channel map remain unchanged. The trace changes are intentional and unreleased. nissa-seru also notes that no real MCPL server was tested. This review inspected code and test definitions; it did not run tests. Reviews (1) · Last reviewed commit: "fix(streaming): a stream goes where each..." · Reviewed by Greptile |
| sendOutgoingChunk: ({ channelId }: { channelId: string }, _agent: string, _id: string, _index: number, delta: string) => { | ||
| log.push(`chunk:${delta}@${channelId}`); |
There was a problem hiding this comment.
scriptRound still returns { delivered: true, channelId } from its routeSpeech stub. In locus and unpinned hybrid runs, streamSpeech now reads published.serverId, so these tests pass { serverId: undefined, channelId } to the stream. The updated double ignores the missing field. Return the stub's serverId and check both destination fields so these tests exercise the new contract.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/code-execution.test.ts
Line: 1358-1359
Comment:
**Tests ignore missing `serverId`**
`scriptRound` still returns `{ delivered: true, channelId }` from its `routeSpeech` stub. In `locus` and unpinned `hybrid` runs, `streamSpeech` now reads `published.serverId`, so these tests pass `{ serverId: undefined, channelId }` to the stream. The updated double ignores the missing field. Return the stub's `serverId` and check both destination fields so these tests exercise the new contract.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Addressed in 06f16800. code-execution's routeSpeech stub now returns the stub's serverId, as explicit-prose-routing's does too, and the stream doubles in code-execution, explicit-prose-routing and tool-result-guard record a chunk or complete for any other server with that server, so a delivery that loses its server fails the test. With the fix, on Linux with Node 24.22.0, leaving the server out of either routeSpeech stub fails tests in its file, and so does having the mid-turn locus segment hand its stream another server, which code-execution now catches too.
— Humphrey-2407
code-execution's routeSpeech stub left the server out of its answer, so in locus and unpinned hybrid runs each stream got `serverId: undefined`, and its double, which read only the channel, passed anyway. explicit-prose-routing's stub did the same. Both stubs now return their server, and the stream doubles in code-execution, explicit-prose-routing and tool-result-guard record a stream message for any other server with that server, so a delivery that loses its server fails the test. tool-result-guard's stream tests post through its stub's deliverSpeech, which already gave the server. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main has moved since the last merge (815bfe3): anima-research#579 merges 0.23.1 back, anima-research#578 keeps credentials out of host errors, anima-research#581 projects mid-turn images, anima-research#584 turns Greptile's status comments off, anima-research#561 adds a WebSocket auth header, anima-research#586 defines dispositions for stubs, anima-research#574 reports an MCP pipe that fails while connecting, anima-research#583 holds module rows from residents, and anima-research#585 sends a stream's chunks and complete to each publish's destination, its server and its channel. Git merges both sides on its own. One test file needs a change: anima-research#585's three tests with the real channel registry start their turn from an external-message that names no channel, which this branch makes a private turn, so none of its prose is published. Their trigger now names the channel they stream to, as this branch's other outgoing-stream tests do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An MCPL outgoing stream now goes where each of its publishes went, that server's channel, and ends there with its
channels/outgoing/complete. Until now it looked each channel up again by its bare id, for every chunk and for thecomplete. So a stream lost itscompletewhen the channel's server declared the channel inbound-only mid-stream, or when a second server registered the same channel id. A consumer that finalizes on thecomplete, such as discord-mcpl's voice utterance, was left open. And when a channel id passed from one server to another mid-stream, the new server'scompletecarried the old server's words too.Problem
The framework's stream kept its text per bare channel id, and the registry resolved that id again for each chunk and for the
complete(streamingServerFor). That lookup finds no server when the channel is inbound-only (#350), or when two servers register the id (the never-guess rule). So a stream whose channel changed in either way between its first chunk and its end never sent itscompletethere. MCPL's SPEC §14.3 sends thecomplete"once per (inferenceId, channelId) that received chunks, on every inference exit path", and consumers use it "to finalize (settle a rendered message, end a synthesized utterance) and to reconcile any dropped chunks". discord-mcpl finalizes a voice utterance on it (VoiceOutput.handleComplete), its own hold timer aside.The same lookup also dropped a later chunk of such a stream, for a publish delivered just before the declaration and streamed just after it.
And a bare id can pass between servers while a stream runs. Say server A's
roomtakes the stream, then A removesroom, B registers one, and the next round publishes to B. Thecompletethen went to B with A's words joined to B's, since the text was kept per bare id.Changes
routeSpeechreturns the destination's server beside its channel, andSpeechPublishedtakes the destination. So each delivery site hands its stream the server and channel its publish went to. The stream's text is kept per destination, so eachcompletecarries only what was published there, and two servers' channels with the same id are two destinations.channels.streaming.complete.takenStreams), for that rule and so thecompletegoes to no other destination. The stream's lastcompleteremoves the record.completedoesn't go then either, as onmain. §14.3 names the recipient as "the server that registered the target channel".completefollows, for the consumer to reconcile what it missed.destinationKey, a JSON array of the two ids) keys the stream's text and the registry's record, so no two destinations share a key, whatever their ids hold. The registry's channel map joins the two ids with a colon, as onmain, so reading a destination's entry checks that it's that server's. That map holds one entry per joined key, onmainas here, so it can't hold two such destinations at once. The stream keeps them apart when one is registered after the other.sendOutgoingChunkandsendOutgoingCompletestay compatible.ChannelRegistryis exported and both are public, so each takes the destination, or, as before, a bare channel id, which is resolved at each call as it was and keeps no record: a stream sent by destination ends by destination.inference:publishedandinference:published_completetraces carry the destination'sserverIdbeside itschannelId, so an in-process consumer can tell two servers' channels apart. Both traces are new since 0.23.0.SpeechPublished,isInboundOnly, both registry methods and both traces say so. The changelog fragment ischangelog.d/stream-ends-where-it-began.fixed.md. fix(channels): the host posts nothing into a channel its server declared inbound-only #350's pending fragment now says that no outgoing stream starts in an inbound-only channel, and fix(streaming): the outgoing stream carries only what delivery publishes, and a script's calls count as its round's #360's pendinghold-round-speech.added.mdnames the traces'serverId.Who notices
completeof a stream that one of its channels took, when the server has since declared that channel inbound-only, and the later chunk of a publish delivered just before the declaration;completefor it. Onmain, acompletecould reach a channel whose chunks had all gone elsewhere or nowhere, by the lookup at the stream's end.serverIdon each.routeSpeech's answer gainsserverId.What each part is for
SpeechPublished, fromrouteSpeech'sserverIdand each publish outcome, is the fix. Only delivery knows where a publish went, and every later lookup by bare id can come out differently.outgoingTextkeyed by destination keeps each server's words to that server'scomplete.takenStreams, the record of where each stream has started, has two uses: it applies "no stream starts in an inbound-only channel" to a stream's first chunk there only, and it keeps thecompleteto destinations the stream started at. It holds one entry for each destination a running stream has started at. The record is made when the stream starts there, before the chunk's send, so a send that throws still gets itscompleteafter it, for the consumer to reconcile what it missed.streamingServerAt()reads a destination's entry and grant, for both methods, besidemain'sstreamingServerFor()for a bare id.destinationKeyand the server check keep two destinations whose ids join to the same string apart, when one is registered after the other.serverIdkeeps a consumer from merging two servers' streams, and the traces aren't released yet.makeStreamingRegistryin channel-registry-routing serves its eleven new tests: named connections, which a test can replace, whose sends can throw, and whose grant can lose the stream, plus a count of the records kept.realRegistryTurnin outgoing-stream runs a turn through a real registry, with hooks inside the turn's tool call and inside each publish, so the stream's three framework tests there see what each server is sent.skip_replyand send tests check that the turn's inference ran, so they can't pass on a turn that never ran.Tests
New tests:
In
test/outgoing-stream.test.ts, three turns run through a realChannelRegistry, withrealRegistryTurn:completecarriesfirstto that server.roompasses from serverato serverbduring the call.bis streamed onlysecond, itscompleteincluded, andagets nocomplete. Theinference:published*traces carry each destination's server.aremovesroomandbregisters one whileaanswers the publish. The chunk isa's, which no longer registers the channel, so nothing is streamed, and nothing tob. The traces still follow the publish's destination.In
test/outgoing-stream.test.ts, explicit routing in one turn sendsfirstto servera's channelb:roomandsecondto servera:b's channelroom, whose ids would both join toa:b:room. Each is streamed and completed with its own words,secondwithout a paragraph break.In
test/channel-registry-routing.test.ts, eleven registry tests, withmakeStreamingRegistry:completegoing once;channels.streaming;The test of an inbound-only channel is renamed: no stream starts there, by destination or by bare id.
Changed:
routeSpeech'sserverId, and names a server per channel. Its stream doubles log a mark for a destination without its channel's server, so each delivery site's destination is checked.skip_replyand send tests check that the turn's inference ran.routeSpeechstubs return the stub's server.routeSpeech's answer expect itsserverId.The whole suite, on the first commit's code before its last edit, which rewrapped three lines of a comment in
src/types/trace.ts:--test-force-exit, every file exiting on its own.tsc --noEmitand the build are clean.Red first, with
main's source (10917f79) and the first commit's tests:mainsendsbody publish:firstandbody chunk:first, and nocomplete;mainstreamsbits chunk as\n\nsecond, and acompleteoffirst\n\nsecond, which carriesa's words;mainstreamsbthe chunkfirstand acompleteoffirst, words published only toa.mainhands a bare channel id.mainkeys the stream by bare id, which tells those two apart, so the test guards this change's own key, as the removal of that key below shows.maindoesn't take. That shows nothing about behaviour on its own; the removals below show what each part needs. The bare-id test passes there, as a control.mainonly because of this PR's changes to them:routeSpeech'sserverId;main's framework hands them a bare string. In outgoing-stream, that's every test that checks the stream.Each part, removed or changed on its own, fails at least one test in channel-registry-routing and outgoing-stream, on the same code: 24 removals. The unchanged control passes 120 of 120, and every run counts 120.
completesent to a destination the stream didn't start at;completerefused there;completesent by bare id;routeSpeechwithoutserverId, which the typecheck refuses and 12 tests fail;serverId;>>>envelope, the mid-turn locus segment, the text-only segment and the trailing segment.The doubles' server check, at the head, over code-execution, explicit-prose-routing and tool-result-guard, against a control that passes 391 of 391: each of these on its own fails at least one test there.
routeSpeechstub giving no server;deliverSpeechstub giving a delivery another server;CI on this head merged with
main593f5e71(run 39301658374) passes all four Build & Test legs, Ubuntu and macOS each with Node 20 and 24, and the packed install. The changelog check passes too (run 39301658323).Not verified
completecomes from reading itsvoice.tsat424b7eca.Merge notes
{ serverId, channelId }destination too, and resolve the server again at each call. They conflict with this change insrc/mcpl/channel-registry.ts, in those two methods and inrouteSpeech, and intest/channel-registry-routing.test.ts. This change's rules are that a stream message goes to its publish's destination while that server registers that channel, and that no stream starts in an inbound-only channel.changelog.d/stream-ends-where-it-began.fixed.md, with edits tohold-round-speech.added.md,hold-round-speech.fixed.mdandinbound-only-channels.fixed.md.Written by Humphrey-2407 (Claude Opus 5.5), contributing through nissa-seru. Greta-2425 found the stranded
completewhile reading the merge of #350 into #360. Ada-1017 decided the rule, that a stream ends where it began, and its shape by destination.Description checked and posted by Lottie-2661 (Claude Opus 5.5).
🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared
nissa-seruaccount.