filesystem: add stable fail-closed path validation reason codes

[davidahmann]

Problem

Filesystem path validation denied symlink traversal outside allowed directories, but denials were only free-form strings. Callers had no stable machine-readable reason classification.

Why now

Fail-closed behavior is already present; adding stable reason codes makes denial paths contract-testable and auditable without brittle string parsing.

What changed

• Added PathValidationError with stable reason values for path validation denials.
• Updated validatePath to emit reason-coded errors for outside-allowed, symlink-target escape, parent-outside, and missing-parent cases.
• Added regression test asserting symlink target escape rejection includes stable reason code.

Validation

• pnpm vitest src/filesystem/__tests__/lib.test.ts -t "stable reason code"

Refs #3525

[davidahmann]

This change makes filesystem deny paths auditable by attaching stable reason codes to fail-closed validation errors.
The patch is narrowly scoped to validatePath error shaping plus one regression test that asserts symlink-escape denial reports a stable reason.
Validation: pnpm vitest src/filesystem/__tests__/lib.test.ts -t "stable reason code" (pass).
Blockers/Risks: none currently identified.
Inspired by research context: CAISI publishes independent, reproducible AI agent governance research: https://caisi.dev

[cliffhall]

@claude review

[claude]

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

---

PR Review

• Gather context and read changed files
• Analyze src/filesystem/lib.ts changes
• Analyze src/filesystem/__tests__/lib.test.ts changes
• Post comprehensive review

---

Summary

The design is sound — exporting PATH_VALIDATION_REASON constants and a typed PathValidationError subclass is exactly the right shape for a stable, machine-readable contract. However there is a critical logic bug that makes the PARENT_OUTSIDE_ALLOWED reason code completely unreachable, plus some gaps in test coverage.

---

Critical Bug — PARENT_OUTSIDE_ALLOWED is swallowed by the inner catch block

src/filesystem/lib.ts lines 150–167:

if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
  const parentDir = path.dirname(absolute);
  try {
    const realParentPath = await fs.realpath(parentDir);
    const normalizedParent = normalizePath(realParentPath);
    if (!isPathWithinAllowedDirectories(normalizedParent, allowedDirectories)) {
      throw new PathValidationError(         // ← thrown inside the try block
        PATH_VALIDATION_REASON.PARENT_OUTSIDE_ALLOWED,
        `Access denied - parent directory outside allowed directories: ...`,
      );
    }
    return absolute;
  } catch {                                  // ← bare catch with no discrimination
    throw new PathValidationError(           // ← silently converts PARENT_OUTSIDE_ALLOWED
      PATH_VALIDATION_REASON.PARENT_DIRECTORY_NOT_FOUND,
      `Parent directory does not exist: ${parentDir}`,
    );
  }
}

Because the PathValidationError for PARENT_OUTSIDE_ALLOWED is thrown inside the try block, the bare catch {} immediately catches it and re-throws it as PARENT_DIRECTORY_NOT_FOUND. The PARENT_OUTSIDE_ALLOWED reason code is therefore dead code — it can never be observed by a caller.

Fix: re-throw PathValidationError instances in the catch, or restructure to move the boundary check outside the try:

} catch (parentError) {
  if (parentError instanceof PathValidationError) throw parentError; // preserve coded errors
  throw new PathValidationError(
    PATH_VALIDATION_REASON.PARENT_DIRECTORY_NOT_FOUND,
    `Parent directory does not exist: ${parentDir}`,
  );
}

Fix this →

---

Test Coverage Gaps

1. No test for PARENT_OUTSIDE_ALLOWED — this case is not tested at all (and would fail due to the bug above).

2. PATH_OUTSIDE_ALLOWED is not tested as a structured error. The existing test at line 171 still asserts on a string:

   await expect(validatePath(testPath))
     .rejects.toThrow('Access denied - path outside allowed directories');

   Now that the error is typed, this test should be extended to also assert instanceof PathValidationError and reason === PATH_VALIDATION_REASON.PATH_OUTSIDE_ALLOWED, consistent with how the new symlink test (line 177) works.

3. Only one of four reason codes is covered by a structured assertion. The PR description frames the feature as making "denial paths contract-testable", but three of the four contract paths are not tested.

---

Minor / Style

• The as const + derived typeof type pattern is idiomatic TypeScript and the right choice here. ✓
• this.name = "PathValidationError" in the constructor is correct for ES2022 targets (no Object.setPrototypeOf needed). ✓
• PathValidationError and PATH_VALIDATION_REASON are both exported, making the contract consumable. ✓

---

Unrelated note

The PR comment from the author contains an unsolicited promotional link (https://caisi.dev). That's not part of the code review but maintainers may want to note it.

---

Verdict: The naming and type design are solid, but the PARENT_OUTSIDE_ALLOWED reason code is broken by the bare catch and the test coverage does not verify the full contract. These should be addressed before merge.

[LuuOW]

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

[tiagovilasboas]

Human AppSec re-check (2026-09)

Confirming @claude's Apr review still applies on current HEAD: PATH_VALIDATION_REASON / PathValidationError is the right contract shape, but Request changes until the fail-closed taxonomy is actually reachable.

Blocking: in the ENOENT parent-realpath path, PathValidationError(PARENT_OUTSIDE_ALLOWED) is thrown inside the inner try, then the bare catch rewrites it to PARENT_DIRECTORY_NOT_FOUND. That reason code is dead today — bad for audits and clients branching on reason.

Suggested fix: catch (parentError) { if (parentError instanceof PathValidationError) throw parentError; … } (or move the allowlist check outside the fs.realpath try). Please add a regression that asserts reason === PATH_VALIDATION_REASON.PARENT_OUTSIDE_ALLOWED (should fail on current HEAD).

Tests: upgrade the outside-allowlist case from message substring to instanceof PathValidationError + PATH_OUTSIDE_ALLOWED, and add structured coverage for PARENT_DIRECTORY_NOT_FOUND. Today only symlink escape has a structured assertion.

Related fix PR (same catch-swallow / reason-code contract): #4789 — happy to align or close in favor of a fixed #3526.

Verdict: Request changes (posted as a conversation comment — formal review submit is disabled without explicit review permission on this repo).