Skip to content

Add structured filesystem startup validation errors - #3513

Open
davidahmann wants to merge 1 commit into
modelcontextprotocol:mainfrom
davidahmann:codex/issue-3512-fs-startup-errors
Open

Add structured filesystem startup validation errors#3513
davidahmann wants to merge 1 commit into
modelcontextprotocol:mainfrom
davidahmann:codex/issue-3512-fs-startup-errors

Conversation

@davidahmann

Copy link
Copy Markdown

Problem
Filesystem server startup failures for missing or invalid roots were only emitted as free-form stderr text, which makes automated classification and audit receipts brittle.

Why now
Issue #3512 asked for a machine-readable startup taxonomy. Operators need stable error codes when validating fail-closed root configuration in CI and wrapper processes.

What changed

  • emit structured JSON-line startup validation events alongside existing human-readable stderr
  • cover argv validation failures for inaccessible paths, non-directories, and "no accessible directories"
  • reuse the structured emission for MCP roots validation paths

Validation

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

Refs #3512

@davidahmann

Copy link
Copy Markdown
Author

User impact: Wrapper processes can now classify filesystem startup failures deterministically instead of scraping only free-form stderr.
Change: The filesystem server now emits structured JSON-line startup validation events alongside the existing human-readable messages and covers the argv validation cases in tests.
Validation:

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

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

@cliffhall cliffhall added bug Something isn't working server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem labels Apr 20, 2026
@cliffhall

Copy link
Copy Markdown
Member

@claude review

@claude

claude Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: Add structured filesystem startup validation errors

  • Gather context and understand the request
  • Read changed files
  • Analyze code quality, correctness, and security
  • Post review feedback

The approach is clean and the general design is sound — a new startup-errors.ts module with a typed event emitter, wired into both the argv validation path in index.ts and the roots validation path in roots-utils.ts. Tests are integration-style and sensibly spawn the real server binary.

However there are a few issues worth addressing, one of which is a semantic correctness problem.


Issues

1. kind: 'filesystem_startup_validation' is wrong for runtime roots changes (semantic bug)

getValidRootDirectories is called not only at startup but also mid-session whenever the client sends a RootsListChangedNotificationSchema notification (index.ts:740-750). Emitting kind: 'filesystem_startup_validation' during a live roots update is semantically incorrect and will mislead operators who rely on this field for classification.

// roots-utils.ts — called at both startup AND runtime
emitStartupValidationEvent({
  kind: 'filesystem_startup_validation',  // wrong when called from a runtime roots update
  source: 'roots',
  ...

The fix is either to (a) rename the kind to something source-agnostic like 'filesystem_validation', or (b) pass the kind as a parameter so callers can set it contextually. Fix this →


2. StartupValidationEvent type is not exported (startup-errors.ts:11)

The type is defined but unexported, so the test file is forced to re-declare it locally (startup-validation.test.ts:9-16). Since the PR's stated goal is to let operators parse these events programmatically, the type should be exported so external consumers can import it.

// startup-errors.ts — line 11
type StartupValidationEvent = {   // ← should be `export type`

Fix this →


3. paths in argv_no_accessible_directories event contains expanded paths, not original args (index.ts:103-108)

// index.ts — line 103
emitStartupValidationEvent({
  code: 'argv_no_accessible_directories',
  source: 'argv',
  paths: allowedDirectories,   // ← these are normalized/symlink-expanded paths
  message,
});

By this point, allowedDirectories has been through expandHome, path.resolve, and potentially fs.realpath. On macOS, a single user-provided /tmp/foo might appear as both /tmp/foo and /private/tmp/foo in this array. Operators correlating the event against their original argv will be confused by paths they never specified. The paths field should contain the original args (before expansion), or the event should document this clearly. Fix this →


4. No test coverage for source: 'roots' events

The startup-validation.test.ts only covers the argv source path. The changes to roots-utils.ts add three new emitStartupValidationEvent calls (root_invalid_or_inaccessible, root_not_directory, root_validation_error) with zero test coverage. The existing roots-utils.test.ts also doesn't verify that structured events are emitted at all. Adding a spy/mock on emitStartupValidationEvent in roots-utils.test.ts would be the lightweight path.


Minor observations

  • Double-emission pattern: Each error produces two stderr lines — the human-readable console.error(message) immediately followed by the JSON event (which also contains message). This is intentional per the PR description, but worth documenting in the module: parsers need to skip non-JSON lines rather than trying to parse every stderr line.

  • formatDirectoryError helper is only used in roots-utils.ts: The helper is kept there, which is correct. No issue, just noting it wasn't moved to startup-errors.ts.

  • roots-utils.ts:95: catch (error) in the root_validation_error case — since parseRootUri already resolves the realpath, a stat failure here typically means a race condition (directory deleted between realpath and stat). This is fine but worth a comment given how unusual the case is.


Summary

The feature is useful and the implementation is mostly correct. The semantic issue with kind: 'filesystem_startup_validation' being emitted during live roots updates is the most important fix — it will break operator classification if they rely on this field. The missing type export and the paths field contents are the next priorities.

@LuuOW LuuOW left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technical audit: Verified MCP server implementation for consistency with current SDK patterns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants