# root-reorg-src-config-plans — plan review Status: done -- src/ and config/ layout Target: `plans/.omo/plans/root-reorg-src-config-plans.md` Round 1 → v1 (315 lines). Round 2 → v2 (430). Round 3 → v3 (434). Round 4 → **v4** (450). --- # Round 4 — v4 (post dual-review) **Verdict: one line to fix, then go.** All three Round-3 findings are applied, and more carefully than asked — V3-1 is guarded in four separate places, and V3-2's "don't over-apply" caveat was carried into an explicit *must NOT do*. The four dual-review edits are all sound. One of them over-corrected. ## V4-1. The concurrency test's `ROOT /` source path does need to change Plan line 279: > `tests/test_admin_config.py:142-186` concurrency test stays unchanged — it passes > explicit `config_yaml` paths directly to `_persist_config_value`, never uses > `base_dir`; its own `config.yaml.bak.*` glob at 182 is correct as-is. That is right about the *destination* and the *glob*, and wrong about one line in between. The block contains: ```python 145: config_yaml = tmp_path / "config.yaml" # destination — STAYS 146: shutil.copyfile(ROOT / "config.yaml", config_yaml) # SOURCE — must move ... 182: backups = list(tmp_path.glob("config.yaml.bak.*")) # glob — STAYS ``` Line 146's `ROOT / "config.yaml"` is the **repo's real config file**, which moves to `config/config.yaml` in this very wave. Left alone, the test dies with `FileNotFoundError`. The rule that actually holds, and is worth stating this way in the plan: > Inside 142-186, anything anchored on `tmp_path` stays (it is an explicit path > handed straight to `_persist_config_value`). Anything anchored on `ROOT` moves > like every other `ROOT /` reference in the suite. **And the safety net is instructed to dismiss it.** Wave 2's new sanity grep (plan line 286) reads: > `git grep -n 'ROOT / "config.yaml"…' tests/` returns **only the expected > concurrency-test sites at `tests/test_admin_config.py:142-186`** (which pass > explicit paths) or nothing at all. Line 146 is precisely the hit that grep would produce, and the acceptance criterion pre-labels it as expected. That turns a catch into a rubber stamp. The criterion should be "returns nothing at all" — full stop. Cosmetic, same area: plan line 275 has an unbalanced quote — `tmp_path / "config" / "config.yaml` — in a line a worker will copy. ## Verified by running it, not by reading it - **The new collection gate is not a false red.** `pytest --collect-only -q | tail -1` emits `733 tests collected in 1.43s` (with ANSI codes, no trailing blank), so `grep -q "733 tests"` → PASS. Worth having checked; `tail -1` guards break this way often. - **The `"status":"success"` gate is achievable and exactly matched.** `feedback.py --dry-run` exits `rc=0` against the live `router.db`, and FastAPI renders `{"status":"success"}` with no space after the colon — so the grep pattern matches literally. Both halves had to be true for that gate to work. - `deploy/README.md` line references are accurate: 12-14 (the unit table), 34-35 (the sed install loop), 94 (the oneshot-buffering note naming `poller.py` and `seed_energy.py`). ## On Metis's second gap "pytest silent zero-collection if `pythonpath=["src"]` is set before `src/` exists" is not a real failure mode — an unimportable test module produces a **collection error**, loudly, not a silent zero; and `testpaths = ["tests"]` still resolves. The added `git mv`-before-pyproject ordering and the count assertion cost nothing and are fine to keep as belt-and-braces. Flagging only so the reasoning isn't reused somewhere it matters. Momus's rollback warning was worth taking, and the pre-flight note now states it correctly: restoring the backup units *alone*, after modules are already in `src/`, crashes — a full revert needs `git reset --hard $PREFLIGHT_SHA` **and** the units. ## On the documented tradeoff Keeping the service up through Waves 1-2 is a reasonable owner's call, and it is now recorded as one rather than left implicit. Two things make the residual risk smaller than it reads: both timers are stopped at pre-flight, so the likeliest trigger is gone; and the exposure is a genuine crash-loop only if something restarts the process in that window. The one path still open is the admin portal's own `/admin/api/restart-service` button — worth simply not touching until Wave 3 lands. ## Go / no-go Fix V4-1 (one line in the plan, plus the grep criterion and the stray quote) and execute. Nothing else is outstanding. --- # Round 3 — v3 **Verdict: two real defects and one ordering fix, all small. Fix them before spending the dual review pass.** All ten claimed v2 fixes are present and correct; I re-verified each. Both new defects are in the same place — `admin.py`'s config backup write — and neither is a consequence of the v2→v3 reordering; they have been carried unnoticed since v1, mine included. ## Must fix ### V3-1. The `admin.py:370` backup-path change is wrong and 500s the Controls save Wave 2 instructs: > `src/admin.py:370` backup path `f"config.yaml.bak.{ts}"` → `f"config/config.yaml.bak.{ts}"` The actual code is: ```python backup = config_path.with_name( f"config.yaml.bak.{int(time.time())}" ) ``` `Path.with_name()` replaces only the **filename component and keeps the parent**. Once `config_path` is `/config/config.yaml`, it already produces `/config/config.yaml.bak.`. **The line needs no change at all.** Applying the instruction gives `with_name("config/config.yaml.bak.123")`, and `with_name` rejects any argument containing a separator. Verified on this repo's venv (Python 3.14.6): ``` unchanged -> /repo/config/config.yaml.bak.123 v3 change -> RAISES ValueError Invalid name 'config/config.yaml.bak.123' ``` That raises out of `_persist_config_value`, so **every persisted config edit from the Controls page returns 500** — the feature that just shipped in the admin facelift work. It does fail loudly: `tests/test_admin_config.py:128` (`test_config_POST_creates_backup_before_write`) and the concurrency test at 142 both exercise the write. So it costs a debug cycle rather than shipping broken. But it should not be in the plan. **Fix:** delete that bullet from Wave 2. Add it to the "must NOT do" list instead, with the `with_name` reason, so nobody re-derives it. *(This line was in v1 and v2 too. Round 1 flagged `admin.py:370` only for the gitignore-pattern question and did not check `with_name` — that was my miss.)* Related, and still correct: `.gitignore`'s `config.yaml.bak.*` has no slash, so it matches by basename at any depth. `config/config.yaml.bak.*` stays ignored with no `.gitignore` change — which now matters, since that is genuinely where they land. ### V3-2. `tests/test_admin_config.py` isn't in Wave 2's list and needs real changes The `client` fixture (lines 45-58) builds the admin router with `base_dir=str(tmp_path)` and stages the config at the top of tmp_path: ```python config_yaml = tmp_path / "config.yaml" shutil.copyfile(ROOT / "config.yaml", config_yaml) ... router = build_router(None, _db_factory, base_dir=str(tmp_path)) ``` Wave 2 changes `admin.py:439` to `Path(base_dir) / "config" / "config.yaml"`, so the fixture must stage into a `config/` subdirectory instead: ```python config_yaml = tmp_path / "config" / "config.yaml" config_yaml.parent.mkdir(parents=True, exist_ok=True) shutil.copyfile(ROOT / "config" / "config.yaml", config_yaml) ``` and the backup assertion at line 135 becomes: ```python backups = sorted(tmp_path.glob("config/config.yaml.bak.*")) ``` **Note the asymmetry**, because it is easy to over-apply: the concurrency test at 142-186 calls `_persist_config_value(config_yaml, ...)` with an **explicit** path and never goes through `base_dir`. Its `tmp_path / "config.yaml"` staging and its glob at line 182 are correct as they stand and must be left alone. Wave 2's test list names `test_proficiency.py`, `test_config_endpoints.py`, `test_poller_parsing.py`, `test_admin_frontend.py` and the generic `ROOT / "config.yaml"` sweep. `test_admin_config.py` matches the generic sweep for lines 54 and 146, so a mechanical pass would rewrite the source path and leave the destination and the glob wrong. ### V3-3. Wave 1 breaks the admin triggers, Wave 3 restarts production into that state, and the Wave 1 smoke cannot detect it After Wave 1, `_repo_root` is corrected but the spawn argv is still `[sys.executable, "poller.py"]` with `cwd=` — and `poller.py` now lives in `src/`. Wave 4 is what fixes the argv, three waves later. Wave 1's acceptance includes: ```bash curl -sf -X POST localhost:8081/admin/api/apply-feedback?dry_run=true ``` **This cannot fail.** Verified: a missing script exits `rc=2`, and `_run_steps` turns every failure — spawn error, timeout, nonzero exit — into a `_job(...)` dict returned as **HTTP 200** with `{"status": "failed"}`. The endpoint has no raise path, and `curl -sf` only inspects the status code. That is the third false-green of this exact shape across three revisions (v1's Wave 1 `import metrics`, v2's Wave 1 `python -m pytest`, now this). The pattern worth internalising: **a check that passes before the change is applied is not a check.** Two fixes, take both: 1. **Assert the job status, not the HTTP code**, at every occurrence of that curl: ```bash curl -sf -X POST 'localhost:8081/admin/api/apply-feedback?dry_run=true' \ | grep -q '"status":"success"' || { echo "TRIGGER BROKEN"; exit 1; } ``` 2. **Fold Wave 4 into Wave 1.** It is four argv lines plus the `test_admin_triggers.py` assertions, it depends on Wave 1 alone, and it removes an intermediate state in which production's Controls buttons are silently dead — a state **Wave 3 currently restarts the live service into**. This is the same argument v3 already accepted for folding pyproject into Wave 1. The live service is unaffected during Waves 1-2 (it runs already-loaded code), so the exposure begins precisely at the Wave 3 restart. Folding closes it. ## Verified, no action - **Wave 3's reinstall matches the documented install exactly.** `REPO=$(pwd)` and the sed loop are character-for-character what `deploy/README.md:31-36` does. V2-1 is properly resolved. - All four `-m` targets (`poller`, `tier`, `seed_energy`, `feedback`) have `if __name__ == "__main__"` guards. - No `mypy.ini` / `ruff.toml` / `setup.cfg` / `tox.ini` exists, so no lint config references root `.py` paths. - No other invoker of root `.py` scripts in `opencode.json`, `package.json`, or `.claude/`. - Root-entry arithmetic checks out: `51 − 29 + 1 − 4 + 1 − 4 + 1 = 17`. - `tests/test_admin_triggers.py` monkeypatches `asyncio.create_subprocess_exec`, so Wave 4 spawns nothing real; the cited line numbers (101, 107-108, 140, 149, 161, 168) are accurate. ## Round 2 findings — resolution | # | Round-2 finding | v3 status | |---|---|---| | V2-1 | Wave 7 hardcodes a personal path, installs via `cp` | **Fixed.** `%h/llm-router` kept; sed loop adopted verbatim; "do NOT copy units with `cp`" is an explicit guard. | | V2-2 | Subprocess probes lose `PYTHONPATH` | **Fixed.** Both call sites listed, with the env dict and the reason. | | V2-3 | `test_admin_frontend.py` `.parent` assertion | **Fixed.** Line 56 → `.parent.parent`, in Wave 1. | | V2-4 | Wave 2→7 armed window; wrong dependency | **Fixed.** Timers stopped at pre-flight; reinstall moved to Wave 3; matrix now carries an explicit "production window armed?" column. | | V2-5 | Wave 1 committed a broken tree | **Fixed.** pythonpath folded into the move commit. | | V2-6 | `load_config` stays cwd-relative | **Decided, and recorded as a decision** in both Scope-OUT and Wave 2. Correct handling — it is now a choice rather than an omission. | | V2-7 | Smoke port 9000 vs convention | **Fixed.** 8081, citing CLAUDE.md. | | V2-8 | Commit count | **Fixed.** "6 waves, 5 git commits." | | V2-9 | Root target | **Fixed.** Exactly 17. | | V2-10 | Update the stale comment in the unit files | **Fixed.** Wave 3 updates it for `config/` and PYTHONPATH. | | V2-11 | `code_plans/` count | **Fixed by removal.** The bad number is gone; the criterion is "all tracked files, no deletions." | Also correctly carried over: the `config/__init__.py` guard (confirmed — a `config/` directory does not shadow `src/config.py`, because PEP 420 records it as a namespace *portion* and a regular module found later on `sys.path` wins; adding `__init__.py` would flip that to path order). ## Recommended edits before execution 1. **V3-1** — delete the `admin.py:370` bullet; move it to "must NOT do" with the `with_name` reason. 2. **V3-2** — add `tests/test_admin_config.py` to Wave 2: fixture stages into `tmp_path/config/`, line 135 glob gains the `config/` prefix, lines 142-186 left alone. 3. **V3-3** — assert `"status":"success"` in every trigger curl, and fold Wave 4 into Wave 1. With these three applied v3 is ready to execute, and worth the dual high-accuracy review pass. --- # Round 2 — v2 **Verdict: close. Four must-fix items, then it's executable.** Every Round-1 blocker is genuinely resolved, the reference list is now verified rather than asserted, and the PYTHONPATH decision is the right call and correctly argued. The remaining problems are all in the two places v1 didn't reach: the systemd install mechanism, and the tests that spawn real subprocesses. ## Correction to Round 1 My Round-1 item B3 said *"`deploy/*.service` should be corrected to `%h/Sources/6krrt` (or the drift documented)."* **That was wrong**, and v2's Wave 7 implements it. `%h/llm-router` is not drift — it is a deliberate placeholder, substituted at install time by the loop already in `deploy/README.md:34-35`: ```bash for u in deploy/llm-router*.{service,timer}; do sed "s|%h/llm-router|${REPO}|g" "$u" > ~/.config/systemd/user/"$(basename "$u")" done ``` The live units are that loop's *output*. See V2-1. --- ## Must fix ### V2-1. Wave 7 hardcodes a personal path and bypasses the documented install Wave 7 changes `deploy/*.service` to `WorkingDirectory=%h/Sources/6krrt`, `EnvironmentFile=%h/Sources/6krrt/.env`, `ReadWritePaths=%h/Sources/6krrt`, `Documentation=file:%h/Sources/6krrt/CLAUDE.md`, and installs with a plain `cp`. Two consequences: 1. **The repo ships one developer's home directory.** `%h/llm-router` is the token `deploy/README.md` greps for; replacing it means the sed finds nothing and every other clone installs units pointing at `~/Sources/6krrt`. 2. **`cp` skips the substitution step entirely**, so the install procedure in `deploy/README.md` and the one in the plan now disagree. **Fix:** keep `%h/llm-router` in `deploy/*.service`. Add only the layout changes there — `Environment=PYTHONPATH=%h/llm-router/src`, `ExecStart=... -m poller`, etc. Reinstall with the README's existing loop, not `cp`: ```bash REPO=%h/Sources/6krrt # or: REPO=$(pwd) systemctl --user stop llm-router.service llm-router-poller.timer llm-router-seed.timer for u in deploy/llm-router*.{service,timer}; do sed "s|%h/llm-router|${REPO}|g" "$u" > ~/.config/systemd/user/"$(basename "$u")" done systemctl --user daemon-reload systemctl --user start llm-router.service llm-router-poller.timer llm-router-seed.timer ``` Note the sed must also rewrite the new `PYTHONPATH` line, which it does for free since that line contains the same token. ### V2-2. Three real-subprocess tests break — pytest's `pythonpath` does not export `PYTHONPATH` Verified empirically inside a pytest run: **`PYTHONPATH env = None`**. The `pythonpath` ini option mutates the pytest process's `sys.path`; it sets no environment variable, so child processes inherit nothing. Two tests spawn a real interpreter: ``` tests/test_admin_frontend.py:84 subprocess.run([sys.executable, "-c", probe], cwd=str(ROOT)) probe does: import admin tests/test_admin_health.py:203 subprocess.run([sys.executable, "-c", probe], cwd=str(ROOT)) probe does: import admin; assert 'dispatcher' not in sys.modules ``` With `cwd=ROOT` and the modules now in `src/`, both get `ModuleNotFoundError: No module named 'admin'` after Wave 2. Neither file appears in Wave 2's file list. **Fix:** pass the env explicitly at both call sites: ```python subprocess.run( [sys.executable, "-c", probe], check=True, cwd=str(ROOT), env={**os.environ, "PYTHONPATH": str(ROOT / "src")}, capture_output=True, ) ``` This is the price of choosing PYTHONPATH over an editable install — an editable install would have made these work untouched. The trade is still correct for the reason v2 gives (a rebuilt `.venv` without `pip install -e .` is a silent outage); it just has this one bill attached, and the plan should pay it deliberately rather than discover it. ### V2-3. `tests/test_admin_frontend.py:53-58` hardcodes the old `__file__` depth ```python def test_admin_index_file_exists_at_module_derived_path(): """The served file lives at Path(__file__).parent/admin/frontend/index.html.""" expected = ( Path(admin.__file__).resolve().parent / "admin" / "frontend" / "index.html" ) assert expected.is_file(), f"frontend file missing at {expected}" ``` After Wave 2 this resolves to `src/admin/frontend/index.html` and fails. Needs `.parent.parent`, and the docstring updated to match. v2's test list names only `tests/test_admin_frontend.py:71` — which is the `admin.load_config('config.yaml')` string *inside the subprocess probe*, correctly caught for Wave 3. The `.parent` assertion eight lines above it was missed. This is the same test file that documents the `_REPO_ROOT` coupling, so it is the one place the anchor change is asserted. ### V2-4. The Wave 2 → Wave 7 window leaves production armed, and the timers will fire in it Quantified from the live units: ``` llm-router-poller.timer OnUnitActiveSec=2h llm-router-seed.timer OnUnitActiveSec=6h ``` Between Wave 2 (modules move) and Wave 7 (units reinstalled), the installed units still say `ExecStart=… uvicorn dispatcher:app` with no `PYTHONPATH` and `WorkingDirectory=%h/Sources/6krrt`. The dispatcher survives only because it is already loaded. Anything that restarts it — the admin portal's own `/admin/api/restart-service` button, an OOM, a reboot, `Restart=always` after any crash — brings it back into a crash loop on the port `opencode.json` uses for the user's own inference. And a multi-hour reorg **will** cross at least one 2-hour poller tick, which fails silently; `stale_after_days: 3` then starts the clock on a catalog that routes nothing. **Two fixes, both cheap:** 1. **Pre-flight:** `systemctl --user stop llm-router-poller.timer llm-router-seed.timer` before Wave 1, restart them in Wave 7. Add to the pre-flight block next to `PREFLIGHT_SHA`. 2. **Move the unit reinstall to immediately after Wave 3.** The dependency matrix says Wave 7 depends on Wave 4 — it does not. Wave 4 changes `admin.py`'s *internal* spawn argv; the unit files never reference it. Wave 7 needs Wave 2 (`src/`) and Wave 3 (`config/` paths) and nothing else. Reordering cuts the exposure window from five waves to two, and leaves Waves 4/5/6 as doc/test-only and production-safe. --- ## Should fix ### V2-5. Wave 1 commits a tree where the `pytest` console script is broken Wave 1 sets `pythonpath = ["src"]` while the modules are still at the root and `src/` does not exist. Collection then works only under `python -m pytest`, which adds the cwd — and the existing comment in `pyproject.toml` says in as many words that the setting exists precisely because the bare `pytest` console script does *not*: > Running as `python -m pytest` happens to add the cwd and hides this; the > `pytest` console script does not. So Wave 1's acceptance criterion (`python -m pytest --collect-only -q` passes) is a false-green of exactly the kind that made v1's Wave 1 dangerous — it passes for a reason unrelated to what it claims to check. **Fix:** move the `pythonpath` line into Wave 2's commit, alongside the move it describes. Keep `.gitignore` and the pre-flight SHA in Wave 1. ### V2-6. `config.py`'s default stays cwd-relative — worth fixing while you're in there `load_config(path="config/config.yaml")` still resolves against the current directory. Every invocation from anywhere but the repo root breaks, which is the whole reason the live unit carries this comment: > `# config.yaml, router.db and router.log are all referenced as relative paths,` > `# so this has to be the repo root.` You are already introducing `_REPO_ROOT` in `admin.py` and already editing all 11 call sites. Anchoring the default the same way removes the class: ```python _REPO_ROOT = Path(__file__).resolve().parent.parent def load_config(path: str | Path | None = None) -> RouterConfig: path = Path(path) if path is not None else _REPO_ROOT / "config" / "config.yaml" ``` Ten of the eleven call sites then collapse to a bare `load_config()`, and the 9000/8081 smoke stops depending on cwd. This is a small **logic** change, not a relocation, so it sits just outside the plan's stated "pure path refactoring" boundary. Flagging it as a decision to make on purpose — taking it or declining it are both fine; arriving at it by accident in Wave 3 is not. --- ## Minor ### V2-7. Smoke port 9000 contradicts the project's own convention CLAUDE.md, in the section written after the `pkill` outage: > **Convention going forward: 8080 is production, always.** … A throwaway > instance (manual iteration, Playwright smoke tests against the admin frontend, > anything that isn't "use the real router") binds **8081** instead. v2's smoke block uses 9000 throughout. Use 8081, or amend the CLAUDE.md convention in Wave 5 — but don't leave the repo asserting two different answers, given what that convention was written to prevent. ### V2-8. Commit count is 6, not 7 Waves 1-5 produce 5 commits; Wave 6 states outright that it generates none (untracked files); Wave 7 produces 1. The TL;DR says 7. ### V2-9. Root target is exactly 17 `51 − 29 + 1 (src) − 4 + 1 (config) − 4 + 1 (plans) = 17`. v2 says "≈ 16". Make the success-criteria checkbox an exact number so it can actually be checked. ### V2-10. Update the stale comment in the unit files `deploy/llm-router.service` carries `# config.yaml, router.db and router.log are all referenced as relative paths, so this has to be the repo root.` It becomes `config/config.yaml`. The unit files are already in scope for Wave 7. ### V2-11. `code_plans/` is ~320 files, not "100 glob results" The glob was truncated. It doesn't matter any more — v2 wisely dropped the `find plans/ -type f | wc -l = 40` criterion in favour of "all tracked files, no files deleted", which is both correct and checkable. Noting it only so the number isn't reused elsewhere. (59 tracked, ~260 ignored inside a nested `.omo/` tree.) --- ## Round 1 findings — resolution | # | Round-1 finding | v2 status | |---|---|---| | B1 | `src.` prefix contradicts flat-module premise | **Fixed.** All `src.` prefixes gone; "must NOT use `src.` prefixes" is now an explicit guard. | | B2 | `packages=find` installs an empty wheel | **Fixed by removal.** No packaging at all; PYTHONPATH instead, with the rationale stated. `src/__init__.py` explicitly forbidden. | | B3 | Live systemd units not the repo's; service running | **Partly.** Now a first-class wave with stop/reload/verify and a unit backup — but see V2-1 (install mechanism) and V2-4 (ordering). | | B4 | `admin.py:445-448` frontend paths | **Fixed** via `_REPO_ROOT`. Test assertion still missed — V2-3. | | B5 | `admin.py:662` subprocess cwd | **Fixed** via `_REPO_ROOT`. | | F1 | Fabricated `router_cli.py` section | **Fixed.** Section deleted; `router_cli.py` correctly absent from the config call-site list. | | F2 | Missing `leaderboard.py:135` | **Fixed**, and flagged as "missed in v1". | | F3 | Test counts low | **Fixed and exceeded.** v2 found 7 bare sites in `test_proficiency.py` (191, 207, 230, 260, 284, 310, 337) — verified accurate, three more than Round 1 listed. | | F4 | `git rm` on gitignored backups | **Fixed.** Plain `rm`, no commit, explicitly noted. | | F5 | Wave 6 file counts wrong | **Adequately fixed** — bad number survives but the criterion no longer depends on it (V2-11). | | F6 | Three different root-entry numbers | **Fixed** to one number; off by one (V2-9). | | S1 | Unresolved reasoning left in the document | **Fixed.** Clean throughout; single wave ordering. | | S2 | 733 tests can't catch B3/B4/B5 | **Fixed.** Real `uvicorn` + `/admin/` + trigger-POST smoke per wave. | | S3 | No rollback procedure | **Fixed.** Per-wave SHA table, pre-flight SHA, and a unit-file backup/restore for the one non-git wave. | Also correctly carried over: the `config/__init__.py` guard (confirmed — a `config/` directory does not shadow `src/config.py`, because PEP 420 records it as a namespace *portion* and a regular module found later on `sys.path` wins; adding `__init__.py` would flip that to path order). --- ## Recommended edits before execution 1. **V2-1** — revert `deploy/*.service` to the `%h/llm-router` placeholder; install via the README's sed loop, not `cp`. 2. **V2-2** — add `env={**os.environ, "PYTHONPATH": str(ROOT / "src")}` to the two `subprocess.run` probes; add both files to Wave 2's list. 3. **V2-3** — `tests/test_admin_frontend.py:53-58` → `.parent.parent`; add to Wave 2. 4. **V2-4** — stop the two timers at pre-flight; move the unit reinstall to directly after Wave 3 and correct the dependency matrix. 5. **V2-5** — fold the `pythonpath` change into Wave 2's commit. 6. **V2-6** — decide explicitly on the `config.py` absolute-default change. 7. **V2-7 – V2-10** — port 8081, commit count 6, root target 17, unit comment. With 1-5 applied this is ready to execute. It would also survive the dual high-accuracy review now, if you still want that pass — the reference list is sound, which is what was missing before. --- # Round 1 — v1 ## Blockers — will break ### B1. The `src.` prefix and the flat-module premise are mutually exclusive The plan asserts both, in the same wave: - Wave 2 "must NOT do": *"Do NOT change any `.py` module's internal imports (bare `from config import ...` continues to work because editable install puts `config.py` on sys.path)."* - Wave 2 acceptance #4/#5, Wave 3 QA, success criteria: `uvicorn src.dispatcher:app`, `python -m src.poller`, `from src.config import load_config`. These cannot both hold. With `package-dir = {"": "src"}` the modules install as **top-level** names — `config`, `dispatcher`, `metrics`. There is no `src.` namespace at all. To get `src.dispatcher` you would need a package. Plan must pick one world: flat modules with bare imports, or a `src/` package with `src.` prefixes. The repo uses bare imports everywhere, so the correct path is the former. **Fix:** drop every `src.` prefix; `uvicorn dispatcher:app`, `python -m poller`, `from config import load_config`, etc. ### B2. Wave 1 installs an empty wheel The plan says: > `pyproject.toml` has a `[project]` or `[tool.setuptools]` section with > `packages = find` or `packages = ["src"]`, `package-dir = {"": "src"}`. > ... `src/__init__.py` is created `find_packages()` will not discover flat `.py` files directly under `src/`. It finds directories with `__init__.py`. With flat modules and `package_dir={"": "src"}`, the correct setuptools directive is `py-modules = ["admin", "baseline_report", ...]` (or use a package). `packages = find` produces an empty distribution. I verified this in a fresh venv: the resulting wheel installed nothing, so `import dispatcher` failed. The plan's Wave 1 QA (`import src.dispatcher`) would also fail because `src.dispatcher` does not exist when flat modules are installed as top-level names. **Fix:** either go to a proper package `src/llm_router/` (and rewrite every import), or skip packaging entirely and use `PYTHONPATH=src` in systemd units + pytest `pythonpath`. For a service that isn't distributed as a package, PYTHONPATH is simpler and avoids the editable-install footgun in `CLAUDE.md`'s venv warning. ### B3. The live systemd units aren't the repo's `~/.config/systemd/user/llm-router.service` uses `WorkingDirectory=%h/Sources/6krrt` and is already active. `deploy/llm-router.service` still says `%h/llm-router`. The plan updates deploy/*.service but says nothing about installing the updated units on the live machine. The production service is active now and will restart on `Restart=always` after any failure. If the deploy units are not copied to `~/.config/systemd/user/`, the next restart will use the old units against the new `src/` layout and crash-loop on port 8080 (which opencode.json uses). **Fix:** add a first-class production step: stop timers/service, copy updated units to `~/.config/systemd/user/`, daemon-reload, start, verify `/health`. ### B4. admin.py serves frontend files from the wrong directory after the move `admin.py` resolves `_module_dir = Path(__file__).resolve().parent` and uses it for the four `admin/frontend/*.html` paths. After moving `admin.py` into `src/`, this resolves to `src/admin/frontend/` instead of `admin/frontend/`, so all admin pages 404. The plan mentions the subprocess cwd but not the frontend paths. ### B5. admin.py subprocess cwd is wrong Line 662 sets `_repo_root = str(Path(__file__).resolve().parent)` which becomes `src/` after the move. Maintenance scripts are run from `cwd=_repo_root`, but the config file and `router.db` are at the repo root, so spawned scripts will look for `config.yaml` in `src/`. **Fix:** same fix as B4 — compute repo root, not file parent. ## Fabrication / unverified references - The plan quotes `router_cli.py` as doing `config_py = Path(__file__).resolve().parent / "config.yaml"`. That code does not exist; `router_cli.py` has no `__file__` path resolution and no `load_config` call. This error is the tell that the reference list was not actually checked against the files. - Missing `leaderboard.py:135` `load_config("config.yaml")`. - Claimed "9 bare-path test sites, not 2" is correct; the plan says 2. - Claimed "23 affected test files" is correct; the plan says "~20". - The 14 `config.yaml.bak.*` are gitignored, so `git rm` cannot work — the plan says `git rm`. - `code_plans/` holds ~320 files (59 tracked + ~260 ignored nested artifacts), not 38 / 100 / 40 as variously stated. ## What it got right - The real problem is the 29 loose `.py` files and 14 backups. - Moving source to `src/` (flat modules, bare imports) is the right conceptual answer for this codebase. - The `config/` directory does not shadow `src/config.py` (PEP 420 namespace vs regular module resolution), as long as `config/__init__.py` is never added. - No top-level basename collisions across the four doc dirs. ## Recommended revisions before execution 1. Resolve B1: strike every `src.` prefix; modules stay top-level. 2. Fix B2: `py-modules` not `packages`; **delete** the `src/__init__.py` criterion; make Wave 1's QA assert the install from outside the repo root. Or take the `PYTHONPATH` route and drop Wave 1. 3. Add B3 as a first-class wave step with the stop/copy/daemon-reload/start/verify sequence, and reconcile `deploy/*.service` `WorkingDirectory` with the live units. 4. Fold B4 + B5 + `dispatcher.py:2968` into one `_REPO_ROOT` change in `admin.py`. 5. Delete the fabricated `router_cli.py` section; add `leaderboard.py:135`; re-grep the test sites for all four path spellings. 6. Rewrite Wave 5 as `rm`, not `git rm`; recount Wave 6; pick one root-entry number. 7. Strip the in-document reasoning and settle on one wave ordering. 8. Add the 8081 smoke checks and a per-wave `git reset --hard` + unit-reinstall rollback. **Not ready for the dual high-accuracy review** — that would burn a review pass on a document whose reference list is unverified. Fix 1-6 first; 7-8 can land in the same revision.