PR Review Summary

Status: Approved

§9.0 Premise Pre-Flight: no structural trigger — #13052 is a live, valid tooling-hazard bug (operator-tree mutation), the fix shape (explicit caller repoPath + refusal) directly closes it, no authority bypass. Standard review.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Correctly closes #13052's operator-tree-mutation hazard (refuse without explicit caller repoPath; checkout only in the verified git-root) and removes a prior ADR-0019 violation (the old path mutated aiConfig.projectRoot; the new one reads it diagnostic-only). Test-verified (I ran the 4 focused tests, 4/4). The one §8 consistency gap (a sibling template line) is a cosmetic non-blocker, so Approve over Approve+Follow-Up.

Peer-Review Opening: Strong fix — and a nice piece of dogfooding, since reviewing it exercised exactly the manual-gh pr checkout + HEAD-verify path you added. I checked out at the verified head and ran the contract tests. One non-blocking consistency follow-up below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13052 (close-target — the operator-tree-mutation advisory), the full diff (PullRequestService.mjs guard, openapi.yaml, tests, pr-review-guide.md, GitHubWorkflow.md), ADR 0019 (AiConfig SSOT, my prior context), and my own #13052 memory on the hazard.
• Expected Solution Shape: the tool must stop defaulting to a mutation of the server/operator checkout — require an explicit caller workspace root, verify it, run gh pr checkout only there, and return verifiable read-back state. It must NOT introduce a second AiConfig source-of-truth, and the refusal path must be graceful (not a hard crash) for legacy callers.
• Patch Verdict: Matches. buildCheckoutPullRequest refuses with CALLER_WORKSPACE_REQUIRED when repoPath is absent (verified: no git/gh executed), guards REPO_PATH_NOT_GIT_ROOT against subdir ambiguity, runs checkout in the verified gitTopLevel, and returns {repoPath, branch, headSha}. aiConfig.projectRoot is read as a default param for diagnostics only — the old gh pr checkout … {cwd: aiConfig.projectRoot} mutation is gone (ADR-0019-correct).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13052
• Related Graph Nodes: ADR 0019 (AiConfig SSOT — alignment verified), ai/mcp/server/github-workflow/openapi.yaml (tool contract), pr-review-guide.md §2.2 (Empirical Checkout Mandate), #13047 (sibling AGENTS_STARTUP refresh in this Fable-coordination batch).

🔬 Depth Floor

Challenge (residual + follow-up, both non-blocking):

1. The honest-contract residual: the guard verifies repoPath is a git top-level, but cannot verify it's the caller's own workspace (MCP transport carries no caller cwd). So a buggy/wrong caller could still pass a valid-but-not-theirs git root and mutate it — the fix moves the hazard from "silent default mutation of the server root" to "explicit, verifiable caller-specified mutation," which is the correct interim. The PR frames this honestly, and the {repoPath, headSha} read-back is the right mitigation (the caller/reviewer can see exactly what got mutated). Worth a future follow-up: a session-registered caller workspace (the alternative the PR considered + deferred) once transport/registry supports it, so the path can be validated as the caller's.
2. Schema-vs-behavior: repoPath is required: false in openapi but behaviorally required (refuses without it). Intentional, to keep the legacy-numeric refusal graceful rather than a hard schema reject — defensible for the migration window; the tool description states the refusal, so a careful consumer sees it.

Rhetorical-Drift Audit (§7.4):

• PR description: every claim (refusal, repoPath validation, read-back, ADR-0019 diagnostic-only, sibling-tool sweep) matches the diff I read — no overshoot.
• Anchor & Echo: the new buildCheckoutPullRequest JSDoc precisely describes the guard; no metaphor drift.
• [RETROSPECTIVE]: N/A — none authored.
• Linked anchors: ADR 0019 accurately cited (the fix genuinely keeps projectRoot diagnostic-only).

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None — author's ## Evolution note documents the real publish-time GH_TOKEN/temp-worktree friction (pushed from the authenticated shared shell via git -C), useful ambient signal for the cross-clone auth story.
• [RETROSPECTIVE]: "Don't let a shared MCP server infer caller cwd — require it explicitly and return verifiable read-back" is the right durable pattern for any caller-workspace-mutating tool; the {repoPath, headSha} echo is the reusable mitigation shape.

🎯 Close-Target Audit

• Close-target: #13052 via newline-isolated Resolves #13052.
• #13052 is a tooling-hazard bug (not epic-labeled) — valid close-target.

Findings: Pass.

📑 Contract Completeness Audit

• The PR modifies a public surface (the checkout_pull_request MCP tool contract). The contract is fully + correctly specified in openapi.yaml (the repoPath requestBody + the new CheckoutPullRequestResponse schema with all five fields) plus the PR body's refusal-code enumeration. The openapi.yaml IS the authoritative contract home for an MCP tool — more so than a ticket-body table — so I'm treating §5.4 as satisfied by the schema rather than demanding a duplicate ledger on a bug ticket.

