feat(doctor): the reranker weights are verified against the published digest (#734, slice 3) - #972

Claude finished @cdeust's task in 2m 36s —— View job

Review 1 of 3 (ADR-1119) is done

• Read the PR diff
• Ran five reviews: code quality, performance, test coverage, documentation accuracy, security
• Posted the findings as a top-level comment: feat(doctor): the reranker weights are verified against the published digest (#734, slice 3) #972 (comment)

None of the five reviews found a Blocker. The posted comment has three Notes, no action before merge.

Verdict: approve

Review 1 of 3 (ADR-1119) — PR #972, commit 9d25ef3

I ran five reviews: code quality, performance, test coverage, documentation accuracy and security. None of them found a Blocker.

What I checked:

• Pinned digest. EXPECTED_MODEL_SHA256 equals the reranker_model_sha256 in benchmarks/results/repro/20261014T232318Z/MANIFEST.json, and that manifest says reranker_state: loaded. All 131 committed reranker_model_sha256 entries carry this same value.
• Same cache directory as recall. The doctor reads the directory that hooks/wiring.py registers behind core.reranker_model.reranker_cache_dir, and builds the path with the same model_path_under. So it hashes the file FlashRank loads.
• Layers and craftsmanship. doctor_reranker.py sits where the other doctor_*.py modules sit. core/reranker_identity.py imports only the standard library. doctor.py was already over the size limit and is in the baseline. No new numeric constant needs a # source:.
• Existing tests. No existing test checks the exact manifest key set. The test that requires _recorded_failures to come last in both check lists still passes.
• Docstrings. The claim that the check never downloads, deletes or loads a model holds: it only reads the file in chunks, and catches OSError only. No text claims the weights are checked before FlashRank loads them.
• Hot paths. Recall and server startup do not hash the file. Each manifest still hashes it once.

Notes, no action before merge

• mcp_server/doctor_reranker.py:33 — the mismatch fix says to delete the .onnx file. If FlashRank only downloads when the whole model directory is missing, it will not download again. I could not check FlashRank's source here. Pointing the fix at the parent directory avoids the question.
• benchmarks/_repro.py:83-84 and benchmarks/lib/write_manifest.py:91-92 — model_identity() runs inside the same broad except as ensure_reranker_loaded(). A hashing error after a successful load records everything as unresolved and loses the loaded state, and neither manifest builder has a test.
• reranker_model.model_identity() always compares against the L-12 pin. The reranker-matrix cells that swap _MODEL_NAME will record mismatch. That is true, but nothing documents it.

Verdict: approve

Claude finished @cdeust's task in 2m 10s —— View job

Review 2 of 3 (ADR-1119) — PR #972, commits after 9d25ef3 (2df9d02)

Review 1 found no Blockers, so nothing was carried over. I reviewed only the fixup 2df9d02 (mcp_server/doctor_reranker.py, tests_py/test_doctor_reranker.py). I ran five reviews on it: code quality, performance, test coverage, documentation accuracy and security. None of them found a Blocker.

What I checked:

• The fix now names the directory, which settles review 1's first note. model_path_under builds {cache}/{_MODEL_NAME}/{_MODEL_FILE} with /, and neither name contains a /. So rsplit("/", 1)[0] is always {cache}/ms-marco-MiniLM-L-12-v2, on Windows too, and never the cache root, ~ or /, even when XDG_CACHE_HOME is empty. It is the directory Ranker._prepare_model_dir checks: ADR-0244 quotes FlashRank 0.2.10 downloading only when that directory is absent. The FlashRank source is not installed here, so this rests on the ADR.
• The docstring still holds. The check only reads the file. "Remove" appears only in the fix text, which goes through str.format as an argument, so there is no injection.
• The tests catch a revert. The trailing space in f"Remove {dir} " means a fix that names the .onnx file again fails both assertions. _joined returns exactly what production builds, because the fixture's directory name equals _MODEL_NAME.
• Limits. The file has 70 lines and the function about 27. Lint, tests and the craftsmanship gate were not run locally because this job was not allowed to run commands. I posted this here instead of with gh pr comment because this job could not write the comment body to a file. CI Pipeline was still running when I posted.

Notes, no action before merge

• tests_py/test_doctor_reranker.py:23-28: _joined never ends if no parent has the expected name (Path("/").parent == Path("/")). pytest-timeout (300 s) would end it, but f"{tmp_path}/{_MODEL_NAME}" would be simpler.
• mcp_server/doctor_reranker.py:51: the doctor rebuilds the directory by taking apart core's path format. A model_dir_under(cache_dir) in core/reranker_model.py would keep that format in one place.
• _ABSENT_FIX: "run one recall" does not download in a server that has already failed to load, because _flashrank_failed (core/reranker.py:70) stays set for the life of the process. "Restart Cortex, then run one recall" would be accurate.
  · Branch fix/734-reranker-identity

Verdict: approve

CI autopilot : unit en échec sur 2df9d02 (run 42808456857), à cause du gate de mutation (ADR-1122) ; merge n'échoue que parce que unit a échoué.

• Diagnostic : deux fichiers modifiés gardaient des mutants vivants.
  • core/reranker_model.py : aucun test sélectionné n'appelait model_identity, d'où le mutant x_model_identity__mutmut_1, non couvert.
  • doctor_reranker.py : 13 mutants survivaient, parce que les tests ne vérifiaient qu'un fragment du détail et de l'aide, et jamais optional dans la branche OK.
• Correctif (fixup 3aa5320, tests seulement, le code de production n'a pas changé) :
  • les tests du doctor vérifient les arguments exacts du Check (nom, ok, détail, aide, optional) dans les quatre branches ;
  • un test appelle reranker_model.model_identity avec les providers configurés et vérifie le chemin lu.
• Reçu local : GITHUB_BASE_REF=main python scripts/ci_mutation_diff.py donne 4 file(s), 0 failed, 0 untested, avec 0 survivant dans chacun des quatre fichiers. Les suites liées donnent 457 passed et 15 skipped ; le gate de craftsmanship passe.
• Non vérifié : la CI de 3aa5320 (en cours) et la matrice des trois OS.

Generated by Claude Code

Claude finished @cdeust's task in 1m 42s —— View job

Review 3 of 3 (ADR-1119) is done

• Listed the commits after 2df9d02. There is one, 3aa5320, and it changes only tests.
• Read the tests against mcp_server/doctor_reranker.py and core/reranker_model.py. This job was not allowed to run pytest, ruff or the craftsmanship gate, so check CI Pipeline on 3aa5320.
• Ran five reviews: code quality, performance, test coverage, documentation accuracy and security.
• Posted the findings: feat(doctor): the reranker weights are verified against the published digest (#734, slice 3) #972 (comment)

None of the five reviews found a Blocker, and the comment has three Notes. The whole PR is accepted: no file or commit has an open Blocker.

Verdict: approve
· Branch fix/734-reranker-identity

Review 3 of 3 (ADR-1119) — PR #972, commits after 2df9d02 (3aa5320)

Reviews 1 and 2 found no Blockers, so nothing was carried over. I reviewed only 3aa5320, which changes only the tests (tests_py/core/test_reranker_identity.py, tests_py/test_doctor_reranker.py). I ran five reviews: code quality, performance, test coverage, documentation accuracy and security. None of them found a Blocker.

What I checked:

• Exact assertions match production. The detail and fix strings expected by the four identity tests match the ones mcp_server/doctor_reranker.py builds, character for character, optional=True included. The unreadable-file test builds the same PermissionError(13, …, path), so it also pins the printed path. This settles review 2's first note: _joined is gone.
• New model_identity test. It replaces _cache_dir_provider and _model_sha256_provider through monkeypatch, which restores them. The path it asserts is the one model_path_under builds.
• Isolation. The only write goes to tmp_path. The real ~/.cache/flashrank is never read, and nothing goes to the network.
• Limits. The files have 58 and 107 lines and every function is short. This job was not allowed to run pytest, ruff or the craftsmanship gate, so these results come from reading the code. Check CI Pipeline on 3aa5320 before merging.

Notes, no action before merge

• tests_py/test_doctor_reranker.py:3-5: the docstring says every test records the Check arguments, but test_the_check_runs_on_both_backends only checks list membership.
• tests_py/core/test_reranker_identity.py: the model_identity stub returns the pinned digest. A body that returns VERIFIED without looking at the digest would survive; a stub that returns a different digest and expects MISMATCH would catch it.
• mcp_server/doctor_reranker.py:54: only PermissionError is tested, so narrowing except OSError (for example to PermissionError) would survive.

Accepted: the whole PR (9d25ef3, 2df9d02, 3aa5320). No file or commit has an open Blocker.

Verdict: approve