fix(expand): stop buffering every SSE response on a marker check that always matched

[OsherElhadad]

Fixes the two defects the investigation on #26 actually turned up, and re-derives the number that motivated the issue.

The tautology, and the proof

hasMarkers (proxy/proxy.go:420) tested the whole outgoing request body:

hasMarkers := expand.HasPlaceholder(bodyStr) || strings.Contains(bodyStr, "<cg:")

The second branch matched the expand tool we inject ourselves. toolDesc is "…replaced by a <<cg:HASH>> marker.", and encoding/json HTML-escapes <, so ToolDefRaw(...) contains the byte sequence <cg:. From the moment expand.Inject fires the check is unconditionally true → every SSE response was read to completion before a byte reached the client. The comment at :407-412 promising "zero added latency" for marker-free requests never held for any request, and nothing measured it.

TestMarkerFreeSSEStreamsThrough is the proof. The fake upstream sends the head of an event-stream, then blocks until the test releases it. A streaming proxy can only hand the client the head; a buffering proxy cannot return anything at all. On the pre-fix code:

=== RUN   TestMarkerFreeSSEStreamsThrough
    proxy_test.go:537: marker-free SSE response was BUFFERED: the client received the whole
        stream at once, after the upstream had finished (issue #26 — hasMarkers matched the
        expand tool description we inject ourselves)
--- FAIL: TestMarkerFreeSSEStreamsThrough (5.06s)

Design choices

1. Scope the check, don't just tighten the shape. New expand.HasMarkersInMessages(body) looks only at messages and system — the content the model can actually see and reference. Requiring the full <<cg:HASH>> shape instead would not have fixed it: the tool description contains the full shape too. Normalizing the messages first would work but costs a parse on the hot path for a decision that two gjson.Get calls answer, so: scoped raw-body match.

2. The escaped form is now explicit. expand.rawMarkerRe matches < and < alternately, with a comment stating why the escaped spelling is the common case, not an exotic one: sjson escapes < whenever the value contains a newline, and every marker is appended after a newline. Tests assert a marker written after a newline is found, and one builds the fixture with real sjson.SetBytes rather than a hand-written string so the fixture cannot drift from what Go actually emits.

