Skip to content

Comments

fix(filesystem): add symlink startup test and clarify catch block comment#3361

Open
olaservo wants to merge 1 commit intomodelcontextprotocol:mainfrom
dandeliongold:fix/filesystem-symlink-followup
Open

fix(filesystem): add symlink startup test and clarify catch block comment#3361
olaservo wants to merge 1 commit intomodelcontextprotocol:mainfrom
dandeliongold:fix/filesystem-symlink-followup

Conversation

@olaservo
Copy link
Member

Summary

Follow-up to #3254 (symlinked allowed directories fix), implementing the two suggestions from the review:

  • Integration test: Add test in startup-validation.test.ts that spawns the server with a symlinked directory argument, verifying the startup code correctly produces both path forms. This closes the gap between the unit test in path-validation.test.ts (which manually constructs the allowedDirectories array) and the actual startup behavior.
  • Code comment: Clarify the catch block comment in index.ts to document the design tradeoff: when a directory doesn't exist at startup, only the unresolved form is stored, so full symlink support requires the directory to exist at startup.

Test plan

  • All 146 filesystem tests pass (7 test files)
  • New symlink startup test passes on Windows
  • Test gracefully skips on systems without symlink permissions (EPERM)
  • Build succeeds with no TypeScript errors

🦉 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

…ment

Follow-up to modelcontextprotocol#3254 (symlinked allowed directories fix).

- Add integration test in startup-validation.test.ts that spawns the
  server with a symlinked directory argument, verifying the startup
  code correctly stores both path forms
- Clarify catch block comment documenting the design tradeoff when a
  directory doesn't exist at startup (only unresolved form is stored)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant