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:

```python
# `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.