PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The containment fix is correct, follows the file's own idiom, and its red-proof discriminates. One line introduces a failure mode this same file documents as forbidden 30 lines above — an unclassified resolution error escaping a security guard — and #17611 makes the precondition live rather than theoretical. That is a bounded repair inside the method, not a reshape.

Peer-Review Opening: The fix is right and the specimen choice is the good part — /atest/playwright/ is exactly the shape the old guard admitted, so the arm genuinely goes red against the old code rather than passing for adjacent reasons. Reusing ensureSandboxed's own path.relative idiom instead of hand-rolling a prefix compare is the second right call. One line needs to fail closed the way the rest of the file already does.

Full form, not micro — basis named: ai/mcp/server/** is a fleet-critical never-zone under §6.4, and the close-target carries the security label. Size does not buy the light path here.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Live #16508 and its Contract Ledger; #15818's prior disposition of this same guard; the changed-file list; FileSystemService.mjs at 2b6e63bfd0 including ensureSandboxed and canonicalize in full; current origin/dev at e794edc74e (pulled fresh — I reviewed against a 9-commit-stale worktree earlier today and will not repeat it); live #17611 for the cwd precondition.
• Expected Solution Shape: Replace the substring test with a resolved-path segment-containment check against the canonical suite root, leaving ensureSandboxed untouched as the outer gate. It must not hand-roll a prefix compare, must reject traversal and symlink forms that normalize outside, and must fail closed on any state where containment cannot be established — the discipline this file already states for itself.
• Patch Verdict: Matches the expected shape on the comparison and the specimen; contradicts it on failure-state handling. path.relative + isAbsolute + ..-segment is the same idiom ensureSandboxed: uses at its own boundary check, so the fix is consistent rather than novel — good. relative === '' is additionally rejected, correctly, because the suite root is a directory and not a spec; that is a real difference from the outer gate and the comment says why.
• Premise Coherence: Coheres with verify-before-assert — the ticket is a rediscovery of a residual #15818 named and could not home, and the PR treats it as decidable-today rather than waiting on #16481's architecture question. That split is the right one.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16508
• Related Graph Nodes: #15818, #16481, #17611; concepts sandbox-containment, path-canonicalization, mcp-file-system
• Origin Session ID: 728a756d-71df-48e6-8dad-0bac498ca23e

🔬 Depth Floor

Challenge: suiteRoot's fs.realpath sits outside every catch, so a resolution failure escapes this security guard as a raw fs error — the exact class this file forbids in its own words.

const
    safePath  = await ensureSandboxed(absolutePath),
    suiteRoot = await fs.realpath(path.resolve(process.cwd(), 'test/playwright')),   // ← unguarded
    relative  = path.relative(suiteRoot, safePath);

The method's only try wraps execFileAsync. So an ENOENT, EACCES on an ancestor, or ELOOP on that line propagates raw.

ensureSandboxed, in this same file, refuses precisely that and says why:

"A resolution failure — EACCES on an ancestor, ELOOP, a vanished parent — leaves containment UNPROVEN, and unproven must be refused, not surfaced as a raw fs error. A bare EACCES reads to a caller (and to an agent reading the tool result) as an I/O problem worth retrying, when the truth is that the jail could not answer."

It then throws its own classified 403 Forbidden: Canonical containment could not be established (<code>). The new line reintroduces the state that rationale exists to eliminate, one guard later.

The precondition is live, not hypothetical. path.resolve(process.cwd(), 'test/playwright') assumes the process cwd is the repo root. #17611 is OPEN and records that the Codex harness launches Neo's MCP servers with no cwd, so npm/process.cwd() resolves from wherever the GUI started. Under that condition this line throws ENOENT and run_playwright_test returns an unclassified filesystem error instead of a refusal.

Verified rather than reasoned: fs.realpath(path.resolve('/tmp','test/playwright')) → ENOENT, and nothing in the method catches it.

What I checked and did NOT find a problem with, since a security review should say what it cleared:

• The canonical claim holds. The comment asserts "safePath and suiteRoot are both canonical." ensureSandboxed returns canonicalize(path.resolve(absolutePath)), and canonicalize realpaths with correct dangling-symlink handling via lstat. So the two sides of path.relative are like-for-like — which is the thing that would silently break a containment compare.
• The red-proof discriminates. /…/atest/playwright/probe.spec.mjs returns true for the old includes('test/playwright/') and resolves to ../../tmp/… under the new check. The arm therefore goes red against the old code for the right reason, not an adjacent one.
• Cost of the acceptance arm is not a problem. It invokes the real runner, which looked expensive; measured instead of asserted — the whole spec is 18 passed in 10.6s locally, because no root Playwright config exists so the nested invocation fails fast and the arm still proves the guard admitted the path. I was about to raise this and the measurement killed it.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: claims match the diff; the containment framing is exactly what shipped.
• Anchor & Echo: the new JSDoc names the behavior (canonical path is inside the project suite) without overshooting into guarantees the method does not make.
• [RETROSPECTIVE] tag: N/A — none present.
• Linked anchors: #15818's prior disposition genuinely establishes this as a homed residual, not borrowed authority.

Findings: Pass on drift; the finding is behavioral and sits in Required Action 1.

🧠 Graph Ingestion Notes

• [KB_GAP]: A guard that computes its own reference point inherits that computation's failure modes. ensureSandboxed classifies its resolution failures; a second guard resolving a second root must classify its own, or the method's error contract is only as good as its most recently added line.
• [TOOLING_GAP]: None. Reviewed at 2b6e63bfd0 against a freshly pulled origin/dev (e794edc74e); the containment behavior was executed rather than read.
• [RETROSPECTIVE]: Reusing ensureSandboxed's path.relative idiom rather than inventing a prefix compare is what makes this diff easy to trust — the reviewer checks one pattern twice instead of two patterns once. The formatting-alignment noise in the same commit is harmless here but does make the security-relevant hunk share a diff with cosmetic churn.

🎯 Close-Target Audit

• Close-targets identified: #16508
• Confirmed not epic-labeled — bug, ai, architecture, security.

Findings: Pass.

📑 Contract Completeness Audit

• #16508 carries a Contract Ledger row for the guard.
• The diff matches it: segment-boundary containment against the resolved suite root, ensureSandboxed untouched as the outer gate.
• The Ledger's "Fallback / Error Semantics" column says "Any path not resolving inside the root is refused" — the unresolvable-root case is not covered by the implementation. See RA-1.

Findings: Contract drift confined to the error-semantics cell.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 2b6e63bfd0; author's four new arms map 1:1 onto ACs 2–4.
• Reviewer falsifier: ran the spec locally after applying the patch to fresh dev — 18 passed (10.6s); separately confirmed the old guard admits the specimen, so the red-proof is real.
• Test location: correct — test/playwright/unit/ai/mcp/server/file-system/, mirroring the source path.

Findings: Pass. AC-5 also holds: ensureSandboxed is untouched in the diff.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: no runtime AC beyond unit reach (Evidence), no OpenAPI surface (MCP budget), no skill/convention surface (Cross-Skill).

📋 Required Actions

To proceed with merging, please address the following:

• Classify the suite-root resolution failure instead of letting it escape. Bring suiteRoot's fs.realpath inside a guard that refuses with the file's own vocabulary — the 403 Forbidden: … could not be established (<code>) shape ensureSandboxed already uses — so an unresolvable suite root is a refusal rather than a raw ENOENT/EACCES/ELOOP. #17611 makes this reachable today: an MCP server launched without a cwd resolves test/playwright somewhere it does not exist. An unproven boundary must fail closed, which is this file's stated rule and the reason I am blocking on one line in an otherwise correct fix.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 - Correct layer, correct method, and it reuses the containment idiom already proven in this file rather than minting a second one. Capped only by the new line's divergence from the file's own error-classification discipline.
• [CONTENT_COMPLETENESS]: 85 - New JSDoc is precise and the inline comment explains both the segment-boundary reasoning and the deliberate relative === '' exclusion. The @throws does not mention the unresolvable-root path, which is the same gap as RA-1.
• [EXECUTION_QUALITY]: 80 - Behavior verified by running it: containment holds, traversal and symlink forms refuse, the legitimate path passes. One unhandled rejection path keeps this out of the 90s.
• [PRODUCTIVITY]: 95 - Closes a residual that outlived its original ticket and was independently rediscovered from outside; four arms, all AC-mapped.
• [IMPACT]: 80 - The narrower of two guarantees, but the one that matters, because a Playwright spec is arbitrary JavaScript and this method executes it.
• [COMPLEXITY]: 40 - Small diff over subtle ground; the correctness argument depends on both sides of the compare being canonical, which required reading two helpers to confirm.
• [EFFORT_PROFILE]: Quick Win - one guard, high security value, one line from mergeable.

🖖 Reviewed by Grace (Claude Opus 5, Claude Code). Session 728a756d-71df-48e6-8dad-0bac498ca23e.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z