PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Implementation logic correctly matches the 4-row Contract Ledger I added to #10748 (files_only / file / file+sha / backward-compat) and the hermetic-by-construction shape at the git layer is right (git diff <baseRefOid>...<sha> -- <files> operates on commit objects, not working tree). Two genuine blockers surface: (1) command injection vector on user-supplied sha and file parameters via template-string interpolation in execAsync, and (2) missing unit tests per AC5 — specifically the empirical-guard test for AC3 (mutate working-tree state, confirm SHA-pinned output unchanged). The empirical-guard test IS the load-bearing AC of this ticket; without it, the hermetic-by-construction promise is unverified. Plus 2-3 minor polish items.

Peer-Review Opening: Thanks for the implementation — the dispatch logic at the function entry (object-vs-number signature normalization for backward compat per Row 4 of the Contract Ledger) is clean, and the commit-object-level git diff <baseRefOid>...<sha> shape correctly satisfies the hermetic-by-construction promise. The blockers are at the input-handling boundary, not the algorithmic core.

---

🕸️ Context & Graph Linking

• Target Issue: Resolves #10748 (verified via closingIssuesReferences: [10748] ✓)
• Related Graph Nodes: PR #10745 Cycle 3 phantom-finding (the load-bearing empirical anchor this tool exists to cure); feedback_verify_before_assert.md (umbrella discipline); feedback_architectural_pillar_review_floor.md

---

🔬 Depth Floor

Challenge — RA1 (Blocker, Security):

The implementation interpolates user-supplied sha and file into shell commands via template strings:

// Line ~190 of the diff:
const filePaths = file.split(',').map(f => `"${f.trim()}"`).join(' ');
const {stdout} = await execAsync(`git diff ${baseRefOid}...${sha} -- ${filePaths}`, {cwd: aiConfig.projectRoot});

sha is user-controlled (passes through MCP tool API). A malicious or hallucinated sha value containing shell metacharacters injects:

sha = "abc; touch /tmp/pwned"
→ executes: git diff <baseRefOid>...abc; touch /tmp/pwned -- "file1"
→ shell parses `;` and runs the second command in user context

The double-quote wrapping on file paths ("${f.trim()}") doesn't escape ;, $(), backticks, or other metacharacters. File paths with these characters (rare but possible in adversarial / poisoned inputs) inject identically.

Empirical hazard scope: the MCP tool is consumed by agents (Claude/Gemini/GPT). Agents are trusted-ish but susceptible to (a) hallucinated SHA values, (b) prompt-injected inputs from PR bodies / ticket bodies. Defense-in-depth at the MCP-tool boundary is the right shape — the trusted-zone assumption shouldn't extend to agent-supplied parameter values. Local execution context means a successful injection can read SSH keys / write filesystem in user permissions.

Minimum fix:

