test(client): Agent Skills — root-swap races the descriptor walk does not close (SEC-8985 row 2)

[pkaeding]

What

Three failing tests, TestRootSwapRaces in packages/client/tests/test_skills_fs.py, stacked on #66. They state the contract the SDR response gives for SEC-8985 row 2 (every destructive operation runs relative to a descriptor "held for the duration of the reconcile") and show the code does not meet it yet. No implementation change in this PR.

The gap

_resolve_root validates the root once and returns a Path. Nothing holds it open. Each write and each prune then opens <root>/<key> by path with O_NOFOLLOW | O_DIRECTORY and pins that directory. O_NOFOLLOW guards only the final component, so the root and every ancestor are re-resolved on every such open. Swap the root for a symlink after validation and the open follows it: the descriptor is pinned to <outside>/<key>, and every descriptor-relative step behind it runs against the wrong directory. atomic_write_in does refuse the manifest rewrite afterwards (the run reports an error), but by then the skill file is already outside the root.

The existing TestSymlinkAttacks race tests swap <root>/<key> and prove that window is closed. These swap <root> itself.

Precondition: write permission on the root's parent, not on the root. The README privilege-separation checklist denies the agent identity the root, the skill directories, the files and the manifest, and says nothing about the parent. In the documented layout (<app>/.claude/skills) the parent is .claude, which the agent identity typically owns.

What the tests show

Test	Swap fires at	Result on this branch
create	os.mkdir(<root>/a)	SKILL.md written into <outside>/a/
clobber	first O_NOFOLLOW open of <root>/a	<outside>/a/SKILL.md, never in the manifest, replaced with served content
prune	first O_NOFOLLOW open of <root>/a	<outside>/a/SKILL.md unlinked, then <outside>/a removed by the rmdir

uv run pytest packages/client/tests/test_skills_fs.py -k TestRootSwapRaces:

FAILED TestRootSwapRaces::test_root_swapped_at_the_skill_directory_create_cannot_redirect_the_write
  AssertionError: assert [PosixPath('/.../outside/a')] == []
FAILED TestRootSwapRaces::test_root_swapped_at_the_skill_directory_open_cannot_clobber_an_outside_file
  AssertionError: assert 'served update\n' == 'precious\n'
FAILED TestRootSwapRaces::test_root_swapped_at_the_prune_cannot_redirect_the_unlink
  AssertionError: assert (False)        # victim.exists()

The swap is fired from an os.mkdir / os.open interceptor (_SwapRootDuring, the sibling of the existing _SwapDirectoryDuring), so it lands at the narrowest possible instant rather than by timing. Nothing in the implementation is touched.

Why the build is red

Deliberately. The tests assert the contract, the code violates it, and the failing run is the demonstration. They go green when the fix below lands, with no marker to remove. Rest of the suite on this branch: 727 passed, 3 failed (these three); ruff check and ruff format --check clean.

The fix (not in this PR)

Pin the root once at the top of write_skills and walk from it: root_fd = os.open(root, O_RDONLY | O_DIRECTORY | O_NOFOLLOW), then os.mkdir(key, dir_fd=root_fd) and os.open(key, O_DIRECTORY | O_NOFOLLOW, dir_fd=root_fd) for each skill directory, and atomic_write(..., dir_fd=root_fd) for the manifest. os.open, os.mkdir, os.stat, os.unlink and os.replace all take dir_fd on POSIX and safe_fs.SUPPORTS_DIR_FD already probes for them. If the fix is not taken, the minimum is adding the root's parent to the README verification checklist and changing the "held for the whole reconcile" sentence in the response and the design-doc comment to "the skill directory is pinned per operation".

Companion PR with the same three tests for the TypeScript SDK: launchdarkly/js-ai-sdk#49.

🤖 Generated with Claude Code

via LD Research 🤖