Files
6krrt/plans/root-reorg-src-config-plan-review.md
adlee-was-taken 3523dcf93e docs(plans): give every plan a Status line so the queue is greppable
plans/ held 58 documents and exactly one said whether it was open. The rest
mixed finished work, reviews of shipped work, parked specs and genuinely
pending ones, with nothing distinguishing them, so "how many plans are in
the queue" had no answer short of reading all 58.

Now `grep -H '^Status:' plans/*.md` is the answer:

    50 done   3 in progress   2 planned   2 reference   1 parked

Statuses were derived rather than guessed: CLAUDE.md's own built list and
"What's NOT built yet" section, plus checking the subject exists in the
code. A review of work that shipped counts as done -- it records what was
found, it is not a request for anything. `reference` separates the two docs
that are conventions rather than work items (admin-design-standards,
admin-work-framework), which otherwise read as permanently-open plans.

The vocabulary is deliberately five words. A larger one invites "mostly
done" and "blocked-ish", which is how the directory became unreadable.

test_plans_declare_status.py keeps it from rotting: a new plan without a
marker fails, as does an unknown status, one buried below the eighth line,
or an open status with no reason -- "planned" alone is the state that rots,
since nobody can tell later whether it waits on a decision, a dependency,
or just nobody's turn.

Also updates the sweep plan with what landed and what did not, including
that #9 was not a defect.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
2026-09-08 18:55:16 -04:00

32 KiB
Raw Permalink Blame History

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:

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:

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:

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:

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:

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:

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:

    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).

  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:

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:

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:

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

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:

_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).


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