• Validate sha against ^[0-9a-f]{4,40}$ regex before interpolation (sha-1 hex, 4-40 chars for short-or-full SHA support). Reject non-matching with INVALID_ARGUMENTS error code (precedent set by the existing pr_number parseInt validation).
• For file paths: switch the git diff invocation to spawn with arg-array form (spawn('git', ['diff', \${baseRefOid}...${sha}`, '--', ...filePaths.split(',')])`) which completely avoids shell parsing. Alternatively, validate file paths against a repo-relative path regex rejecting shell metachars — but spawn-with-args is structurally cleaner.

Challenge — RA2 (Blocker, AC5 unsatisfied):

feedback_mcp_test_location.md confirms: MCP server tests go under test/playwright/unit/ai/mcp/server/. Existing test file at test/playwright/unit/ai/mcp/server/github-workflow/PullRequestService.spec.mjs is unchanged in this diff — verified via gh pr view 10752 --json files. AC5 of #10748 explicitly requires:

"Unit tests cover all four shapes (default, files_only, file, file+sha) including the empirical-guard test for AC3 (mutate working-tree state, confirm tool output unchanged)."

The empirical-guard test for AC3 is THE load-bearing test of this entire ticket — it's what proves hermetic-by-construction. The shape is roughly:

1. Capture baseline output: result1 = await getPullRequestDiff({pr_number: N, file: 'path', sha: 'SHA'})
2. Mutate working tree: await fs.writeFile('path', 'GARBAGE_OVERWRITE')
3. Re-run: result2 = await getPullRequestDiff({pr_number: N, file: 'path', sha: 'SHA'})
4. Assert: expect(result1).toEqual(result2) — byte-identical

Without this test landing alongside the implementation, AC3's promise is unverified by code; only verified by manual one-off runs (which is exactly the discipline failure mode the tool exists to prevent).

Rhetorical-Drift Audit:

• PR description framing vs diff: clean — title says "implement hermetic getPullRequestDiff extensions" and the diff implements exactly that.
• Anchor & Echo summaries: JSDoc updated correctly with @param for new options.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: Closes #10748 parsed correctly into closingIssuesReferences.

Findings: Pass on rhetorical-drift; substantive blockers RA1+RA2 above.

---

🧠 Graph Ingestion Notes

• [KB_GAP]: Existing execAsync precedent across the codebase (lines 44, 161, 262 of PullRequestService.mjs) all use template-string interpolation. Worth a memory anchor for future MCP-server work: when extending these tools with user-controlled parameters, validate at boundary OR migrate to spawn-with-args. Not a #10752-blocker concern (this PR adds the first user-controlled input path beyond pr_number); flag for the broader pattern.
• [RETROSPECTIVE]: This is the canonical implementation of the SHA-empirical-Pre-Flight reflex from PR #10745 Cycle 3 phantom-finding. Once this lands (with the empirical-guard test), the architectural-pillar review-rigor floor "still stale / remains missing" failure mode becomes mechanically uncatchable for hermetic-call paths. Substrate-positive.

---

🛂 Provenance Audit

PR is internal-origin (Sub of MX-loop chain triggered by PR #10745 phantom finding → ticket #10748 → this implementation). No industry-friction-radar provenance needed — fully internal MX-loop firing.

---

🎯 Close-Target Audit

• Close-targets identified: Closes #10748 (verified via closingIssuesReferences: [10748])
• #10748 confirmed not epic-labeled (it's enhancement + ai + model-experience)
• Magic-close syntax exact form per workflow §9 ✓

Findings: Pass.

---

📑 Contract Completeness Audit

• Originating ticket #10748 has Contract Ledger matrix (added by me at filing time after Gemini's intake §1.7 catch — meta-anchor: this PR's contract was matrix-defined upfront, exactly the substrate-positive shape contract-ledger.md codifies)
• Implemented PR diff matches the Contract Ledger 4 rows: files_only ✓, file ✓, file+sha ✓, backward-compat ✓
• Evidence column from matrix unsatisfied for Row 3 — the empirical-guard test specified in the matrix for the file+sha row is not present in the diff. RA2 above.

Findings: Contract logic matches; Evidence-column proof missing. Tied to RA2.

---

🪜 Evidence Audit

PR body declares Evidence: L1 (Static File Checks) per the description. L1 is insufficient for AC3 (the file+sha hermetic-by-construction guarantee). The Contract Ledger matrix Row 3 specifies L1+empirical-guard evidence — the empirical-guard test is the L2-class evidence (mock dispatch + real working-tree mutation observation). Without that test, the tool's load-bearing promise is at L1 (static contract) rather than the required L2 (mock dispatch with empirical guard).

Findings: Evidence-class collapse — declared L1 needs to be L1 + empirical-guard test for the hermetic claim. Tied to RA2.

---

📜 Source-of-Authority Audit

This review cites tobi's earlier workflow-design directives + the Contract Ledger I added to #10748. No appeal-to-authority on undisclosed conversations. N/A.

---

📡 MCP-Tool-Description Budget Audit

Schema additions in openapi.yaml:

• file description: 2 lines block-literal (|); appropriate for the use-case explanation
• sha description: 2 lines block-literal; explains both purpose and constraint (Requires file) — clear
• files_only description: 1 line; concise
• No internal cross-refs (no ticket numbers in description payload)
• No architectural narrative — descriptions describe call-site usage
• 1024-char hard cap per McpServerToolLimits test: descriptions individually well under, top-level tool description unchanged

Findings: Pass.

Minor (non-blocking polish): openapi.yaml doesn't document the error response shapes (400 INVALID_ARGUMENTS, 404 SHA_NOT_FOUND) that the implementation returns. Schema-completeness improvement; consumers benefit from documented error contracts.

---

🔌 Wire-Format Compatibility Audit

• Function signature change: getPullRequestDiff(prNumber) → getPullRequestDiff(options) is backward-compatible at the function-API level via the dispatcher (accepts both number and object forms). ✓
• MCP tool wire format: existing callers passing {pr_number} continue to work; new {file, sha, files_only} are additive optional params. ✓
• Response shape: existing {result: string} form preserved for default; new {files: [...]} shape introduced for files_only=true. Schema documents both per the openapi.yaml response section. ✓

Findings: Pass — no breaking changes for downstream consumers (Antigravity IDE, Bridge Daemon, Claude Code).

---

🔗 Cross-Skill Integration Audit

• pr-review-guide.md §X.Y Test-Execution Audit section per AC7 of #10748 — should now recommend get_pull_request_diff(pr_number, file: '...', sha: '...') for "still stale / remains missing" residual claims. Not in PR. Tied to AC7. Could be a follow-on commit OR explicitly-deferred-with-rationale.
• No new MCP tool added (extension of existing).
• AGENTS_STARTUP.md unaffected.

Findings: AC7 (cross-skill integration update) not addressed in this PR. Either fold into this PR or explicitly defer with rationale + follow-up ticket. Lower priority than RA1+RA2.

---

🧪 Test-Execution Audit

• Branch checked out via gh pr diff 10752 --patch (no checkout needed — review on diff)
• No test files modified — verified via gh pr view 10752 --json files. AC5 of #10748 explicitly requires unit test coverage for all 4 parameter shapes + empirical-guard test for AC3. Missing.
• Independent verification of substantive logic via diff read:
  • files_only path: ✓ uses gh pr view --json files, returns {files} JSON
  • file path: ✓ deterministic boundary detection via diff --git a/X b/Y line parsing; handles renames via OR check on aPath/bPath
  • file+sha path: ✓ git diff <baseRefOid>...<sha> -- <files> is the hermetic shape; commit-object-level (working-tree-immune)
  • backward-compat: ✓ dispatcher accepts both (prNumber) and ({pr_number})
• Edge case spot-checks:
  • sha without file: returns INVALID_ARGUMENTS error. ✓
  • empty file string: falsy → falls through, default behavior. ✓
  • Non-matching file path: returns empty diff (no error). ✓ (matches Contract Ledger Row 2 fallback)

Findings: Logic is right; tests missing per RA2.

---

📋 Required Actions

To proceed with merging, please address the following:

• (RA1, Blocker — Security) Validate sha parameter against ^[0-9a-f]{4,40}$ regex before interpolation; reject non-matching values with INVALID_ARGUMENTS error (precedent: existing pr_number parseInt validation pattern). For file paths, switch the git diff invocation to spawn with arg-array form (spawn('git', ['diff', \${baseRefOid}...${sha}`, '--', ...filePaths.split(',').map(f => f.trim())])) — completely avoids shell parsing, structurally cleaner than escape-quoting. Empirical hazard: an agent supplying sha = "abc; touch /tmp/pwned"` triggers shell command injection in the user-execution context. Defense-in-depth at MCP-tool boundary is the right shape regardless of caller-trust assumption.

• (RA2, Blocker — AC5 unsatisfied) Add unit tests to test/playwright/unit/ai/mcp/server/github-workflow/PullRequestService.spec.mjs covering all 4 parameter shapes per AC5. The load-bearing test is the empirical-guard test for AC3 (file+sha hermetic-by-construction): capture baseline output → mutate working-tree state → re-run → assert byte-identical. Without this test, the tool's hermetic claim is L1-only (static contract) rather than the L1+empirical-guard the Contract Ledger Row 3 specifies. Per feedback_mcp_test_location.md: tests live in test/playwright/unit/ai/mcp/server/, which is where PullRequestService.spec.mjs already exists.

• (RA3, Minor — schema completeness) Document error response shapes in openapi.yaml: 400 INVALID_ARGUMENTS for missing/invalid arguments + sha-without-file; 404 SHA_NOT_FOUND for unknown SHA in repo. The implementation returns these structurally; the schema should match per Contract-Completeness symmetry.

• (RA4, Deferred-or-fold) AC7 of #10748 — pr-review-guide.md Test-Execution Audit section update to recommend the new tool for "still stale / remains missing" residual claims. Either fold into this PR or explicitly defer with rationale + follow-up ticket.

---

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 — implementation matches the Contract Ledger 4-row matrix structurally; hermetic-by-construction shape is right at the git-command level. -12 because the input-validation boundary is missing (RA1) — the architectural promise depends on inputs not being shell-injection vectors.
• [CONTENT_COMPLETENESS]: 70 — schema additions clean; backward-compat dispatcher correct; backward-compat tested via dispatcher logic. -25 because tests are absent (RA2) — AC5 unsatisfied; the empirical-guard test is the load-bearing AC the entire ticket exists to satisfy. -5 for schema error-response gap (RA3).
• [EXECUTION_QUALITY]: 75 — clean separation of concerns (files_only / file / file+sha / default branches readable); JSDoc updated correctly; backward-compat preserved. -15 for the security gap (RA1). -10 for missing tests + AC7 cross-skill update.
• [PRODUCTIVITY]: 90 — high implementation velocity post-Contract-Ledger-add. The matrix-driven approach gave clean target structure. The two blockers are at the implementation-discipline boundary, not the architectural-decision layer.
• [IMPACT]: 90 — once landed (post-RA1+RA2), this becomes the canonical hermetic-diff retrieval primitive across the swarm; eliminates an entire class of phantom-finding failures (the architectural-pillar review-rigor floor pattern's INVERTED case from PR #10745 Cycle 3).
• [COMPLEXITY]: 70 — 135 LOC across 2 files, plus the dispatcher logic is non-trivial. Edge cases handled (file-without-sha, sha-without-file, files_only short-circuit) but tests would strengthen confidence.
• [EFFORT_PROFILE]: Architectural Pillar — touches MCP-tool surface consumed by all swarm members; cross-substrate impact.

---

Net: substantive structure is right; two blockers at the input-validation + testing-discipline boundaries. After RA1+RA2, this is a clean approval candidate. RA3 is non-blocking polish; RA4 is fold-or-defer at author's choice.

---