fix(routing): select the listed matrix by the loader's rule, not last-write-wins - #294
Conversation
…ing list`/`show` `amplifier routing list` showed a user matrix and a bundle matrix of the same name as peers. Only one is ever loaded: hooks-routing's mount() searches `[*custom_routing_dirs, bundle routing/]` and takes the first hit, so a file in ~/.amplifier/routing/ silently makes the shipped bundle matrix dead -- and nothing in the CLI said so. That is why a matrix change shipped in the bundle can be completely inert on a host. The precedence rule is NOT re-derived here. It is consumed from hooks-routing's own `resolve_matrix_source()` (routing-matrix PR #52), loaded by file path out of the same cached bundle directory the CLI already globs -- app-cli does not depend on hooks-routing as a distribution, and `routing list` never mounts a bundle, so neither an import nor the session-time `model_role_resolver` capability is reachable from this process. When the cached bundle predates PR #52 (no `resolve_matrix_source`), the CLI draws no marker at all rather than guessing the search order: a wrong shadowing claim is worse than none. Unshadowed output is byte-identical to before, in both text and JSON. - amplifier_app_cli/lib/routing_provenance.py: locate + load the bundle's matrix_loader, classify custom vs bundle routing dirs, resolve one MatrixSource per matrix name. - commands/routing.py: row marker (`⚠ shadows bundle`), a footer naming the file in use and each file it suppresses, the same note on `routing show`, and MatrixSource.to_dict() in `--format json`. - tests/test_routing_shadowing.py: 15 tests -- shadowed marks the winner, unshadowed output byte-identical, no user routing dir, and the old-bundle degradation path.
…-write-wins
`amplifier routing list` built its rows with
matrices[data["name"]] = (data, path) # over sorted(discovered files)
which diverges from hooks-routing in two independent ways at once: it keyed on
the `name:` field INSIDE each YAML (the loader resolves by file STEM), and it let
the LAST file in sort order win (the loader takes the FIRST hit in
`[*custom_routing_dirs, bundle routing/]`).
The two rules agreed only by alphabetical accident -- `~/.amplifier/cache/...`
sorts before `~/.amplifier/routing/...` because "c" < "r". When they disagree the
command asserts something false: it names a file as in use that the loader would
never read. A user file `my-fast.yaml` declaring `name: balanced` overwrote the
row for the real `balanced` matrix outright.
Rows are now keyed by file stem, and the file behind each row is
`MatrixSource.path` -- the value hooks-routing's own `resolve_matrix_source()`
assigns to `matrix_path` and loads. Those fields are reachable from the CLI
through the seam PR #293 already built, so precedence is not re-derived a third
time. Only the winning file is parsed, so a shadowed file can no longer supply a
row's description, `updated:` date or compatibility count.
Also: `routing use` now writes the filename the loader resolves (it could write
an unloadable internal name before), a `name:`/stem disagreement is surfaced
rather than silently keyed on the internal name, and every JSON row carries
`matrix_file`.
When the cached bundle predates routing-matrix PR #52 there is no
`resolve_matrix_source` to ask. The shadowing MARKER is still withheld (#293's
rule: a wrong marker is worse than none), but a row must point at some file, so
selection falls back to the first candidate in `[*custom_dirs, *bundle_dirs]` --
the same list hooks-routing builds as `search_dirs`, in one labelled function.
Tests: `tests/test_routing_winner_selection.py` constructs both disagreement
classes explicitly, each with a non-vacuity gate that re-runs the old algorithm
inline and asserts it picks the other file. Full suite green (1605 passed);
existing routing list/show/use tests unmodified.
7aa6bd9 to
450f309
Compare
…ntent conflicts, keep 9kk's fix)
Merge-queue verification — PR #294 (
|
| # | Gate | Method | Result |
|---|---|---|---|
| 1 | Key disagreement test exists, genuine, non-vacuous | Read tests/test_routing_winner_selection.py in full. TestSortOrderDisagreement and TestNameStemDisagreement each construct a tree where _last_write_wins() (old algorithm, re-run inline) and resolve_matrix_origins/resolve_winning_paths (loader's rule) disagree, each gated by its own test_the_two_rules_actually_disagree_on_this_tree non-vacuity check. Ran the file: 17/17 pass. |
PASS |
| 2 | Fail-before / pass-after | Checked out parent commit 7aa6bd9 (adq's HEAD, pre-fix), copied in only tests/test_routing_winner_selection.py from fbbcf1a, ran it: 14 failed, 3 passed (the 3 passes are the non-vacuity gates that only exercise the old algorithm + #293's pre-existing resolve_matrix_origins, not the fix). Restored the fix's own source files (routing.py, routing_provenance.py) on top: 17 passed, 0 failed. |
PASS |
| 3 | Unshadowed output unchanged (test required) | TestUnchangedBehaviour::test_agreeing_tree_lists_every_matrix_once — stock layout, asserts no shadow/file says name: text and all three matrices listed once. Passes. |
PASS |
| 4 | No user routing dir at all (test required) | TestUnchangedBehaviour::test_no_user_routing_dir_at_all — asserts the dir doesn't exist, list still returns both bundle matrices cleanly. Passes. |
PASS |
| 5 | Consumes ell's published fields, doesn't re-derive precedence | Read lib/routing_provenance.py: resolve_winning_paths() takes origins from resolve_matrix_origins(), which calls hooks-routing's own dynamically-loaded resolve_matrix_source() (the #293 seam) and uses MatrixSource.path/.source/.shadowed verbatim — falling back to directory precedence only when that function is unreachable (pre-#52 cached bundle), which is honestly documented, not silently substituted. |
PASS |
| 6 | Full suite green + clean union proof | uv run pytest -q on the merged branch: 1626 passed, 1 skipped, 13 deselected, 1 xfailed. pytest -m integration: 13 passed. ruff check on all three touched files: clean. Clean-union diff-stat proof under "Conflict" above. Remote CI on the pushed merge commit (33702681720): all green except Windows (see gate 8). |
PASS |
| 7 | No unvalidated performance claim | Grepped PR body, DONE-NOTE, and touched files for performance language — this is a correctness fix (row-selection precedence), no speed/latency claims made anywhere. | PASS |
| 8 | Windows CI | Compared 3 points in history: main@31ad917 (#292, pre-#293): 2 Windows failures, both in test_timedout_session_resumable.py (checkpointing/session-store timing, unrelated to routing, predates this whole arc). main@90b6b3b (#293, current tip): 5 failures — same 2 plus 3 new in test_routing_shadowing.py, all from _display_path()'s hardcoded ~/-forward-slash rendering not matching Windows paths. This PR does not touch _display_path() (confirmed byte-identical between main and this branch). This PR's Windows run has 8 failures = the same 5 plus 3 more in its own new test_routing_winner_selection.py, hitting the identical pre-existing _display_path() bug via new assertions, not a new bug. Ubuntu/macOS (both Python versions) and both integration jobs are fully green. |
Pre-existing, not a blocker — disclosed |
Merge
All gates pass. Marked ready for review and merging with --admin: this repo has a required-review ruleset and I am the PR author (no second reviewer available in this loop) — disclosing that per the lane's honesty rule.
DONE-NOTE —
model_performance-9kkamplifier routing listpicked the winning matrix row by last-write-wins, not by theloader's rule. It could therefore name a file as in use that the loader would never read.
Spend: $0.00. No API calls, no DTU, no infrastructure created or registered.
Everything here is code reading, unit tests, and one local CLI invocation.
Stacked on adq's open PR #293 (
lane/adq-routing-list-shadowing), as instructed —#293 was still open at the time of writing. Base retargets to
mainonce #293 merges.Deliverable 1 — the last-write-wins resolution, quoted, and why it diverges
amplifier_app_cli/commands/routing.py:153-160(as oforigin/main@31ad917;154-170after #293 renamed it to_load_all_matrices_with_paths):fed by
routing.py:141:Two independent divergences from the loader, in one line:
name:field inside the YAMLsearch_dir / f"{default_matrix_name}.yaml"sorted()[*custom_routing_dirs, routing_dir]The loader's rule, quoted from the shipped bundle
(
amplifier_module_hooks_routing/__init__.py:88-96, the pre-#52 inline form):and its post-#52 form (
routing-matrix@320f24e,__init__.py:108-111→matrix_loader.py:73-144):Why they agreed until now, by accident:
sorted()puts~/.amplifier/cache/…before~/.amplifier/routing/…only because"c" < "r".The user file therefore landed last and won under both rules. Nothing enforced that;
renaming either directory silently flips the CLI's answer with no error.
The two ways it broke, both now covered by tests:
name:≠ stem. A user filemy-fast.yamldeclaringname: balancedsorted lastand overwrote the row for the real
balancedmatrix.routing listandrouting show balancedthen displayed a file the loader can never resolve asbalanced— the tool actively asserting something false. Androuting usewrote thatinternal name into settings, which the loader appends
.yamlto → "Matrix file notfound — routing disabled".
~/.amplifier/routing/and last-write-wins hands back the bundle file while theloader loads the user file.
Deliverable 2 — the fix (DRAFT PR, branch
lane/9kk-routing-list-lastwrite)Rows are now keyed by file stem, and the file behind each row is the one
resolve_matrix_source()would load.resolve_matrix_origins()(shipped by #293) already reaches hooks-routing's ownresolve_matrix_sourceby loadingmatrix_loader.pyout of the cached bundle, and itsMatrixSource.pathis literally the value the loader assigns tomatrix_path. So thefields ell published are reachable from the CLI — via #293's seam — and this change
consumes them rather than re-deriving precedence a third time. No new seam was needed.
lib/routing_provenance.py— newresolve_winning_paths(matrix_files, origins):{stem: winning_path}, taken verbatim fromMatrixSource.pathwhen available.commands/routing.py—_load_all_matrices_with_paths()now selects the winner firstand parses only that file, so a shadowed file can no longer supply a row's
description,
updated:date, or compatibility count._load_all_matrices()is keyed by stem, which also fixesrouting usewriting a valuethe loader cannot resolve.
name:/stem disagreement is surfaced (row marker, footer note,declared_nameinJSON) instead of being silently keyed on the internal name.
matrix_file— which file this row is.routing use <internal-name>is refused and names the filename to use instead.Decision recorded: what happens when
resolve_matrix_sourceis unreachableA cached bundle older than routing-matrix PR #52 has no function to ask. This is the
live state on the measurement host —
~/.amplifier/cache/amplifier-bundle-routing-matrix-972b0ce7f0cbc2f7carries no
resolve_matrix_source, soresolve_matrix_origins()returns{}there today.#293's rule is "a wrong shadowing marker is worse than none", and that is kept — no marker
is drawn. But row selection is not symmetric with a marker: a marker may be omitted,
because "no claim" is truthful; a listing row cannot be omitted, so something must be
chosen. Choosing by alphabetical accident is what this item is about.
Chosen: fall back to the first candidate in
[*custom_dirs, *bundle_dirs]— the samelist hooks-routing builds as
search_dirs, using #293's existingclassify_routing_dirs().It lives in one function, is labelled as a fallback in its docstring, and is only reached
when the authoritative answer is unavailable. Recorded here rather than escalated, per the
lane's no-waiting rule.
Deliverable 3 — the disagreement test
tests/test_routing_winner_selection.py(17 tests). The two rules are made todisagree explicitly, and each disagreement class carries a non-vacuity gate that
re-runs the old algorithm inline (
_last_write_wins()) and asserts it picks the otherfile — so the tests cannot quietly stop testing anything if the trees stop colliding.
.amplifier/zz-cache/…so it sorts after.amplifier/routing/TestSortOrderDisagreement(5)my-fast.yamldeclaringname: balancedmy-fast.yaml, keyedbalanced, realbalancedrow gonebalanced.yaml, plus a separatemy-fastrowTestNameStemDisagreement(6)Also pinned:
shadowing marker (
TestProvenanceUnreachableFallback, 2);still picks the user file, and an unparseable winner drops the row rather than
letting the shadowed loser stand in (
TestUnchangedBehaviour, 4).Honest limitations
resolve_matrix_sourceis exercised through a stand-in reproducing the upstreamcontract (
routing-matrixd17d03c/ verified against320f24e), becausehooks-routing is a bundle module with nothing to import in a test environment. This
mirrors fix(routing): mark a matrix that shadows a same-named matrix in
routing list/show#293's own approach; the CLI's real consumption path (locate bundle → loadmodule by file path → call → render) is exercised end to end.
test_routing_shadowing.pyandtest_routing_winner_selection.py. Deliberate: de-duplicating it means editingfix(routing): mark a matrix that shadows a same-named matrix in
routing list/show#293's test file while fix(routing): mark a matrix that shadows a same-named matrix inrouting list/show#293 is open.would load X" rests on quoting
resolve_matrix_sourceand on fix(routing): mark a matrix that shadows a same-named matrix inrouting list/show#293's dynamic load ofthe real function, not on a booted session. (confidence: measured for the CLI's own
selection; inferred for the loader's end behaviour.)
Verification
uv run pytest -q→ 1605 passed, 1 skipped, 13 deselected, 1 xfailed.routing list/show/usetests (test_routing_commands.py,test_routing_shadowing.py,test_routing_matrix_registration.py) → 101 passed,unmodified.
ruff checkclean on all three touched files; the repo's 14 pre-existing findingsare unchanged (verified by stashing).
console output byte-identical to the pre-change branch, and
--format jsonnow namesthe winner per row —
anthropic→~/.amplifier/routing/anthropic.yaml,openai→~/.amplifier/routing/openai.yaml, the other ten → the bundle cache. Bothshadowed matrices resolve to the user file, which is what a session loads.
What remains open
currently draw will appear for
anthropicandopenai. Worth re-running the smoke checkthen — it is the first host state where the authoritative path, not the fallback, is live.
_show_matrix_details()still titles the panel frommatrix_data["name"], so aname:/stem mismatch shows the internal name in that one header. The disagreement isreported alongside it; unifying the header was left out of scope.