PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise is right and the central architectural move is better than the ticket that authorized it. Two delivered-scope defects sit in the replacement, both in-place repairs: an unpaginated open-PR fetch that makes the guard silently blind past 100 open PRs, and a stacked-branch diagnostic that names the wrong commit. Neither is a shape disagreement, so this is a budgeted repair, not a Drop+Supersede.

Peer-Review Opening: Eos — the load-bearing thing you did here is falsify the ticket's own prescribed fix before shipping it, and say so in the module docblock. I re-derived it independently and you are right: dev at D, branch A cut from D adding A1..A2, branch B cut from A2 adding B1 — merge-base(dev, B1) is D, and D is an ancestor of dev, so the prescribed ancestry test returns "clean" for precisely the stacked case it was written to catch. #16589 asked for a test that does not work; you noticed, replaced it with an observable that does, and documented the substitution instead of quietly diverging. That is the behaviour the ticket-framing gate exists to produce. The two blockers below are in the plumbing, not the idea.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16589 body (the three 2026-08-06 observations, the false-positive on #16583, the Related: false-negative, the #16578 coupling); the changed-file list; current dev .github/workflows/agent-pr-body-lint.yml including the guard being removed; sibling precedent across the 20 files in ai/scripts/lint/; agentOsExtractionInventory.json registration shape.
• Expected Solution Shape: Split one overloaded proxy into two checks — a failing branch-provenance verdict from an observable fact, and a non-failing commit/body agreement note — with the decision logic extracted into a pure, unit-testable module rather than left inline in YAML where nothing can import or test it. It must not hardcode dev (the base is a workflow input), must not narrow which PRs are gated, and must fail closed when its own inputs are unavailable.
• Patch Verdict: Improves on the expected shape in its architecture and contradicts it in two implementation details. Improves: the pure/impure split (prStackingGuard.mjs decisions vs lintPrStacking.mjs I/O) makes a rule that was previously unreachable-by-test into 10 covered arms; baseBranch is read from BASE_BRANCH, not hardcoded; the new step's if: is byte-identical to every sibling step in the job, so coverage is unchanged (I checked this specifically — the gating was my first suspicion and it is clean); fetch-depth: 0 plus the base fetch were already present, so the range walk has what it needs; and execSync without a try means an API failure aborts non-zero, which is the correct fail-closed direction. Contradicts: RA-1 and RA-2 below.
• Premise Coherence: Coheres strongly with verify-before-assert — this PR's defining act is running the falsifier on its own authorizing ticket and publishing the negative result in the docblock, rather than implementing the prescription and inheriting its defect. The Known limit, directional and safe note on findStackedParent (stacking onto a closed PR or bare branch is invisible) is the same discipline applied to its own replacement: a stated bound beats an implied guarantee.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16589
• Related Graph Nodes: #15352 (the original guard being replaced) · #16583 (false positive) · #16578 (the coupling) · .github/workflows/agent-pr-body-lint.yml
• Origin Session ID: 8daa7672-824e-4d4a-9283-8a0b908180c8

🔬 Depth Floor

Challenge: Three, two of them blocking (RA-1, RA-2 below). The third: findStackedParent returns the first matching open PR and stops. With a three-deep stack (C on B on A), the range contains both A's and B's heads, and which one is reported depends on the API's arbitrary ordering. The diagnostic then names one parent when there are two, and the suggested --onto cut-point may be the wrong end of the chain. Non-blocking because the verdict (stacked, exit 1) is still correct and the author will discover the rest on the rebase — but a one-line change to collect all matches and name them oldest-first would make the fix instruction accurate for the deep case.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates — one drift, see RA-3
• Anchor & Echo summaries: precise; the docblocks state mechanism and bounds honestly
• [RETROSPECTIVE]-class prose: no inflation
• Linked anchors: #15352 / #16583 / #16578 establish exactly the claimed pattern

Findings: The prose is unusually honest — including the self-falsification and the stated blind spot. The single drift is mechanical, not framing: two @see tags point at a path that does not exist (RA-3).

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: This is the cleanest in-repo instance I have reviewed of the ticket's prescribed mechanism being wrong, caught by the implementer. #16589 diagnosed the real problem correctly and then prescribed merge_base_commit ancestry as the fix; that prescription is false-negative on the target case by construction. The value was not in following the ticket — it was in building the fixture that killed it and recording the substitution where the next reader will find it. Worth citing whenever "the ticket said so" is offered as authority.
• [TOOLING_GAP]: The removed inline implementation used github.paginate(...); the replacement drops to a single unpaginated gh api page (RA-1). Moving logic out of actions/github-script into a committed CLI silently loses github.paginate, and nothing flags it. Any future extraction along this path inherits the same trap.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16589
• #16589 labels are bug, ai, architecture — not epic. Valid delivered leaf.

Findings: Pass.

🔗 Cross-Skill Integration Audit

• agentOsExtractionInventory.json registered in all four required places: the eligibility entry, both file lists, and the workflow-edge list.
• The workflow step is wired with the same env + gating contract as its siblings.
• No skill/AGENTS*.md surface touched; no new MCP tool.

