fix(doctor): a symlink loop fails only the pairing and artifact checks (#729, slice 4) - #919

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

Review of PR #919 (review 1 of 3): blocked

• Read the diff and the repo standards
• Ran all five reviews: code quality, documentation accuracy, performance, security, test coverage
• Checked the findings against the source, ci.json and pyproject.toml
• Posted the review: fix(doctor): a symlink loop fails only the pairing and artifact checks (#729, slice 4) #919 (comment)

Blocker: the new code comments say Path.resolve() raises OSError on a symlink loop from Python 3.13. It doesn't: with the default strict=False, 3.13 raises nothing. So on 3.13 the Runtime artifact identity check passes with "0 mcp_server files" instead of failing. The new test_a_package_path_in_a_symlink_loop_fails_the_check_only therefore fails on 3.13, which requires-python = ">=3.11" supports, and the CHANGELOG claim is false there. CI is green only because the unit tests are pinned to 3.12.

Fix: use resolve(strict=True) at doctor_artifact.py:193, then correct the two comments and the CHANGELOG entry.

I worked this out from the CPython docs and by tracing the code; I couldn't run it, because no 3.13 interpreter was available here.

Verdict: block

Review 1 of 3 (ADR-1119): PR #919

All five reviews ran (code quality, documentation accuracy, performance, security, test coverage). Performance and security found nothing. The other three found the same problem on their own.

Blocker

The new comments and the CHANGELOG get Python 3.13 wrong, and the new artifact loop test fails on 3.13. Files: mcp_server/doctor_artifact.py:185-186, mcp_server/doctor_pairing.py:105-106, CHANGELOG.md (the new entry), tests_py/test_doctor_artifact.py:247-258.

• The comment says Path.resolve() raises OSError on a loop "from 3.13". The 3.13 pathlib docs say otherwise: "Symlink loops are treated like other errors: OSError is raised in strict mode, and no exception is raised in non-strict mode." Both call sites use the default strict=False, so from 3.13 resolve() raises nothing on a loop.
• What that does to the artifact check on 3.13 (traced through the code, not run, because no 3.13 interpreter was available here):
  1. _identify (doctor_artifact.py:193) resolves loop-a/x without an error.
  2. The code goes to _source_tree. There is no .git, so it calls tree_members → rglob("*"), which skips the loop's scandir error and returns no files.
  3. The check returns ok=True with "0 mcp_server files".
• So test_a_package_path_in_a_symlink_loop_fails_the_check_only fails on 3.13, which requires-python = ">=3.11" supports. The CHANGELOG line "Each of these two optional checks now fails alone and names the loop" is false there. The PR body's mutation note ("On 3.13 the loop raises OSError") rests on the same wrong premise.
• The pairing check still fails on 3.13, but for another reason: opening <loop>/.claude-plugin/plugin.json raises ELOOP. Its test passes either way.
• CI is green only because ci.json pins unit to 3.12.

Fix: in doctor_artifact.py:193 use Path(mcp_server.__file__).resolve(strict=True). It raises RuntimeError on 3.11/3.12 and OSError on 3.13+, and the except tuple already catches both. Then rewrite both comments to: "RuntimeError up to 3.12; from 3.13 only strict=True raises (OSError)". Adjust the CHANGELOG to match. Fix this →

Notes, no action before merge

• Catching RuntimeError also catches RecursionError and NotImplementedError. The check still fails visibly, but str(NotImplementedError()) is empty, so the detail would end in : with nothing after it.
• On 3.13+ the pairing loop test no longer guards resolve() being inside the try. strict=True at doctor_pairing.py:103 would make the test take the same path on every version.
• test_a_distribution_installed_elsewhere_reads_the_pyproject is correct: removing the location comparison turns the version into 9.9.9 and fails the test.

Not verified: no Python 3.13 run. The 3.13 behaviour comes from the CPython docs and a trace through the code. Tests were not re-run in this review.

Verdict: block

Revue 1 : le bloquant est confirmé et corrigé dans 2a19bdc (fixup! de de1a1b5).

Ce que j'ai vérifié. Python 3.13.18 est installé ici. Sur une boucle de liens symboliques, Path('a/x').resolve() se comporte ainsi :

Sur 3.13, au commit de1a1b5, test_a_package_path_in_a_symlink_loop_fails_the_check_only échoue, comme la revue l'a déduit.

Correctif.

• doctor_artifact._identify résout Path(mcp_server.__file__).parent.resolve(strict=True). Je résous le répertoire du paquet, qui existe toujours, et non le fichier : les tests pointent __file__ vers un fichier absent du paquet.
• doctor_pairing.plugin_pairing résout Path(plugin).resolve(strict=True), comme le suggérait la note. Le test de boucle passe donc par le même chemin sur toutes les versions.
• Les commentaires des deux modules, le CHANGELOG et la note de mutation du corps de la PR décrivent maintenant le comportement réel de 3.13.

Reçus.

• Les tests du doctor donnent 64 passed et 1 skipped, sur 3.11, 3.12 et 3.13.
• Sur 3.13, CLAUDE_PLUGIN_ROOT=<boucle> python -m mcp_server.doctor va jusqu'au bout. Le check de pairage échoue seul : [Errno 40] Too many levels of symbolic links: '…/a'.

Notes sans action.

• RecursionError et NotImplementedError attrapés par RuntimeError : le check échoue quand même de façon visible. Je n'ai pas cherché de chemin réel qui les lève, je laisse en l'état comme le propose la revue.
• Le test test_a_distribution_installed_elsewhere_reads_the_pyproject est inchangé.

Generated by Claude Code

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

Review 2 of 3 (ADR-1119): PR #919

• Read the commit pushed since review 1 (2a19bdc)
• Checked that the review-1 Blocker is fixed
• Ran all five reviews on 2a19bdc (code quality, documentation accuracy, performance, security, test coverage)
• Posted the verdict. gh pr comment was refused by this job's permission rules, so the review is posted here instead.

The Blocker from review 1 is fixed in 2a19bdc. That commit is the only change since de1a1b5, and it is all this review covers.

• doctor_artifact.py:194 now uses Path(mcp_server.__file__).parent.resolve(strict=True), and doctor_pairing.py:103 uses Path(plugin).resolve(strict=True). With strict=True, a symlink loop raises RuntimeError on 3.11 and 3.12, and OSError (ELOOP) on 3.13 and later. Both are in the except tuple, so on every Python allowed by requires-python >=3.11, each of the two checks fails on its own and names loop-a.
• The comments and the CHANGELOG entry now describe that behaviour correctly.
• Every existing test that patches mcp_server.__file__ points at a directory it created, so strict=True does not newly fail any of them.
• When CLAUDE_PLUGIN_ROOT points at a missing directory, the check still fails as OSError with the same reinstall fix. Only the path named in the message changes.

None of the five reviews found a Blocker. Performance and security found nothing.

Notes, no action before merge

• doctor_pairing.py:114 still uses non-strict Path(mcp_server.__file__).resolve().parent. On 3.13 or later, a loop on that path is reported as "unsupported pairing" instead of "cannot read". The pairing and artifact checks can also now name different roots when __init__.py is a symlink.
• The pairing loop test does not fail if strict=True is reverted, on any version: read_bytes() on plugin.json raises ELOOP anyway. The artifact loop test fails only on 3.13, and CI's 3.12 job passes either way.
• No test covers the new failure text when CLAUDE_PLUGIN_ROOT points at a missing directory.

Not verified: tests were not run in this review (the sandbox here did not allow pytest). Python 3.13 behaviour comes from tracing CPython's pathlib/posixpath.realpath, not from a 3.13 run.

Verdict: approve

CI autopilot : échec de unit (et de merge, qui agrège unit) corrigé dans c3b0209 (run 42309498918, fixup! de de1a1b5).

Diagnostic. Les tests passaient tous (12035 passed). Ce qui a échoué, c'est la porte de mutation sur les lignes modifiées (ADR-1122) : quatre mutants de strict=True survivaient, doctor_artifact._identify mutmut_2 et mutmut_4, doctor_pairing.plugin_pairing mutmut_19 et mutmut_21. Sous Python 3.12, strict ne change rien sur une boucle de liens, qui lève RuntimeError dans les deux cas. En revanche, il change le cas d'un répertoire absent, et aucun test ne le couvrait (c'était aussi la note 3 de la revue 2).

Correctif. Deux tests, sans changer le code :

• test_a_missing_plugin_root_is_named_not_its_manifest : un CLAUDE_PLUGIN_ROOT absent fait échouer le check en nommant ce répertoire, et non plugin.json.
• test_a_missing_package_directory_fails_the_check_only : un répertoire de paquet absent fait échouer le check, au lieu d'être décrit comme un arbre source vide.

Reçus locaux.

• GITHUB_BASE_REF=main python scripts/ci_mutation_diff.py reproduisait les quatre mêmes survivants avant ces tests. Après, il donne 2 file(s), 0 failed, 0 untested.
• Sans strict=True, les deux nouveaux tests échouent.
• Les tests du doctor donnent 66 passed et 1 skipped, sur 3.11, 3.12 et 3.13.

Non vérifié. Je n'ai pas relancé la suite complète localement. La CI de c3b0209 est en cours.

Generated by Claude Code

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

Review 3 of 3 (ADR-1119): PR #919

• Found the commits pushed since review 2 (2a19bdc): only c3b0209, which adds two tests and changes no source file
• Checked that the earlier Blocker is still fixed
• Ran all five reviews on c3b0209 (code quality, documentation accuracy, performance, security, test coverage)
• Run the tests and linters: blocked by this job's permission rules
• Posted the verdict. I couldn't post it with gh pr comment, because writing the comment body to a file was not permitted, so it is here instead.

The earlier Blocker is still fixed. Review 1 found a Blocker and review 2 confirmed the fix in 2a19bdc. Both strict resolves are still in place: doctor_artifact.py:194 and doctor_pairing.py:103. This review covers only c3b0209.

None of the five reviews found a Blocker. Performance and security found nothing.

• test_a_missing_plugin_root_is_named_not_its_manifest fails if strict=True is reverted at doctor_pairing.py:103, on every supported Python. A non-strict resolve lets manifest_version fail on <missing>/.claude-plugin/plugin.json, so the assertion "plugin.json" not in detail catches it. This test covers the failure text for a missing CLAUDE_PLUGIN_ROOT, which review 2 found untested, and kills the pairing strict=True mutant.
• test_a_missing_package_directory_fails_the_check_only fails if strict=True is reverted at doctor_artifact.py:194, on 3.11, 3.12 and 3.13. A non-strict resolve reaches _source_tree, and rglob returns no files, so the check reports ok=True with 0 files. The ok == False assertion catches that, so this mutant is now killed on CI's 3.12 job too. The test's comment ("not an empty source tree") is accurate.
• Neither test writes outside tmp_path, starts a subprocess or changes os.environ.

Accepted: the whole PR (de1a1b5, 2a19bdc, c3b0209, all 5 files). No Blockers are open, so nothing needs to go to a refinement PR.

Notes, no action before merge

• test_doctor_artifact.py:267 and test_doctor_pairing.py:208 check part of the path ("gone", missing.name), while the loop tests next to them check the full str(path).
• The CHANGELOG entry describes only the symlink loop. Strict resolve also makes a missing plugin root or package directory fail by name.
• doctor_pairing.py:114 still uses a non-strict Path(mcp_server.__file__).resolve() (carried over from review 2).

Not verified: this job's permission rules blocked pytest, ruff and check_craftsmanship.py, so none of them ran here. The behaviour of the mutants comes from reading the code, not from running it. CI is the check of record.

Verdict: approve