Fall back to an NTFS junction when the symlink escape test can't create a symlink

[maxisbey]

Fixes #3408

test_safe_join_rejects_symlink_escape called symlink_to() unguarded. Windows only allows that for elevated processes or with Developer Mode on, so on an ordinary Windows dev machine the test errored with WinError 1314 before it ever reached safe_join. CI didn't notice because the hosted Windows runners are allowed to create symlinks.

What changed

Test-only, one file (tests/shared/test_path_security.py, +9/-1):

• The symlink_to() call is wrapped in try/except OSError.
• Anything other than winerror == 1314 on win32 is re-raised unchanged, so a real symlink failure still fails the test on every platform.
• For that one error the test creates a directory junction with _winapi.CreateJunction instead. Junctions need no privilege and Path.resolve() follows them like a symlink, so the PathEscapeError assertion still runs.

A plain pytest.skip would be smaller, but the escape assertion would then not run on exactly the machines this is for, and since tests/ counts towards the 100% coverage gate, ./scripts/test would stay red there anyway.

The except arm is marked # pragma: lax no cover because it only runs on Windows machines that can't create symlinks, which CI doesn't have. The import _winapi is inline because the module only exists on Windows.

How it was checked

• The test file passes on Linux on Python 3.10, 3.11, 3.12, 3.13 and 3.14.
• ruff check, ruff format --check and pyright are clean for the file. pyright --pythonplatform Windows is clean too, so the exc.winerror and CreateJunction(str, str) usage type-checks against the Windows stubs.
• Coverage for the file is 100% and strict-no-cover passes.
• The fallback's control flow was exercised on Linux by running the real test function with Path.symlink_to patched to raise and a fake _winapi, on each of the Python versions above:
  • win32 + winerror 1314: CreateJunction is called once as (target, link) and the test passes
  • win32 + winerror 5: the original OSError is re-raised, no junction
  • non-Windows OSError (which has no winerror attribute): the original error is re-raised, no AttributeError

Not verified: the junction arm has not been run on a real unprivileged Windows machine. I only have Linux here, and CI's Windows runners take the symlink path, so they won't exercise it either. If someone who hit #3408 can run uv run --frozen pytest tests/shared/test_path_security.py on this branch, that would close the gap.

_{AI Disclaimer}

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked the premise of the junction fallback: safe_join resolves via Path.resolve() (src/mcp/shared/path_security.py:158,171), and on Windows os.path.realpath has followed junctions as well as symlinks since Python 3.8, so the PathEscapeError assertion should still exercise the escape path on the junction arm. The exc.winerror access is also safely behind the sys.platform != "win32" short-circuit on non-Windows.

Extended reasoning...

Test-only change in tests/shared/test_path_security.py wrapping a symlink_to call in a try/except that falls back to _winapi.CreateJunction on WinError 1314; no src/ or security-sensitive runtime surface is touched. The inline findings cover the convention points (inline import, new lax pragma, pre-existing match=); the note above records that the junction-vs-symlink resolution behaviour and the winerror guard were examined and ruled out as problems.

[claude]

🟡 nit (optional): maintainers get an inline import _winapi inside the except arm at tests/shared/test_path_security.py:151, which the repo's import rule asks to keep at file top. The rule allows an inline import only when a top-level one genuinely cannot work; _winapi is Windows-only, but a top-level if sys.platform == "win32": import _winapi (the pattern already used in src/mcp/os/win32/utilities.py:19) does work on every platform. Fix: hoist the import into a platform-guarded top-level block (with # pragma: lax no cover if needed for the Linux cells) and keep the except arm to the CreateJunction call, or state why the guarded top-level form is unsuitable here. [also at: tests/shared/test_path_security.py:151 - nit: AGENTS.md requires all imports at the top of the file, with inline imports allowed only when a top-level import genuinely can't work.]

Why this was flagged

