feat(code-reviewer): add boundary review for env/credential/spawn changes - #219

Problem

The code-reviewer skill classified review effort purely by line and file count and analyzed each diff entirely within the repository. A small change at an env-var, credential, spawn/exec, or network boundary therefore got the lightest path, and the contracts of the external binaries and services that consume those values — the actual blast radius — were never examined. That is the failure mode behind the bad approval on block/buzz#3398: a 68-line diff at the agent-spawn seam passed review with four misses that repo-local, size-routed analysis could not surface.

What the patched skill does

Boundary Relevance scan (SKILL.md). A two-tier diff scan sets boundary_relevant. Tier 1 — hard runtime-boundary constructs (env::, process.env, Command::new/.spawn(, auth-header construction, credential assignment) — sets the flag directly. Tier 2 — broad lexical hits (password, endpoint, https://, bare exec) — sets it only after a one-line semantic confirmation that the value actually crosses a process/auth/network boundary at runtime, so lexical coincidence doesn't gate a verdict. Any boundary_relevant diff routes to the Phase 2B orchestrated path regardless of size.

Boundary & External Contract specialist (references/subagent-template.md). A fifth Phase 2B opus agent whose lens is explicitly outside the repo: which binaries/services consume the touched values, what their env/API contracts require (read from their docs/source), what behavior each value activates downstream, and whether paired values (key + base URL) travel together.

Linked-issue scope-drift check (SKILL.md, PR Mode). Step 1 derives the PR's owner/repo, parses closing references (bare Fixes/Closes/Resolves #N against the target repo; qualified owner/repo#N and full issue URLs keep their explicit repo), and runs gh issue view <N> --repo <owner/repo>. Implementation scope vs. issue scope is a 🟡 finding minimum, 🔴 when the drift widens a security/credential/auth surface. A failed lookup is a named blocker to the check, not silent continuation.

Executed mutation check (references/subagent-template.md, Functionality & Testing lens). For fix PRs, the Functionality specialist reverts only the production hunks in a scratch worktree (preserving added tests when production and test hunks share a file), verifies the reversal, runs the project's full suite, and expects RED. A suite that stays green with the fix removed = 🔴 "untested at its seam". The protocol has one authoritative home; the Phase 2A inline lens references it rather than duplicating it. Inability to execute is a named unmet precondition, never a silent skip.

Ordered verdict tree (SKILL.md). (1) any verified HIGH-confidence 🔴 → REQUEST_CHANGES — a verified blocker is never suppressed by the Comment cap; else (2) any applicable mandatory check not executed (including a needed live probe the reviewer can't run) → Comment naming it, with applicability stated (issue comparison only when a linked issue exists, mutation only for fix PRs); else (3) APPROVE. A single-agent 🔴 at HIGH confidence with orchestrator concurrence can request changes — the old 2-agent bar contradicted the mutually-exclusive lens design. Forward-momentum philosophy stays intact for every non-boundary change.

The skill version is bumped 1.1.1 → 1.2.0. A prompt-contract test (tests/unit/test_code_reviewer_skill_contract.py) asserts the Phase 2B Functionality briefing carries the mutation instruction, the protocol has a single authoritative home, boundary diffs route to Phase 2B, and the verdict tree never caps a blocker — the existing CI asserts nothing about boundary_relevant or mutation behavior.

How this catches the #3398 misses