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
151 lines
8.9 KiB
Markdown
151 lines
8.9 KiB
Markdown
# Spec: two design decisions deferred out of the context-pruning/framing fix pass
|
|
|
|
Status: done -- session_cache.py
|
|
|
|
**Origin.** The fix pass for
|
|
[`context-pruning-and-framing-review.md`](context-pruning-and-framing-review.md)
|
|
(reviewed in
|
|
[`context-pruning-and-framing-fixes-review.md`](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
|
|
|
|
> **Settled** — see
|
|
> [`session-classification-cache-ttl.md`](session-classification-cache-ttl.md).
|
|
> Invalidation (§"Design questions to settle before implementing", item 2)
|
|
> is a minutes-based config TTL. The rest of this section is kept for the
|
|
> reasoning trail; the successor doc is the one to build from.
|
|
|
|
### 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.
|