The diff adds import _winapi at tests/shared/test_path_security.py:151, inside the except OSError arm that only runs on Windows machines that cannot create symlinks. AGENTS.md says all imports go at the top of the file, with the sole exception of a top-level import that genuinely cannot work. An unconditional top-level import _winapi fails on Linux, but a sys.platform == "win32" guarded top-level import (as in src/mcp/os/win32/utilities.py:19) works everywhere, so the exception is at best borderline here. Nothing fails at runtime: on Linux the sys.platform != "win32" check at line 149 re-raises before the import is reached, and on Windows the module exists. The cost is the one the rule names: the Windows-only dependency on a private CPython module is hidden inside a branch instead of being visible in the file's import block.

Verification: nit. Rule in AGENTS.md:56-59. The diff adds import _winapi inside the except OSError arm at tests/shared/test_path_security.py:151. The repository's own established pattern at src/mcp/os/win32/utilities.py:19-25 shows a platform-guarded top-level import works on every platform. Nothing fails at runtime; this is purely the written import convention not being followed.

[claude]

🟡 nit (optional): AGENTS.md says to avoid adding new coverage pragmas, and documents # pragma: lax no cover as for "lines covered on some platforms/versions but not others". The diff adds # pragma: lax no cover on the except OSError arm, which the PR description says no CI matrix entry executes (hosted Windows runners can create symlinks), so it is not covered on any platform CI runs. Fix: either restructure so the fallback is exercised by a test on all platforms (e.g. a small helper with symlink_to patched to raise OSError(winerror=1314) and a stub _winapi in sys.modules), or, if a pragma must stay, use the form whose documented meaning matches the line.

Why this was flagged

Nothing fails at runtime. The instruction guards the 100% branch-coverage gate from accumulating excluded lines; here the excluded block contains real logic (the winerror check and the CreateJunction call) that the PR itself says has not been run on a real unprivileged Windows machine, and the pragma also hides the if ... raise branch arcs. The author reports exercising the control flow manually on Linux via patching, but that check is not committed as a test. Small consequence: a future regression in the fallback (wrong argument order to CreateJunction, AttributeError on winerror) would go unnoticed by CI.

Verification: AGENTS.md (base d639cf7) "Coverage" section: "Avoid adding new # pragma: no cover, # type: ignore, or # noqa comments." with the audit command git diff origin/main... | grep -E '^\+.*(pragma|type: ignore|noqa)', and it documents # pragma: lax no cover as the form to "Use for lines covered on some platforms/versions but not others."

[claude]

🟣 pre-existing, not blocking: Maintainers keep a test that pins SDK-authored error text via match="escapes base" at tests/shared/test_path_security.py:155, which the test-quality skill forbids. The PR touches this very test (lines 146-153) and now adds a junction arm whose only assertion is this message match, so a wording change in src/mcp/shared/path_security.py:174 would fail the test for a non-behavioural reason. Fix: assert the raise via pytest.raises(PathEscapeError) and check .code or snapshot the message per the skill, covering both this test and the sibling match="null byte" tests at lines 131/136.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

This is the only ledger entry, so it is nominated by requirement; the dismissal's pre_existing claim is accurate (base commit has the same match="escapes base" at the same line). The trigger is any rewording of the f-string at src/mcp/shared/path_security.py:174 (f"Path {target} escapes base {base_resolved}"); the test at tests/shared/test_path_security.py:155 then fails on text rather than behaviour. The PR modifies the body of this test (lines 146-153) and routes a new junction arm into the same assertion, so the author was editing the test and the skill's rule (.claude/skills/test-quality/SKILL.md:60: never match= on message text) applies to the touched test. Base branch behaves identically; the consequence is test fragility, not a production defect. Remedy: drop the match= and assert the error type or .code.

Verification: pre-existing. Triggering condition: any rewording of the f-string at src/mcp/shared/path_security.py:174 makes tests/shared/test_path_security.py:155 fail on text rather than behaviour. However, the violating line is NOT introduced by this PR: git show d639cf7f:tests/shared/test_path_security.py has the identical match="escapes base" at line 147.