3523dcf93e
plans/ held 58 documents and exactly one said whether it was open. The rest
mixed finished work, reviews of shipped work, parked specs and genuinely
pending ones, with nothing distinguishing them, so "how many plans are in
the queue" had no answer short of reading all 58.
Now `grep -H '^Status:' plans/*.md` is the answer:
50 done 3 in progress 2 planned 2 reference 1 parked
Statuses were derived rather than guessed: CLAUDE.md's own built list and
"What's NOT built yet" section, plus checking the subject exists in the
code. A review of work that shipped counts as done -- it records what was
found, it is not a request for anything. `reference` separates the two docs
that are conventions rather than work items (admin-design-standards,
admin-work-framework), which otherwise read as permanently-open plans.
The vocabulary is deliberately five words. A larger one invites "mostly
done" and "blocked-ish", which is how the directory became unreadable.
test_plans_declare_status.py keeps it from rotting: a new plan without a
marker fails, as does an unknown status, one buried below the eighth line,
or an open status with no reason -- "planned" alone is the state that rots,
since nobody can tell later whether it waits on a decision, a dependency,
or just nobody's turn.
Also updates the sweep plan with what landed and what did not, including
that #9 was not a defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
162 lines
13 KiB
Markdown
162 lines
13 KiB
Markdown
# Review: fixes for `context-pruning-and-framing-review.md`
|
|
|
|
Status: done -- review of shipped work
|
|
|
|
**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.
|
|
|
|
## 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.
|