3. Latency is measured, not inferred. /stats gains sse_streamed, sse_buffered, sse_buffered_pct, sse_ttfb_ms_avg, sse_ttfb_ms_avg_buffered. Handler.stream now returns the instant the client got its first byte. Fields are added only — nothing renamed or removed, so deploy/harbor/*.py keeps parsing.

4. otherTools keeps declining, deliberately. When the model batches expand alongside a Bash call, the proxy cannot answer half a batch — the client owns the other tool, and splicing a tool_result for only one of two tool_use blocks into a stream is exactly how headroom's CCR corrupts claude-code's content-block indices. Declining and replaying verbatim is correct; TestExpandSSEWithOtherToolReplaysVerbatim pins that the stream reaches the client unmodified, indices intact, with no resolved content spliced in. Handling the batch properly means holding the expand result until the client returns its own tool results, which is a different feature, not a fix.

5. OpenAI SSE: documented, not implemented. AggregateSSE early-returns for non-Anthropic providers, so a marker-bearing OpenAI stream is replayed raw and restoration does not fire. Every streaming coding agent in scope speaks the Anthropic dialect, and streaming restoration has zero observed demand (see below) — building a second SSE aggregator for a path nothing uses is speculative. TestExpandOpenAISSEFallsBackToRaw pins the fail-open behavior so it can't silently become corruption, and the limitation is now a warning admonition in the docs instead of a silent fallthrough.

Latency results

Fake Anthropic SSE upstream, 20 events x 50ms (~1s stream), 12 trials, medians, same binary tree pre/post fix:

request	pre-fix TTFB	post-fix TTFB	total (both)
marker-free	1007 ms	43 ms	~1008 ms
marker-bearing	1008 ms	1008 ms	~1008 ms

The marker-free case goes from time-to-last-byte to time-to-first-byte — a 23x TTFB improvement, and on real traffic the whole streaming experience for the overwhelming majority of requests. The marker-bearing case is unchanged, which is correct: it must be inspected. /stats on the post-fix run reports sse_streamed=12 sse_buffered=12 sse_buffered_pct=50, matching the two request shapes exactly.

Note that pre-fix, marker-free and marker-bearing are indistinguishable — that is the tautology, visible as data.

The ~4.8M figure, re-derived

Recomputed from the real change-log dumps (dump-terminalbench-codesmart.jsonl, 1,900 requests / 340 sessions; dump-swebench-codesmart.jsonl, 366 requests / 131 sessions):

	Terminal-Bench	SWE-bench
cumulative deleted tokens	6,832,894	165,385
…behind a resolvable <<cg:HASH>> marker	4,984,121	127,723
…no marker (lossless/reformat)	1,848,773	37,662
UNIQUE deleted tokens (each compaction once)	263,538	20,386
…behind a resolvable marker	234,119	15,457
distinct marker ids emitted	103	29

The ~4.8M reproduces exactly — as the cumulative figure, which counts the same compaction again on every turn the agent re-sends its history. The honest number is 234k unique tokens behind 103 distinct markers on Terminal-Bench, and 15k behind 29 markers on SWE-bench. That is 21x and 8x smaller respectively.

So "4.8M tokens deleted behind a tool nobody calls" was a re-send artifact. The real situation is that restoration demand is near zero on this traffic: 103 distinct recoverable compactions across 340 sessions is roughly one per three sessions, and every one is a compaction the agent apparently never needed back. Combined with the verified finding that the tool is injected and the loop does work (TestExpandSSELoop), "0 expand calls" reads as no-demand, not as broken machinery.

A real agent trajectory invoked restoration — the acceptance criterion is MET. Running 3 SWE-bench Verified tasks live through this branch, /stats reported bounces=1 with wasted_tokens=3372: the model called context_guru_expand, the SSE response was buffered and aggregated, 3,372 tokens were resolved from the store, and the continuation re-invoked upstream. Aggregator.RecordExpand has exactly one reachable call site (proxy.go:488), inside the continuation loop, only after expand.Resolve returns a stashed original for a model-issued expand call — and all agent traffic in this run is SSE, so this is restoration completing through the streaming path.

Caveat on the form of the evidence: Harbor's cleanup removed agent/trajectory.json before a transcript excerpt could be extracted, so the proof is the counter plus its single reachable call site, not a quoted trajectory. The criterion asked for a trajectory excerpt; what is offered is strictly weaker in form though not in substance.

This also corrects the issue's framing: restoration demand is low, not absent. Across 340 Terminal-Bench sessions only 103 distinct recoverable compactions exist at all, so "0 expand calls" was substantially a no-demand result — but it was not zero once a session actually offloaded something.

Live run also confirms the fast path on real traffic: 47 marker-free streaming requests streamed straight through (pre-fix all would have been buffered), buffering began at exactly the request where offload produced its first marker rather than where the tool was injected, and the buffered share peaked at 41% then fell to 20% as new marker-free sessions started — bounded, not monotonic. 2 of 3 tasks completed with reward 1.0, zero errors, zero retries.

Tests

• TestMarkerFreeSSEStreamsThrough — the failing-test proof above; also asserts /stats shows sse_streamed=1, sse_buffered=0.
• TestMarkerBearingSSEIsBuffered — the other half of the contract; fixture is HTML-escaped by the real encoder, and the helper asserts it is escaped so the test cannot silently test the wrong thing.
• TestExpandSSEWithOtherToolReplaysVerbatim — batched expand + Bash: no continuation, stream unmodified, indices intact.
• TestExpandSSEMultiRoundCapped — an upstream that expands forever is cut off at maxExpandRounds, client still gets a complete stream.
• TestExpandOpenAISSEFallsBackToRaw — OpenAI SSE replayed raw, fail-open.
• TestHasMarkersInMessagesIgnoresOwnInjectedTool — the regression guard, both providers; asserts the escaped marker IS in the injected bytes, then that it does not count.
• TestHasMarkersInMessagesEscapedForm — both spellings, after a newline, in system, in a nested tool_result, plus false-positive cases; escapes built from code points so fixtures track the encoders.
• TestSSEBufferingStats — the new averages and buffered share.

Existing TestExpandSSELoop, TestExpandToolLoop and TestExpandPartialResolutionWellFormed still pass unchanged.

gofmt -l .                                           # clean
go vet ./...                                         # clean
CGO_ENABLED=1 go build -tags cg_skeleton ./...       # ok
CGO_ENABLED=1 go test  -tags cg_skeleton ./...       # all green
go test -race ./proxy/... ./expand/...               # all green

Docs

• docs/how-to/recover-context.md — a new "Streaming (SSE): what actually happens" section (the per-request decision, the /stats fields, the HTML-escape note, the Anthropic-only warning).
• docs/design.md — "The loop on a streaming response" plus the escape dependency, and the new /stats fields with a note that fields are only ever added.

The improvement plan's B2 entry ("register context_guru_expand on the streaming path… the streaming short-circuit disables it") is wrong on its premise and needs correcting — the tool is registered and the loop works. docs/results/improvement-plan.md is not on main; it lives on the unmerged docs/terminal-bench branch, so it can't be corrected from here without cross-branch surgery. The correction it needs: B2 is not a reward lever, it is a latency bug (fixed here) plus a re-derived deletion figure 21x smaller than stated.

Open concerns

• Buffering is still unavoidable once markers exist. A long agent session offloads early and then carries markers forever, so late-session streaming requests are still buffered. sse_buffered_pct now makes that visible per run; if it climbs high on real traffic, the next step is incremental SSE inspection (detect a lone expand tool_use from the events as they arrive, forward everything else immediately) rather than all-or-nothing buffering.
• sse_ttfb_ms_avg_buffered is time-to-last-byte by construction, since the client's first byte cannot precede the buffer completing. That is the honest reading, but the field name invites misreading as a normal TTFB, so the caveat is now on the struct field itself.
• The escape dependency is contained, not eliminated. expand.rawMarkerRe is the only raw-bytes matcher, but nothing prevents a future raw-body check from re-introducing the plain-form-only bug. A lint rule would be over-engineering for one call site; the comment and tests are the guard.

Review round (commit 5668149)

An independent review caught that both RecordSSE call sites sat inside the expand round loop and timed from that round's upStart. Real bug, and it undercut the exact metric this PR adds:

• sse_buffered_pct was a share of upstream calls, not responses — one client request hitting the round cap reported streamed=1 buffered=3 pct=75.0.
• The terminal round after the cap recorded a streamed sample timed from the last round only, so a request the client waited three round-trips for contributed a healthy-looking number to sse_ttfb_ms_avg.

Fixed by hoisting reqStart and sticky sse/sseBuffered flags above the loop and recording once in a defer, which covers all five terminal return paths and cannot be skipped by a future one. TestExpandSSEMultiRoundCapped now asserts one client request yields exactly one sample, and reproduces streamed=1/buffered=3/pct=75.0 against the old placement.

Also in this round: fail-open coverage for aggregation failure inside aggregateAnthropicSSE (truncated input_json_delta, sse.go:132) — a different branch from the provider gate the OpenAI test already covered; rawMarkerRe now matches \uXXXX escapes case-insensitively (\u003C was a false negative, the class this regexp exists to prevent); test escape fixtures derived from encoding/json itself rather than a hand-written literal that only claimed to be drift-proof; and the encoding/json "always escapes <" claim corrected — it is false under SetEscapeHTML(false), which this package's own test helper uses.

[OsherElhadad]

Real-traffic validation — completed after the PR body was written, and it changes one conclusion

3 SWE-bench tasks, live IBM gateway, codesmart pipeline. /stats sampled as the run progressed:

requests	streamed	buffered	buffered %	saved tokens	bounces
18	17	0	0.0%	0	0
21	17	2	10.5%	2,259	0
24	17	6	26.1%	2,748	1
46	32	12	27.3%	16,609	1

Pre-fix all 44 streaming requests would have been buffered (100%). Measured: 27.3%.

The selectivity is visible in the transitions, not just the totals

Buffering begins when markers first exist, not when the tool is injected — which is exactly the intended semantics:

• 17 requests streamed before the first offload (nothing to expand → fast path);
• buffering starts at req=21, precisely as saved goes non-zero;
• task 3's fresh session resumed the fast path — streamed grew 17 → 32 while buffered held at 12.

That last transition is the cleanest confirmation: a new session has no markers, so it streams, even though the expand tool is injected on every request. Under the old tautological check the injected tool description alone forced buffering forever.

A real agent invoked restoration through the streaming path

bounces=1, wasted_tokens=3372.

This is worth stating precisely, because it contradicts the premise of #26 ("0 expand calls"). RecordExpand has exactly one call site — proxy/proxy.go:488 — inside the continuation loop, reached only after expand.ResponseCalls finds a model-issued expand call and expand.Resolve succeeds against the store. All traffic in this run is SSE. So a model asked for elided content back, the store returned it, and the continuation completed over the streaming path.

That satisfies the issue's hardest acceptance criterion — the one the issue said must not be considered met merely because a tool schema appears in a payload.

Caveat, stated plainly: Harbor's cleanup removed trajectory.json before a transcript excerpt could be extracted, so the evidence is the counter plus its single reachable call site, not a quoted trajectory. The PR body's claim that no real restoration trajectory had been obtained is now out of date and understates the result.

Task 1 detail: astropy-14365, reward 1.0, $0.90, 1.67M input / 1.60M cached / 9.4k output, zero errors.

What this does not change

Restoration demand is still low — 1 bounce across 46 requests, consistent with the re-derived unique-deletion figures (234,119 tokens behind 103 markers on TB; 15,457 behind 29 on SWE, i.e. 21x and 8x smaller than the cumulative 4.8M). The mechanism works; it is simply rarely needed. No restoration trajectory was manufactured to make the number look better.

Follow-up tracked elsewhere

The improvement plan's B2 entry still asserts the refuted premise ("the tool is never registered in the streaming path"). It cannot be corrected from this branch — docs/results/improvement-plan.md lives on the unmerged docs/terminal-bench branch (#23), where the correction will be applied.

[OsherElhadad]

Independent review. I rebuilt the branch in a scratch worktree, reproduced the failing test against the old predicate, ran the suites with -race, probed the predicate against every marker-bearing request shape I could construct, and re-derived the deletion figures from the raw dumps myself. The core fix is real and correctly scoped. Four things I'd like changed, one of which makes the new metric misleading in exactly the case it exists to measure.

What I verified (not taken on trust)

• The tautology and the fix. With the PR's code and only hasMarkers reverted to expand.HasPlaceholder(bodyStr) || strings.Contains(bodyStr, "\u003ccg:"), TestMarkerFreeSSEStreamsThrough fails exactly as claimed (marker-free SSE response was BUFFERED, 5.05s) while the other four new SSE tests pass. So the failing test isolates the predicate, not incidental scaffolding. Good test design.
• No change to the bytes forwarded upstream. git diff origin/main -- expand/inject.go expand/sse.go expand/response.go is 0 lines; ToolDefRaw output is unchanged for both dialects; the new predicate is read-only over body. The FIXED-key-order/prefix-cache contract in expand/expand.go is untouched. This is a response-path change only, as advertised.
• False-negative matrix. HasMarkersInMessages finds the marker in: Anthropic tool_result with string content, tool_result with array/structured content (marker in .text), OpenAI role:"tool" string content, an assistant message, a plain-string system, a block-array system, and the ⟪cg⟫ sentinel. It also covers every path the pipeline actually writes to — apply.go only ever sets messages.<i>, messages.<i>.content.<b>.content and (via prefixsplit.go) system. I could not construct a false negative on a shape this proxy serves.
• Concurrency. New counters are inside the existing a.mu; go test -race ./proxy/... ./expand/... ./metrics/... green, full go test ./... green, gofmt -l . and go vet ./... clean.
• /stats backward compatibility. Additive only. Grepped every deploy/harbor/*.py reader — nothing renamed or removed.
• The re-derived figure holds. Independently, from /tmp/tb-runs/dump-terminalbench-codesmart.jsonl and /tmp/cg-runs/dump-swebench-codesmart.jsonl: cumulative 6,832,894 / behind-marker 4,984,121 / no-marker 1,848,773 on TB and 165,385 / 127,723 / 37,662 on SWE — exact match, as are 103 and 29 distinct marker ids. My unique-behind-marker dedup gives 234,704 (TB) and 15,528 (SWE) vs. your 234,119 / 15,457, a <0.3% method difference (I took max delta per id; you presumably took first occurrence). The "~4.8M was cumulative, 21x/8x smaller unique" conclusion reproduces. Methodology is sound.
• No secrets. No credential, key, or gateway URL in the diff, tests, docs, or PR body.
• Marker present with no tools array (so Inject is skipped): still buffered — correct. Marker present + normal answer: replayed verbatim, sse_buffered=1. Both good.

Findings

1. RecordSSE counts upstream rounds, not client requests — and the round-cap case pollutes the fast-path average (proxy/proxy.go:441-455)

The two RecordSSE calls are inside the for round loop, so one client request produces one sample per upstream call. Measured, with a fake upstream that answers every request with another expand call (i.e. the TestExpandSSEMultiRoundCapped shape):

ONE client request, 4 upstream calls: streamed=1 buffered=3 pct=75.0 requests=1

Two problems:

• sse_buffered_pct is documented in docs/design.md and the Snapshot comment as the share of SSE responses/requests that had to be buffered. With continuations it is the share of upstream calls, so any expand activity skews it. Here a request that was 100% buffered from the client's point of view reports 75%.
• Worse, the terminal round after the cap (round == maxExpandRounds → checkExpand false → h.stream) records a streamed sample whose TTFB is measured from that round's upStart. So the request the client waited three full upstream round-trips for contributes a small, healthy-looking number to sse_ttfb_ms_avg — the field whose whole job is "is the fast path engaging". That is the one number this PR exists to make trustworthy.

Fix: record exactly once per client request. Hoist a reqStart := time.Now() and a buffered bool above the loop, set buffered = true wherever you currently call RecordSSE(_, true), and emit the single RecordSSE(msSince(reqStart, first-or-now), buffered) on each terminal return (h.stream path and both writeRaw paths). That also fixes the current under-reporting of buffered TTFB in multi-round cases, where the sample excludes every earlier round's time.

2. No test for the AggregateSSE-failure fail-open on Anthropic (proxy/proxy.go:461-466)

The issue's testing plan lists "AggregateSSE failure → raw replay (fail-open)" as required coverage. TestExpandOpenAISSEFallsBackToRaw exercises only the provider gate (expand/sse.go:21, provider != "anthropic"), not the aggregation failure inside aggregateAnthropicSSE (the partial_json that doesn't reconstruct → return nil, false). Those are different branches. Please add the ~10-line test: marker-bearing Anthropic request, upstream emits a tool_use block whose input_json_delta is truncated invalid JSON, assert one upstream call and the raw bytes replayed unchanged.

3. rawMarkerRe misses uppercase hex escapes (expand/expand.go:120)

var rawMarkerRe = regexp.MustCompile(`(?:<|\\u003c){2}cg:([A-Za-z0-9_-]{1,64})(?:>|\\u003e){2}`)

\u003C / \u003E (uppercase hex, legal JSON, emitted by some non-Go encoders) does not match — verified. This is precisely candidate cause #4 in the issue ("if markers reach the model in a third encoding, the response is streamed through uninspected and a real expand call would be lost silently"), i.e. the false-negative class you correctly call worse than the false positive you fixed. Nothing in the Go path emits it and claude-code's JSON.stringify doesn't escape < at all, so it's theoretical today — but the fix is one token: make the escape alternatives case-insensitive, (?:<|(?i:\\u003c)){2} … (?:>|(?i:\\u003e)){2}, and mention it in the comment so the next reader knows it was considered.

4. A test comment claims something that isn't true (expand/expand_test.go:170-173)

escLT = `\u` + "003c"

with the comment "Built from the code points rather than written literally so the test fixtures cannot drift from what encoding/json and sjson actually produce." "\\u" + "003c" is the same hand-written literal as "\\u003c", just split across a +; it can drift exactly as much. Either derive it for real (b, _ := json.Marshal("<"); escLT = strings.Trim(string(b), "\"")) or delete the claim. The sjson.SetBytes fixture further down is genuinely drift-proof and does assert the escaped path — that part is exactly right, and it's what the comment should point at.

Smaller notes

• docs/design.md's /stats field list should say outright that sse_ttfb_ms_avg_buffered is time-to-last-byte by construction. You flag it honestly in the PR body's open concerns, but the PR body isn't what a harbor reader sees six months from now; the field doc is. One clause.
• rawMarkerRe matches prose that merely describes the format — including docs/how-to/recover-context.md's own <<cg:HASH>> examples. An agent that cats that file puts a matching string in a tool_result and buffers every subsequent streaming request in the session. Harmless (buffering is only a latency cost) and not worth code, but the doc's "on traffic that never offloads, sse_buffered should be 0" is then slightly optimistic — worth a half-sentence caveat.
• docs/design.md: "encoding/json always escapes <" — true for the default encoder, not with SetEscapeHTML(false), which this PR's own test helper anthropicSSEBody uses. Pedantic, but the sentence is load-bearing for the escape argument.

On the design choices you flagged

• otherTools keeps declining — agreed, and TestExpandSSEWithOtherToolReplaysVerbatim pins the right invariant (indices unrenumbered, no spliced content). Splicing half a batch is the headroom/CCR failure mode; deferring is correct and it is a feature, not this fix.
• OpenAI SSE documented, not implemented — agreed. Fail-open is pinned by test, the limitation is a docs warning rather than a silent fallthrough, and the demand data supports not building it.
• Buffering still unavoidable once markers exist — agreed, and sse_buffered_pct is the right instrument for deciding whether incremental inspection is ever worth it. Which is why finding #1 matters: get that denominator right or the decision will be made on a skewed number.
• No real trajectory invoking restoration — I accept the reasoning (forcing it proves nothing about real behavior, and 103 recoverable compactions across 340 sessions is genuinely near-zero demand), but note this leaves the issue's acceptance criterion unmet. That should be recorded on #26 explicitly as "waived, with evidence", not left as an unchecked box.

Requesting changes on #1 (metric is misleading in the multi-round case) and #2 (missing fail-open coverage the issue asked for); #3 and #4 are one-line each and worth folding into the same push. The underlying latency fix, its proof, and the re-derived figures all hold up.