[leonlaiyc]

Problem / Motivation

Goal: the tests for reading a session-pid mapping file without O_NOFOLLOW run on Windows, the platform that read path exists for, instead of failing there before they reach it.

TestNoNofollowPlatform in test/test_session_pid_sig.py covers session_pid_sig._read_regular_nofollow on a platform without O_NOFOLLOW: an lstat pre-check that refuses a symlink, then a (st_dev, st_ino) identity check that refuses a file swapped in between the lstat and the open. Its docstring names Windows as that platform, and each test simulates it on POSIX with monkeypatch.delattr("os.O_NOFOLLOW").

delattr raises AttributeError when the attribute is absent, which is the case on Windows. All three tests fail there at that line, and all three are strict xfails in test/windows-expected-failures.txt.

Why it matters

The mapping directory is agent-writable, so these checks are what keep the trusted MCP process from following a planted symlink when it reads a session's pid mapping. On Windows they are the only defence, and today they run only on POSIX hosts that remove the flag to imitate Windows, never on the Windows matrix.

Not a goal

• _read_regular_nofollow itself; no product code changes.
• Other tests that remove a POSIX-only os attribute: the rest already check for it first or are POSIX-only.

What changed (motivation → approach → change)

• Symptom: on Windows the three tests fail with AttributeError: module 'os' has no attribute 'O_NOFOLLOW' before reaching the code.
• Root cause: each test removes os.O_NOFOLLOW to make a POSIX host look like Windows, and monkeypatch.delattr refuses to remove an attribute that is not there.
• Change, in test/test_session_pid_sig.py: delattr(..., raising=False). POSIX still removes the flag; Windows, which has none, runs its own path. The class docstring says so.
• test_symlink_present_at_lstat_refused plants a real symlink, so it carries requires_symlinks from conftest.py. GitHub's Windows runners hold the privilege and run it; a Windows host without it skips it instead of failing on WinError 1314.
• The three strict-xfail lines are gone from test/windows-expected-failures.txt.

Backwards compatibility

Compatible: test-only; no product code changes.

Removes nothing: three strict-xfail lines go, and no assertion is dropped or loosened.

Tests

• test/test_session_pid_sig.py on a Windows host with the lines removed: 58 passed, 5 skipped, on Python 3.13.5 and on 3.12.13. test_regular_file_still_reads and test_lstat_open_swap_refused now pass there, the second because the identity check refuses the decoy the patched lstat vetted. This host has no symlink privilege, so test_symlink_present_at_lstat_refused is among the skips and runs on the Windows matrix of this PR.
• On main, the same host fails all three with the AttributeError above.
• No POSIX host here: the Linux and macOS matrix runs the unchanged simulated path.
• black, isort, flake8, check_comment_history, check_brand_name, and test_flake_pattern_ratchet.py + test_windows_gap_list.py (65 passed) pass.
• Sweep for the same mistake: git grep -n "delattr(" -- test/ src/, narrowed to os attributes, finds three other sites. test_embeddings_threads.py checks hasattr first, test_platform_compat_group_privacy.py skips without getxattr, and test_snapshot_merge_core_files_pinned.py is POSIX-only by its pytestmark.
• Trial merges with every open PR that edits test/windows-expected-failures.txt (test(windows): burn down six path-separator xfails #23777, test(windows): burn down the gateway-run xfails by patching the rlimit seam #23781, test(windows): pin USERPROFILE in the two subagent cwd tilde tests #23786, test(windows): write the materialize fixtures with LF, skip the symlink case #23788, test(windows): run the stdio probe's stderr stubs as Python scripts #23799, test(crew): give weak crew, agent and subagent tests real checks and merge twins #23887) are clean.

Proof: not provable -- test-only change

Manual verification

N/A — test-only change; the Windows runs above are the check, and the Windows matrix runs these tests on this PR.

Related Issues

Fixes #24025

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

UX Review (Opus 5.5, fork) — ⏭️ skipped

Revision 65c06b2d81372b14e2c468be826ce01c3c3944ab touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Does not block merge.

[github-actions]

First Principles Review (Opus 5.5, fork) — ⏭️ skipped

Revision 65c06b2d81372b14e2c468be826ce01c3c3944ab ships no reviewable capability (docs, tests or generated files only), so there is nothing to inventory. Does not block merge.

[github-actions]

Design Review (Opus 5.5, fork) — ✅ PASS

Design-level review of 65c06b2d81372b14e2c468be826ce01c3c3944ab via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

Design-Verdict: PASS

Nothing blocks: this is a test-only change on an existing seam. It reuses the requires_symlinks marker that other tests already use, and the Windows matrix runs the three un-xfailed tests on this PR.

[DESIGN-REVIEWED] 65c06b2

Description read

[DESCRIPTION-READ] 8edfe3eca90604133fc3ab58b654f6ec41dc1c296d527c1cbca7e1e6b50f7f5c

That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

[github-actions]

Opus 5.5 Review (fork) — ✅ no blocking findings

Reviewed 65c06b2d81372b14e2c468be826ce01c3c3944ab via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 65c06b2

[github-actions]

GPT 6.1 Review (fork) — ✅ no blocking findings

Reviewed 65c06b2d81372b14e2c468be826ce01c3c3944ab via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 65c06b2

[leonlaiyc]

The three reds on 65c06b2d81 are outside this diff, which changes only test/test_session_pid_sig.py and three list lines:

• Backend Tests (3.12, 2): test/test_native_skill_projection_rewrites.py::test_two_concurrent_releases_each_prune_their_own_view ("the second concurrent release skipped its prune on the busy in-process lock and its view lingered"). That is the open flake Flaky: test_two_concurrent_releases_each_prune_their_own_view sees a lingering view since #23913 #23965.
• Backend Tests (3.12, 7): test/test_side_readonly_spec_reap.py::test_an_unreadable_or_protected_checkout_keeps_its_variant ("ExceptionGroup: multiple unraisable exception warnings"). That is the open flake Flaky: test_an_unreadable_or_protected_checkout_keeps_its_variant (unraisable PosixPath finaliser at teardown) #23860.
• Security Scope Review: lane run 40202809375 failed at Resolve and validate PR (authoritative from GitHub), and the generate job was cancelled at 35 minutes, so the publish job posted [scope-floor:unsettled] ("the generate job did not finish"). It is the same lane failure as test(windows): pin USERPROFILE in the two subagent cwd tilde tests #23786's run 40114410361.

Coverage Gate then failed closed on those shards (backend-test=failure -- failing closed). It is not a coverage verdict. Design, Opus 5.5 and GPT 6.1 have no blocking findings on this head, and all eight Windows shards passed, including the three tests this PR moves off the expected-failures list. A fork cannot re-run workflows: could a maintainer re-run the two failed shards of the CI run and the Security Scope lane run 40202809375?

[leonlaiyc]

Closing as superseded by #25209, merged today. It makes the same change to the three TestNoNofollowPlatform tests in test/test_session_pid_sig.py (delattr(..., raising=False) and removing their lines from test/windows-expected-failures.txt), and covers the symlink test through test/requires-real-symlinks.txt. Rebasing this branch onto main would leave an empty diff, so nothing here remains to land.