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