test(client): Agent Skills — the filesystem abuse matrix (5/5)

[XieX]

PR 5 of 5 splitting draft #45 for review. Stacked on #53. This is the tip of the stack — its tree is #45's, plus the changes for review findings 1, 2 and 3 and the new safe_fs unit tests.

main ← #50 ← #51 ← #52 ← #53 ← split/skills-fs-hardening

Note

Aug 28: rebased in place for the stack-wide bytes pivot (Skill.content: bytes, frontmatter() deleted). Test helpers here construct Skill values from encoded bytes now; the defenses under test are unchanged. This branch also carries the rename-containment assertion fix cherry-picked across the rebase.

What's here

Tests only, no source. The adversary: every filesystem defense PRs 3 and 4 introduced now has a test that fails if the defense is removed.

• Path traversal — a key that escapes the root, a key that is the manifest filename, an over-long key, and a key that resolves outside after realpath. Each refused before any filesystem call, and the containment check asserted on inode identity rather than on path strings.
• Symlink attacks — a symlinked skill directory, a symlinked target file, and <root>/<key> swapped for a symlink at the exact instant of the rename and of the unlink. That last one is the narrowest possible version of the window descriptor pinning exists to close, fired from the interception point (os.replace / os.unlink) rather than from implementation internals.
• The no-*at() shape — the path fallback Windows takes for every operation, exercised with the capability probe forced off, so the platform that cannot pin a descriptor is not the untested one. The TOCTOU tests skip off that same flag deliberately: a probe that wrongly reported "unsupported" must not also silently skip the tests that would have caught it.
• Non-regular files and clobber protection — a fifo or a directory where SKILL.md belongs, and a file at a managed path with no matching manifest entry: reported and left alone, never overwritten and never removed.
• Corrupt manifests — unreadable, unparseable, not an object, malformed entries, and a manifestVersion this release cannot read. No overwrites, no prunes, an error action naming the manifest, and the manifest itself left as it was found.
• Atomicity — a crash injected between the write and the rename leaves neither a partial file nor a temp file, and the one recorded rename is proved to have moved SKILL.md within the target's own directory. Where the platform has renameat that is asserted by descriptor identity (src_dir_fd == dst_dir_fd, resolving to the skill directory's (st_dev, st_ino)), which is stronger than comparing path strings because it also rules out the descriptor having been redirected between the check and the rename.
• Telemetry — the three signal names are asserted as an allowlist, not a floor: any other name reaching the emitter fails the test. The two deliberately excluded names are called out explicitly rather than left to the subset check. No signal carries a filesystem path or the skill body, an emitter that raises never fails the reconcile, and client.track() is never reached.

Notes for reviewers

Two cases are worth naming, because the obvious test does not reach the guard:

• The unencodable-content cases pin contentHash to the sha256 of the bytes a non-strict encoder would have fabricated. An arbitrary wrong hash is rejected by the mismatch check first and never exercises the encoder guard at all, so a naive test here passes against an implementation that reaches for errors="surrogatepass".
• The redaction cases smuggle the body through contentHash and through key. A sweep using a well-formed digest under a valid key reaches neither replacement branch.

The set of tests in test_skills_fs.py at this tip matches #45's modulo the bytes pivot (helpers build Skill.content as bytes; the deleted frontmatter() tests were removed in PR 1) — nothing else was dropped in the split. Two classes are reordered, which is why the file shows more diff churn than the content change warrants.

Testing

uv run pytest → 1280 passed. ruff check, ruff format --check, and mypy packages/*/src all clean.

🤖 Generated with Claude Code

---

Note

Overview
Tests only — expands test_skills_fs.py so every filesystem hardening behavior from earlier stack PRs has a regression test in one place (no implementation changes).

The module gains interception helpers (_ReplaceSpy, _SwapDirectoryDuring, inode-based _assert_atomic_rename_of) to assert SDK validation blocks writes before os.replace, and to simulate rename/unlink races and injected rename failures.

New coverage includes atomicity (0644, single in-directory rename, no partial files on failure), a parametrized path traversal matrix (hostile keys must not attempt SKILL.md renames), symlink refusal and descriptor-pinned TOCTOU cases (skipped when SUPPORTS_DIR_FD is false), a forced no-*at() Windows-shaped fallback, FIFO/non-regular paths, clobber protection, corrupt manifest fail-closed behavior, and telemetry allowlists (materialized/revoked/integrity only, no paths or bodies, manifest version redaction, integrity property parity across accessor vs write paths).

^{Reviewed by Cursor Bugbot for commit c88d5bf. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit e87c5f9. Configure here.}