f0ebd83a09
Adds a real-time view of actual routing tasks to the TUI dashboard: backend: - events.py: in-memory decision-event broker (pure stdlib, thread-safe). Bounded ring buffer + fan-out queues. persist_route_decision publishes here after each write so the TUI sees decisions without polling. - dispatcher.py: GET /events/decisions SSE endpoint — replays recent decisions then streams live ones with :heartbeat keepalive. Wired into persist_route_decision's write path. tui: - tui.py: DashboardApp now consumes /events/decisions via a background thread (call_from_thread). Decisions table updates live without waiting for the 5s /metrics poll. New columns: id, kind, category, tier, ctx, selected, est $. Number keys 1-6 cycle panels. - tui_screens.py: DecisionDetailScreen modal — press Enter or e on any decision row to see the full JSON (runner-ups, rejected reason, feature flags, confidence, context size). - tui_model.py: pure data layer extracted from tui.py — build_model, build_category_breakdown, decision_row. Testable without a terminal. - tui_sse.py: background-thread SSE consumer with reconnect. 17 new tests (events broker, SSE endpoint, TUI data model, detail popup, live decision handling). 562 total, all passing. lsp_diagnostics clean.
160 lines
13 KiB
Markdown
160 lines
13 KiB
Markdown
# 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.
|
|
|
|
## 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.
|