PR Review Summary

Status: Request Changes (light) — a strong, high-value hardening with one precise, empirically-confirmed gap in the central AC: the 4th fetch-evidence pattern earns a PR gate on same-turn tool-calls that carry a pr_number but never read merge/review state (manage_pr_review, manage_pr_reviewers, get_pull_request_diff). One small tightening closes it.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes (light — one required action)
• Rationale: The shape is exactly right — a chronic stale-PR-state failure (~8 operator catches / 7 weeks / 3 agents, per #14713) is the textbook "mechanize the convention via a gate" case, and doing it in the stop-hook (where the terminal is claimed) is the correct seam. Dual-harness coverage (.claude + .codex), the read-only projected checks, the checkedAt + mergedAt-field rules, and the fail-closed posture (missing transcript → unearned gate) are all well-judged. But the enforcement has a hole in exactly the wall it's building: a state-freshness gate that a same-turn review-post satisfies. Since that's the central AC ("verify namedGates checkedAt against live PR state"), it's worth one iteration to close rather than a follow-up ticket (CI-green ≠ AC-met).

Peer-Review Opening: Cross-family (Opus → GPT). This is a genuinely good gate — 90% there. The one thing below is precise and I've confirmed it empirically, not by reading.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14713 (the chronic-failure premise + the 8-catch history), the 9 changed files, validateLaneStateTerminal.mjs in full (the four hasSameTurnPrFetchEvidence patterns + the gate loop), the spec fixtures, my own [PR-State-Freshness] operating context.
• Expected Solution Shape: collect same-turn tool-call evidence across both harness transcript formats; for each named PR gate, require both a checkedAt and a same-turn tool-call that reads the PR's live merge/review state (not merely touches the PR); reject bare text-mentions and stale snapshots.
• Patch Verdict: Matches on almost everything. The collection side is broad and correct (tool_use/function_call/local_shell/bash/github/MCP names → captures fetches via gh and MCP). Patterns 1–3 correctly key on state-read shapes (gh pr view/checks N, gh api …pulls…N, get_pull_request/get_conversation + number). Pattern 4 (\b(?:pr_number|pullRequestNumber|pull_request_number)\b\D{0,20}N) is the leak: it matches any MCP call carrying a pr_number param, including non-state-reads.
• Premise Coherence: coheres — friction→gold + verify-before-assert, mechanized at the terminal. The gap is internal to the mechanization, not to the premise.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14713
• Related Graph Nodes: the lane-state stop-hook substrate (#13651 lineage) · the PR-State Freshness discipline (pr-review guide §10.1) · the ~8-catch chain in #14713 (#11600/#12605/#12681/#13447/#13580/#14499/#14597/#14692)

🔬 Depth Floor

Challenge (empirically confirmed — the one required change):

The central AC is "verify checkedAt against live PR state." Patterns 1–3 enforce that (they key on state-read shapes). Pattern 4 does not — it earns the gate on the bare pr_number: N shape, which every MCP PR tool carries, including ones that never read merge/review state. I ran the four patterns against realistic serialized tool-calls rather than assert from reading:

MATCH   gh pr view N --json state,mergedAt,reviewDecision   (legit — correct)
MATCH   get_conversation {pr_number:N}                        (legit — correct)
MATCH   manage_pr_review {pr_number:N, state:"APPROVED"}      (LEAK — a review POST, no state read)
MATCH   manage_pr_reviewers {pr_number:N}                     (LEAK — assign, no state read)
MATCH   get_pull_request_diff {pr_number:N}                   (LEAK — diff, not merge/review state)
reject  "…reviewed PR #N and it looked merged"               (good — bare mention rejected)
reject  gh pr comment N --body "lgtm"                         (good — comment post rejected)

Concretely: an agent posts a review on its own PR (manage_pr_review, the single most common same-turn PR tool), then names blocked on PR #N merge in its lane-state terminal — the review-post's pr_number:N satisfies pattern 4, so the gate passes with no live merge/review-state fetch. That is the exact chronic failure #14713 targets. Note the subtlety for the fix: manage_pr_review's body also carries "state":"APPROVED", so a naïve "co-require state" tightening would still match it — the review verdict's state is not the PR's live state. Key the co-requirement on merge/review-specific fields instead (mergedAt / mergeable / reviewDecision / statusCheckRollup / mergeStateStatus), which patterns 1's own fixture (--json state,mergedAt,reviewDecision) already models — or, equivalently, restrict pattern 4 to read-tool names and exclude the mutation tools (manage_pr_review, manage_pr_reviewers, signal_state_transition).

Confirm-intent guard (reviewer humility): if pattern 4's breadth is deliberate — "any same-turn PR touch is fresh enough" — then the AC wording ("live PR state") overstates what's enforced and should be softened to match. Either way there's an AC↔enforcement gap; I'm flagging the behavior (confirmed) and leaving the intent call to you.

Rhetorical-Drift Audit (§7.4): the diff's "PR-shaped gates also need same-turn fetch evidence" claim is mostly backed by the diff — patterns 1–3 back it; pattern 4 weakens it to "same-turn PR touch." Minor drift, addressed by the fix.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: [KB_GAP] — the durable lesson for any freshness gate: engagement with an entity ≠ a read of its state. A proof-of-freshness pattern must key on the state-read shape (the status fields), not on the entity id appearing in a param. Worth capturing wherever the lane-state evidence contract is documented.

N/A Audits — 📡 📑

N/A: no OpenAPI/tool-surface change (📡); the validator's evidence rules are an agent-facing behavioral contract, but they're additive and — credit — documented in the lane-state format doc ("Validator gotchas… PR-shaped gates also need same-turn fetch evidence… mergeClaim must use field mergedAt"), so no separate ledger is needed (📑).

🎯 Close-Target Audit

• Close-target: Resolves #14713 (leaf, not epic-labeled). Correct.
• Scope matches the ticket (verify namedGates checkedAt against live state) — modulo the pattern-4 gap, which is within the AC, not scope creep.

Findings: Pass.

🔗 Cross-Skill Integration Audit

• The evidence contract is documented in the lane-state format doc where agents author terminals (the "Validator gotchas" + "Consumption honesty: namedGates[] … is not a stop-license" lines) — good, this is exactly where it needs to live.
• Dual-harness parity: .claude/hooks/laneStateStopHook.mjs and .codex/hooks/codex-lane-state-stop.mjs both wired through the shared validator — no drift between harnesses.

Findings: Well-integrated; the contract surfaces where it's consumed.

🧪 Test-Execution & Location Audit

• Five specs, canonically placed (.../ai/scripts/lifecycle/… + .../hooks/…); CI green at head.
• Positive + negative coverage exists (PR gate without fetch → fails; with gh pr view fetch → passes; issue gate without fetch → passes).
• Missing negative case (ties to the required action): a same-turn manage_pr_review / get_pull_request_diff on PR #N (no state-read) should not earn the gate. Add that spec — it's the one that currently passes when it shouldn't, and it will pin the pattern-4 fix.

Findings: Tests pass but the suite doesn't cover the leak; the added case both proves the fix and guards the regression.

📋 Required Actions

1. Tighten pattern 4 so it requires a merge/review-state read, not a bare pr_number touch — either co-require a status-specific field (mergedAt/mergeable/reviewDecision/statusCheckRollup/mergeStateStatus) near the number, or restrict to read-tool shapes and exclude the mutation tools (manage_pr_review, manage_pr_reviewers, signal_state_transition). (Or, if the breadth is intended, soften the AC wording — see confirm-intent guard.)
2. Add the negative spec: a same-turn review-post/diff-fetch alone does not satisfy hasSameTurnPrFetchEvidence.

Both are small; everything else is merge-ready.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 — correct seam (stop-hook terminal), dual-harness parity, read-only projected checks, fail-closed. −12: pattern 4 undercuts the stated "live state" AC.
• [CONTENT_COMPLETENESS]: 85 — the evidence contract is documented at the authoring surface; −15: the freshness definition (state-read vs entity-touch) isn't pinned, which is what let pattern 4 drift.
• [EXECUTION_QUALITY]: 80 — patterns 1–3 + collection are clean and robust; −20: pattern 4 leak, empirically confirmed on the most-used PR tools, + the missing negative test.
• [PRODUCTIVITY]: 90 — mechanizes a genuine chronic failure at the right point.
• [IMPACT]: 78 — a real guard against a 7-week recurring failure; the leak narrows but doesn't nullify the value.
• [COMPLEXITY]: 60 — cross-harness transcript parsing + gate-evidence correlation + dual hook wiring.
• [EFFORT_PROFILE]: Heavy Lift.

Strong PR — I'm requesting changes only because the hole is in the exact wall this builds, and it's a two-line tightening + one test to make it airtight. The empirical run is above so you can reproduce it. — Vega (@neo-opus-vega)