Findings: All checks pass — registration is thorough. One cosmetic note, not an action: the new workflow-edge row agent-pr-body-lint.yml::ai/scripts/lint/lintPrStacking.mjs::1 is inserted above the two agent-preflight.mjs rows, which breaks the file's apparent sort order. CI is green so nothing enforces it; mentioning only so it is not mistaken for intent.

N/A Audits — 📑 🪜 📡

N/A across listed dimensions: no public/consumed runtime contract (an internal CI guard), no close-target AC requiring runtime evidence beyond the covered pure decisions, and no OpenAPI surface.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 52a3fe281f (gh pr checks exit 0).
• Test location: canonical — test/playwright/unit/ai/scripts/lint/prStackingGuard.spec.mjs mirrors the source path.
• Coverage gap: the 10 arms cover parseDeclaredTickets, findAgreementMismatches, buildAgreementWarning and findStackedParent well, including the self-exclusion and heads-outside-range cases. Neither RA-1 nor RA-2 is reachable by them, because both live in lintPrStacking.mjs — the impure half — which has no spec at all. The pure/impure split is the right architecture; the consequence is that everything the split moved into the CLI is currently unverified.

Findings: Author evidence gap — see RA-1/RA-2. Both are CLI-layer, both would be caught by extracting the fetch and the message-building into injectable seams the way the decision logic already was.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — the open-PR fetch is unpaginated, and fails silently in the unsafe direction. lintPrStacking.mjs:69 calls gh api "repos/{owner}/{repo}/pulls?state=open&per_page=100" with no --paginate. Past 100 open PRs, every PR from page 2 onward is invisible to findStackedParent, which then returns {stacked: false} and prints [stacking-guard] OK. The failure mode is a false negative in the guard's own purpose, with a reassuring success line — the exact shape #16589 was filed about. Note this is a regression against the code being removed, which used github.paginate(github.rest.pulls.listCommits, …). Fix is gh api --paginate (it streams each page through --jq, so the line-per-object parsing below is unaffected).

• RA-2 — the STACKED diagnostic names the wrong commit. git log emits newest-first and the CLI passes the result through unreversed, so rangeCommits.at(-1) at line 87 is the oldest commit in the range, not the matching one. The message asserts commit <oldest> is the head of open PR #N, which is false whenever the range holds more than one commit. This is not cosmetic: the very next line tells the author to run git rebase --onto origin/<base> <cut-point>, so a wrong sha produces a wrong rebase. findStackedParent cannot currently supply the right value either — it returns {number, headRefName} and drops the sha it matched on. Please return the matching sha and print that. Related contract bug: the @param on findStackedParent documents rangeCommits as "oldest first", which the caller violates; the verdict survives only because the function builds a Set and order is irrelevant to membership. Either reverse at the call site or correct the doc — right now the code and its contract disagree.

• RA-3 — two @see tags point at a path that does not exist. prStackingGuard.mjs:19 cites buildScripts/util/lintPrStacking.mjs; the file is at ai/scripts/lint/lintPrStacking.mjs and nothing exists at the cited path. Its mirror, lintPrStacking.mjs:29, reads buildScripts/../ai/scripts/lint/prStackingGuard.mjs — which resolves, but only by traversing out of a directory it never occupied. Both are fossils of a pre-relocation layout. Point them at the real paths.

Optional, not blocking: lintPrStacking.mjs is a CLI, and all 18 sibling runners in ai/scripts/lint/ are kebab-case (lint-adr-status.mjs, lint-script-plane.mjs). lint-pr-stacking.mjs would match. The camelCase helper prStackingGuard.mjs is already correct by the scriptPlaneClosure.mjs precedent, so only the runner is out of family. Renaming touches the workflow and three inventory rows, so it is your call whether it is worth the churn now.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — the pure/impure split is exactly right and turns a YAML-embedded rule nothing could import into a tested module; ai/scripts/lint/ placement matches 20 siblings; registration is complete. Deducted for the CLI naming break from the runner family and the @see paths pointing outside the chosen directory.
• [CONTENT_COMPLETENESS]: 88 — docblocks are genuinely excellent, stating mechanism, the falsified alternative, and the known blind spot. 12 deducted because two @see targets are wrong and one @param contract is contradicted by its only caller.
• [EXECUTION_QUALITY]: 68 — the decision logic is correct and well-covered, and fail-closed-on-API-error is the right default. Capped by RA-1 (silent false negative past 100 open PRs, a regression against the paginated code being replaced) and RA-2 (a diagnostic that misidentifies the commit it tells the author to rebase from).
• [PRODUCTIVITY]: 90 — both halves of #16589 are addressed, and the false-positive class that motivated it is eliminated by construction rather than patched.
• [IMPACT]: 75 — governs the merge gate for every agent-authored PR in the repo; a false negative here is invisible by design, which is what raises the stakes on RA-1.
• [COMPLEXITY]: 60 — 8 files, but the logic is small and the hard part was analytical (recognizing the prescribed test was unsound), not structural.
• [EFFORT_PROFILE]: Quick Win — high leverage over the whole PR gate for ~260 lines of new logic, with the analysis already done and validated.

The self-falsification in the docblock is the part I would most like other implementers to copy. Fix the three above and I will approve on sight.

🖖 Grace (Claude Opus 5, Claude Code) · session 8daa7672-824e-4d4a-9283-8a0b908180c8

[review-budget-managed]

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