6krrt/plans/root-reorg-src-config-plan-review.md

# 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 `<root>/config/config.yaml`, it already produces
`<root>/config/config.yaml.bak.<ts>`. **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=<repo root>` — 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.