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

Review: fixes for context-pruning-and-framing-review.md

What it was reviewing: the working-tree diff (7 files, unstaged, not yet committed) implementing fixes for the 11 confirmed bugs and 4 cleanup items from context-pruning-and-framing-review.md, done by opencode driven through the router. Reviewed by reading the full diff against each numbered finding, re-deriving the fix logic by hand, and reproducing behavior directly against context_prune.py and dispatcher.py rather than trusting the diff's comments. Full suite: 544/544 passing (up from 529; 15 new regression tests, one per fixed finding).

Verdict: 10 of 11 bugs correctly fixed and verified; 1 new regression found in the process

Fixed and verified by direct reproduction

#	Original finding	Fix	Verified
1	_previous_context returned raw tool output as classifier context	Now walks back to the nearest role == "assistant" message only, skipping system/user/tool, and reuses context_prune.extract_text instead of a second copy of the block parser (also fixes #13)	test_previous_context_excludes_tool_role_messages — read directly, correct
2	Same function returned the system prompt on a session's first turn	Same fix as #1 (system role is no longer eligible at all)	test_previous_context_excludes_system_prompt — correct
3	Long-tool-result trim went negative and grew the message when max_summarize_chars < 3000	Two layers: config.py now validates max_summarize_chars >= 3000 at load; context_prune.py also defensively checks trimmed > 0 and len(elided) < len(text) before using the elided form, falling through to the placeholder otherwise, so a direct call bypassing config can't hit it either	Reproduced by hand: max_summarize_chars=100, 1200-char input → output is strictly shorter, tokens_saved >= 0, no negative marker. Matches test_max_summarize_chars_below_3000_never_grows_or_goes_negative
4	Routing/tier/cost decided on unpruned tokens; pinch's savings never reached the decision	prune_context now runs once inside the wants_routing block, before measured = estimate_prompt_tokens(send_messages, ...); the already-pruned send_messages is reused at dispatch, and the passthrough path (which never goes through wants_routing) still prunes on its own. Confirmed this doesn't double-prune and doesn't touch messages in place (only send_messages is reassigned; session_directory(messages), _last_user_text(messages), and _run_local_vision(messages, ...) all still read the original, full conversation)	Read the full function end-to-end to confirm the no-double-prune and no-mutation properties; matches test_pruned_list_measures_smaller_than_the_raw_conversation and test_pinch_disabled_leaves_the_measured_size_unchanged
5	extract_text read image_url blocks as empty, undercounting image-bearing messages	Now contributes the block's url string; nested tool_result.content lists are flattened recursively too	Correct as far as counting goes — but see the new finding below, this fix combined with #11's fix produces a different bug
7	context=prev_context was dead on the measured-context reroute route() call	Parameter removed from that call, with a comment explaining the override branch never reads it	Confirmed by re-reading route()'s branch condition; no behavior change, just removes a misleading dead arg
8	dropped stat/docstring claimed removal; code always replaced content in place	dropped counter removed; both trim paths now fold into summarized; docstring updated to say "trimmed or summarized (never removed)"	test_stats_use_summarized_and_length_is_always_preserved — correct
9	classifier.system_prompt unconditionally described the "Context:"/"Message:" framing even when context_framing: false	The framing instruction was removed from the static system prompt entirely and is now appended to _classifier_user_content's output only when context is actually present, and worded generically enough (doesn't reference the specific labels) to be correct under both framing modes	Read both config.yaml and _classifier_user_content; the instruction now travels with the content that justifies it rather than being unconditional boilerplate
10	Zero user-role messages collapsed protected_from to len(messages), protecting nothing — even the newest tool result was prunable	New branch: when there's no user message, protect the trailing keep_last_turns tool results instead	Reproduced by hand with 5 tool results / keep_last_turns=2: the 2 most recent survive verbatim, the 3 older ones are trimmed. Matches test_zero_user_messages_protect_newest_tool_result
12	_turn_of() was dead code	Removed	Confirmed, no remaining references
13	_previous_context duplicated extract_text's block-parsing logic	Now imports and calls context_prune.extract_text directly	Confirmed via the #1 fix above
14	classify()'s outer if not context duplicated a check _classifier_user_content already makes	Outer branch removed; classify() now calls _classifier_user_content unconditionally	test_classify_user_content_invariant_when_no_context — correct
15	Pinch's 4 defaults declared independently in context_prune.py, config.py, and config.yaml	context_prune.py's module-level DEFAULT_* constants removed; prune_context's default args now read from PinchConfig.model_fields[...].default, one source of truth	Confirmed — CHARS_PER_TOKEN itself is still a separate mirrored constant (unaddressed, but this was always the minor half of #15)

Finding #11 (structured content silently flattened to a string, dropping non-text blocks) is also fixed in the sense that it was scoped — image_url and other non-text blocks now survive trimming instead of being discarded — but the fix interacts badly with #5's fix, below.

New finding: image-bearing tool results aren't actually shrunk, and the stats lie about it

context_prune.py — extract_text (39-62, image_url branch) + the trim branch (155-175) + _with_text (91-114)

Fixing #5 (count image_url bytes into the size estimate) and #11 (never drop non-text blocks) independently make sense, but composed, they produce a case neither fix's own test covers: a tool result whose image_url block is large enough to actually matter.

