[tsarlandie-oai]

Stacked on #1409. The base is DaleSeo/skills-fs.

Motivation

The catalog walks a skill directory and reads file contents later. If a regular file or one of its parent directories is replaced with a symlink between those steps, the read can publish a hidden or sibling file under the original skill URI. Both paths can be inside the configured catalog root.

Changes

• Enumerate each directory through its open handle. Open child directories relative to their parent with symlink following disabled.
• For each file read, reopen each path component relative to the preceding directory handle with symlink following disabled. Verify the opened file is regular before reading it under the existing byte budget. A failed reload keeps serving the previous snapshot.
• Use the cap-fs-ext companion to cap-std under the optional skills-fs feature.

Validation

• The regression test walks real files, installs replacement symlinks, and verifies that both a leaf swap and an ancestor swap fail before any replacement bytes are cached. It reproduced the issue on feat: load skills from a directory tree #1409 at 58f424a.
• cargo test -p rmcp --features skills-fs,client --lib --test test_skill_catalog passed all 298 library tests and 30 catalog integration tests on Linux. All 14 test_skills protocol tests also passed.
• A separate Linux check replaced a walked file with a FIFO. The read returned an error immediately with no writer attached.
• cargo clippy -p rmcp --no-default-features --features skills-fs --lib -- -D warnings, cargo +nightly fmt --all -- --check, and the repository spelling check passed.
• cargo check --target x86_64-pc-windows-gnu passed for the skills-fs library. The repository’s full CI workflow runs on PRs targeting main or release.

[chatgpt-codex-connector]

Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-07T06:07:34.323450Z	abf030a	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[github-actions]

@jamadeo is on triage rotation this week and is the reviewer for this one.

Labels come from the PR labeler. Please review the change.

_{Automated triage. Labels are a first pass and the assigned maintainer has the final say.}

[jokemanfire]

Reviewed the read path against the walk, and the ordering claim holds up:

• every ancestor is opened from its parent with open_dir_nofollow, and the leaf is opened with follow(No), so a component swapped after the walk fails closed instead of resolving;
• file.metadata() comes from the already-open handle, so there is no second path resolution between the type check and the read;
• nonblock(true) before the is_file() check is the right order for a FIFO substitution.

Four things I would like before this lands:

1. The nonblock flag has no test. The symlink test exercises the no-follow half, but nothing exercises "a FIFO was swapped in and we must not block forever". A #[cfg(unix)] test that mkfifos over a walked path and asserts an error (rather than a hang) would justify the flag; without it, a future cleanup could drop nonblock and reintroduce a trivial DoS.

2. The escape claim is only tested inside the root. public/assets -> ../.private is still inside the catalog root. Since the doc comment claims "no file outside the root is served", add a case where the link target is a second tempdir outside the root; it is cheap and it is the claim that matters.

3. expect("a walked file has a name") in read_regular_file. If Entry::path is ever produced by something other than walk (it is a plain String on a struct), this panics inside a request path. Returning InvalidInput costs one ok_or_else, and rejecting ../empty components at the same time removes a foot-gun without affecting walked paths.

4. Hard links are outside the guarantee. A hard link created inside the root that points at an outside file will still be served, because it is a regular file. That is a normal limitation of path-based confinement, but the doc comment currently reads as absolute; worth one clause so the security claim stays precise.

Not blocking: walk reads entry.metadata() for a Kind::File size, which can follow a symlink installed after enumeration. It is harmless today because the read path refuses symlinks, but if size feeds the byte budget, a note (or symlink_metadata) would keep the two paths consistent.