This implements the feature requested in #3457. Happy to make any changes based on feedback.

@claude review

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

PR Review: feat(filesystem): add --follow-symlinks and --symlink-depth options

• Gather context and understand the request
• Read changed files (lib.ts, index.ts, symlinks.test.ts, README.md)
• Analyze security, correctness, and edge cases
• Post review feedback

Overall the implementation is well-structured and the circular symlink detection is correct. The default behavior is genuinely unchanged. I have a few findings ranging from a write-access security implication that needs clearer documentation, to a correctness issue in the depth-counting logic, to a handful of code-quality items.

Security: Write access through symlinks is not called out in the README

validatePath is used by both read and write tools (write_file, edit_file, move_file). When --follow-symlinks is on, validatePath can return a path like /etc/passwd as the "valid" target, and the caller will then write to it.

The README only says: "Allow symlinks to point to targets outside allowed directories" — nothing about writes. A user enabling this flag for NFS read access probably doesn't realize they've also enabled writes to arbitrary outside paths via a planted symlink.

Recommendation: Add a prominent note in README.md under the --follow-symlinks flag documentation:

⚠️ This flag applies to all operations including writes. A symlink inside an allowed directory can point to any file outside (up to --symlink-depth hops), and write tools will follow it. Only enable this flag if you trust the contents of your allowed directories.

Fix this →

Correctness: Double fs.realpath call on the non-symlink code path

lib.ts lines 199–220: when the path is not a symlink, there are two separate fs.realpath(absolute) calls with the same input:

// First call
let realPath: string;
try {
  realPath = await fs.realpath(absolute);
} ...

// Second call — same argument, always same result
let resolvedAbsolute = absolute;
try {
  resolvedAbsolute = await fs.realpath(absolute);
} ...

// Then both normalizedReal and normalizedResolved are checked,
// but they will always be equal

resolvedAbsolute is redundant — it will always equal realPath. The && condition on line 222 therefore reduces to !isPathWithinAllowedDirectories(normalizedReal, ...) alone. The second branch is dead code that also adds an extra syscall.

Fix this →

Correctness: Incorrect hop count in test comment

symlinks.test.ts line 198:

// Chain has 2 hops outside allowed dirs, exceeds maxDepth of 1

The chain is finalLink → link2 → link1 → targetFile. All three destinations (link2, link1, targetFile) land in forbiddenDir, so resolveSymlinkHopByHop increments hopsOutsideAllowed three times, returning [targetFile, 3]. The test still passes because 3 > 1, but the comment (and the companion test at line 209 that says "Chain has 2 hops outside allowed dirs, within maxDepth of 3") is wrong. maxDepth: 3 allows exactly 3 outside hops, which is exactly what this chain produces — so the "within" test is actually at the edge, not comfortably within.

Fix this →

Code quality: stats?.isSymbolicLink() optional chaining is covering for test mocks

lib.ts line 195:

const stats = await fs.lstat(absolute);
if (!stats?.isSymbolicLink()) {

fs.lstat either resolves with a valid Stats object or throws — it never returns null or undefined. The comment says: "Use optional chaining to handle test mocks where stats might be undefined". This is a test-driven workaround that muddies production code. Better to fix the mock, or if the mock genuinely needs to return undefined, use a proper type assertion or add an explicit null check with a throw. Optional chaining here silently treats undefined stats as "not a symlink", which could mask bugs.

Code quality: Test skipping pattern hides skipped tests

The symlink tests use this pattern:

if (!symlinkSupported) {
  console.log('   ⏭️  Skipping - symlinks not supported');
  return;
}

Vitest marks these tests as passed, not skipped. CI will look green even though most of the test suite didn't run. Use vitest's built-in mechanism instead:

let symlinkSupported: boolean;
beforeAll(async () => { symlinkSupported = await getSymlinkSupport(); });

it('test name', async function() {
  if (!symlinkSupported) this.skip();
  // ...
});

Or restructure using describe.skipIf.

Fix this →

Minor: --symlink-depth silently has no effect without --follow-symlinks

index.ts lines 110–117 parse and store --symlink-depth=N unconditionally, but if --follow-symlinks is not also passed, the value is stored in lib.ts and never consulted. A user who passes --symlink-depth=3 thinking it affects something will get no feedback. A one-liner warning at startup would help:

if (symlinkMaxDepth !== 1 && !followSymlinks) {
  console.error("Warning: --symlink-depth has no effect without --follow-symlinks");
}

Summary

The core logic (hop-by-hop resolution, depth enforcement, circular detection) is correct. The security model is sound given the flag is opt-in. The write-access documentation gap is the most important thing to address before merging.