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

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

Status: done -- review of shipped work

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

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)

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.