Classify filesystem root and path validation failures

[davidahmann]

Problem
Filesystem clients need to distinguish path-outside-roots and missing-parent failures without parsing ambiguous free-form text.

Why now
The startup path already emits machine-readable validation receipts in the built server, but validatePath still surfaced several denial classes as generic text. Tightening that taxonomy makes the reference server easier to validate in clients.

What changed

• added stable error-code prefixes for out-of-roots, missing-parent, invalid-path, and permission-denied validation failures in validatePath
• updated filesystem tests to assert the more explicit path-validation surface while keeping startup validation tolerant of the existing structured startup receipt

Validation

• npm test -- __tests__/startup-validation.test.ts __tests__/lib.test.ts ✅

Refs #3606

[davidahmann]

Operator impact: filesystem clients now get explicit path-validation classifications for out-of-roots, missing-parent, invalid-path, and permission-denied failures instead of relying on ambiguous free-form messages.

Minimal change: validatePath now emits stable code-prefixed errors and the filesystem tests were updated to assert that narrower validation surface while remaining tolerant of the existing startup receipt shape.

Validation:

• npm test -- __tests__/startup-validation.test.ts __tests__/lib.test.ts

Risk/blocker: none in the changed path.
Inspired by research context: CAISI publishes independent, reproducible AI agent governance research: https://caisi.dev

[Chelebii]

Heads-up: the CI failures on Build fetch, Build git, and Build time are caused by Python lockfile drift (src/fetch/uv.lock) rather than the filesystem changes in this PR.

I opened a dedicated fix in #3598 to refresh the lockfile.

Local verification on the fix branch:

• uv sync --locked --all-extras --dev — passed
• uv run --frozen pyright — 0 errors
• uv run --frozen pytest — 20 passed

Once #3598 is merged, the Python-side CI should turn green for this PR as well.

[Chelebii]

I traced the failing Python CI job (Build fetch) and it has the same root cause as #3610: src/fetch/pyproject.toml and src/fetch/uv.lock are out of sync.\n\nThe failure is:\n\n ext\nThe lockfile at uv.lock needs to be updated, but --locked was provided.\n\n\nI prepared the minimal fix here:\n- https://github.com/Chelebii/servers/pull/1\n\nIt refreshes src/fetch/uv.lock so the
equires-dist metadata matches httpx>=0.27.\n\nIf you want, you can cherry-pick this commit directly:\n- 785b340\n

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 11s —— View job

---

PR Review: Classify filesystem root and path validation failures

• Gather context and read changed files
• Analyze diff and code changes
• Provide review feedback

---

Summary

This PR introduces machine-readable error code prefixes to validatePath in lib.ts, making it easier for clients to programmatically distinguish between the four denial classes without parsing free-form text. The changes are minimal and focused. Overall the approach is sound, with a few items worth addressing before merge.

---

Core Change (src/filesystem/lib.ts)

The new helper function (lib.ts:13-15) is clean:

function formatFilesystemValidationError(code: string, detail: string): Error {
  return new Error(`[${code}] ${detail}`);
}

The [code] bracket format is consistent with the existing [missing_roots] pattern already used in index.ts:86 and index.ts:749. Good alignment.

The four new codes are:

Code	Trigger
path_invalid	path.resolve / relative path resolution throws
path_outside_allowed_roots	path or symlink target is outside roots
path_parent_missing	parent directory does not exist
path_permission_denied	EACCES / EPERM on fs.realpath

---

Issues

1. Overly broad catch for parent directory errors (lib.ts:152-154)

} catch {
  throw formatFilesystemValidationError('path_parent_missing', `Parent directory does not exist: ${parentDir}`);
}

This bare catch wraps all parent-directory errors — including EACCES on the parent — as path_parent_missing. A permission-denied failure on the parent directory would be mislabeled. Consider inspecting the error code here too:

} catch (parentError) {
  if ((parentError as NodeJS.ErrnoException).code === 'EACCES' || (parentError as NodeJS.ErrnoException).code === 'EPERM') {
    throw formatFilesystemValidationError('path_permission_denied', `Permission denied: ${parentDir}`);
  }
  throw formatFilesystemValidationError('path_parent_missing', `Parent directory does not exist: ${parentDir}`);
}

Fix this →

2. Missing test coverage for two of the four new codes

path_invalid (triggered by invalid path construction) and path_permission_denied (triggered by EACCES/EPERM on fs.realpath) have no assertions in lib.test.ts. The test for path_permission_denied is straightforward to add using the existing mock pattern.

Fix this →

3. Stale alternatives in startup test regex (startup-validation.test.ts:85)

expect(result.stderr).toMatch(
  /argv_no_accessible_directories|missing_roots|Error: None of the specified directories are accessible/
);

argv_no_accessible_directories does not appear anywhere in the current codebase. The three-way alternation obscures the actual expectation. Consider simplifying to just assert what the code actually emits:

expect(result.stderr).toContain('[missing_roots]');

4. The path_invalid branch silently absorbs path_invalid input (minor)

The branch catches an exception from path.resolve — which in practice can only be triggered by truly pathological inputs on unusual OSes. The existing catch {} is safe, but it's also worth noting that this branch currently has no test that can trigger it in the vitest suite (since path.resolve on Node doesn't typically throw).

---

Style Notes

• The snake_case error codes are consistently formatted across all four cases.
• The helper function avoids any duplicated string-building logic — good.
• The formatFilesystemValidationError name is clear and mirrors the scope of validatePath.

---

Non-code Concern

The PR author's comment contains promotional content appended at the end:

"Inspired by research context: CAISI publishes independent, reproducible AI agent governance research: https://caisi.dev"

This is unrelated to the change and looks like spam text. Worth noting for repo maintainers.

---

Verdict

The core change is correct and a genuine usability improvement for clients. The two missing test cases (especially path_permission_denied) and the overly broad catch on parent-dir errors are worth fixing before merge. The rest are minor.

[Chelebii]

Follow-up branch with the Claude review fixes:

• branch: Chelebii:pr3607-validation-followup
• commit: 92fafafc6b56ce6a6b06b18a07bca8706faf2783
• commit URL: Chelebii@92fafaf

Included changes:

• narrow the parent-directory catch so EACCES/EPERM become [path_permission_denied] and other structured errors are preserved
• add regression tests for [path_invalid], [path_permission_denied], and parent-outside-root handling
• tighten the startup validation assertion to the actual [missing_roots] output

Validation:

• npm run test --workspace @modelcontextprotocol/server-filesystem -- __tests__/lib.test.ts __tests__/startup-validation.test.ts
• npm run build --workspace @modelcontextprotocol/server-filesystem

If useful, you can cherry-pick 92fafaf onto this PR branch.