feat: turn loop, block folding, transforms, tool dispatch and hooks (4/5) - #11
Open
bdchatham wants to merge 23 commits into
Open
feat: turn loop, block folding, transforms, tool dispatch and hooks (4/5)#11bdchatham wants to merge 23 commits into
bdchatham wants to merge 23 commits into
Conversation
The first half of PR 4. Decides when one turn has ended, which is the piece the
rest of the turn loop is built on.
Upstream ends a turn on the typed terminal event alone:
if isinstance(event, _TURN_TERMINAL_EVENT_TYPES):
return
That is correct for its own case and wrong for ours, and the difference is not
style. Two harness families put the turn's end in different places. On a
terminal-backed harness a response.completed means only that the prompt reached
the harness; the turn ends on an idle status edge naming a response. Taking the
lifecycle terminal there hands a caller a partial answer it believes is whole.
So the tracker holds the three facts no single event carries: whether the server
has echoed this turn's prompt, which responses predate it, and which signal this
harness family ends on. Every rule here was established in the agentic driver by
a failure that reached a real run, and this file carries them so PR 5 can delete
that copy:
- The echo is matched on the item id or the pending id, because the anchor is
whichever the post returned. The item id is compared first, so a turn holding a
pending anchor is not matched by an unrelated message.
- An unanchored turn cannot cross the boundary. Both comparisons are between
strings, so an absent item id would otherwise match anchor "" and unlock every
rule that waits on the boundary.
- An idle edge needs the boundary, a response id, and a response this turn did
not inherit. A bare idle edge is session churn.
- A failed edge naming a response is narrowed the same way. One naming no
response is not: that is how the server reports a session-level fault, and
before the boundary it is what a caller is waiting on.
- A superseded session is reported, not followed. Which session to address next
is the caller's decision.
- The first cause wins, so the reported reason is what stopped the turn.
Which harness a session runs stays the caller's knowledge: TurnOptions.End takes
the value, and this package holds the rules it selects. The zero value is the
stricter rule, because the two mistakes do not cost the same — a partial answer
read as whole against a turn that waits for its deadline.
Nine mutations, each asserted as landed, each caught: dropping the anchor guard,
the prior check, the boundary check, the bare-idle rejection, the failed-edge
narrowing, the harness mode, first-cause-wins, the prior-map copy, and flipping
the default to the looser rule.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rendering half of PR 4. An Event is what the server sent; a Block is what a caller draws. The stream carries deltas, duplicate reports and lifecycle noise no renderer wants, so this is the smaller set a switch is worth writing over. Fourteen variants, sealed the same way as Event, with BlockContext as the behaviour every one carries — which is what OnlyAgent reads, so it belongs on the interface rather than on each variant. Each variant embeds an unexported struct that supplies both, so a new one restates neither. Five transforms, plus two Go affordances upstream does not need: - Pipe composes left to right, matching the order a caller says it. - SkipBlocks and OnlyBlocks take predicates rather than a list of types, because Go has no variadic type parameter and a list of reflect.Type would move the mistake from compile time to run time. IsBlock[T] builds the common case and the compiler checks the type belongs to the union. - OnlyAgent treats an empty name as "every agent", so a caller passes a configured value through without branching on whether it was set. - SkipIntermediateEnds holds each end back until something follows it. A tool loop reaches a terminal response once per iteration, so a caller rendering every end draws the turn as finished several times. - MergeTextAcrossIterations reports one answer per response instead of one per iteration, and still flushes when a stream ends without a terminal response — otherwise a dropped stream loses the text it had already gathered. No transform swallows an error, which is asserted for all five at once: a dropped error leaves a caller reading a truncated turn as a complete one. Also corrects both seal claims. Verified from outside the module: an independent implementation is rejected, and a type embedding an exported variant is accepted, because embedding promotes the marker. So the seal stops a foreign implementation and is not a proof, and both docs now say that rather than "only this package can add a variant". Eight mutations, each asserted as landed. Three survived the first pass and the tests were the fault, not the code: the order test composed two transforms that commute, the skip-intermediate fixture put a block between every end so holding the first and holding the last behaved identically, and the merged-context assertion read a path fed by the end block rather than by the accumulator. All three now fail when they should. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes PR 4. A caller sends a prompt and reads the answer without writing the loop. BlockStream folds events into blocks. The wire is not a rendering model: text arrives as deltas, reasoning on a second delta channel, a tool call and its result as two undeclared items, and one turn's tool loop reaches a terminal response once per iteration. Three parts of that carry their own reasoning: - Text flushes on a word boundary, not at the threshold, because a renderer cannot un-draw a chunk that ended mid-word. An unbroken run longer than the threshold is still emitted, since waiting for a boundary that never comes holds the whole answer back. - A doubled tool report is one block. Under the MCP path a call and its result each surface twice with the same call id, and both are the same call. - The closing ReasoningBlock is suppressed when the section already streamed chunks, so a renderer showing both does not draw the same reasoning twice. Text arriving is what closes a section; no event states it. Chat drives the turn. The order is the part that matters: the subscription has to exist before the prompt is posted, or the turn can be answered with nobody listening and its events are missed. Client.Stream is lazy, so the prompt is posted from inside the read loop once the subscription is live, bounded by a short wait for the server that sends no heartbeat. A tool call and an approval are both answered before the turn's end is read, because the server parks the turn on each and the terminal event only follows. Both are answered even when they fail: an unregistered tool posts an output naming the mismatch rather than leaving the turn parked to its deadline, and a panicking tool fails its own call instead of the loop. Only an action_required item is dispatched — a completed one was run by the server, and running it again would repeat a write or a deploy. An approval with no decision is declined. That is this package's own behaviour and not a policy: it cannot know what a caller would approve, and approving runs a tool under their identity. It is answered against the session the request names, which is the sub-agent's own when its prompt is mirrored into an ancestor's stream. A Turn is single-use. A second read would post the prompt again and the server would answer both. Thirteen mutations, each asserted as landed. Three survived the first pass and the tests were at fault: the word-boundary fixture used words short enough that a fixed cut landed on a space by luck, the reasoning fixture ended in a newline so it was already flushed, and no test covered a server-run tool at all. A fixture guard now rejects a frame whose discriminator is in no decoder — one mis-spelled "session.input_consumed" arrived as an UnknownEvent, crossed no boundary, and cost three ten-second hangs before the cause was visible. FR-029: 47 of upstream's 50 public symbols map to a Go symbol. The other three — LocalServer, QueryResult and QueryStream — are recorded in doc.go as deliberate absences with the reason for each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…king
Five reviewers' worth of findings, resolved by deleting the mechanism that caused
most of them.
This branch hand-rolled turn startup: a goroutine, a `ready` channel closed on
the first event, a one-second fallback timer, an `anchors` channel, and a
blocking anchor read. The package already ships StreamOptions.OnSubscribed,
documented as "the supported way to post the input that starts a turn" — and its
own doc explains exactly why the mechanism I built cannot work:
the acknowledgement cannot be recognised by inspecting events. The server
sends the identical {"type": "session.heartbeat"} payload for two different
things — the subscription acknowledgement, and the keepalive it emits every
15 seconds — so "send when I see a heartbeat" sends again on every keepalive
markReady fired on the first event, whatever it was. Measured by the reviewer:
the prompt went out 500ms before the relay acknowledged the subscription, and a
turn whose events all arrived was never attributed to the caller because the
boundary was never crossed — 21 events delivered, then a context deadline.
Posting from the hook deletes all of that machinery and four findings with it:
- A stream that fails to open never runs the hook, so a turn that never started
posts nothing. Before, a 503 handed the caller ErrUnavailable — which this
package documents as "usually transient" — with the prompt already sent, so a
retry ran it twice.
- A caller who abandons the sequence before reading never runs it either, which
is what finally makes Chat.Prompt's promise true.
- The anchor is recorded before the first event reaches the caller, so the
boundary check cannot race it and the blocking read is unnecessary. That read
was the mainline, not a fallback: a slow post stalled the stream for its whole
duration, measured at 3 seconds with the turn already on the wire.
- Chat now refuses a caller-supplied OnSubscribed rather than silently dropping
it. Two hooks meant two prompts for one turn, and Turn's single-use guard does
not cover that because both posts belong to one read.
Two correctness bugs, each reproduced before the fix:
The side effects yielded and discarded the result, then the loop yielded again.
A caller writing `if err != nil { break }` — the ordinary thing — got "range
function continued iteration after function for loop body returned false". A
library panicking a caller's process. Every yield now goes through one emit that
records a stopped consumer, which is the discipline blockstream.go already used.
One call_id ran a tool twice. blockstream.go documents the MCP path delivering a
call twice and folds it away for a renderer; the executor had no such guard, so a
deploy, a spend or a signature ran twice for one authorisation. The existing test
covered only the status=completed case, so the suite read as if this were closed.
Also: a panicking OnElicitation escaped the loop and answered nothing, parking
the turn — upstream maps a hook exception to decline, and now so does this. Two
server-chosen strings reached errors unsanitized and unbounded. ToolCallInfo's
ResponseID and Iteration were documented and never populated. Every function_call
now fires the call hooks with ExecutedBy derived from the item's status, rather
than firing only for client-run calls and labelling them "server".
Test hygiene, from the dissenter's mutations rather than from review by reading:
TestTheSubscriptionExistsBeforeThePromptIsPosted passed 30/30 with the entire
readiness mechanism replaced by time.Sleep(10ms). It asserted the arrival order
of two requests, which any fast mechanism satisfies. Replaced by two tests that
assert the consequence — nothing posted when the stream fails, nothing posted
when nobody reads — which only pass if the post comes from the subscription.
Two FR tests wrapped their assertions in !errors.Is(err, ErrTurnIncomplete), so
neither could tell a finished turn from an unfinished one. Removed; both now fail
when turn-ending is broken, and passed unchanged otherwise, which is what shows
the guards were unreachable clutter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…efault's cost Three findings on the turn tracker, all from the dissenter's measurements. A status edge names the session whose status changed, and nothing read it. A sub-agent's events are mirrored into an ancestor's stream — which is why resolving an elicitation has to be told which session named the request — so an edge on this stream is not necessarily this session's. Measured: a child's idle edge ended the turn against work that was never this turn's, and the caller read half an answer with a nil error. A child's setup failure did the same before this turn had even spoken. An edge naming no session is still taken, because that is how the server reports a session-level fault and refusing it would drop the failure a caller is waiting on. TurnEndsOnIdleStatus claimed the cost of the wrong choice was "a turn that runs to its deadline — a failure a caller already knows how to see". There is no deadline. The server's heartbeat keeps the stream's idle watchdog fed, so a caller on the wrong rule with context.Background() blocks forever and gets no error at all. The doc now says it blocks, tells a caller to pass a deadline because that deadline is the only signal the mistake produces, and names the symptom that identifies the other rule. The superseded error rendered a server-chosen conversation id raw and unbounded: 5,131 bytes carrying a newline and a terminal title-set escape. Sanitized and capped, like every other server string in this package. Also records what this package does not know. transform.go asserts that a tool loop reaches a terminal response once per iteration, and turn.go ends the turn at the first one — a contradiction the dissenter measured as a truncated answer reported as success. The claim came from upstream's own docstring, and upstream contradicts itself the same way: its block stream is fed by a send() that returns at the first terminal, so its skip_intermediate_ends can never see a second one. The vendored description does not state whether one turn can reach a terminal response twice. Rather than guess, observeResponseTerminal now says the first terminal ends the turn and that the contract is unstated, so the fix is a server-side answer rather than a guess here. Three mutations, each caught: dropping the session filter, making the filter reject an unnamed session, and unsanitizing the superseded id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The idiom lens found five public affordances that were declared and unreachable,
one dead function, and a field nobody chose to export. All of it pre-release, so
all of it still cheap.
Nothing ever wrote BlockContext.Agent or Depth, so every block reported the root
agent and OnlyAgent matched nothing a real stream produced — one of five
transforms, dead against every caller. The response's model is where the wire
names the agent, and its dotted depth is the nesting, so the fold records both at
the response start.
The test for OnlyAgent passed because it built a block with blockCtx{Ctx: …}, an
in-package literal no caller and no production path can write. It proved the
transform worked on data nothing produces. Both new tests drive a real fold.
blockCtx's field was exported, so it was promoted onto every variant: settable
from outside the package, so a caller could rewrite what Context reports, and
marshalled as an untagged "Ctx" key into any persisted transcript. go doc showed
neither. Unexported, with blockAt as the one constructor. The seal doc also
claimed "the same means as Event" — Event's variants declare their own methods and
promote nothing, so the mechanisms differ and the difference is what created the
field.
flushPendingResults could not emit. The only writer of an execution's Output set
seenResults in the same statement, so its guard was always true and two call sites
read as doing work that could not happen. Removed rather than revived: reviving it
would also need an answer for map-order rendering, and nothing observable was
missing.
BlockContext.Turn is the tool-loop pass, not the turn, and Turn is now an exported
type meaning one prompt and everything it produces. Renamed to Iteration; one word
cannot be both.
OnReasoningEnd was documented as bracketing a section with OnReasoningStart and
was never fired, so a caller's spinner never stopped. Removed with its context
type rather than shipped unimplemented — a shipped field is a contract. Reasoning
completion arrives on the block sequence, and the doc says so.
MergeTextAcrossIterations claimed to report one answer per response. It flushes at
every terminal, so on a stream carrying one end per iteration it reports one per
iteration — exactly what its doc said it prevented. Its fixture carried a single
end, which is the shape a stream has only after SkipIntermediateEnds has run. The
doc now names the composition and a test pins both halves. Its "context kept"
comment described the path that does not run.
&ToolRegistry{} panicked on a nil map write. The read paths tolerated a nil
receiver; the zero value was the case they missed.
doc.go said QueryResult and QueryStream were deliberately absent because they are
"the one-shot Responses surface". That is false: query() is a method on
SessionsChat, over the same durable Sessions API this package reaches, and
plan.md — which FR-029 names as the record — schedules both for this PR. The
section now lists only what will not be built, names tool with its real reason,
and says the fold is composable today. Two claims about accepting an approval also
said "the caller's identity" where contracts.go, in this same package, says the
session owner's execution identity — different principals, different blast radius.
Also: PriorResponseIDs is the one obligation a caller has and appeared nowhere in
the Turns section; two test comments narrated history against a rule in AGENTS.md;
and turn_test.go's fixture used session.input_consumed, the exact trap documented
one file over.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t invisible plan.md maps upstream's QueryResult and QueryStream to this PR, and they were not built — doc.go claimed instead that they were deliberately absent, on a reason that was false. Built now, on the composition doc.go had told callers to write themselves: BlockStream over Chat.Send, then SkipIntermediateEnds, then MergeTextAcrossIterations. Chat.Query returns the answer and the artifacts. It returns what it gathered alongside any error, because a turn that fails partway still produced whatever it produced. Chat.QueryStream reads the text as it arrives and is single-use for the same reason a Turn is. Files carries the ids and names the stream reported; the bytes are a Download, because an artifact can be any size and folding them in would decide that for the caller. Writing the first test for Query found a fold defect nothing else could see. A message item is the server's complete statement of a text section, so it is the authoritative TextDone — but foldMessage cleared the pending delta buffer before reporting it. Below the flush threshold those deltas were the only TextChunk a live reader would get, so an answer shorter than thirty characters streamed nothing at all. The buffer is now flushed first and the item's text still reports the section. And it resolved the terminal-response question this branch had recorded as open. transform.go asserted a tool loop reaches a terminal response once per iteration, inherited from upstream's docstring, while turn.go ends the turn at the first — the contradiction the review measured as a truncated answer reported as success. The description enumerates no status distinguishing a pass's terminal from a turn's, and if a turn emitted several with no way to tell them apart, upstream's own send() would truncate every tool-loop turn in a shipped SDK. So a tool loop's passes happen inside one response, which is why the response id holds across them and why ToolGroup.Iteration counts passes rather than responses. turn.go's rule was right; the claim in transform.go was the defect, and SkipIntermediateEnds is for a sequence spanning several turns rather than for one turn's iterations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 2's prose review found that settling the terminal-response question swept two of six passages. block.go, blockstream.go, hooks.go and chat.go all still asserted that a tool loop reaches a terminal response once per iteration — the claim turn.go now settles the other way — so the package contradicted itself in four more places than before the fix. A reversal has to carry everything written under it, and this one did not. Eight more claims the code beside them does not support: - StreamHooks said "A hook cannot change the turn." OnElicitation decides whether a parked tool runs with the session owner's execution identity, and doc.go sends security-conscious callers to exactly this sentence. Now: every hook but one only observes, and the exception is named. - Its doc said hooks run "from the goroutine draining the turn", implying one this package starts. stream.go promises it starts none. - Block.Context still promised "which turn it belongs to" after BlockContext.Turn became Iteration — the one surviving passage re-merging the two words that rename existed to separate. - doc.go said the package does not reach the snapshot route and a caller should call it directly. Sessions.Get and Sessions.ListItems have existed since PR 3, and a hand-rolled call gets none of the redirect and credential policy doc.go spends a section on. The recovery path now names both methods. - TurnOptions.PriorResponseIDs is the one obligation the docs place on a caller, and it had no executable path: "from the session snapshot" names no symbol, and the snapshot carries a single optional ActiveResponseID rather than a set. Both doc.go and the field now name Sessions.Get and the field to read, and say that only a true value counts. Verified by writing the construction the doc describes. - TurnEndsOnIdleStatus claimed its deadline was "the only signal this mistake generates". A deployment that caps stream duration reports ErrTurnIncomplete instead, which reads as a transport fault and sends an operator to debug the network. Now stated, with sizing guidance the previous advice lacked. - doc.go said Client.Chat posts the prompt. It binds a session; Chat.Send posts. - describesThisSession documented one exception and the code has two. Two comments narrated a shipped defect in the past tense, against AGENTS.md's rule that comments state the present: blockCtx's field and Register's nil map. Both now describe what would happen, not what did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sweep reached the production files and stopped at the test that names the behaviour. Its comment still described a fixture shape the settled contract rules out, and its name said the ends need folding rather than what it checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 2's dissenter found that the OnSubscribed rework replaced a race with a silent no-op, and it is the most serious thing in this branch. stream.go fired the subscription hook only from the decoded-event arm of its frame switch. So an SSE comment keepalive, an empty frame, or one frame this build cannot decode all left the hook unfired — and Chat.Send, which posts its prompt there, posted nothing, yielded nothing and reported only the caller's own deadline. Measured on all three inputs. A comment keepalive is what nginx, haproxy and envoy emit to hold an SSE connection open, so any relay in front of the server killed every turn. The mechanism this replaced had a one-second fallback; the rework deleted it and put nothing in its place. The hook now fires on the first frame of any kind, which is what "the subscription is live" actually means — a keepalive proves it as well as an event does. The dispatch guard added in 935dd65 was keyed on call id alone. Each agent numbers its own calls and a sub-agent's items ride an ancestor's stream, so one agent's call_1 suppressed another's — and the suppressed call was never answered, parking the very agent the guard exists to protect. Keyed by agent and call now, and a true duplicate is reported rather than dropped in silence: a silent drop is indistinguishable from a call this client never saw. observeSuperseded was left unfiltered when its sibling gained a session filter, and it is the event whose error names where a caller should go next — so any conversation on the stream could redirect a caller to one of its choosing. Filtered. And the filter itself accepted an omitted conversation id as a match, on a field the description marks required and non-nullable; an omission is a dropped field or a sender hoping absence reads as consent. Now taken only on the failed branch, where losing a session-level fault would leave a caller waiting on a turn that failed. One hook was guarded against a panic and seven were not. A panic in OnToolCallStart pre-empts the output post and parks the session until its deadline — a larger blast radius than the hook that was guarded first. The recovery moved into fire, so it covers every hook and cannot be forgotten when one is added, and it reports rather than swallows: a server chooses the fields a hook reads, so it chooses the input that trips one. Chat.Send documented "an error ends the sequence" and three sites broke it, with the error arriving before the event that caused it. Tracking is now separate from the side effects: the tracker advances before the end is read, the side effects run after the event they belong to, and the doc says which sentinels end the sequence and which do not. The model-chosen tool name was unbounded at three sites in tool.go, against a rule errors.go states — the fourth appearance of this class here. The unregistered-tool output also posted the whole registry to the server, so one bogus call enumerated the caller's capability surface; the server now hears only the name it asked for. Idiom findings: eight comments narrated a defect in past tense, added by the same commits that deleted two of exactly that kind. BlockContext.Agent said "empty for the root agent", which the fold made unreachable. Depth counts dots in an agent name, which the description never says means nesting, and now says so. ToolGroup documented a batch it never batches. blockState.pending became write-only when its only reader was deleted, and ToolGroup.Iteration duplicated its own embedded context and nothing read it. Both removed. Six mutations, each caught: unscoping the dispatch key, silencing the duplicate, unfiltering the supersede, accepting an omitted session, unguarding the hooks, and restoring the decoded-event-only subscription notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 2's systems review applied 44 mutations and 19 survived. Two findings and four of those gaps were real; the rest of its list was already closed by the previous commit. The agent fix from e99ac5d made things worse, not better. The fold held one slot for the current agent and overwrote it at each new response start. A text delta names no response, so a mirrored sub-agent's start re-credited the parent's own words to the child: measured, one of six blocks kept the root's identity and OnlyAgent("coder") returned an empty answer. Before the fix it matched nothing; after it, it truncated. Truncation is worse, because the caller reads a partial answer as whole. The wire cannot be made to say which response a delta belongs to, so crediting is only ever right while one response is live. The fold now tracks which responses are live and stops crediting once two are, for the rest of the turn. Crediting nothing is recoverable; crediting wrongly is not. A ResponseStartBlock still names its own model, because that event does say which response it belongs to. The fold's text accumulation was quadratic: fullText += delta copies the whole answer per delta, on the goroutine draining the stream. A 512 KB answer cost 802 ms of read-loop time. With a strings.Builder, 32,000 deltas went from 802 ms to 3.5 ms — 228x — and the per-doubling ratio from 5.18x to under 2x. Four mutations the suite could not catch, each now pinned: - A stream ending before the turn does reported success, because ErrTurnIncomplete had zero references anywhere in the suite. Reaching it needed a fixture that ends its stream cleanly with the terminal sentinel: closing the socket instead is a transport failure, which the loop reports first. - The client re-ran a call the server had already run, because no test drove a function_call whose status is anything but action_required. Now covered for completed, in_progress and empty. - The session filter was defeated at its only production call site — every end-to-end test used the lifecycle rule, which never reaches observeStatus. A turn driven through the idle rule now pins it. - A tool's failure was posted to the server and never reported to the caller. It is the caller's own code that failed, and their own sequence is the only place they reliably look. Also removes a test that was lost rather than deleted: the server-run-tool case went missing when this file's tail was rewritten in the first fix round, which is how the mutation survived at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 20, 2026
bdchatham
changed the title
fullText is a strings.Builder and the comment above it says why: appending to a string copies the whole answer per delta, which is quadratic in its length and runs on the goroutine draining the stream. Three fields twelve lines below did exactly that, and the newline scan walked the whole accumulated buffer on every delta, which is the same cost again. Measured at 31 MiB of reasoning, four frames at the frame cap: 37.4s of CPU with no newlines and 17.0s with them, against 27ms for the text path. After: 23ms and 10ms. Allocation to fold 1.2 MiB went from 24.5 GB to 13.9 MiB. reasoning is a byte slice rather than a Builder because the line flush slices it, which a Builder cannot do. The scan now starts where the previous one stopped. Both costs land on the socket-draining goroutine while the idle watchdog is suspended, so a long reasoning section made the client slow and silent at once. TestTheReasoningFoldAllocatesLinearly pins allocation per input byte rather than wall time, which would flake. It reports 11.4 against a bound of 40, and 20125.7 before the fix. Raised by three of five cross-review lenses independently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A bare blank line inside doc.go's comment block ended the comment group, so the package doc godoc publishes began 140 lines in. `go doc .` opened on LocalServer — a symbol this package deliberately does not have, described under a heading saying so — and 139 of 256 lines went unrendered, including six of eleven sections and the whole Turns section this branch adds. AGENTS.md tells a contributor to read doc.go before changing the public surface. Most of it was not readable where a consumer reads it. One character fixes it. TestThePackageDocIsNotSevered stops it recurring, because nothing else can: gofmt does not mind a blank line in a comment, and TestEveryDocLinkResolves walks every comment group rather than the attached one, so the orphaned half still passed its own link check. Found independently by three of five cross-review lenses, which is how obvious it is once you run `go doc` and how invisible it is otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SendMessage and PostEvent answer a refusal with HTTP 200, denied true and a reason. Both call sites read only the transport error, so a policy denial was indistinguishable from a dropped packet. PostEvent's own doc warns against exactly this. A denied prompt starts no turn: the anchor stays empty, no rule can cross the boundary, and the read ran to the caller's deadline before blaming the stream with ErrTurnIncomplete — a sentinel whose doc says it means the opposite. A denied tool output is worse, because the tool already ran. The side effect happened, the server never learned the answer, and it stays parked on a call it will not accept one for. So its terminal never arrives either. That case now ends the turn on the refusal rather than on the deadline, which is what makes the caller read the right cause last: measured 1.5s to 0.09s. New ErrInputDenied, and a turnRun.unfinishable flag kept separate from stopped — a caller who left and a turn that cannot finish are different facts, and only the second should suppress the incomplete report. Found by two of five cross-review lenses independently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…calls The guard was keyed on the item's agent_name as well as the call id, so the party it defends against chose half the key: the same call, replayed under a second agent name, ran a second time. A deploy, a spend or a signature is what that costs, which is what the field's own doc says the guard is for. The compound key was justified on a wire shape the server cannot produce. The output this package posts carries call_id and output and nothing else — FunctionCallOutputData declares exactly those two — so a call id issued twice concurrently is one the server could not route an answer to either. Upstream's python client posts the same two fields and, on the execution path, guards nothing at all: it invokes unconditionally and records the id afterwards only to suppress a duplicate end hook. So keying on the call alone matches the server's own correlation model and is still stricter than upstream. TestACollidingCallIDAcrossAgentsRunsBoth asserted the opposite and is inverted rather than deleted, carrying the reasoning that changed. The call id being the server's to choose is also why the guard cannot bound how many calls a turn makes: a fresh id per ask is a fresh call. ChatOptions gains MaxToolCalls, defaulting to 256 — well above a legitimate turn rather than close to one, taking the server's own newest-100-items snapshot as the signal for a turn's scale. Reaching it ends the turn, because the server is parked on a call this package declined and its terminal never arrives. A negative value removes the cap. BREAKING CHANGE: a turn that legitimately runs more than 256 tools now stops and reports ErrToolCallBudget. Set ChatOptions.MaxToolCalls to raise or remove it. Raised as CRITICAL by the cross-review's security lens, with the agent_name variation demonstrated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ElicitationCtx carried the session, the id, the message, the phase, the policy name and the content preview. Upstream's ElicitationRequestCtx carries four more: mode, url, requestedSchema and the response id. Those four are what make a decision informed. A "url" mode elicitation asks the caller to approve an out-of-band flow, and this package refuses an off-host redirect on the unary path for exactly the reason a destination matters — so asking for an approval while hiding the destination gave that up. requestedSchema is the shape a "form" answer takes, and the response id is what correlates an approval with the work it gates. Aligning with upstream rather than inventing a shape: upstream's client is the reference for this surface, and it already decided what an approver needs to see. Also raises DefaultMaxToolCalls to 1024, an order of magnitude over the server's own newest-100-items signal for the scale of a turn. Two elicitation findings from the cross-review are deliberately NOT addressed here, because upstream does neither and matching it is the priority: there is no replay guard, so a re-sent elicitation is asked again; and TargetSessionID is honoured as the server sends it, which upstream also does (`params.target_session_id or self._session.id`). Both are recorded in the review ledger. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OnlyAgent read block.Context() without a nil check, so it panicked where the
other four transforms pass a nil block through. Transform is exported and takes
any sequence, so safety depended on the argument value. A filter is not the place
to decide a caller's input is wrong.
SkipIntermediateEnds cleared the held end on every non-end block, so any block
after the last end deleted every end from the sequence. The fold's own trailing
text flush is exactly such a block, which made the pipeline deliver no
ResponseEndBlock at all on a dropped stream — the one case a caller waiting for
an end cannot recover from. A block after an end means the sequence has not
stopped, not that the end was intermediate.
The fold emitted an error and continued, so blocks arrived after a terminal error
and the salvage on the line below was unreachable for `if err != nil { return }`
— the pattern Client.Stream's own doc recommends. It now flushes first and stops:
delivery order goes from [Start Chunk ERROR Chunk Done] to [Start Chunk Chunk
Done ERROR].
Raised by the cross-review's stream lens, each with a failing probe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit stopped SkipIntermediateEnds discarding a held end when a non-end block followed. That was the wrong file. Upstream's skip_intermediate_ends discards it too, deliberately and with the same comment — "a non-end block arrived, so the buffered end was intermediate" — and its docstring says it yields "the one not followed by another block from the same turn". That is only sound while a terminal response is the last block a turn produces, and upstream holds that invariant in its fold rather than its transform: the text flush happens inside the terminal handler, immediately before the end, and nothing is emitted after the loop at all. Ours flushes in the terminal handler too, then flushed again after the loop unconditionally. On a stream that drops after a turn completed, that second flush put a TextDone behind the last end, which the transform then read as proof the end was intermediate. The salvage now runs only when no terminal arrived, which is what it was for. What remains, and is now inherited rather than ours: text arriving after a terminal with no new response still yields chunks behind the end, because the delta handler has no terminal-seen guard. Upstream's does not either, so the same shape produces the same result there. Recorded in the review ledger rather than diverged from. The OnlyAgent nil guard stays. Upstream reads block.ctx.agent unconditionally, so this is deliberately more defensive than upstream — but it changes no contract and removes a panic whose reachability depended on a caller's argument value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oolCallInfo event.go says a live turn never carries ResponseCreatedEvent: the harness emits created and in_progress as an inseparable pair and the server drops the created half at the publish chokepoint feeding every subscriber. chat.go keyed on created anyway, so on every live turn OnResponseStart never fired and every response id the turn reported was empty. The suite passed because its fixture sent created — a shape event_test.go in the same tree says the server never sends. Upstream reads all three: _RESPONSE_START_EVENT_TYPES is CreatedEvent, QueuedEvent, InProgressEvent, and its hook fires once per response id. So does this now, which is why announceResponse keeps a set: three events for one response is one start, not three. ToolCallInfo loses ResponseID and Iteration and gains ItemID, which is upstream's sessions-API shape and for the reason upstream states: an item carries id, call_id, name and arguments, and a response id and a loop iteration are not on that wire. Both fields were filled from the reader's own state — the state keyed on the event above — so both were always empty or zero. A field that is always empty is worse than an absent one, because a caller writes an audit record against it. AgentName goes too. Upstream's sessions-API tool info omits it while its observation hook keeps it, which is a coherent line: an observer wants everything the item offers, and a tool does not need a server-chosen string to answer. OnToolCallStart still reports it. TestAToolCallCarriesItsResponseAndIteration asserted the fields this removes, and passed only on that impossible fixture. Replaced with one that sends in_progress. BREAKING CHANGE: ToolCallInfo drops ResponseID, Iteration and AgentName, and gains ItemID. A tool reading the first three was reading a zero value. Closes the last of the cross-review dissenter's eight findings; all eight of its tests now pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Cross-review addressed: 12 findings closed, 3 deliberately not, 9 openFive blinded reviewers on isolated trees, four of which wrote failing tests rather than arguing. The dissenter's eight tests now all pass. Eight commits, The principle that decided most of thisUpstream is the guide; correctness before security posture. Applied consistently, it did more work than the findings did — it reclassified three findings from defects to inherited behaviour, dissolved three of four one-way doors, and caught a fix of mine that was a divergence dressed as a correction. ClosedTwo that were wrong against the server's own contract. The dispatch guard keyed on the item's
Two silent hangs. One performance defect with a large number. Three reasoning accumulators appended to a plain string twelve lines below the comment forbidding it, and the newline scan rewalked the whole buffer per delta. 37.4s → 23ms at 31 MiB; 24.5 GB → 13.9 MiB of allocation to fold 1.2 MiB. Both costs landed on the socket-draining goroutine with the watchdog suspended, so a long reasoning section made the client slow and silent at once. One that made this PR's own documentation invisible. A bare blank line in Plus: Deliberately not closedThree findings, two of them CRITICAL, where upstream does the same thing:
These are real gaps and they are recorded rather than fixed, because diverging from upstream on the approval path costs more than the posture buys today. They are the right thing to raise upstream. One fix revertedI changed The procedural lesson, worth stating: check upstream before fixing, not after. Two tests that were green and wrongBoth pinned wire shapes the server cannot produce. Breaking changes, all free at v0.1.2
Direction from hereAlign on the wire, then improve posture. The most valuable remaining work is the two places we still diverge from upstream, both in attribution: our Check upstream first on the rest. Findings 16 (item variants routed to Leave the posture gaps documented, not silently carried. The elicitation replay, the unbounded per-turn state, and the raw control bytes in hook payloads are all real. They are worth an upstream issue rather than a local divergence — which is what we did for the |
A text delta names no response, so the fold infers which one produced it. That inference is unsound while two responses are live — a mirrored sub-agent's start would re-credit the parent's own words to the child — so the fold stops crediting. It stopped permanently, which made one sub-agent silence OnlyAgent for the rest of a subscription: measured at one block kept out of fourteen, across two later turns that each had a single live response. The ambiguity is the overlap, not the stream. Attribution is now recomputed whenever the live set changes, so it resumes when the overlap ends. The same probe now keeps ten of fourteen. Liveness is also recorded before the start-block dedupe rather than after it. A response re-announced once it had been terminal never re-entered the live set, so an overlap the fold could not see became an attribution it got confidently wrong: the parent's delta arrived credited to the sub-agent, at the sub-agent's depth. That case now reports no agent, which is the honest answer. Upstream carries no unattributable state and credits whichever response it saw last. This is deliberately stricter, and it is the one place in this layer where that is true: confidently wrong attribution on a session tree is worse for a caller than a filter that finds nothing. Not fixed, and it needs a server bug to reach: a response announced with an empty id is tracked nowhere and produces no start block. ResponseObject.ID is declared required. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TestAttributionRecoversWhenAnOverlapEnds was added at attribution_test.go, a path that already held TestNoticeNamesEveryFileCarryingUpstreamProse and its three helpers. Writing the file replaced them. Nothing caught it. Deleting a test does not fail a build, no other file referenced those helpers, and all three CI checks stayed green — so the branch lost the gate that keeps NOTICE exhaustive in both directions and reported success. The NOTICE test is restored byte-for-byte from main; the new one moves to block_attribution_test.go, which says what it is about. Both run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#10 is merged, so this now targets
maindirectly and the diff is its own.Shape
The one design decision worth your attention
Turn termination cannot be a stateless predicate. Upstream ends a turn on the
typed terminal event alone:
That is wrong for us. Two harness families put the turn's end in different
places: on a terminal-backed harness a
response.completedmeans only that theprompt reached the harness, and the turn ends on an idle status edge naming a
response. Taking the lifecycle terminal there hands a caller a partial answer it
believes is whole.
So
turn.goholds the three facts no single event carries — whether the serverhas echoed this turn's prompt, which responses predate it, and which signal this
harness ends on. Every rule in it was established in the agentic driver by a
failure that reached a real run, and all nine of its assertions are ported so
PR 5 can delete that copy.
Which harness a session runs stays the caller's knowledge.
TurnOptions.Endtakes the value; this package holds the rules it selects. The zero value is the
stricter rule, because the two mistakes do not cost the same.
Notable, with the reasoning
The prompt is posted from
StreamOptions.OnSubscribed, which this packagealready documents as the supported way to start a turn. Three properties follow: a
stream that fails to open posts nothing, a
Turnnobody reads posts nothing, andthe anchor is recorded before any event can be tested against it.
Blocks credit an agent only while one response is live. A text delta names no
response, so crediting is right with one live and wrong with two — a mirrored
sub-agent's start would re-credit the parent's own words. The fold stops crediting
rather than guessing. Crediting nothing is recoverable; crediting wrongly is not.
A tool call and an approval are answered even when they fail. The server parks
a turn on both. An unregistered tool posts an output naming the mismatch; a
panicking approval hook declines. Declining with no hook is this package's own
behaviour, not a policy — accepting authorises a tool to run with the session
owner's execution identity.
One turn reaches one terminal response. The description enumerates no status
distinguishing a tool-loop pass's terminal from a turn's, and if a turn emitted
several with no way to tell them apart, upstream's own
send()would truncateevery tool-loop turn. A loop's passes happen inside one response.
Review record
Two
/xreviewrounds,Class: shared-stack,Tier: T3, four blinded lenses each,dissenter rotated between rounds. Every lens dissented in both rounds. What
that found, in order of severity:
breakon an error panicked the callerrange function continued iteration after…call_idran a tool twiceErrUnavailable("usually transient") + prompt sentnilerrorOnSubscribed, in a file I had readChat.Senda silent no-op behind any SSE proxy: pingkeepalive → 0 posts, 0 events, deadlineOnlyAgenttruncate rather than missRound 2's systems lens applied 44 mutations; 19 survived, 10 were real gaps.
Four were correctness-grade and are now pinned:
ErrTurnIncompletehad zeroreferences in the suite, no test drove a
function_callwith a status other thanaction_required, the session filter was defeated at its only production callsite, and a tool's failure never reached the caller.
Four of my own tests passed for the wrong reason and were replaced. The worst
asserted the load-bearing ordering invariant and passed 30/30 with the entire
mechanism replaced by
time.Sleep(10ms).Verification
bin/check.shall five legs ·golangci-lint0 issues ·go test -race -count=2· 180 tests · 88.4% coverage · zero module dependencies.
Every guard is mutation-proven, with the mutation asserted as landed before the
run.
Three decisions I did not make alone
plan.md's Go column disagrees with the code in ~14 rows — it recordsToolHandler,ToolCall,ToolCallable,ToolSchema,MergeText,FormatToolArgs,ErrToolCallDenied; this shipsToolRegistry,ToolCallInfo,ToolFunc,MergeTextAcrossIterations,FormatToolArgsBrief,ErrToolNotRegistered. Three more (ToolState,ElicitationMethod,ElicitationRequest) are absent. That table is FR-029's record, so one sidehas to move. Some of my names are better; some of the plan's are. Pre-release,
so cheap either way.
Today a post that fails after the request was written carries no matchable
sentinel, so a caller cannot tell a safe retry from a double turn. A new
ErrPromptOutcomeUnknownis a public-surface one-way door. Money moves throughthis path.
BlockContext.Depthship at all? It counts dots in an agent name.Nothing in the description says a dot means nesting, and a version number puts
one there. Documented as this package's inference; whether it exists is a
pre-release call.
Deferred, with the condition to un-defer
blockState.executionsretains each call's decoded arguments for the turn —107 MiB at 800 calls × 64 KB. Un-defer at the first turn over ~500 tool calls or
any argument payload over ~1 MB.
SendInputhas no idempotency keyserver-side, so this likely needs a server change; raising it now even though the
client work is deferred.
PR guards those.
enforces symmetry against the agent spec before the stream opens; this SDK does
not. Un-defer when one process shares a registry across sessions of differing
trust.