# 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`](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:

```python
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.