a9fa56ee1d
The survey was written to the shared checkout and never committed, so it would have been lost the way WI-5 was. Both files land together: the survey as `reference`, the review beside it. Every claim was checked against the code rather than taken on trust, and the result splits cleanly: the two recommendations about small modules were read from source and are accurate (`Source` really is a Literal at proficiency.py:61 with exactly the six members quoted; the three do-not-refactor calls are all correct). The two about the largest files were not. `context_prune.py` has no `pinch()` function -- the survey names one, and quotes the file's 455 lines as that function's length. Its proposed `_summarize_prose()` would be inventing a feature: the pinch stage trims tool results and nothing else, deliberately. The real finding underneath is `prune_context` at 216 lines, which is the extract candidate. The `config.py` split names six classes that do not exist (`ProfileConfig`, `TiersConfig`, `AllowlistConfig`, `TierOverrideConfig`, `EnergyConfig`, `BalanceConfig`) and finds no home for 18 of the 31 real ones, nor for `load_config`. More to the point, it splits the wrong axis: of 72 validators, eleven hang off `RouterConfig` and cross-check sections against each other, so those 380 lines stay in one file under any split. The counter-proposal is one file, not nine -- lift the 112-line loader out and leave the schema. The package-name collision that split would create (`src/config/` beside the repo-root `config/` data directory, which has no `__init__.py`) was tested in a temp tree rather than argued about: a regular package wins over a namespace portion regardless of path order. Not a blocker, recorded because it is the first thing that looks like one. Also corrected: the survey's `**Status**:` header was neither the bare `Status:` form nor a valid value, so it failed three of `tests/test_plans_declare_status.py` the moment it reached `plans/`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
174 lines
7.5 KiB
Markdown
174 lines
7.5 KiB
Markdown
# 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.
|