143 lines
8.6 KiB
Markdown
143 lines
8.6 KiB
Markdown
# Spec: two design decisions deferred out of the context-pruning/framing fix pass
|
|
|
|
**Origin.** The fix pass for
|
|
[`context-pruning-and-framing-review.md`](../code_reviews/context-pruning-and-framing-review.md)
|
|
(reviewed in
|
|
[`context-pruning-and-framing-fixes-review.md`](../code_reviews/context-pruning-and-framing-fixes-review.md))
|
|
explicitly declined two items as design decisions rather than confirmed
|
|
bugs: the `TaskRequest.context` dual-use ambiguity (review finding #6,
|
|
partially mitigated but not resolved), and classifying once per session
|
|
instead of once per message (an existing item on the project's own "what's
|
|
NOT built yet" list). Both are forward-looking — no bug is being reported
|
|
here, no code changes accompany this document.
|
|
|
|
---
|
|
|
|
## 1. `TaskRequest.context` carries two unrelated meanings
|
|
|
|
### The problem
|
|
|
|
`context` was originally, and is still documented as, "assembled context
|
|
(docs/code) to send with the task" — a `/route`/`/dispatch` caller pastes
|
|
reference material alongside a task description. `chat_completions` now
|
|
also uses the same field to carry the prior conversational turn
|
|
(`_previous_context`), so a short follow-up like "Yes" inherits that turn's
|
|
complexity instead of being classified as trivial in isolation.
|
|
|
|
The current fix (moving the framing instruction from the static system
|
|
prompt into `_classifier_user_content`'s output, appended only when
|
|
`context` is non-empty) narrowed the blast radius — the instruction no
|
|
longer reaches every classify() call, only ones that actually supply
|
|
`context` — but it didn't resolve which of the two meanings a given
|
|
`context` value has. A `/route` caller pasting 800 lines of Django code
|
|
under `task="Refactor this"` gets the same "a short follow-up continues the
|
|
prior turn, classify by the CONTEXT's complexity" instruction that exists
|
|
for the conversational case.
|
|
|
|
### Why this might not need fixing
|
|
|
|
The instruction's general principle — "short task + large/complex context
|
|
implies a non-trivial task" — is arguably a reasonable heuristic for the
|
|
docs-paste case too, even though it was written for conversational
|
|
follow-ups. It has never been measured against real `/route`/`/dispatch`
|
|
traffic with `context` set. This project's own stated epistemics apply
|
|
directly here: `POST /outcome` is "the only ground truth," and several
|
|
sections of the README describe correcting an assumption only after
|
|
measuring it, not before. Speculating about which framing is "more
|
|
correct" without a measurement repeats the mistake the project has already
|
|
named and moved past.
|
|
|
|
### Options, if it turns out to matter
|
|
|
|
| Option | Sketch | Trade-off |
|
|
|---|---|---|
|
|
| A. Decouple the mechanisms | Keep `TaskRequest.context` as the public docs/code field, untouched. Give `chat_completions`'s conversational-continuation signal its own internal path — e.g. `classify()` gains a private `prior_turn: Optional[str]` parameter distinct from `context`, so the instruction is only ever built for genuine conversational continuations | Cleanest semantically; requires touching `classify()`'s signature and both call sites; `/route`/`/dispatch` behavior is provably unaffected |
|
|
| B. Tag the field | Add a `context_kind: Literal["reference", "prior_turn"] = "reference"` field to `TaskRequest`; only `chat_completions`'s internal calls set `"prior_turn"`; the instruction is only appended for that kind | Smaller diff than A; adds a field to the public request model that external callers never need to know about |
|
|
| C. Leave as-is, measure | No code change. Watch `route_decisions` (already logs `task_category`/`task_tier`/`source` per request) for `/route`/`/dispatch` calls that supply `context` and see whether their tier/category looks skewed relative to before this change | Zero engineering cost; consistent with the project's own "measure before correcting" pattern; only viable if `/route`/`/dispatch`-with-`context` traffic is common enough to be observable |
|
|
|
|
**Recommendation:** C first. This codebase already has the instrumentation
|
|
(`route_decisions`, `feedback.py` folding in outcomes) to tell whether this
|
|
is a real problem instead of a theoretical one, and the existing pattern in
|
|
this project — cost-vs-eco, tier-from-price, the whole classifier-model
|
|
swap — is "measure, then fix what the measurement shows," not "fix what
|
|
looks fishy." If `/route`/`/dispatch`-with-`context` traffic turns out to
|
|
be rare or nonexistent, this is not worth A's or B's added surface at all.
|
|
|
|
---
|
|
|
|
## 2. Classify once per session, not once per message
|
|
|
|
### The problem, restated from the README
|
|
|
|
> ~10s of local overhead on every message is a real tax for an interactive
|
|
> agent... Still unaddressed: classify once per session rather than per
|
|
> message, cache by prompt hash, or skip classification for short prompts.
|
|
|
|
With the local `mistral-nemo` classifier this is now ~1.7s/call
|
|
(§"The classifier is the latency floor"); with a cloud classifier
|
|
(measured against `deepseek-v4-flash`) it's ~1.0s. Either way, every single
|
|
turn in a long agent session pays this again, even though the session's
|
|
*category* (coding_general, debugging, etc.) rarely changes turn to turn —
|
|
what changes is mostly the token count, which `chat_completions` already
|
|
measures directly via `estimate_prompt_tokens` and doesn't need the
|
|
classifier for.
|
|
|
|
### The mechanism already half-exists
|
|
|
|
`route()`'s override branch (`dispatcher.py:673`) already skips
|
|
`classify()` entirely whenever `task_category`, `task_tier`, and
|
|
`required_context_tokens` are all supplied — this is exactly what the
|
|
measured-context reroute (fixed in review finding #4) uses today, just
|
|
within a single request. Session-level caching is the same mechanism
|
|
applied across requests: classify once, store `(task_category, task_tier)`
|
|
keyed by session, and on every later turn in that session call `route()`
|
|
with the cached category/tier plus a **freshly measured**
|
|
`required_context_tokens` (which is cheap — pure token counting, no model
|
|
call) — landing on the override branch and skipping the classifier
|
|
round-trip entirely.
|
|
|
|
### Design questions to settle before implementing
|
|
|
|
1. **Session identity.** `session_fingerprint`/`session_directory`
|
|
(dispatcher.py) already derive a session identity from message content
|
|
for observation purposes. Whether that's the right key for a
|
|
*classification* cache (vs. e.g. a client-supplied session id, if
|
|
opencode's protocol carries one) needs checking — a cache keyed on the
|
|
wrong signal either misses constantly (no benefit) or collides across
|
|
genuinely different sessions (wrong category persists into unrelated
|
|
work).
|
|
2. **Invalidation.** A session's task can genuinely change category mid-way
|
|
(debugging turns into a docs-writing turn turns into refactoring). Pure
|
|
"classify once, cache forever" risks staleness. Candidate triggers to
|
|
re-classify: a large jump in `required_context_tokens` between turns (a
|
|
proxy for "something new started"), a fixed number of turns (e.g.
|
|
re-classify every 20), or a TTL. This needs the same "measure before
|
|
deciding" treatment as everything else in this project — a cheap thing
|
|
to instrument via `route_decisions.source` (add a `"cached"` value
|
|
alongside `"classifier"`/`"override"`/`"fallback"`) and watch category
|
|
drift over real sessions before picking a policy.
|
|
3. **Interaction with escalation and retries.** `apply_escalation` currently
|
|
runs on every fresh classification. A cached category/tier bypasses it
|
|
entirely on cache-hit turns — need to decide whether escalation state
|
|
should also be cached per-session or re-evaluated each turn (it's cheap,
|
|
pure Python, so probably always re-evaluate rather than cache).
|
|
4. **Storage.** In-memory dict keyed by session identity is the obvious
|
|
starting point (matches the process lifetime of the dispatcher; a
|
|
restart just means the next turn in every active session re-classifies
|
|
once, which is a safe failure mode) — no new persistence layer needed
|
|
unless multi-process deployment becomes a requirement.
|
|
|
|
### Recommendation
|
|
|
|
Worth building — the latency case is strong and the mechanism is a small
|
|
extension of code that already exists (the override branch) rather than a
|
|
new one. But settle invalidation policy (§2) with a measurement pass first,
|
|
the same way `context_framing`'s default-on-and-measure and the classifier
|
|
model swap were each decided by running both and comparing, not by
|
|
argument. A reasonable first cut: cache with no re-classification, ship
|
|
behind a config flag (default off, matching every other new-and-unproven
|
|
knob in this project — `pinch.enabled`, `min_tool_proficiency`), watch
|
|
`route_decisions` on real sessions for category drift, then decide whether
|
|
any invalidation trigger is actually needed or whether "classify once,
|
|
never again" is good enough in practice.
|