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

Review: code-analysis-refactoring-opportunities.md

Status: done -- review complete, one recommendation accepted with a changed shape

Reviewed 2026-09-12 against src/ at 1e2e806. Every claim below was checked by reading the code or running it, not by reasoning about it.

---

Verdict

Two of the four recommendations were written against the code. Two were not, and both of those concern the largest files in the analysis -- config.py (1601 lines) and context_prune.py (455). The pattern is worth naming, because it predicts where to look next time: the small modules got accurate assessments and the large ones got plausible-sounding guesses.

#	Recommendation	Verdict
1	Split config.py	Right instinct, wrong plan -- see below
2	Extract context_prune.py helpers	Premise is false; a real version exists
3	Source as Enum	Accurate, and correctly ranked Low
4	Type alias standardization	Accurate, no objection

The "What NOT to Refactor" table is correct on all three counts, verified: cost_score/eco_score really are aliases of normalize_inverted (scoring.py:58-59), and events.py really does hold four module-level singletons (events.py:24-30).

---

1. Splitting config.py

The instinct is sound. The proposed layout is not usable as written.

The proposed structure names six classes that do not exist

config.py has 31 classes. The plan's nine-file tree names thirteen types, of which these six are invented:

Plan says	Reality
ProfileConfig	RoutingProfile
TiersConfig	TieringConfig
AllowlistConfig	does not exist
TierOverrideConfig	does not exist (ContextOverride is the nearest)
EnergyConfig	does not exist (LocalEnergyConfig is the nearest)
BalanceConfig	does not exist
Profile enum	the only enum is FlexPreference

It also finds no home for 18 of the 31 real classes -- Objective, ContextConfig, ProficiencyConfig, EscalationConfig, IterationConfig, FreshnessConfig, PinchConfig, PinchRelevanceConfig, SessionCacheConfig, ExplorationConfig, CircuitBreakerConfig, DatabaseConfig, LocalVisionConfig, LocalComputeConfig, LocalDispatchModel, DispatchProvider, LoggingConfig, DispatchSettingsConfig -- nor for load_config, _merge_overlay or summary_lines.

The part that makes the file long is the part that cannot be split

Measured:

span	lines	what
1 - 1108	1108	30 leaf models, genuinely independent
1109 - 1488	380	RouterConfig and its validators
1489 - 1601	112	load_config, _merge_overlay, summary_lines

config.py carries 72 validators, and eleven of them hang off RouterConfig and deliberately reach across sections: local energy against its tariff, classifier.mode against exactly one cloud primary, gaming mode against a configured cloud classifier, routing.tool_use_category against the real category list, profile names against BUILTIN_PROFILES, default_profile against known profiles, default_provider against configured providers, local-dispatch categories, balance_url against parser and telemetry, and the verifier model against differing hosts.

Those 380 lines stay in one file under any split, because cross-checking sections is what they are for. The plan's stated benefit -- "clearer ownership of each config section" -- is weakest exactly where the file is hardest to read. Splitting moves the easy 1108 lines and leaves the hard 380 untouched.

The name collision is safe -- checked, not assumed

src/config.py becoming src/config/ puts a package next to the repo-root config/ data directory, which has no __init__.py. The service runs from the repo root, so sys.path[0] is '' and the data directory is found first.

Built the exact layout in a temp tree and imported it: a regular package wins over a namespace portion regardless of path order, and config resolved to src/config/__init__.py both with PYTHONPATH=src and with '' forced first. Not a blocker. Recording it because it is the first thing that looks like one.

What to do instead

If this is worth doing at all, the split that pays is not by config section but by kind, and it is one file, not nine:

• move load_config, _merge_overlay and summary_lines into src/config_loader.py, leaving config.py as schema only.

That is 112 lines out, no package, no __init__.py shim, no import-resolution question, and it separates the two things that actually differ: the schema is declarative and rarely read top-to-bottom; the loader has real control flow (overlay merging, ROUTER_IGNORE_LOCAL_CONFIG, backup rotation) and is read when something goes wrong.

The nine-way split can follow later if the leaf models keep growing. It should not go first: a 60-file import surface re-exported through config/__init__.py is a lot of churn to buy navigation in the third of the file that was never the problem.

---

2. Extracting context_prune.py helpers

There is no pinch() function. The module's public functions are prune_context, trim_candidates, order_by_relevance, estimate_tokens and extract_text, plus four private helpers already extracted (_text_only, _tool_name, _first_user_turn_indexes, _with_text).

"455 lines" is the file's length, quoted as a function's.

Of the three proposed extractions, _summarize_prose() would be inventing a feature: the pinch stage trims tool results and nothing else, on purpose -- CLAUDE.md states it, and there is no prose-summarization path to extract.

There is a real finding underneath. prune_context runs lines 240-455 -- 216 lines, easily the longest function in the module and the genuine extract candidate the analysis was reaching for. Anyone acting on item 2 should start there, and should read the protected_max_chars note in docs/pinch.md first, since prefix protection is the part with a documented gotcha.

---

3 and 4

Source really is a Literal at proficiency.py:61, and the six members in the plan match the source exactly -- this one was read, not guessed.

Worth pricing before anyone does it: the strings appear in 14 comparison sites across src/ and 32 across tests/, against only 3 type-annotation sites. A plain Enum makes every one of those comparisons silently False. If it is done, it has to be class Source(str, Enum), and the payoff is small enough that "Optional" is the right ranking.

Item 4 is unobjectionable and equally optional.

---

Corrections to the document's own figures

• "897 tests" -- the suite is 1912 as of this review. CLAUDE.md already said 1155, so 897 predates even that. Anyone citing this document's numbers should re-count.
• "src/config.py 1300+" -- 1601. Directionally fine.
• Every other line count in the Files Analyzed table is exact.
• The document did not pass tests/test_plans_declare_status.py: its header read **Status**: Analysis complete, recommendations ready, which is neither the required bare Status: form nor one of the five valid values. Corrected in place to Status: reference, which is what it is -- a survey, not a work plan. Three tests were failing on it the moment it landed in plans/.

---

Unrelated, found while checking

config/ holds 47 config.local.yaml.bak.<epoch> files. The persisted-config write path backs up on every admin write and never prunes. Harmless today, but that directory is the one place holding the irreplaceable overlay, and a backup set nobody can date-order by eye is not much of a safety net. Worth a retention cap.