Requesting changes (posting as a comment — GitHub won't let the author submit a formal Request-Changes review on their own PR). Blocking item: the streaming perInvocation leak below.

Reviewed for correctness and traced the invocation-complete paths + concurrency. Strong PR — fail-open everywhere, careful locking (accumulate under lock → snapshot → emit audit outside lock), the expand-tool-output-never-recompressed tail-chase fix, and the "wire format byte-identical when off" prompt-cache path is test-asserted. Builds clean; compress + llm/providers tests pass. One real bug on the primary path.

🔴 perInvocation leaks (and compression metrics are missing) on the streaming paths

TakeInvocationTotals — which both pops perInvocation[correlationID] and adds the compression_* fields — is called in exactly one place: executeTask (runner.go:1552). But there are three invocation_complete emission sites, and the other two are the sendSubscribe streaming handlers that run their own ExecuteStream loop instead of executeTask:

• tasks/sendSubscribe (JSON-RPC SSE) → emits at 1296, no Take.
• REST POST /tasks/sendSubscribe → emits at 1821, no Take.

Both streaming handlers set a correlation ID on ctx (WithCorrelationID) and use the shared compressed client + hooks, so a streaming invocation that compresses does call recordCompression → bumpInvocation, adding a perInvocation[cid] entry. Since only executeTask pops, and perInvocation has no TTL/sweep, every streaming invocation that compresses leaks its bucket permanently. sendSubscribe is a primary A2A mode, so with compression on this grows unbounded over process lifetime.

Same root cause, second gap: the PR advertises invocation_complete gaining compression_saved_tokens_total / compression_count / expansion_count, but those are only added in executeTask — so streaming invocation_complete events don't carry them. The feature is under-delivered on the common path.

Fix (one change closes both): factor the "TakeInvocationTotals(ctx) + populate the three compression fields" block out of executeTask into a helper, and call it at all three EmitInvocationComplete sites (1296, 1565, 1821). Optionally also give perInvocation a size cap / periodic sweep so a missed Take can't leak — but the shared helper is the real fix and also ships the metrics.

🟡 Secondary

• recent marker map is unbounded — every emitted marker hash is remembered (rememberMarkers) and never evicted. Small strings, but grows for the process lifetime; a cap / LRU would bound it (you only need recent markers for imperfect-hash resolution).
• bbolt is a single-writer file lock. .forge/ctxzip.db can be opened by only one process; two agents on a shared volume (K8s multi-replica) or forge run + forge serve on the same dir will collide. Fail-open only helps if ccr.NewBoltStore errors rather than blocks on the flock — worth confirming it sets an Open timeout, and documenting the single-writer constraint in the compression doc ("one store per process/replica").
• Inflation guard is == 0, not <= 0 (hook.go: res.SavedTokens() == 0). If ctxzip can ever return negative savings (inflated output), that guard misses it and you'd record negative savings + apply the inflated bytes. Depends on ctxzip's contract; low risk, but <= 0 is safer.

Note (in the PR's favor)

go.mod is correct — ctxzip is a direct require and bbolt is correctly // indirect (transitive via ctxzip; compress imports ctxzip/ccr, not bbolt). No go mod tidy needed.

Verdict

Request changes for the streaming perInvocation leak + missing metrics — a genuine unbounded leak on the main path and a gap between advertised and actual streaming behavior. The shared-helper refactor is small and fixes both. The bbolt single-writer note and recent cap are worth doing but non-blocking.

Resolved in deb06fd — all four findings addressed:

🔴 Streaming perInvocation leak + missing metrics — fixed with the suggested shared helper. The pop+populate block is now appendCompressionFields(ctx, fields), called at all three EmitInvocationComplete sites (executeTask, JSON-RPC SSE sendSubscribe, REST sendSubscribe). The helper's doc comment names all three sites explicitly so a future fourth emission path knows the contract. Pinned by TestAppendCompressionFields_PopsAndPopulates: fields populated on first call, one-shot pop (second call returns zeros — no double-count if two sites ever fire), nil-safe with compression disabled.

🟡 Leak backstops (took the "optionally also" suggestion): perInvocation is now bounded to 1024 buckets with oldest-touched eviction — a future missed pop degrades to bounded memory instead of an unbounded leak. recent markers bounded to 2048 with oldest-emitted eviction (only recent markers matter for transcription repair; exact hashes still resolve via the store). Both pinned by tests (TestPerInvocationBuckets_Bounded, TestRecentMarkers_Bounded).

🟡 Inflation guards: both seams tightened from == 0 to <= 0. Confirmed ctxzip's SavedTokens() clamps at zero today, so this is defense-in-depth against a future contract change — noted as such in the code comment.

🟡 bbolt single-writer: confirmed ccr.NewBoltStore sets bolt.Options{Timeout: 5 * time.Second} — a second process errors after 5s rather than blocking, so fail-open engages (warning logged, agent runs uncompressed). Documented in context-compression.md § Failure posture: one store per process/replica, give each replica its own store_path.

Gate: build ✅, vet ✅, gofmt ✅, forge-core 32 packages ✅, forge-cli/runtime full suite ✅.

And thanks for the go.mod note — glad the direct/indirect split checked out.

🤖 Generated with Claude Code

Update: live-hardening round 2 — envelope compression, honest metrics, compress-then-truncate

Since the review fixes (deb06fd), five more commits landed, each driven by a live k8s-triage run against a real cluster (cli_execute + kubectl, gpt-5.4). Summary of what changed and why.

🔍 Live finding 1: 28–37KB kubectl outputs compressed 0%

cli_execute wraps output in a single-line JSON envelope — {"stdout":"...\n...","stderr":"",...} — whose escaped newlines defeat every content detector (zero physical newlines: not an array, not line-oriented, not prose). Fix (ctxzip v0.1.1): JSON-envelope recursion — large string fields are decoded (real newlines restored), compressed through the normal routing path, and spliced back; output stays valid JSON, untouched fields byte-identical, key order preserved (deterministic). Plus identifier-collapsing line signatures (kubectl rows barely deduped — every pod name is a unique token) and warn added to the error floor (kubectl TYPE=Warning events matched nothing).

🔍 Live finding 2: compression_saved_tokens_total: 1,257 on an invocation that actually avoided ~31K billed tokens

The field counted compression events; savings are realized per resend — compressed history saves its delta on every subsequent LLM call, attributed to nothing. Fix (2e38661→8d2f629): each marker remembers its saved-token delta; the client wrapper credits, on every outbound call, all markers riding in the request. invocation_complete now reports compression_saved_tokens_total as realized wire savings (matches the provider bill) with compression_event_saved_tokens kept for reconciliation against context_compressed events. TTL-immune (no store reads), per-correlation isolation unchanged, compounding pinned by test.

🔍 Live finding 3: a 108KB kubectl get pods -A -o json was destroyed before compression saw it

The loop's pre-hook truncation cut the envelope mid-JSON-string — destroying data and breaking the JSON so compression bailed; the mangled 12K-token blob rode through four more calls. Fix (ed1dd4c): with compression enabled, the loop runs execute → safety ceiling (16× cap, abs 4MB) → hooks (guardrails → compression) → normal cap — the hook sees full output, envelopes stay intact, guardrails now scan the full payload (strictly more correct), and the normal cap applies to the usually-tiny compressed result. Gated by LLMExecutorConfig.DeferToolResultTruncation; compression-off deployments keep today's order byte-identical (existing truncation test untouched). ctxzip v0.1.2 additionally salvages envelopes that still arrive cut mid-string: intact prefix compressed, valid JSON re-emitted, _ctxzip_note telling the model the tail was destroyed upstream, not offloaded.

🐛 Also fixed

TUI compression step hung on selection — never emitted StepCompleteMsg (the wizard's sole advancement path) and didn't reset on back-navigation; both fixed per the ChannelStep/SkillsStep house patterns, pinned by three step tests (eec813c).

📊 Validation — same agent, same cluster, same questions across the series

Invocation 1: −63% billed input vs baseline, answer quality unchanged (both CrashLoopBackOff pods with restart counts), zero expansions needed. Metrics reconcile: inv 1 wire == event (each marker rode one call); inv 2 wire ≈ 9× event (recovered compressed history × 4 calls — the compounding finding 2 was missing). *Inv 2 includes a 40s cluster API Bad-Gateway retry loop (infra flake; error output correctly kept verbatim).

Session-level: ~38% saved, compression_count/expansion_count honest at both extremes (surgical runs correctly report 0).

Dependency

ctxzip pinned at v0.1.2 (was v0.1.0 at review time; v0.1.1 and v0.1.2 are the findings above).

Docs updated throughout (context-compression.md gained truncation-interaction and single-writer sections; audit-logging reflects the field semantics; /sync-docs mapping honored).

🤖 Generated with Claude Code

Reviewed the e2e-fix commits (ed1dd4c compress-then-truncate, 8d2f629 realized wire savings, eec813c TUI hang, cfe7bf9 ctxzip v0.1.1) — correct, well-gated, and tested; no regression to the perInvocation fix (builds clean, defer-truncation test + compress -race + cli compression tests green). Verified the realized-savings change is a dual metric (SavedTokens one-time vs WireSavedTokens per-resend), not a double-credit.

One minor, non-blocking edge case to flag for follow-up:

Post-hook truncation can cut a compression marker mid-string. In deferred mode the size cap is applied to the compressed result by byte offset (loop.go):

if e.deferToolTruncation && len(result) > e.maxToolResultChars {
    result = result[:e.maxToolResultChars] + "\n\n[OUTPUT TRUNCATED ...]"
}

This only fires when the compressed result still exceeds the cap (rare — incompressible or all-keep_patterns content), but when it does the cut can land inside a <<ctxzip:HASH ...>> marker, leaving the model a broken marker it can't expand. The offloaded content is still in the store, just unreachable via that truncated marker.

Suggested fix: truncate at the last complete marker boundary before the cap (or drop a trailing partial marker) so post-hook truncation never corrupts a marker. Low severity given how rarely it triggers, but worth closing.

Resolved in 8e0fec6 — the post-hook cut is now marker-aware.

truncateToolResult backs the cut up to the marker's start whenever the byte offset would land inside a <<ctxzip:HASH ...>> marker, so truncated output carries whole markers or none — never a corrupted pointer to content that's still in the store. Applied at both deferred-mode cut sites (the pre-hook safety ceiling too, for the edge where raw tool output echoes marker-like text); the legacy non-deferred path is untouched.

One wrinkle worth noting: the runtime can't import the compress package (compress imports runtime — cycle), so the marker prefix is pinned as runtime.CompressionMarkerPrefix with a guard test in the compress package asserting it equals ccr.MarkerPrefix — the literals can't silently drift.

Pinned by five unit cases (straddling marker dropped whole, complete marker kept, plain cut, unterminated marker, under-limit no-op) plus a loop-level test where a compression hook's output places a marker straddling the cap and the LLM must never see a partial marker. Lint + runtime/compress suites green locally.

🤖 Generated with Claude Code