6krrt/plans/code-analysis-refactoring-opportunities.md

Four small fixes, scoped down from the refactoring survey

Status: planned -- decision-complete; execute in order, each item lands on its own

Superseded scope. This file used to hold a four-item refactoring survey of src/. That survey was reviewed against the code (review-code-analysis-refactoring-opportunities.md) and two of its four recommendations rested on things that are not in the codebase -- a pinch() function that does not exist, and six config.py classes that do not exist. The survey is kept in git history; do not execute it.

What replaces it is the work that survived the review, plus three defects found while doing it. Nothing here is a refactor for its own sake: items 1-3 are live defects with a reproduction, and item 4 is the one structural change the review endorsed.

Intended executor: opencode (Atlas), directly. These are small and independent; no Prometheus plan artifact is needed beyond this file. Land each item as its own commit so any one can be reverted alone.

Every figure below was measured on the live database and the live service at 1e2e806 on 2026-09-12. Re-check before trusting any of them.

---

Item 1 -- the TUI renders the literal string "None" in three cells

Priority: high (visible defect, one-line fix, already has a precedent in the same function)

src/tui.py:763-775 builds each decision row by wrapping every value in str(). Three of those columns are nullable, so a NULL renders as the literal None:

cell	source column	NULL rows today
category	task_category	4
tier	task_tier	4
ctx	required_context_tokens	4

The rows are kind='passthrough', which is why this shows up now: passthrough decisions do not carry a classification.

The fix is already written two lines below, for the column someone hit first:

# `or ""` so an absent profile is blank, never the literal "None".
str(r.get("profile") or ""),

Apply the same treatment to category, tier and required_context_tokens. Do not touch selected: tui_model.py:218 already maps it to "none" via or "none", deliberately, and that is a different string from the Python repr.

Recorded previously in .omo/notepads/tui-overhaul/issues.md -- close that note out as part of this item.

Verify: add a row to the existing TUI table test asserting that a decision with NULL task_category / task_tier / required_context_tokens renders empty cells and that the string "None" appears nowhere in the rendered row.

---

Item 2 -- harden the demand-ceiling comparison, and correct CLAUDE.md

Priority: high (the documented safety claim is now false)

CLAUDE.md says of the demand-ceiling TypeError:

Latent only: zero such rows exist today, checked on the live DB.

That is no longer true. There are 4 rows with NULL required_context_tokens as of 2026-09-12.

It has not fired, and the plan must not claim it has. metrics.py:1433-1441 filters kind = 'chat' AND task_tier = ?, and all four NULL rows are kind='passthrough' with a NULL task_tier, so none reach _percentile. /metrics returns 200. Two filters are the only thing standing between the current data and the crash.

Two changes:

1. src/metrics.py -- make _percentile (line 46) and the three required_context_tokens collection sites NULL-safe. Filter NULLs out in SQL (AND required_context_tokens IS NOT NULL) rather than in Python: the MAX(...) sites at lines 1374, 1382, 1406, 1644, 1652, 1670 already ignore NULLs the way SQL aggregates do, so filtering in the one row-returning query at line 1436 makes every site consistent. A NULL is "this request did not state a context requirement", which is not a zero and must not be averaged in as one.
2. CLAUDE.md -- replace the "zero such rows exist today" sentence with what is actually true: NULL rows exist, they are kind='passthrough', and the kind/task_tier filters are what keep them out of the comparison.

Verify: a test that inserts a kind='chat' decision with NULL required_context_tokens at a tier that has a ceiling, then calls the warning function and asserts it returns without raising. That test must fail before the fix.

---

Item 3 -- cap the config backup retention

Priority: medium (unbounded growth in the directory holding the irreplaceable file)

config/ holds 45 config.local.yaml.bak.<epoch> files. Two write paths create them and neither prunes:

• src/admin.py:467-470 -- config.yaml.bak.<epoch> (0 on disk today)
• src/admin.py:587-591 -- config.local.yaml.bak.<epoch> (45 on disk today)

config.local.yaml is gitignored and holds the live classifier settings, so this is the one directory where backups genuinely matter -- which is the argument for making them legible, not for keeping every one forever.

Add a retention cap after each successful write: keep the newest N (N = 10, as a named module constant, not a literal), delete older ones, matching only <name>.bak.<digits> so nothing else in config/ can be caught. Deleting a backup must never fail the write -- wrap the prune so an error is logged and swallowed; the write has already succeeded by that point and reporting it as failed would be worse than the clutter.

Also worth fixing while in there: the filename is int(time.time()), so two writes in the same second silently overwrite each other's backup. The existing files show consecutive-second writes (...877, ...878), so this is reachable. Either widen to a monotonic counter or accept it and say so in a comment -- decide and record which.

Verify: a test that performs 15 overlay writes and asserts exactly 10 backups remain, that they are the 10 newest, and that no non-matching file in the directory was touched.

---

Item 4 -- move the loader out of config.py (OPTIONAL, do last)

Priority: low, and more expensive than the review first estimated.

The review proposed lifting _merge_overlay, load_config and summary_lines (src/config.py:1489-1601, 112 lines) into src/config_loader.py, leaving config.py as schema only. The reasoning stands: the schema is declarative and rarely read top to bottom, while the loader has real control flow (overlay merging, ROUTER_IGNORE_LOCAL_CONFIG, backup rotation) and is read when something has gone wrong.

The cost was under-stated and this is the correction. load_config / summary_lines are imported by 49 files (9 in src/, 39 in tests/, plus baseline_report.py). The obvious mitigation -- re-exporting from config.py -- does not work: config_loader must import RouterConfig from config, so a re-export in the other direction is a module-level import cycle.

So this item is genuinely "move 112 lines and update 49 import statements". The edit is mechanical and the test suite covers it completely, but it is churn bought for readability alone.

Do this only if items 1-3 land clean and there is appetite. If it is skipped, say so and close the item rather than leaving it open -- an indefinitely-deferred refactor in a plans file is noise.

---

Explicitly out of scope

Carried over from the review, so nobody re-derives them:

• Splitting config.py by config section (the survey's nine-file tree). Of 72 validators, eleven hang off RouterConfig and cross-check sections against each other; those 380 lines stay in one file under any split. The proposal moved the easy 1108 lines and left the hard third untouched.
• Source as a plain Enum. The strings appear at 14 comparison sites in src/ and 32 in tests/, against 3 annotation sites. A plain Enum makes all of them silently False. If ever done, it must be class Source(str, Enum).
• Extracting _summarize_prose() from the pinch stage. There is no prose summarization; the stage trims tool results only, on purpose. prune_context at 216 lines (context_prune.py:240-455) is a real extract candidate, but it is the money path and has a documented protected_max_chars gotcha in docs/pinch.md. Not now.

---

Definition of done

• Items 1-3 each landed as their own commit, each with the test described.
• CLAUDE.md's demand-ceiling paragraph reflects the live database.
• .omo/notepads/tui-overhaul/issues.md note closed.
• Item 4 either done or explicitly closed as skipped.
• Full suite green (1918 at the time of writing) on Python 3.10 and 3.14.
• No change to config/config.local.yaml -- md5sum it before and after.