perf: context-path efficiency — slim delta contract, redundant-hop removal, shared provider scaffold, lookup caches (MOT-4014)#525
Merged
Conversation
assemble recounted the entire working list from scratch (re-serializing every message, tool, and the prompt) after every pipeline stage — four full passes per call — and selection::split_turn recomputed a suffix sum with per-element re-serialization, O(k^2) per oversized turn. - assemble: estimate each message/tool/prompt once; stage recounts are now O(1)-per-message sums over a size memo kept in lockstep by the mutating passes (compaction split_off mirrors the memo; the prompt is re-measured after its re-render). A debug_assert pins the memo to a from-scratch recount so every debug test run is an equivalence oracle. - prune/emergency_reduce: _with_sizes variants re-estimate rewritten messages into the memo (plain passes unchanged for context::prune). - selection: select/split_turn take the size memo and use one prefix-sum array; per-candidate range costs drop to a subtraction. - compact: one size memo per call feeds select/tokens_before/tokens_after. Totals are byte-identical (same fold order, same saturating ops): golden, schema, and all 84 BDD scenarios pass unmodified.
…ing check Two redundant full-payload round trips per turn step: - The final context::count-tokens call recomputed exactly what context::assemble had just returned whenever nothing mutated the request afterwards (the common case: no pre_generate appends, no orphan patches, prompt unchanged). Plumb assemble's token_count through Assembled and skip the re-count unless the request actually diverged; assemble's count is overhead-inclusive, so no double-add. A context-manager test pins the two functions' arithmetic equal so the substitution stays valid. - has_user_after_watermark reloaded and deserialized the ENTIRE active path to answer "any user entry after the watermark?". session-manager gains an additive after_entry_id param on session::messages (strict- after, same session/invalid_cursor error as a stale cursor when the entry left the active path), and the harness fetches just the post-watermark suffix, falling back to the full scan on invalidation. The steering delta fetch keeps include_custom=true and no roles filter — the watermark can be a bookkeeping entry, and a roles filter would drop it off the filtered path (pinned in messages.feature).
…UTF-8 chunk splits
Every provider crate carried verbatim copies of the same plumbing —
router_client.rs was byte-identical across four crates, state.rs differed
by one constant, and the stream pump literally carried a 'shared
extraction into llm-router is a listed follow-up' comment in four crates.
The copies had already drifted: only provider-anthropic buffered UTF-8
across network chunks; the OpenAI-style providers decoded each chunk with
String::from_utf8_lossy, corrupting any multibyte codepoint split across
two chunks into U+FFFD.
New llm_router::provider_scaffold (providers already path-dep the lib):
- router_client: provider-protocol wrappers, parameterized by provider id
(openai-codex keeps its vault/oauth extras locally, sharing 'call')
- state: registration-token persistence, parameterized by scope
- pump: send_event + heartbeat pump (tests moved with it)
- sse_transport: error_chain, append_utf8_chunk (cross-chunk UTF-8
buffering with regression tests), normalize_crlf, and a generic
drain_sse_blocks over a per-provider decode closure
- names: the ::/__ tool-name codec
All six providers now import these; the five OpenAI-style providers pick
up append_utf8_chunk (fixing the split-codepoint corruption) and CRLF
block reframing (SSE permits CRLF; bare find("\n\n") missed it).
Provider-specific SSE decoding, request building, and error
classification stay per-crate. llamacpp's error_chain copy lived in
errors.rs and is gone too; its integration test gained a bounded retry
around refresh_models for a pre-existing config-propagation race
(verified failing on the unconverted baseline).
Tests: cargo test green in llm-router and all six provider crates
(engine-backed integration suites included).
…hot path Every provider stream call made two serial engine round trips before its first upstream byte — state::load_token then router::provider::resolve — for a registration token that changes only on re-registration and a resolve response (credential/api_url/max_tokens) that changes only when an operator edits the router's config entry. context-manager likewise hit router::models::budget on every non-inline assemble/compact call. - provider_scaffold::cache::ScaffoldCache: an in-process token cell (no TTL; store_token refreshes it) plus a 30s-TTL resolve cache. Providers cannot observe the router's config entry, so the TTL bounds operator- edit staleness; eager invalidation on router::ready (restarted router) and on auth-classified failures (rotated credential) — invalidation only drops the cache, retry/failover stays the router's job. The scaffold pump now returns the terminal Error frame's ErrorKind so stream handlers can react; all six providers wire the cache through make_stream/register (openai + llamacpp also through their embed paths; codex's vault/oauth credential flow is deliberately uncached — the vault refreshes expiring tokens on its own resolve). - context-manager: CachingModelResolver decorator over Deps.resolver ((provider, id)-keyed; 60s TTL for known budgets, 5s for unknown models so a fresh reconcile lands quickly, errors never cached), flushed by a best-effort router::models::changed binding — a failed bind degrades to TTL-only staleness, never blocks boot. Cold paths (discovery refresh, declare) stay direct by design: the config-change refresh probe must see fresh resolve output. Tests: cache unit suites in llm-router and context-manager (fake clock / counting resolver); cargo test green across llm-router, all six providers (engine-backed integration suites included, exercising the 401 and router::ready invalidation paths), and context-manager.
Every delta variant of AssistantMessageEvent carried a full cumulative
partial: AssistantMessage. For a stream of N chunks the provider rebuilt
and re-serialized the whole accumulated message on every chunk, the
router parsed it, cloned it, and re-serialized it, and the harness parsed
it again — O(N²) CPU and allocation on every hop, and every frame shipped
the delta AND the entire message so far. A long thinking turn (~3000
deltas) serialized ~90 MB across the pipeline for ~250 KB of content.
The contract change (types/events.rs): partial becomes optional on the
three delta variants only — producers emit slim frames ({type, delta}).
Block-boundary frames (start / *_start / *_end) keep required cumulative
snapshots: thinking signatures and finalized function-call arguments
exist only there, and they bound reconstruction state to O(blocks).
A legacy fat delta still parses and is honored as an authoritative
snapshot, so old producers interop with new readers.
- llm-router: new chat::accumulate::PartialAccumulator (last boundary
snapshot + open-block deltas; mid-call death degrades raw args with the
same replay-safe shape providers use) feeds abort/no-terminal synthesis
in the relay — chat.rs/synthesize.rs unchanged. The relay now forwards
every frame except Usage/Done/Error as the ORIGINAL string (only those
three are cost-enriched), pinned by a byte-identical forwarding test —
the per-frame re-serialize is gone even for legacy producers.
- providers (×6): delta constructors emit partial: None; the per-delta
build_partial/build_content full-message clone is gone. Boundary and
terminal frames unchanged. Contract pin tests added per crate.
- harness: mirror enum + local PartialTracker keep session::update-message
streaming cumulative at the same coalesce cadence; the in-flight
args_acc/_streaming enrichment is unchanged, and open_call_id falls
back to the boundary snapshot on slim frames.
- docs: tech-specs README event union (partial? on delta rows + the
boundary-snapshot invariant) and the llm-router.md example updated.
BREAKING(wire, skew only): an OLD router/harness cannot parse slim delta
frames from a NEW provider (required-field serde error → frame skipped).
Release readers before writers: harness → llm-router → providers. Every
other pairing is compatible. Note: harness/evals/conformance (unmerged
feat/harness-conformance-e2e branch) mirrors this enum and must update
its frames.rs + streamed-text fixture + schema goldens when it lands;
tech-specs/2026-06-agentic/presentation TurnSection.tsx still shows the
old contract (follow-up).
Tests: cargo test green in llm-router (incl. new accumulate suite +
slim-abort/byte-identical relay tests), harness (tracker suite), and all
six providers (slim pin tests; engine-backed integration suites).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 45 skipped (no docs/).
Four for four. Nicely done. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds memoized context sizing and model-budget caching, changes streaming deltas to optional slim snapshots with accumulator-based reconstruction, introduces shared provider scaffolding and caches, and adds incremental session message retrieval using an entry watermark. ChangesContext sizing and caching
Streaming and provider scaffolding
Session watermark loading
Console invocation timeout
Turn-loop token reuse
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant SSETransport
participant PartialAccumulator
participant Relay
participant Consumer
Provider->>SSETransport: stream SSE chunks
SSETransport->>PartialAccumulator: decoded AssistantMessageEvent values
PartialAccumulator->>Relay: cumulative partial state
Relay->>Consumer: passthrough content and enriched terminal frames
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
provider-xai/src/stream_fn.rs (1)
62-83: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
AuthExpiredis unreachable here.classify_bus_error(&e)only yieldsPermanentorTransient, socache.invalidate()never runs on resolve errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-xai/src/stream_fn.rs` around lines 62 - 83, Update the resolve error handling in the stream function around cache.resolve and classify_bus_error: do not check for ErrorKind::AuthExpired or invalidate the cache there, since classification only returns Permanent or Transient. Preserve the synthetic error event and early return using the classified kind.provider-anthropic/src/stream_fn.rs (1)
65-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the unreachable AuthExpired cache invalidation
classify_bus_errorhere only yieldsPermanentorTransient, so theAuthExpiredbranch never runs. Either teach bus classification to emitAuthExpiredfor auth failures, or drop this branch and update the comment above it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-anthropic/src/stream_fn.rs` around lines 65 - 84, Remove the unreachable AuthExpired invalidation branch from the resolve error handling in the stream flow, since classify_bus_error only returns Permanent or Transient here. Update the nearby comment to match the retained classification behavior, while preserving the synthetic error event and early return.
🧹 Nitpick comments (1)
provider-llamacpp/src/upstream.rs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
decode/[DONE]dispatch closure across 5 provider crates. Each file defines the same closure structure ([DONE] →Stop+Done, else JSON-parse →handle_chunk), differing only in the provider-specificPartialState/handle_chunk/build_finaltypes passed in. Given this PR already extractsappend_utf8_chunk,drain_sse_blocks, anderror_chainintoprovider_scaffold::sse_transport, this remaining block is a natural candidate for the same treatment (e.g. a generic helper takingstate: &mut S, astop_reason/build_finalaccessor, and ahandle_chunkcallback), reducing ~20-line duplication ×5 and the risk of the five copies drifting apart on future fixes.
provider-llamacpp/src/upstream.rs#L101-122: extract thedecodeclosure body into a shared generic helper inllm_router::provider_scaffold.provider-openai-codex/src/upstream.rs#L98-119: same extraction, delegate to the shared helper.provider-xai/src/upstream.rs#L98-119: same extraction, delegate to the shared helper.provider-zai/src/upstream.rs#L100-121: same extraction, delegate to the shared helper.provider-openai/src/upstream.rs#L99-120: same extraction, delegate to the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@provider-llamacpp/src/upstream.rs` at line 1, Extract the duplicated decode/[DONE] dispatch logic from the upstream request flows into one generic helper under provider_scaffold, parameterized by mutable state, stop/final-result access, and the provider-specific handle_chunk callback. Replace the local decode closures in the upstream implementations for llamacpp, openai-codex, xai, zai, and openai with calls to this shared helper, preserving the existing Stop+Done handling, JSON parsing, handle_chunk behavior, and build_final flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@context-manager/src/adapters/cache.rs`:
- Around line 34-38: Add bounded eviction for the entries map in
CachingModelResolver so arbitrary (id, provider) pairs cannot grow without
limit. Update the cache access or maintenance logic to evict entries using a
defined capacity policy or periodically remove stale CacheEntry values, while
preserving existing lookups and wholesale-clear behavior.
In `@llm-router/src/chat/accumulate.rs`:
- Around line 44-75: The TextStart and ThinkingStart handlers must only remove a
trailing matching block when it is an identifiable empty block, never a
completed block containing text. Update the accumulator logic in
llm-router/src/chat/accumulate.rs lines 44-75, add regression coverage for the
omitted-new-empty-block case in lines 208-230, apply the same boundary rule in
harness/src/clients/router.rs lines 502-533 within PartialTracker, and add
equivalent consumer-side coverage in lines 755-790.
In `@provider-llamacpp/src/embed.rs`:
- Around line 96-103: Update the error handling around the cache.resolve call so
authentication-expiration errors are detected using the error information
actually returned by resolve, rather than classify_bus_error(), and invalidate
the cache for that case before propagating the error. Preserve propagation of
all other errors unchanged.
In `@provider-openai/src/embed.rs`:
- Around line 101-103: Update classify_bus_error so auth-related resolve
failures, including router/registration_rejected, return ErrorKind::AuthExpired
instead of Permanent or Transient; this enables the existing cache.invalidate
checks in provider-openai/src/embed.rs lines 101-103 and
provider-openai/src/stream_fn.rs lines 79-82, which require no direct changes.
In `@tech-specs/2026-06-agentic/README.md`:
- Line 369: Update the canonical "functioncall_delta" event type in the
documented union to include its optional function-call ID field alongside
partial and delta, preserving the existing event shape while exposing the
identifier integrations use to associate argument deltas with their call.
---
Outside diff comments:
In `@provider-anthropic/src/stream_fn.rs`:
- Around line 65-84: Remove the unreachable AuthExpired invalidation branch from
the resolve error handling in the stream flow, since classify_bus_error only
returns Permanent or Transient here. Update the nearby comment to match the
retained classification behavior, while preserving the synthetic error event and
early return.
In `@provider-xai/src/stream_fn.rs`:
- Around line 62-83: Update the resolve error handling in the stream function
around cache.resolve and classify_bus_error: do not check for
ErrorKind::AuthExpired or invalidate the cache there, since classification only
returns Permanent or Transient. Preserve the synthetic error event and early
return using the classified kind.
---
Nitpick comments:
In `@provider-llamacpp/src/upstream.rs`:
- Line 1: Extract the duplicated decode/[DONE] dispatch logic from the upstream
request flows into one generic helper under provider_scaffold, parameterized by
mutable state, stop/final-result access, and the provider-specific handle_chunk
callback. Replace the local decode closures in the upstream implementations for
llamacpp, openai-codex, xai, zai, and openai with calls to this shared helper,
preserving the existing Stop+Done handling, JSON parsing, handle_chunk behavior,
and build_final flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b895f8e5-51ec-402e-bae8-d0265b07a510
⛔ Files ignored due to path filters (5)
provider-anthropic/Cargo.lockis excluded by!**/*.lockprovider-openai-codex/Cargo.lockis excluded by!**/*.lockprovider-openai/Cargo.lockis excluded by!**/*.lockprovider-xai/Cargo.lockis excluded by!**/*.lockprovider-zai/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (79)
context-manager/src/adapters/cache.rscontext-manager/src/adapters/mod.rscontext-manager/src/core/prune.rscontext-manager/src/core/selection.rscontext-manager/src/functions/assemble.rscontext-manager/src/functions/compact.rscontext-manager/src/functions/count_tokens.rscontext-manager/src/main.rsharness/src/clients/router.rsharness/src/clients/session.rsharness/src/turn_loop.rsharness/src/types/event.rsllm-router/src/chat/accumulate.rsllm-router/src/chat/mod.rsllm-router/src/chat/relay.rsllm-router/src/lib.rsllm-router/src/provider_scaffold/cache.rsllm-router/src/provider_scaffold/mod.rsllm-router/src/provider_scaffold/names.rsllm-router/src/provider_scaffold/pump.rsllm-router/src/provider_scaffold/router_client.rsllm-router/src/provider_scaffold/sse_transport.rsllm-router/src/provider_scaffold/state.rsllm-router/src/types/events.rsllm-router/tests/integration.rsprovider-anthropic/src/register.rsprovider-anthropic/src/router_client.rsprovider-anthropic/src/sse.rsprovider-anthropic/src/state.rsprovider-anthropic/src/stream_fn.rsprovider-anthropic/src/upstream.rsprovider-anthropic/src/wire/names.rsprovider-llamacpp/src/discovery.rsprovider-llamacpp/src/embed.rsprovider-llamacpp/src/errors.rsprovider-llamacpp/src/register.rsprovider-llamacpp/src/router_client.rsprovider-llamacpp/src/sse.rsprovider-llamacpp/src/state.rsprovider-llamacpp/src/stream_fn.rsprovider-llamacpp/src/upstream.rsprovider-llamacpp/src/wire/names.rsprovider-llamacpp/tests/integration.rsprovider-openai-codex/src/register.rsprovider-openai-codex/src/router_client.rsprovider-openai-codex/src/sse.rsprovider-openai-codex/src/state.rsprovider-openai-codex/src/stream_fn.rsprovider-openai-codex/src/upstream.rsprovider-openai-codex/src/wire/names.rsprovider-openai/src/embed.rsprovider-openai/src/register.rsprovider-openai/src/router_client.rsprovider-openai/src/sse.rsprovider-openai/src/state.rsprovider-openai/src/stream_fn.rsprovider-openai/src/upstream.rsprovider-openai/src/wire/names.rsprovider-xai/src/register.rsprovider-xai/src/responses.rsprovider-xai/src/router_client.rsprovider-xai/src/sse.rsprovider-xai/src/state.rsprovider-xai/src/stream_fn.rsprovider-xai/src/upstream.rsprovider-xai/src/wire/names.rsprovider-zai/src/register.rsprovider-zai/src/router_client.rsprovider-zai/src/sse.rsprovider-zai/src/state.rsprovider-zai/src/stream_fn.rsprovider-zai/src/upstream.rsprovider-zai/src/wire/names.rssession-manager/src/functions/messages.rssession-manager/src/service.rssession-manager/tests/features/messages.featuresession-manager/tests/golden/schemas/session.messages.jsontech-specs/2026-06-agentic/README.mdtech-specs/2026-06-agentic/llm-router.md
💤 Files with no reviewable changes (1)
- provider-llamacpp/src/errors.rs
Comment on lines
+34
to
+38
| pub struct CachingModelResolver { | ||
| inner: Arc<dyn ModelResolver>, | ||
| clock: Arc<dyn Clock>, | ||
| entries: RwLock<HashMap<(Option<String>, String), CacheEntry>>, | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files | rg 'context-manager/src/adapters/cache\.rs|context-manager/src|router|main\.rs|context-manager/src/.*\.rs$' || true
printf '\n== cache.rs outline ==\n'
ast-grep outline context-manager/src/adapters/cache.rs --view expanded || true
printf '\n== search for get_model_budget callers and model validation ==\n'
rg -n "get_model_budget\(|model\.provider|model\.id|validate.*model|catalog|changed" context-manager/src router main.rs -g '*.rs' || trueRepository: iii-hq/workers
Length of output: 13813
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' context-manager/src/adapters/cache.rsRepository: iii-hq/workers
Length of output: 7428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow search for request-time model resolution and router-side model validation
rg -n "context::assemble|count_tokens|compact|ModelResolver|router::models|model.*provider|model.*id|provider.*id" . -g '*.rs'Repository: iii-hq/workers
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the cache implementation and nearby call sites with line numbers
cat -n context-manager/src/adapters/cache.rs | sed -n '1,260p'
echo
rg -n "context::assemble|compact\(|count_tokens\(|get_model_budget\(" context-manager/src main.rs -g '*.rs'
echo
rg -n "provider.*id|model.*provider|model.*id|validate.*model|catalog" context-manager/src router main.rs -g '*.rs'Repository: iii-hq/workers
Length of output: 13301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect only the relevant file if present
if [ -f context-manager/src/adapters/cache.rs ]; then
sed -n '1,220p' context-manager/src/adapters/cache.rs
fi
# Find the call path that supplies provider/id
rg -n "req\.model|model\.provider|model\.id|get_model_budget" context-manager -g '*.rs'Repository: iii-hq/workers
Length of output: 10581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== context-manager input types ==\n'
sed -n '1,260p' context-manager/src/types.rs
printf '\n== context-manager function inputs ==\n'
sed -n '1,180p' context-manager/src/functions/mod.rs
printf '\n== workflow model validation path ==\n'
sed -n '700,830p' workflow/src/functions/start.rsRepository: iii-hq/workers
Length of output: 20430
Cap the model-budget cache
entries is only cleared wholesale and never evicted per key. Because ModelInput accepts arbitrary id/provider, this map can retain one row per distinct pair for the life of the process. Add an eviction policy or a periodic stale-entry sweep.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@context-manager/src/adapters/cache.rs` around lines 34 - 38, Add bounded
eviction for the entries map in CachingModelResolver so arbitrary (id, provider)
pairs cannot grow without limit. Update the cache access or maintenance logic to
evict entries using a defined capacity policy or periodically remove stale
CacheEntry values, while preserving existing lookups and wholesale-clear
behavior.
Comment on lines
+44
to
+75
| AssistantMessageEvent::TextStart { partial } => { | ||
| let mut base = partial.clone(); | ||
| let seed = match base.content.last() { | ||
| Some(ContentBlock::Text { text }) => { | ||
| let text = text.clone(); | ||
| base.content.pop(); | ||
| text | ||
| } | ||
| _ => String::new(), | ||
| }; | ||
| self.base = Some(base); | ||
| self.open = Some(OpenBlock::Text { | ||
| seed, | ||
| acc: String::new(), | ||
| }); | ||
| } | ||
| AssistantMessageEvent::ThinkingStart { partial } => { | ||
| let mut base = partial.clone(); | ||
| let seed = match base.content.last() { | ||
| Some(ContentBlock::Thinking { text, .. }) => { | ||
| let text = text.clone(); | ||
| base.content.pop(); | ||
| text | ||
| } | ||
| _ => String::new(), | ||
| }; | ||
| self.base = Some(base); | ||
| self.open = Some(OpenBlock::Thinking { | ||
| seed, | ||
| acc: String::new(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not misclassify completed blocks as newly opened blocks.
Both accumulators remove any trailing matching content block, although only a newly opened empty block may be removed.
llm-router/src/chat/accumulate.rs#L44-L75: only pop an identifiable empty text/thinking block.llm-router/src/chat/accumulate.rs#L208-L230: add coverage where a completed same-kind block precedes a start snapshot that omits the new empty block.harness/src/clients/router.rs#L502-L533: apply the same boundary rule toPartialTracker.harness/src/clients/router.rs#L755-L790: add equivalent consumer-side regression coverage.
📍 Affects 2 files
llm-router/src/chat/accumulate.rs#L44-L75(this comment)llm-router/src/chat/accumulate.rs#L208-L230harness/src/clients/router.rs#L502-L533harness/src/clients/router.rs#L755-L790
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@llm-router/src/chat/accumulate.rs` around lines 44 - 75, The TextStart and
ThinkingStart handlers must only remove a trailing matching block when it is an
identifiable empty block, never a completed block containing text. Update the
accumulator logic in llm-router/src/chat/accumulate.rs lines 44-75, add
regression coverage for the omitted-new-empty-block case in lines 208-230, apply
the same boundary rule in harness/src/clients/router.rs lines 502-533 within
PartialTracker, and add equivalent consumer-side coverage in lines 755-790.
Comment on lines
+96
to
+103
| let resolved = cache | ||
| .resolve(iii, crate::PROVIDER_ID, token.as_deref()) | ||
| .await | ||
| .inspect_err(|e| { | ||
| if classify_bus_error(e) == ErrorKind::AuthExpired { | ||
| cache.invalidate(); | ||
| } | ||
| })?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== provider-llamacpp/src/errors.rs ==\n'
sed -n '1,140p' provider-llamacpp/src/errors.rs
printf '\n== provider-llamacpp/src/embed.rs ==\n'
sed -n '1,220p' provider-llamacpp/src/embed.rs
printf '\n== search for classify_bus_error and classify() usages ==\n'
rg -n "classify_bus_error|classify\\(" provider-llamacpp/srcRepository: iii-hq/workers
Length of output: 16480
classify_bus_error() can't hit AuthExpired here This cache.invalidate() branch is unreachable, so auth-failure resolves never drop the stale cache entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@provider-llamacpp/src/embed.rs` around lines 96 - 103, Update the error
handling around the cache.resolve call so authentication-expiration errors are
detected using the error information actually returned by resolve, rather than
classify_bus_error(), and invalidate the cache for that case before propagating
the error. Preserve propagation of all other errors unchanged.
Comment on lines
+101
to
+103
| if classify_bus_error(&e) == ErrorKind::AuthExpired { | ||
| cache.invalidate(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the implementation of classify_bus_error
cat provider-openai/src/errors.rsRepository: iii-hq/workers
Length of output: 7217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== classify_bus_error call sites ==\n'
rg -n "classify_bus_error|AuthExpired|registration_rejected|cache\.invalidate\(\)" provider-openai -S
printf '\n== embed.rs relevant slice ==\n'
sed -n '80,120p' provider-openai/src/embed.rs
printf '\n== stream_fn.rs relevant slice ==\n'
sed -n '60,100p' provider-openai/src/stream_fn.rs
printf '\n== resolve-related error mappings ==\n'
rg -n "registration_rejected|provider/invalid_request|provider/upstream_unavailable|AuthExpired" provider-openai/src -SRepository: iii-hq/workers
Length of output: 7262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== upstream.rs slice around AuthExpired assertion ==\n'
sed -n '240,290p' provider-openai/src/upstream.rs
printf '\n== all registration_rejected references ==\n'
rg -n "registration_rejected|AuthExpired|Permanent" provider-openai/src -S
printf '\n== provider-openai tests around resolve/auth ==\n'
rg -n "resolve failed|registration_rejected|AuthExpired" provider-openai/src/*.rs -n -SRepository: iii-hq/workers
Length of output: 6422
Resolve auth failures as AuthExpired.
classify_bus_error currently returns Permanent for router/registration_rejected and Transient otherwise, so the cache invalidation branches in provider-openai/src/embed.rs#L101-L103 and provider-openai/src/stream_fn.rs#L79-L82 never fire. Map the auth-related resolve failure to AuthExpired, or drop the invalidation checks.
📍 Affects 2 files
provider-openai/src/embed.rs#L101-L103(this comment)provider-openai/src/stream_fn.rs#L79-L82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@provider-openai/src/embed.rs` around lines 101 - 103, Update
classify_bus_error so auth-related resolve failures, including
router/registration_rejected, return ErrorKind::AuthExpired instead of Permanent
or Transient; this enables the existing cache.invalidate checks in
provider-openai/src/embed.rs lines 101-103 and provider-openai/src/stream_fn.rs
lines 79-82, which require no direct changes.
| | { type: "thinking_end"; partial: AssistantMessage } | ||
| | { type: "functioncall_start";partial: AssistantMessage } | ||
| | { type: "functioncall_delta";partial: AssistantMessage; delta: string } | ||
| | { type: "functioncall_delta";partial?: AssistantMessage; delta: string } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the optional function-call ID.
The wire event includes an optional id, and the harness uses it to associate slim argument deltas with their call. Omitting it from the canonical union leaves integrations with an incomplete contract.
Proposed correction
- | { type: "functioncall_delta";partial?: AssistantMessage; delta: string }
+ | { type: "functioncall_delta";partial?: AssistantMessage; delta: string; id?: string }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | { type: "functioncall_delta";partial?: AssistantMessage; delta: string } | |
| | { type: "functioncall_delta";partial?: AssistantMessage; delta: string; id?: string } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tech-specs/2026-06-agentic/README.md` at line 369, Update the canonical
"functioncall_delta" event type in the documented union to include its optional
function-call ID field alongside partial and delta, preserving the existing
event shape while exposing the identifier integrations use to associate argument
deltas with their call.
ytallo
changed the title
context::compact makes a summariser LLM call budgeted up to 320s
(context-manager summarizer_timeout_ms), but the console's trigger
wrapper never passed a timeout, so the browser SDK's 30s default killed
every long compaction with 'Invocation timeout after 30000ms:
context::compact' while the summary was still streaming.
The IiiClient trigger wrapper now takes an optional { timeoutMs } passed
through to the SDK, and /compact invokes with 330s — headroom over the
summariser's own outer budget. Other callers keep the 30s default.
Updated the IiiClient trigger wrapper to use a default timeout of 5 minutes for SDK calls when no specific timeout is provided. This change ensures that long-running operations, such as compaction, do not fail due to the SDK's default 30-second timeout.
ytallo
added a commit
that referenced
this pull request
Jul 17, 2026
The delta-frame contract change in #525 was marked with a conventional-commit breaking-change '!', but nine workers (harness, context-manager, memory, and all six providers) pin llm-router via a "^1.0.0" registry dependency constraint. A 2.0.0 release would break that range for all of them, even though the change is designed to interop safely (old producers still parse fine under the new readers). Releasing as a minor bump instead. The erroneous llm-router/v2.0.0 tag and GitHub Release have been deleted.
ytallo
added a commit
that referenced
this pull request
Jul 17, 2026
provider-kimi predates #525's llm-router!: slim streaming delta frames change and still built full AssistantMessage snapshots on every delta, which no longer compiles against the new Option<AssistantMessage> contract. Mirrors the pattern already applied to the other six providers.
ytallo
added a commit
that referenced
this pull request
Jul 17, 2026
provider-kimi predates #525's llm-router!: slim streaming delta frames change and still built full AssistantMessage snapshots on every delta, which no longer compiles against the new Option<AssistantMessage> contract. Mirrors the pattern already applied to the other six providers.
4 tasks
ytallo
added a commit
that referenced
this pull request
Jul 17, 2026
…527) * chore(provider-kimi): wire release CD, set initial version to 0.1.0 Adds provider-kimi to create-tag.yml's worker options and release.yml's tag-trigger patterns per docs/sops/new-worker.md §6, and drops the manifest version from the placeholder 1.0.0 to 0.1.0 for its first release. * fix(provider-kimi): emit slim (partial: None) delta frames provider-kimi predates #525's llm-router!: slim streaming delta frames change and still built full AssistantMessage snapshots on every delta, which no longer compiles against the new Option<AssistantMessage> contract. Mirrors the pattern already applied to the other six providers. * chore(provider-kimi): sync Cargo.lock llm-router pin to 1.2.0 * chore(provider-kimi): keep initial release at 1.0.0 validate_worker.py hard-blocks any worker's manifest version decreasing between merged commits once its source has changed, and main already has provider-kimi at 1.0.0 from the original PR #522 merge. Releasing at 1.0.0 instead of introducing an exemption to that CI gate.
andersonleal
added a commit
that referenced
this pull request
Jul 17, 2026
…#528) * fix(provider-kimi): emit slim (partial: None) delta frames provider-kimi predates #525's llm-router!: slim streaming delta frames change and still built full AssistantMessage snapshots on every delta, which no longer compiles against the new Option<AssistantMessage> contract. Mirrors the pattern already applied to the other six providers. * fix(harness): cut in-flight generation and tool loops on stop harness::stop set a durable abort flag that run_step only observed at step boundaries and after generation returned — router.chat had no local cancellation, so a stop could wait out a full generation (up to router_timeout_ms) or an entire tool-execution phase, and repeat stops redid the whole cascade every click. - TurnCancels (locks.rs): per-turn level-triggered watch registry, fired lock-free by stop.rs; keyed by turn_id so a stale fire can't cancel a newer turn. Single-process, same caveat as SessionLocks. - router.chat gains an abort arm: cuts the await immediately, persists the partial as Aborted, re-fires router::abort (a stop can race ahead of the router's stream registration and no-op), kills the held-open trigger task. - run_step: bail before dispatching generation when the cancel fired during context assembly; check the signal between tool calls (the durable write blocks on the session lock for that whole phase). - stop.rs: idempotency guard (repeat stops become one read), immediate "stopping" phase reason on the existing status channel, cancel signal fired before anything that awaits. - finalize_cancelled: durable "stopped by user." notice entry (e_{turn}_stopped) so the transcript doesn't just end mid-thought; argument-less tool-call blocks truncated by the cut are dropped from the persisted partial (they render as empty cards and can never run). * fix(console): stopping button state, click dedupe, stop-notice id The stop button gave no feedback and stayed clickable after a click: local isStreaming cleared instantly on abort but serverWorking held the indicator, so users clicked repeatedly (each click re-firing harness::stop) with nothing acknowledging the stop. - ChatView: `stopping` state — set on first stop, guards repeat clicks, cleared when the streaming indicator drops. - Composer: stop button disables with a spinner while stopping. - translate: the cancelled stop-reason event carries entryId e_{turn}_stopped, matching the harness's new durable notice so the live event and the reconciled transcript row dedupe. * feat(llm-router): fan router::abort out to provider::<id>::abort router::abort closed the relay reader and left the provider to notice on its next channel write — up to 2×PING_INTERVAL (60s) of billed upstream generation during a silent stretch (e.g. Anthropic server-buffering a large tool_use input). - chat.rs: the inflight abort closer now also fires provider::<id>::abort (detached, best-effort; providers without the function are a harmless error) using the request_id the provider already receives as resolution_key. - provider_scaffold::aborts: StreamAborts registry (request_id → live upstream cancel, level-triggered watch) + a ready-made make_abort handler for providers to register. - pump_abortable: pump with an abort arm — returns immediately on the signal even mid-silence, dropping the upstream receiver, which aborts the upstream task and its in-flight HTTP request. - PING_INTERVAL 30s → 2s: the failed-ping write remains the passive backstop when the active abort is lost; this bounds that path to ~4s. - ProviderAbortRequest/ProviderAbortResponse wire types. * feat(providers): register provider::<id>::abort in all seven providers Wires the scaffold's StreamAborts/pump_abortable into every provider: the stream registers its resolution_key (the router's request_id) while the upstream is live, provider::<id>::abort signals it, and the pump returns immediately — cancelling the upstream HTTP stream instead of letting it bill tokens until a write fails. Same shape everywhere: ABORT_ID/ABORT_DESC in the surface catalog, registration right after stream, resolution_key match around the live pump (plain pump when absent), schema goldens regenerated. provider-xai shares one pump_stream helper across its two stream paths. Cargo.locks pick up the already-committed llm-router 1.2.0 pin. * fix(harness): close stop/abort races found in review - The "stopping" status ack moves under the session lock, after the terminal re-check: issued lock-free it could land after a concurrent finalizer's "done" and strand the session on "working" (the locked re-read returns stopping:false without restoring). fail_turn now takes the session lock too — it was the one lock-free finalizer, the same hole by another door (its only caller runs after run_step returned, so no deadlock). - chat's pump wakeup uses notify_one (latched permit) instead of notify_waiters: a pre-fired cancel can reach the abort arm before the spawned pump polls its notified() future, and an unlatched notify is lost — pump.await would hang on next_binary until natural EOF. - The abort arm prefers final_message over the tracker partial: a stop landing between a terminal Done frame and EOF must not replace the complete assistant entry with the stale pre-Done partial. * fix(console): re-enable the stop button when the abort RPC fails A rejected backend.abortRun left `stopping` stuck true while the server still reported working — the reset effect waits on the streaming indicator, so the disabled button could never retry. Reset on rejection. * fix(providers): cancel parked upstreams, latch pre-setup aborts Review found two gaps in the provider::<id>::abort wiring, fixed uniformly across all seven providers: - The detached upstream task only observed a dropped receiver at its next send, so a silent upstream — parked in a chunk read with nothing to send — kept the HTTP stream and its billed generation alive until the next frame or the read timeout. spawn_upstream (and xai's responses variant) now races the call against Sender::closed(), so receiver drop cancels the HTTP future immediately. - The abort registry entry was created only at pump time, so an abort landing during provider setup (credential resolve/config) hit an unknown id and was lost, letting the request start after its own cancellation. The stream fn now subscribes at entry (level-triggered watch latches early aborts), skips spawning the upstream when already fired, and cleans up at a single point in the make_stream wrapper. Also marks provider::kimi::abort internal like the other six, keeping the control-plane callback out of the agent-facing catalog. * style: cargo fmt across provider crates * fix(console): move stop side effects out of the setState updater React forbids side effects inside a state updater — Strict Mode double-invokes updaters in dev, which would fire abort()/abortRun twice. Dedupe on a ref instead (also synchronous, so two clicks in one frame can't both pass), keeping abort()/abortRun and the failure reset in the event-handler body. * fix(providers): register abort watch before open_sink, RAII cleanup Two review findings across all seven provider stream fns: - The abort watch was created only after `open_sink(...).await`, so a stop landing during sink setup hit an unknown request id, was consumed, and the upstream then started with a fresh untriggered watch. Register before the first await so the (level-triggered) signal latches. - Sequential `aborts.remove(rid)` after the call is skipped if the executor drops the handler future mid-await, leaking the registry entry. Replace with an RAII `AbortGuard` (StreamAborts::register) that deregisters on Drop — covering early returns and cancellation. * style(provider-anthropic): rustfmt the abort_reg registration --------- Co-authored-by: Ytallo Layon <ytallo.layon@gmail.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.
Summary
An audit of the context path (harness → context-manager → llm-router → providers) found the pipeline correct but paying large, avoidable costs on every model call and every streamed token. This branch lands five fixes, one commit each:
perf(context-manager): memoized counting. assemble re-serialized every message, tool, and the prompt from scratch four times per call, and compaction'ssplit_turnwas O(k²) per oversized turn. Now: one estimate per message with a size memo kept in lockstep by the mutating passes, prefix sums in selection. Totals are byte-identical (adebug_assertrecount pins the memo on every debug test run); goldens and all 84 BDD scenarios pass unmodified.perf(harness): no redundant full-payload hops. The finalcontext::count-tokenscall recomputed exactly whatcontext::assemblehad just returned whenever no hook/orphan-patch mutated the request (the common case) — now skipped via the assemble response's owntoken_count. The steering watermark check reloaded the entire transcript; session-manager gains an additiveafter_entry_idparam and the harness fetches only the post-watermark suffix (full-scan fallback onsession/invalid_cursor).refactor(providers): shared scaffold + UTF-8 fix. The six provider crates carried verbatim copies of router_client/state/pump/SSE-transport plumbing (the pump literally said "shared extraction into llm-router is a listed follow-up") — extracted intollm_router::provider_scaffold. The drift this had already caused gets fixed in the same move: the five OpenAI-style providers corrupted multibyte codepoints split across network chunks (from_utf8_lossyper chunk → U+FFFD); they now share anthropic's cross-chunk UTF-8 buffering, plus CRLF SSE reframing. Net −992 lines.perf(providers,context-manager): lookup caches. Every stream call made two serial engine round trips (registration token +router::provider::resolve) before its first upstream byte — now an in-process token cell + 30s-TTL resolve cache, eagerly invalidated onrouter::readyand auth-classified failures (drop-only; retry stays the router's job). context-manager's per-callrouter::models::budgethop becomes a TTL cache flushed byrouter::models::changed. codex's vault/oauth flow is deliberately uncached.perf(llm-router)!: slim streaming deltas — O(N²) → O(N). Every delta frame carried the full cumulative message; providers rebuilt and re-serialized it per chunk and every hop re-parsed it (~90 MB serialized for a ~250 KB thinking turn).partialis now optional on the three delta variants (producers emit{type, delta}); block-boundary frames keep required cumulative snapshots (thinking signatures / final call args live only there). The router relay reconstructs via a shared accumulator for abort/no-terminal synthesis and forwards non-enriched frames byte-identically (no per-frame re-serialize, pinned by test); the harness tracks cumulatively forsession::update-messagestreaming with unchanged_streamingargs enrichment.Readers before writers: harness → llm-router → providers. An old router/harness cannot parse slim delta frames from a new provider (required-field serde error → frame silently skipped); every other pairing interops (fat deltas are honored as authoritative snapshots).
Verification
cargo fmt+clippy --all-targetsclean andcargo testgreen in all 10 crates (context-manager 84 BDD scenarios, session-manager 126, harness 229, llm-router 104 incl. 13 engine-backed integration tests, all six providers incl. their engine-backed suites).partialmaterialized in transit) plus the terminal; the existing abort test covers synthesized terminals from accumulated content.Follow-ups (not in this PR)
feat/harness-conformance-e2e(unmerged) mirrors the event enum inharness/evals/conformance— itsframes.rs, streamed-text fixture, and schema goldens need the same optional-partial change when the branches meet.tech-specs/2026-06-agentic/presentation/src/sections/TurnSection.tsxstill shows the old "partial accumulator on every frame" slide.discover_changed_workers.pyso llm-router lib changes re-test the six dependent providers.Summary by CodeRabbit
after_entry_id, with validation for invalid/forked references.timeoutMsoption forIiiClient.trigger.References
context::assemble's overhead-inclusivetoken_count, keeps the> usableguard), and commit 1 pins counting byte-identical with a cross-function equivalence test. External storage of oversized results remains open on the ticket.context::assemble's own count (contractually within budget), so the under-allocation will surface as a structuredcontext/overflowfrom assemble instead — the root cause (haiku budget resolution falling back to 8192/1024) is fixed on the ticket, not here.partialon every frame re-serialized sessions that size on every hop, every chunk.