Repository navigation
fix(compression): skip CCR on tool outputs to preserve agent loop - #7869
diegosouzapw merged 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a fix to skip CCR compression for tool outputs (OpenAI role: "tool" and Anthropic user messages containing exclusively tool_result parts) to prevent breaking the agent loop when OmniRoute is used as a chat-completion provider. It also adds comprehensive unit tests to verify this behavior. The reviewer suggested using optional chaining when checking the type property of parts in msg.content to prevent potential runtime crashes from unexpected non-object values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (msg.role === "user" && Array.isArray(msg.content)) { | ||
| const parts = msg.content as Array<Record<string, unknown>>; | ||
| if (parts.length > 0 && parts.every((p) => p["type"] === "tool_result")) { | ||
| return { ...msg }; | ||
| } | ||
| } |
There was a problem hiding this comment.
To prevent potential runtime crashes (e.g., TypeError: Cannot read properties of null (reading 'type')), use optional chaining (p?.["type"]) when checking the type property of each part. Since msg.content is parsed from external client input, it could contain null or other unexpected non-object values.
| if (msg.role === "user" && Array.isArray(msg.content)) { | |
| const parts = msg.content as Array<Record<string, unknown>>; | |
| if (parts.length > 0 && parts.every((p) => p["type"] === "tool_result")) { | |
| return { ...msg }; | |
| } | |
| } | |
| if (msg.role === "user" && Array.isArray(msg.content)) { | |
| const parts = msg.content as Array<Record<string, unknown>>; | |
| if (parts.length > 0 && parts.every((p) => p?.["type"] === "tool_result")) { | |
| return { ...msg }; | |
| } | |
| } |
There was a problem hiding this comment.
✅ Fixed in commit f994c5595 (pushed to fix/ccr-skip-tool-outputs).
Applied optional chaining in both places that dereferenced part["type"]:
- The new skip rule:
p?.["type"] === "tool_result". - The existing text-part compression path that I missed in the first pass — the new regression test caught it (TypeError at line 308, before the guard was added).
Added a regression test: does NOT crash on malformed user content with null / non-object parts passes a null entry inside the parts array.
All 5 tests in the new file pass, plus all 57 pre-existing CCR tests still pass.
|
Reviewed and verified — this is a real, well-tested fix. Nice catch on the agent-loop stall. I independently probed the claim by reverting only One small accuracy note for the record (not a blocker): the Anthropic-shape guard ( No conflicts against release/v3.8.49 in either touched file — good to retarget and merge. |
|
Replying to diegosouzapw's review above: Thanks for the verification — and especially for independently reverting Acknowledged on the Anthropic-shape no-op note: you're right — the pre-existing per-part loop only acts on
Happy to fold it into a comment if you prefer, but the code-as-spec form is what I'd keep. Retargeted base from |
When OmniRoute is used as a chat-completion PROVIDER (not as an MCP server),
the upstream LLM cannot call `omniroute_ccr_retrieve` to expand CCR markers
on demand. Replacing tool outputs with `[CCR retrieve hash=… chars=…]`
placeholders therefore makes the LLM stall — it sees an opaque marker
where the actual tool result should be and has no way to recover the
verbatim content.
Scope:
- OpenAI format: `{ role: "tool", tool_call_id, content }`
- Anthropic format: `{ role: "user", content: [{ type: "tool_result", … }] }`
Fix: extend `processMessages` in the CCR engine to skip both shapes
verbatim. The engine still applies to plain user / assistant text blocks,
which is where compression yields token savings AND the LLM can reason
about the marker.
Tests:
- New `tests/unit/compression/ccr-skip-tool-outputs.test.ts` covers both
formats (4 cases) plus a regression guard that plain user text is still
compressed.
- All 57 pre-existing CCR tests still pass.
Reported-by: herjarsa
Refs: AGENTS.md agent feedback — agent loop stalled on bash/read/grep
outputs after CCR collapsed them to markers.
Apply gemini-code-assist review feedback on PR cheaper-inference#7869: - Use optional chaining when reading `part["type"]` so malformed client payloads (null entries, non-object entries in the parts array) cannot throw `TypeError: Cannot read properties of null`. - The skip rule (this branch) and the existing text-part compression path (the next branch) both get the guard, since both dereference `part["type"]` directly. Test: - New defensive case: a user message whose content array contains a `null` entry alongside a `tool_result` must not crash the engine. Refs: gemini-code-assist review on cheaper-inference#7869 (PRR_kwDORPf6ys8AAAABGj5G7Q)
f994c55 to
ef689cc
Compare
|
Cross-checked this against the other two compression PRs in flight (#7863 OmniGlyph-for-Fable-5 and #7855 session-dedup memory bound): no semantic collision — the three touch different engines, and the stacked pipeline runs them at fixed Concretely for the case this PR fixes: on a Fable 5 request carrying tool outputs, session-dedup runs first, CCR now skips the tool output (your fix — previously it became a Suggested land order, purely for rollback hygiene: #7855 → this one → #7863. |
|
Acked on the cross-PR check + the stackPriority chain (session-dedup=3 → ccr=4 → omniglyph=90, fail-open per-step). The end-to-end ordering for a tool-output payload is exactly what I expected to happen but it's good to see it confirmed against the in-flight neighbors. On the land order (#7855 → this → #7863): noted, no action needed from my side — this PR is branch-clean on Standing by for whichever step of the train you want to trigger next — happy to hold while #7855 lands first per your call. |
fca82af
into
cheaper-inference:release/v3.8.49
|
Merged — thanks @herjarsa! Great catch on the agent-loop stall; the tool-output skip composes cleanly with the rest of the compression stack. Validated in the local merge-train (14-PR combined tree, full unit suite + vitest green) on tomni-proxmox-113 @ 0fda37bb4fdb26ba1345fd7b4cac44fbe235cda4, then squash-merged. |
…eaper-inference#7869) * fix(compression): skip CCR on tool outputs to preserve agent loop When OmniRoute is used as a chat-completion PROVIDER (not as an MCP server), the upstream LLM cannot call `omniroute_ccr_retrieve` to expand CCR markers on demand. Replacing tool outputs with `[CCR retrieve hash=… chars=…]` placeholders therefore makes the LLM stall — it sees an opaque marker where the actual tool result should be and has no way to recover the verbatim content. Scope: - OpenAI format: `{ role: "tool", tool_call_id, content }` - Anthropic format: `{ role: "user", content: [{ type: "tool_result", … }] }` Fix: extend `processMessages` in the CCR engine to skip both shapes verbatim. The engine still applies to plain user / assistant text blocks, which is where compression yields token savings AND the LLM can reason about the marker. Tests: - New `tests/unit/compression/ccr-skip-tool-outputs.test.ts` covers both formats (4 cases) plus a regression guard that plain user text is still compressed. - All 57 pre-existing CCR tests still pass. Reported-by: herjarsa Refs: AGENTS.md agent feedback — agent loop stalled on bash/read/grep outputs after CCR collapsed them to markers. * fix(compression): guard CCR against null / non-object parts Apply gemini-code-assist review feedback on PR cheaper-inference#7869: - Use optional chaining when reading `part["type"]` so malformed client payloads (null entries, non-object entries in the parts array) cannot throw `TypeError: Cannot read properties of null`. - The skip rule (this branch) and the existing text-part compression path (the next branch) both get the guard, since both dereference `part["type"]` directly. Test: - New defensive case: a user message whose content array contains a `null` entry alongside a `tool_result` must not crash the engine. Refs: gemini-code-assist review on cheaper-inference#7869 (PRR_kwDORPf6ys8AAAABGj5G7Q) --------- Co-authored-by: herjarsa <herjarsa@users.noreply.github.com>
…eaper-inference#7869) * fix(compression): skip CCR on tool outputs to preserve agent loop When OmniRoute is used as a chat-completion PROVIDER (not as an MCP server), the upstream LLM cannot call `omniroute_ccr_retrieve` to expand CCR markers on demand. Replacing tool outputs with `[CCR retrieve hash=… chars=…]` placeholders therefore makes the LLM stall — it sees an opaque marker where the actual tool result should be and has no way to recover the verbatim content. Scope: - OpenAI format: `{ role: "tool", tool_call_id, content }` - Anthropic format: `{ role: "user", content: [{ type: "tool_result", … }] }` Fix: extend `processMessages` in the CCR engine to skip both shapes verbatim. The engine still applies to plain user / assistant text blocks, which is where compression yields token savings AND the LLM can reason about the marker. Tests: - New `tests/unit/compression/ccr-skip-tool-outputs.test.ts` covers both formats (4 cases) plus a regression guard that plain user text is still compressed. - All 57 pre-existing CCR tests still pass. Reported-by: herjarsa Refs: AGENTS.md agent feedback — agent loop stalled on bash/read/grep outputs after CCR collapsed them to markers. * fix(compression): guard CCR against null / non-object parts Apply gemini-code-assist review feedback on PR cheaper-inference#7869: - Use optional chaining when reading `part["type"]` so malformed client payloads (null entries, non-object entries in the parts array) cannot throw `TypeError: Cannot read properties of null`. - The skip rule (this branch) and the existing text-part compression path (the next branch) both get the guard, since both dereference `part["type"]` directly. Test: - New defensive case: a user message whose content array contains a `null` entry alongside a `tool_result` must not crash the engine. Refs: gemini-code-assist review on cheaper-inference#7869 (PRR_kwDORPf6ys8AAAABGj5G7Q) --------- Co-authored-by: herjarsa <herjarsa@users.noreply.github.com>
Problem
When OmniRoute is used as a chat-completion provider (e.g. by OpenCode, Claude Code, or any OpenAI-compatible client that points its base URL at
/v1), the upstream LLM has no access to theomniroute_ccr_retrieveMCP tool. The MCP tool exists atopen-sse/mcp-server/tools/compressionTools.ts:404, but it is only reachable when a client connects to OmniRoute as an MCP server, not as a chat provider.CCR (Content-Compression-Retrieve) blindly replaces any non-system message whose content is
>= minChars(default 600) with[CCR retrieve hash=<24hex> chars=<N>]. That includes:{ role: "tool", tool_call_id, content }{ role: "user", content: [{ type: "tool_result", … }] }When the upstream LLM sees these markers in the tool output, it cannot expand them. The agent stalls — the LLM has an opaque placeholder where the actual bash / read / grep result should be, and has no way to recover.
Reproduces after ~50 turns once the context window starts to fill, because the retrieval ramp factor raises the effective
minCharslinearly per prior retrieval and progressively smaller blocks fall into the trap.Fix
Extend
processMessagesinopen-sse/services/compression/engines/ccr/index.tsto skip tool outputs verbatim:CCR still applies to plain user / assistant text blocks, where the LLM can reason about the marker (and where the token savings are meaningful).
Tests
tests/unit/compression/ccr-skip-tool-outputs.test.ts(4 cases):role:"tool"(OpenAI) is not compressedrole:"user"with onlytool_resultparts (Anthropic) is not compressedtool_resultpart is also skippedAll 57 pre-existing CCR tests still pass (
npm run test:allnot needed for this surgical change).Risk
Low. The fix is additive skip — it only widens the existing
role:"system"skip rule to two additional message shapes. Other engines in the stack (session-dedup,rtk,lite,caveman,ionizer,headroom, etc.) are untouched.Out of scope (future work)
omniroute_ccr_retrieveinto the upstream provider's function-calling schema.These are larger refactors; the minimal fix here stops the agent-loop breakage now.
Reproduction