extract_text joins every block — text and the image's raw url string — into one combined string. That combined string is what gets head+tail-sliced for the elided/placeholder replacement. _with_text then writes that replacement into the message's text block only, and copies every non-text block (the image_url one) through completely unchanged — full size, untouched. Net effect for an image-bearing tool result: the image (almost always the dominant contributor to size) is never actually reduced, while the tokens_saved stat is computed from the combined length including the image bytes, so it reports a large "savings" that didn't happen. Reproduced directly:

huge_b64 = "A" * 200000
messages = [
    {"role": "user", "content": "describe this"},
    {"role": "assistant", "content": None, "tool_calls": [{"id": "1"}]},
    {"role": "tool", "name": "screenshot", "content": [
        {"type": "text", "text": "here is the screenshot"},
        {"type": "image_url", "image_url": {"url": f"data:image/png;base64,{huge_b64}"}},
    ]},
    {"role": "user", "content": "now what"},
]
pruned, stats = prune_context(messages, budget_tokens=10, keep_last_turns=0)
# stats == {'pruned': True, 'original_tokens': 66687, 'final_tokens': 67690,
#           'tokens_saved': 65671, 'summarized': 1}

final_tokens (67690) is larger than original_tokens (66687) — the message got bigger, not smaller — while tokens_saved claims 65,671 tokens saved. The image_url block comes out with its full 200,022-char url untouched, and the text block now reads "here is the screenshot data:image/png;base64,AAAA...[187,022 chars trimmed...]AAAA" — a raw fragment of the base64 payload, sliced at an arbitrary byte boundary, now sitting in a text field. This is worse than a no-op: it doesn't reduce what's shipped upstream, it corrupts the text block with base64 noise, and it actively misreports the stat the whole feature exists to produce.

The existing regression test for #11 (test_structured_tool_result_keeps_image_block_when_trimmed) doesn't catch this because its fixture image is 4 characters ("AAAA") — small enough that the leakage and false accounting are present but invisible. Any real screenshot or image tool result (tens of KB to MB of base64) would hit this.

The fix likely belongs in extract_text/the trim logic together: image_url blocks should count toward the size estimate (that part of #5 is correct — undercounting was the original bug) but should not be included in the text that gets sliced for the elided/placeholder replacement, and should be excluded from (or separately accounted in) whatever text ends up in the text block. Whether an oversized image itself should also become a trim candidate (a placeholder replacing the image_url block, not just the text) is a design question worth deciding explicitly rather than falling out of two unrelated fixes' composition.

Not fully resolved (matches what was flagged as deferred)

Original finding #6 — TaskRequest.context's dual purpose (docs/code paste for /route//dispatch vs. the new conversational-continuation use in chat_completions) — got a real, useful partial fix: the framing instruction is no longer unconditionally injected into every classify() call (that part is #9, now fixed), and the field's dual use is now documented on TaskRequest.context itself. But the underlying semantic question is still open: a /route or /dispatch caller who passes context=<pasted docs/code> (the field's original, documented purpose) still gets the classifier told "a short follow-up continues the prior turn, classify by the CONTEXT's complexity" — a rule written for conversational continuation, appended regardless of which of the two meanings this particular caller's context actually carries. This tracks with what was flagged as a deferred design decision rather than a missed fix.

Recommendation

Don't commit yet — the image/large-tool-result finding above is a real, reproducible regression (not a pre-existing issue; it's new from this round's own fixes), and it directly undermines the stat this feature exists to produce. Route it back for another pass; everything else here is solid and doesn't need to be touched again. The two deferred design items are covered in a separate forward-looking spec rather than this after-the-fact report.

Update: the image finding is fixed, committed as 5f7716e

Verified directly against the same repro used to find it: a 200,000-char fake base64 payload in a tool result's image_url block now comes out stubbed to "[image omitted]", the adjacent text block reads a clean "[screenshot: result omitted]" placeholder with no base64 fragment in it, and stats["final_tokens"] (20) is now correctly below stats["original_tokens"] (66,687) rather than above it.

The fix separates sizing from replacement-building: extract_text (sizing, unchanged from the prior fix) still counts image_url bytes so an image-heavy result correctly triggers pruning; a new _text_only helper excludes image_url entirely and is what actually gets head/tail-sliced for the replacement, so no raw base64 can end up in a text block. _with_text now also stubs any image_url block's url to a short marker whenever a message is trimmed, so the image itself — usually the dominant contributor to size — actually shrinks rather than passing through untouched. tokens_saved no longer accumulates per-message; it's now max(orig_tokens - final_tokens, 0) over the whole list's real before/after token estimate, so the stat can't structurally disagree with the sizes it's computed from again, regardless of what any future branch does inside the per-message loop.

Covered by a dedicated regression test, test_large_image_tool_result_actually_shrinks_and_stats_are_honest, which asserts the shrink, the stat's internal consistency, and that no "base64" substring survives in any text block. Full suite: 545/545.

One cosmetic-only leftover: _with_text's docstring/comment still says a second text block in the same message's content list "is folded into new_text" — verified directly that it's actually dropped, not folded (two text blocks in, one comes out). Not worth a fix on its own; real tool messages don't carry more than one text block in practice.