Findings: Pass (contract complete in the canonical surface).

📡 MCP-Tool-Description Budget Audit

• The checkout_pull_request description went from a multi-line block to a single concise line (call-site usage: what + when-to-use + when-not + the refusal code) — a budget improvement, well under the 1024 cap, no internal refs/narrative.

Findings: Pass (budget reduced).

🔗 Cross-Skill Integration Audit

• pr-review-guide.md §2.2 Empirical Checkout Mandate — updated to manual gh pr checkout + HEAD-verify (with embedded sunset). ✓
• GitHubWorkflow.md (×2 refs) — updated to the new contract. ✓
• Gap (non-blocking): pr-review/assets/pr-review-template.md:180 still reads "Branch checked out locally (e.g., via checkout_pull_request MCP tool or gh pr checkout)" — the sibling of the §2.2 sentence you updated, left un-caveated. Not broken (it lists gh pr checkout), but for intra-skill consistency it should match. Also learn/blog/context-engineering-done-right.md:1450 has a stale bare checkout_pull_request(1234) example (blog narrative — lowest priority).

Findings: Substantively complete; one cosmetic template-line consistency gap (see follow-up — non-blocking).

🧠 Turn-Memory / Substrate-Load Audit

• The PR touches .agents/skills/pr-review/references/pr-review-guide.md (IN-SCOPE). Author documented the slot rationale (compress-to-trigger, loaded-during-review only, sunset embedded) — a one-sentence rewrite, no new always-loaded rule body. Correctly audited.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Manual git fetch + detached checkout at the PR head (91cdac1cf), HEAD-verified equal to headRefOid (dogfooding the new guide step — not via the guarded tool).
• Canonical location: test/playwright/unit/ai/services/github-workflow/PullRequestService.spec.mjs — correct.
• Ran npm run test-unit -- …PullRequestService.spec.mjs -g "checkoutPullRequest" → 4/4 passed (refusal / not-git-root / read-back happy path / gh-failure). I independently confirmed the author's "3 pre-existing getPullRequestDiff failures are unrelated" claim by scoping to the checkout contract.

Findings: Tests pass — execution verified at the PR head.

N/A Audits — 🛂 🔌

N/A: no major new architectural abstraction (Provenance), and no JSON-RPC / event / DB wire-format change (the response-schema add is a tool-contract extension, audited above).

📋 Required Actions

No required actions — eligible for human merge.

Non-blocking follow-up (author's discretion to fold into this PR or defer): align pr-review-template.md:180's checkout_pull_request mention with the §2.2 mandate, and tidy the context-engineering-done-right.md:1450 blog example. Both cosmetic consistency, neither blocks the fix.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — I actively considered (a) operator-tree-mutation-fix correctness, (b) ADR-0019 alignment (diagnostic-only projectRoot, no singleton mutation — it fixes a prior violation), and (c) the injectable execFileFn test seam, and confirmed all align.
• [CONTENT_COMPLETENESS]: 95 — 5 deducted because the §8 sweep found one un-caveated checkout_pull_request reference left in the sibling pr-review-template.md:180; the body is otherwise a complete Fat-Ticket (Evidence / Deltas / Slot Rationale / Test Evidence / Evolution).
• [EXECUTION_QUALITY]: 100 — checked out at the verified head and ran the 4 contract tests (4/4); guard logic covers refusal / git-root validation / read-back / gh-failure. I considered race / error-shape / partial-state and found none.
• [PRODUCTIVITY]: 100 — I considered whether the hazard is fully closed and the sibling-tool sweep done: default mutation eliminated, ADR-0019 violation fixed, get_pull_request_diff/manage_pr_reviewers/sync_all correctly dispositioned.
• [IMPACT]: 70 — substantive tooling-safety fix protecting the operator workspace across the shared-MCP swarm + an ADR-0019 correction; below higher tiers as a single-tool guard, not a framework/subsystem shift.
• [COMPLEXITY]: 55 — Medium: guard logic + git-root verification + MCP schema extension + injectable-test refactor + substrate edit across 5 files; the load is in the MCP-transport-can't-carry-cwd constraint driving the explicit-repoPath contract.
• [EFFORT_PROFILE]: Heavy Lift — security/tooling-correctness fix with multi-surface coordination (service + schema + docs + guide + tests) at a high-correctness bar.

Clean, correct, and test-verified — approved and eligible for your merge, @tobiu. The dogfooding angle is satisfying: reviewing this PR required the very manual-checkout + HEAD-verify discipline it codifies. Only loose thread is the one sibling template line (non-blocking). 🖖 — Grace