6krrt/code_reviews/context-pruning-and-framing-review.md

# Review: fbcc636, "context-aware framing and relevance-based context pruning"

**What it was reviewing:** the two features ported from the MIT-licensed
`alexrudloff/llmrouter` project — context-aware classifier framing
(`_previous_context` / `_classifier_user_content` in `dispatcher.py`) and
relevance-based context pruning (`context_prune.py`, "pinch"). Reviewed with
`/code-review` at xhigh effort (10 finder angles + 1-vote verify + a gap
sweep), then spot-verified directly: read every file involved, and
reproduced the two most severe findings against the actual `prune_context`
function rather than trusting the description.

## Verdict: full suite is green (529/529), but that's not the same as correct

`pytest` passes clean, including the 10 new `tests/test_context_prune.py`
cases. None of those cases exercise the shapes that break, though: a
`max_summarize_chars` below ~3000, a conversation with zero user messages,
tool content as `image_url` blocks, or the dispatcher-level interaction
between `_previous_context` and `route()`. The suite passing means the
happy path works, not that the port is safe on real agent traffic — which is
exactly the traffic this router's own README says is dominant (tool calls,
long sessions, ~92% cache hits on huge prompts).

Both features are **off by default** (`pinch.enabled: false`,
`classifier.context_framing: true` — framing is on, pruning is off), so
nothing here is live yet. But `context_framing` defaulting **on** means the
framing bugs (#1, #2 below) are already affecting every routed
`/v1/chat/completions` call today.

## Confirmed bugs — verified directly, not just reported

### 1. `_previous_context` feeds the classifier raw tool output, not "the prior turn" — and it's the common case, not an edge case
`dispatcher.py:1560-1588`

```python
for i in range(len(messages) - 2, -1, -1):
    if messages[i].get("role") == "user":
        continue
    ...
```

Only `role == "user"` is skipped while walking backward from `messages[-2]`.
`role == "tool"` is not. In the standard OpenAI tool-loop shape —
`assistant(tool_call) -> tool(result) -> user("ok fix it")` — `messages[-2]`
*is* the tool message, so `_previous_context` returns the raw tool-result
payload (file contents, grep output, JSON), truncated to 200 chars, as
`"Context:"` for the classifier. This isn't a malformed-conversation edge
case; it's what every agent client running a tool loop produces on its very
next turn. The function's own docstring says it exists to let the
classifier "inherit the [prior] turn's complexity" — it's inheriting
arbitrary tool output instead.

### 2. Same function returns the system prompt as "previous context" on a session's first turn
`dispatcher.py:1570-1572`

For `messages = [system, user]`, the backward scan starts at index 0 (the
system message), which isn't `role == "user"`, so it's not skipped — its
first 200 chars come back as the "prior turn." Every fresh session's first
message gets classified with `"Context: <fragment of the system prompt>"`
prepended, which for opencode is ~32K chars of tool definitions.

### 3. Pinch's long-tool-result trim goes negative and *inflates* the message once `max_summarize_chars < 3000`
`context_prune.py:145-153`; `config.py:293-299` (no validator on this field, unlike its two siblings)

```python
head = text[:1500]
tail = text[-1500:]
trimmed = len(text) - 3000
```

This assumes any text reaching this branch is longer than 3000 chars, but
the only guard to get here is `len(text) > max_summarize_chars`, and
`max_summarize_chars` has no lower-bound validator (`budget_tokens` and
`keep_last_turns` both do). Reproduced directly:

```
max_summarize_chars=100, tool content = 800 chars
-> pruned tool content = 1632 chars (grew)
-> stats["tokens_saved"] = -734 (negative)
-> marker literally reads "[...-2,200 chars trimmed...]"
```

Below the default (4000) this can't trigger, but there's nothing stopping
an operator from setting it lower, and when they do, pruning does the
opposite of its job.

### 4. Model/tier selection runs on unpruned tokens; pinch's savings never reach the decision that spends the money
`dispatcher.py` — routing at 1913-1954 vs. `prune_context()` at 2079

`route()` is called (twice — see #7) using `estimate_prompt_tokens(messages, ...)`
on the **full, unpruned** message list, and that's what drives tier
selection, the context-window hard filter, and the cost tiebreak.
`prune_context()` doesn't run until line 2079, well after `decision.selected`
is fixed — it only shrinks the payload actually sent to the model already
picked. So a long tool-heavy session can get routed to a pricier
large-context model based on its pre-pruned size, even though pinch would
have brought the real outgoing request in well under budget. This isn't
wrong on invalid input, it's an ordering bug: pinch's entire stated purpose
(reduce shipped tokens on long sessions) doesn't influence the one decision
where that would save money.

### 5. `extract_text` silently treats `image_url` content blocks as empty
`context_prune.py:39-53`, used for both `orig_tokens` and the per-message trim decision

Only `type == "text"` and `type == "tool_result"` blocks are read; anything
else (including `image_url`) contributes `""`. Two consequences: token
estimates can undercount a session that's actually huge (a tool result full
of base64 image data reads as 0 tokens, so `orig_tokens` may never cross
`budget_tokens` and pruning never triggers), and if pruning does trigger for
other reasons, that same message reads as `len(text) == 0 <= max_summarize_chars`,
so the "only replace if the placeholder is shorter" check (`23 < 0`) is
false and the giant blob is left completely untouched while smaller
genuine text results nearby get trimmed.

### 6. `TaskRequest.context` now has two incompatible meanings sharing one field
`dispatcher.py:151-153` (field docstring: `"Assembled context (docs/code) to send with the task"`) vs. the new use in `chat_completions` (prior conversation turn) vs. `config.yaml:395-398` (system prompt instructions written for the second meaning)

`/route` and `/dispatch` callers have always been able to pass `context` as
pasted docs/code (`dispatch_endpoint` splices it verbatim into a system
message). The classifier's system prompt was changed globally to say
"classify by the CONTEXT's complexity, treating a short message with
complex context as inheriting that complexity" — a rule written for the
conversational-continuation case, but it now applies unconditionally to
every existing `context=<pasted code>` caller too, since it's the same
field and the same prompt. Not obviously wrong, but untested for that
existing use and not called out anywhere as a behavior change to it.

## Confirmed via reproduction — edge cases in pruning itself

### 7. `context=prev_context` is dead weight on the second (measured-context) `route()` call
`dispatcher.py:1942-1954` vs. `route()`'s branch condition at line 673

`route()` only reads `req.context` inside the `classify()` branch, which is
skipped whenever `task_category`, `task_tier`, and `required_context_tokens`
are all supplied together — which the reroute at 1942 always does (it
copies the first decision's category/tier and sets
`required_context_tokens=measured`). So `context=prev_context` on that call
is passed and never read. Harmless today since the first `route()` call
already consumed it, but it means a future fix to `_previous_context` (#1/#2
above) would silently not apply here, and there's nothing marking the
parameter as inert.

### 8. `dropped` doesn't mean dropped
`context_prune.py:100-107` (docstring/stat name) vs. `133-144` (actual behavior)

The docstring and `config.yaml` both say short old tool results are
"dropped entirely," and the counter is literally named `dropped`, but the
code never removes a message from the list — it always replaces `content`
with a placeholder string in place. `len(pruned) == len(messages)` always,
regardless of `dropped`. Any code (or test) later written against the
documented contract — e.g. `assert len(pruned) == len(messages) - dropped`
— would be wrong on every request that drops anything.

### 9. Stale prompt instructions when the framing opt-out is used
`config.yaml:395-398`

The classifier's `system_prompt` unconditionally describes the
`"Context: <prior>\n---\nMessage: <current>"` label format, but that layout
is only actually produced when `classifier.context_framing: true`. Set it
`false` (the documented way to get the legacy `"task\n\n--- context ---\ncontext"`
layout) and the classifier still receives instructions describing a format
it will never see.

### 10. Zero user messages -> the recency guard protects nothing
`context_prune.py:109-115`

Reproduced: with no `role == "user"` message anywhere in the conversation,
`num_protected_turns` collapses to 0 and `protected_from = len(messages)`,
which no real index ever reaches — so the "always keep if `i >= protected_from`"
branch never fires. The single most recent tool result (the one the next
turn actually needs) becomes eligible for trimming, same as the oldest one:

```
messages = [system, assistant(tool_call), tool(20000 chars)]
-> the only tool result gets summarized down, despite being the newest
```

### 11. Trimming silently flattens structured content to a plain string
`context_prune.py:141, 152`

Both trim branches do `{**msg, "content": <str>}` unconditionally, even
when the original `content` was a list of blocks (`[{"type": "text", ...},
{"type": "image_url", ...}]`). A tool result that mixes text and an image
part loses the image permanently the first time it ages past the protected
window — not "trimmed," just gone, with no signal that anything
non-text was there.

## Cleanup — lower severity, no behavior change

| # | Location | Issue |
|---|---|---|
| 12 | `context_prune.py:73-80` | `_turn_of()` has zero call sites anywhere in the repo — leftover from an earlier design. |
| 13 | `dispatcher.py:1573-1585` | `_previous_context`'s content-block extraction duplicates `context_prune.extract_text()` almost verbatim, despite `dispatcher.py:58` already importing from that module (`prune_context` only). A future change to one won't propagate to the other. |
| 14 | `dispatcher.py:420-428` vs. `468-481` | `classify()`'s `if not context: user_content = task else: user_content = _classifier_user_content(...)` duplicates a check `_classifier_user_content` already makes internally (`if not context: return task`, line 477). The outer branch can be deleted; `classify()` can call `_classifier_user_content` unconditionally. |
| 15 | `context_prune.py:27-31`, `config.py:293-299`, `config.yaml:175-177` | Pinch's four defaults and `CHARS_PER_TOKEN` are each declared independently in two or three places. `context_prune.py`'s own comment admits `CHARS_PER_TOKEN` "mirrors dispatcher.CHARS_PER_TOKEN" rather than importing it. `dispatcher.py` always passes `cfg.pinch.*` explicitly, so the module-level defaults in `context_prune.py` are dead in production. |

## Take for next time

The port kept the right invariant (user/assistant/system messages are never
touched, only tool results) but re-derived the surrounding plumbing instead
of reusing what the codebase already had for it — `_previous_context` is a
second, slightly-different copy of `extract_text`'s block-parsing logic, and
it re-introduces exactly the bug `_last_user_text` next to it was written to
avoid (`_last_user_text` correctly scans for the *nearest* user message
rather than assuming position; `_previous_context` assumes `messages[-2]` is
meaningful). Both new-feature bugs that actually change routing behavior
today (#1, #2) are about that same unguarded assumption: agent traffic
doesn't end tidily on a fresh user turn, and this codebase already knows
that everywhere else it touches messages.