Repository navigation
fix(streaming): rehydrate every ordered channel, not just the first choice's content - #32
Conversation
…hoice's content The SSE path gated all rehydration on a string `delta.content` and read `choices[0]` without consulting `index`, against one retention window shared by the whole stream. Three failures fell out of that, all reproduced before the fix and all covered by new tests that fail if it is reverted. A streamed tool call was never rehydrated. Its event carries no string `content`, so the whole event was forwarded verbatim and the application invoked its tool with `[EMAIL_1]` as the argument value. Not a leak -- it fails toward the placeholder -- but it is the reversibility guarantee failing in the channel agentic traffic actually uses. Non-streaming tool calls were already covered by `_rehydrate_json_response`; only the stream was not. Parallel choices corrupted each other. With `n` > 1 the choices interleave on one wire and are told apart only by `index`, so one shared window released the characters held back for one choice into whichever event arrived next. Measured: choice 0 `A:[EMA`, choice 1 `B:hello`, choice 0 `IL_1]` emitted `A:`, `[EMAB:hello`, `IL_1]` -- choice 0's token destroyed rather than restored, and a fragment of a vault token handed to an answer that never produced it. A provider batching `n` choices into one event had every choice but the first forwarded unrehydrated, because `choices` is a list and only `[0]` was walked. Windows are now keyed `(choice index, tool-call index or None)`, bounded by MAX_STREAM_WINDOWS and fail-closed past it (invariant 1) rather than allocating on whatever indices an upstream invents. Reusing another channel's window under a cap would reintroduce exactly the splicing the keying prevents. Tool arguments flush differently from content, and that asymmetry is the point. A client reads content as it arrives, so a tail delivered after `finish_reason` merely lands late. It accumulates `arguments` and parses them as JSON the moment the choice reports finished, so a tail delivered after that is parsed against a truncated document. Held tool text therefore rides inside the finishing event, synthesised as an index-only continuation entry where that event names no tool call; the end-of-stream drain stays as the net for streams that stop without a `finish_reason` at all. `arguments` joins the buffer-owned key set so the sibling redactor skips it. The buffer already redacts model-originated text before rehydrating, in that order; re-scanning after would redact the caller's own restored values back out. The non-streaming walk passes no `skip_keys` and still scans the field. Anthropic keeps one window deliberately: its `index` counts content blocks, which are sequential within a message rather than parallel answers, so keying by block would strand each block's held tail until the stream ended. Declared, not hidden: `arguments` is JSON text, so a restored value containing a quote, backslash or newline can leave the document unparseable. The non-streaming path at `_rehydrate_json_response` has the same property, and escaping blindly is unsafe because neither site knows whether it sits inside a string. Anthropic's streamed `input_json_delta` tool input is still not restored; it needs its own translation and is not in this change.
|
…ring on a finish Two findings from review on the previous commit, both real. Restored tool arguments could stop being JSON. `arguments` is JSON *text*, and the client joins the fragments and parses the result, so a restored value carrying a quote, a backslash or a control character turned one wrong field into a parse error on the whole tool call. The previous commit declared this as a limit; it is cheaper to fix than to document. Tool-call windows now restore against `_JsonStringVaultView`, whose values are the JSON string body of the original -- what `json.dumps` produces without its quotes. Only values are rewritten, so token matching, `max_token_length` retention and the model-originated scan are untouched, and a value needing no escaping passes through byte-identical, which is nearly always. One view per stream, rebuilt only when the vault gains tokens, so it stays off the per-delta path (invariant 2). The window cap could cut off a stream that was finishing normally. On an event that carries only `finish_reason`, `sibling_buffer` is None, and borrowing a window for the sibling scan opened a content channel that had no content behind it -- at exactly MAX_STREAM_WINDOWS that hit the fail-closed path and aborted the stream mid-completion. `_flush_tool_windows_into` now returns a window it drained and the scan borrows that instead; the scan is vault-scoped, not window-scoped, so any open window answers for the event. The non-streaming `_rehydrate_json_response` still has the unescaped-value property. It is the same class of bug in a path this change does not touch, and it wants its own fix rather than a drive-by. Two tests, both verified to fail against the previous commit: a value carrying a quote, backslash, newline and tab that must still parse and round-trip exactly, and a choice finishing at exactly the cap that must complete rather than abort.
|
Both findings fixed in Restored arguments break JSON — fixed rather than declared. Tool-call windows now restore against a vault view whose values are the JSON string body of the original ( Window cap rejects 256 — correct, and it was worse than an unused allocation: at exactly the cap it aborted a stream that was completing normally. Not fixed here: |
Two reversible-redaction gaps closed, both in the response direction where a redaction that cannot be undone is the failure. The LiteLLM guardrail adapter restored placeholders only in message content, so a model-generated tool call reached the application still carrying them (#31). The Shield's own SSE pipeline had the same blind spot plus two of its own: parallel choices shared one retention window and spliced held-back text into each other's answers, and a provider batching choices into one event had all but the first forwarded unrehydrated (#32). Versions moved in the three files bumpversion manages; the benchmark distribution versions independently and is untouched.
Two reversible-redaction gaps closed, both in the response direction where a redaction that cannot be undone is the failure. The LiteLLM guardrail adapter restored placeholders only in message content, so a model-generated tool call reached the application still carrying them (ninadphalak#31). The Shield's own SSE pipeline had the same blind spot plus two of its own: parallel choices shared one retention window and spliced held-back text into each other's answers, and a provider batching choices into one event had all but the first forwarded unrehydrated (ninadphalak#32). Versions moved in the three files bumpversion manages; the benchmark distribution versions independently and is untouched.
What this fixes
The proxy's own SSE path gated all rehydration on a string
delta.contentand readchoices[0]without consultingindex, against one retention window shared by the whole stream. Three failures fall out of that. All three were reproduced before the fix; all three have tests that fail if it is reverted.This is the Shield's own pipeline, not the LiteLLM adapter (#31), and not the JSON-RPC path —
api/main.pyroutesjsonrpc: "2.0"to the stateless v3 engine, whoseStatelessStreamingLexerregex-scans the raw stream and is field-agnostic, so v3 never had this gap.1. Streamed tool-call arguments were never rehydrated
A tool-call event carries no string
content, so the whole event was forwarded verbatim. The application invoked its tool with[EMAIL_1]as the argument value.Not a leak — it fails toward the placeholder, nothing sensitive escapes — but it is the reversibility guarantee failing in the channel agentic traffic actually uses. Non-streaming tool calls were already covered by
_rehydrate_json_response; only the stream was not.2. Parallel choices corrupted each other
With
n> 1 the choices interleave on one wire and are told apart only byindex. One shared window released the characters held back for one choice into whichever event arrived next:Choice 0 reassembles to
A:IL_1]— the token destroyed rather than restored. Choice 1 carries a fragment of a vault token it never produced. Both answers wrong, no error raised.3. Batched choices were skipped
choicesis a list and only[0]was walked, so a provider that batchesnchoices into one event had every choice but the first forwarded unrehydrated.How
Windows are keyed
(choice index, tool-call index or None)— content and each tool call are separate ordered token streams. Bounded byMAX_STREAM_WINDOWSand fail-closed past it (invariant 1) rather than allocating on whatever indices an upstream invents; reusing another channel's window to stay under a cap would reintroduce exactly the splicing the keying prevents.Tool arguments flush differently from content, and that asymmetry is the point. A client reads content as it arrives, so a tail delivered after
finish_reasonmerely lands late. It accumulatesargumentsand parses them as JSON the moment the choice reports finished — so a tail delivered after that is parsed against a truncated document. Held tool text therefore rides inside the finishing event, synthesised as an index-only continuation entry where that event names no tool call. The end-of-stream drain remains the net for streams that stop without afinish_reasonat all.argumentsjoins the buffer-owned key set so the sibling redactor skips it: the buffer already redacts model-originated text before rehydrating, and re-scanning after would redact the caller's own restored values back out. The non-streaming walk passes noskip_keysand still scans the field.Anthropic keeps one window deliberately — its
indexcounts content blocks, sequential within a message rather than parallel answers, so keying by block would strand each block's held tail until the stream ended.Declared limits, not hidden
argumentsis JSON text, so a restored value containing a quote, backslash or newline can leave the document unparseable._rehydrate_json_responsehas the same property today, and escaping blindly is unsafe because neither site knows whether it sits inside a string.input_json_deltatool input is still not restored. It needs its own translation and is deliberately out of this change.Testing
7 new tests in
tests/test_streaming.py. All 7 verified to fail against the unpatched file — they are evidence, not decoration.Full suite in a clean worktree off
main: 928 passed, versus 921 on the same tree without the patch. The two conformance failures in that run (test_every_declared_capability_has_a_proof,test_the_two_ordered_streams_do_not_splice_into_each_other) are pre-existing on cleanmain— identical without this patch, and both pass in isolation, so they are order-dependent flakes unrelated to this change.ruff checkclean.ruff format --checkwas already failing on this file before this change and CI does not run it.