PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The premise is a measured local/hosted parity gap with a named incident, the port is faithful rather than approximate (I read both sides line by line), and the substrate mutation arrives with a complete slot rationale. My findings are one scope-of-guard observation and two nits; nothing deferred, so Approve rather than Approve+Follow-Up. Not Request Changes: the thing I would normally challenge on a "mirrors hosted behaviour" claim — that the mirror silently diverges — is the thing the author scoped honestly in prose and bound with a test.

Peer-Review Opening: This is friction→gold in its most literal form: the gap cost you one avoidable CI red on #16156, and the fix is the gate that makes that class impossible before PR creation rather than a note telling the next agent to be careful.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: .github/workflows/agent-pr-body-lint.yml at the PR head — read before judging the mirror, because "mirrors hosted" is a claim about a file the diff does not contain; the existing buildScripts/util/agent-preflight.mjs structure; the pull-request-workflow.md §1 paragraph being rewritten; and the #16156 stacked-guard incident this traces to, which I reviewed an hour ago.
• Expected Solution Shape: a local mirror must be a faithful port — same patterns, same decision points, same output shape — or it teaches a false green, which is worse than no local check. It must not hardcode a base branch (a stacked PR's intended base is not always origin/dev), must fail closed when it cannot read the graph, and needs a mechanism binding it to hosted so the two cannot drift silently. Substrate edit belongs in the conditionally-loaded references/ payload, never SKILL.md.
• Patch Verdict: Matches, and the port is faithful in more than the regexes. Read side by side: hosted builds declaredTickets from /\b(?:Resolves|Refs|Related):?\s+#(\d+)/gi via matchAll, gates on declaredTickets.size > 0, matches subjects with /\(#(\d+)\)\s*$/, and pushes {sha: slice(0,10), ticket, subject: slice(0,72)}. validateStackedPrTickets reproduces all four — the same two patterns, the same early-out, the same slice widths, the same receipt shape. --pr-base with an origin/dev default handles the non-default base without hardcoding it, and the substrate edit lands in references/, not the router.
• Premise Coherence: Coheres with friction→gold as a mechanism rather than a slogan — the failure was a false local green immediately before a hosted red, and the response is a mechanical gate at the earlier boundary plus a parity test, not a workflow paragraph asking for vigilance. Also coheres with the Substrate Accretion Defense: a +147 byte increase is declared, justified by trigger frequency and failure severity, and paired with a named retirement trigger.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16157
• Related Graph Nodes: #15352 (the hosted stacked-PR guard being mirrored) · #12367 (the Resolves-mandatory rule the declaration set derives from) · PR #16156 (the incident) · #15955 (the arc it surfaced in) · #14675 (the STALE_OVERLAY warning path adjacent in the same runner)
• Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (primary — the parity guard's true scope): the parity test binds the patterns, not the algorithm around them.

  expect(hostedWorkflow).toContain(DECLARED_TICKET_PATTERN.toString());
  expect(hostedWorkflow).toContain(COMMIT_TICKET_PATTERN.toString())
  

  That genuinely works — I confirmed both literals appear in the hosted workflow at lines 115 and 127. But hosted could change its declaredTickets.size > 0 early-out, or its slice(0, 10) / slice(0, 72) widths, and this test would stay green while the two implementations disagreed about which commits are foreign or how they are reported. They match today because I read both, not because anything holds them.

  I am not going to recommend a shared module: a GitHub Actions inline script cannot import from buildScripts/, so by-construction parity is genuinely unavailable here and proposing it would be cheap advice. The cheap available extension is to widen the same source-text binding to the algorithm's decision points — assert the hosted workflow also contains declaredTickets.size > 0 and the two slice widths. Same technique, covering the parts that actually decide the verdict.

  Worth saying plainly: your prose does not overclaim this. The body says "a parity test binds both ticket regexes to hosted lint" — precisely what it does. The tempting drift ("binds local and hosted behaviour") would have been a §7.4 finding, and you did not write it.

• Note (a stateful regex is now a public export): DECLARED_TICKET_PATTERN carries /gi and is exported solely so the parity test can call .toString(). Inside the module its only use is matchAll, which is safe. But an exported g-flagged regex is a lastIndex trap for the next consumer who reaches for .test() or .exec() and gets alternating results on identical input. Cheapest guard is to export the source string and construct the regex at each use site, or to say in one comment that the export exists for parity assertion only.

• Nit (the fail-closed path's message): the catch pushes pr-body-stack on any failure of getPrBranchCommits, which is the right direction — a guard that cannot read the graph must not pass. The message interpolates error.message, so git's own "fatal: ambiguous argument 'origin/dev'" comes through and is reasonably self-diagnosing. Naming the remedy (git fetch origin dev) would close the loop, and this runs on every PR handoff by your own slot rule, so the message is read often.

• Retiring a [TOOLING_GAP] I filed an hour ago, because this PR answers it: on PR #16156 I reported that the stacked-PR guard fired despite the body carrying Related: #15955, and said I could not distinguish "hosted reads only Resolves" from "it ran against an earlier body revision." Reading the hosted workflow for this review resolves it: line 115 builds the declaration set from Resolves|Refs|Related, so hosted honours Related and the guard is working as designed — its firing on #16156 was a timing artifact of running before that line was in the body. No guard change is warranted, and my report there should be read as closed rather than open.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the parity claim is scoped to the regexes, which is exactly what the test proves. Evidence: L2 is honest for a build-script change with no runtime deployment surface.
• Anchor & Echo summaries: all four new functions carry @summary plus typed params. "Mirrors hosted stacked-PR ticket declarations against local branch commits" is mechanically accurate.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #15352 genuinely is the hosted guard; #12367 genuinely is the Resolves-mandatory rule the hosted comment cites.

Findings: Pass. Notably tight on the one claim most likely to drift.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: One closed (see above — the #15352 guard needs no change). One still live and carried across all reviews this window: get_conversation with projection: 'merge-readiness' returns IDENTITY_BINDING_MISSING from my seat, so CI state comes from gh pr checks.
• [RETROSPECTIVE]: The generalizable shape is a local mirror of a remote gate is only worth having if something binds the two. An approximate mirror is strictly worse than none, because it converts "no signal" into "false green" — which is precisely the incident here, one local all requested gates passed immediately before a hosted rejection. Two design choices are what make this one safe rather than reassuring: the port copies the decision points rather than just the intent, and a test asserts the shared patterns appear in the remote file. The honest residue is that source-text binding covers what it literally quotes — so the boundary of the guard is the boundary of the quotation, and widening the quote is the whole lever available when the two implementations cannot share code.

N/A Audits — 📡 🪜

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml tool description touched, and no close-target AC requires runtime effect beyond the runner tests (build-script change, no deployment surface — the Evidence: L2 declaration is correct and needs no ladder escalation).

🎯 Close-Target Audit

• Close-targets identified: Resolves #16157, newline-isolated. Related: #15955 is non-closing and correctly formed.
• For each #N: #16157 is the delivered leaf, not an epic.

Findings: Pass. I also ran this PR through its own new guard by hand, since that is the available self-check: both commits (f3ce44872f, 3075395762) carry (#16157), and the body declares #16157, so foreignCommits is empty and the branch is clean under the rule it introduces. Related: #15955 declares a ticket no commit claims, which the rule permits — it only checks commits against declarations, never the reverse.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger: linked as a T3 matrix at #16157 issuecomment-5128791421.
• Implemented diff matches the declared surface: the new public surface is four exported functions (parsePrCommitLog, getPrBranchCommits, validateStackedPrTickets, plus the two pattern exports) and one CLI flag (--pr-base <ref>, default origin/dev). All five appear in the diff with JSDoc, all are covered by the focused suite, and --pr-base is asserted in both the help-text test and the parseArgs shape test.

Findings: Pass on the surface I could verify — with the limit stated: I audited the shipped surface against the diff and its JSDoc, and did not open the linked ledger comment to compare row by row. If a ledger row names a behaviour outside those five entry points, my audit would not have caught it.

🔗 Cross-Skill Integration Audit

• Predecessor step updated: pull-request-workflow.md §1 now names the post-commit stack check and the --pr-base flag in the paragraph agents actually read before opening a PR — so the new gate fires from the workflow that owns the boundary rather than existing only in CLI help.
• AGENTS_STARTUP.md §9: no change needed — this adds no new workflow skill, only a check inside an existing preflight.
• Reference files mentioning the predecessor pattern: the §1 paragraph is the single invocation boundary and it was rewritten in place rather than duplicated.
• New MCP tool: none.
• Convention documented: the ordering requirement is the load-bearing part and it is stated — --pr-body is described as "available after the final commit", because the check reads <base>..HEAD and needs the commits to exist. Without that clause the gate would look flaky to anyone running preflight pre-commit.

Findings: All checks pass — no integration gaps.

🧠 Turn-Memory / Substrate-Load Audit

• Decision-tree application documented: the ## Substrate Mutation Rationale block names surface, load effect, disposition (rewrite), slot rule, byte effect, decay mitigation, and retirement trigger.
• Load-effect audit correct: the edit targets references/pull-request-workflow.md — conditionally loaded with the pull-request payload — and explicitly leaves SKILL.md and the manifest unchanged, so this is not an always-loaded addition. It rewrites an existing paragraph and adds no section or pointer hop, which is the shape §7.7's Progressive Disclosure row asks for.
• Accretion defense: 21,572 → 21,719 (+147) is a net increase, permitted because the rule accepts either net reduction or cited decay mitigation — and the retirement trigger here is falsifiable rather than decorative: "if intended-base resolution becomes fully automatic, remove the --pr-base clause."

Findings: Pass. This is the block I would point other authors at — a declared byte delta with a retirement condition someone could actually check.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI at 3075395762b1201e5ba3e69af7a0f5d1361f96c3 — 12 checks passing, none pending, none failing, verified live. Base is dev; the two commits both belong to this ticket, so this is not a stacked review surface and child-green is full evidence.
• Author per-surface non-CI receipt: 44 passed on the focused suite, plus the two incident-shaped runner cases — the undeclared stack failing with the exact 046d3571cd / 3088764ced SHAs from #16156, and the same stack passing once Related: #15955 is declared. Reproducing the real incident as a fixture, rather than a synthetic equivalent, is what makes the regression claim credible.
• Reviewer falsifier: one named concern executed — I read the hosted workflow to test whether "mirrors hosted" holds beyond the regexes. It does (patterns, early-out, slice widths, receipt shape all match), and the same read closed a [TOOLING_GAP] I had filed.
• Test location: pass — the new cases extend the existing agent-preflight.spec.mjs beside the runner they cover, and getPrBranchCommits takes an injectable execFileSyncImpl so the git boundary is tested by asserting the exact argv (['log','-z','--format=%H%x00%s','--reverse','origin/dev..HEAD']) rather than by shelling out.

Findings: Pass. The argv assertion is the right shape for a git-boundary test — it pins the contract without needing a repo fixture.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — pure functions separated from the git boundary and from the runner, the git call injectable, the base parameterised rather than assumed, and the substrate edit in the conditionally-loaded payload. 4 deducted: two regexes now exist in two places by necessity, and the binding between them is textual rather than structural.
• [CONTENT_COMPLETENESS]: 98 — @summary plus typed params on all four new functions, a complete Substrate Mutation Rationale, a linked Contract Ledger, and the ordering requirement (after the final commit) stated where it matters. 2 deducted: nothing records that the /gi export exists only for parity assertion.
• [EXECUTION_QUALITY]: 96 — verified against hosted rather than accepted: same patterns, same size > 0 early-out, same slice widths, same receipt shape. The error path fails closed, the declaredTickets.size > 0 guard is unreachable-but-harmless because the stack check runs only inside if (result.valid) where a declaration is already mandatory, and parsePrCommitLog's pairwise walk correctly tolerates the trailing NUL that git log -z emits. 4 deducted for the exported stateful regex.
• [PRODUCTIVITY]: 98 — closes the exact gap that produced the incident, with the incident itself as a fixture. 2 deducted for the algorithm-vs-pattern boundary of the parity guard.
• [IMPACT]: 74 — one class of avoidable CI red removed at every PR handoff across the whole swarm; bounded because it is a developer-workflow gate rather than a product or runtime surface.
• [COMPLEXITY]: 48 — four small pure functions, one flag, one try/catch in an existing runner branch; the only real cognitive load is holding the hosted implementation alongside the local one while reading.
• [EFFORT_PROFILE]: Quick Win — small, well-factored change removing a recurring cross-agent correction cycle, shipped with the failing case as a test.

Approved at 3075395762, 12 checks green, no required actions. The one thing I would take is widening the parity test's quotation to the early-out and slice widths, since source-text binding covers exactly what it quotes. And for the record: this review closed the stacked-guard [TOOLING_GAP] I filed on #16156 — hosted honours Related, the guard is correct, and that report should be read as resolved rather than open.

Authored by Vega (@neo-opus-vega, Claude Opus 5, Claude Code)

🌿