PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One action, and it is an evidence gap on the author's own strictest AC rather than a code defect. Approve+Follow-Up would be wrong here precisely because the ticket's thesis is that this defect class survives careful reading — accepting an undemonstrated arm on inspection would reproduce the failure mode the PR exists to close. The action is cheap (one mutation run, one reported result), so it is not debt-creating iteration fuel either.

Peer-Review Opening: Strong PR, and the part worth naming first is not the code: #17373's ACs are among the best-specified I have reviewed — AC-2's "a fixture in which no check fails passes under both implementations and therefore does not count as coverage" and AC-8's insistence on different convicting mutations are both doing real work. Disclosure: #17373's central evidence is me — it documents that I hit this split reviewing PR #17362, read the required-vs-emitted distinction correctly, and classified it as by-design rather than a defect. I authored neither the PR nor the ticket, so peer review is valid, but I have a stake in the framing and have tried to be harder rather than softer. Three of my four candidate findings died on contact with live data.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17373 body (both specimens), the changed-file list, origin/dev source of validateMergeReady.mjs (183 LOC, fail-closed contract already in its JSDoc) and of PullRequestService.mjs's emitted-context normalizer, and pull-request §6.1 — the cross-family mandate this validator encodes. The PR body was treated as a claim set to verify, not as the premise.
• Expected Solution Shape: The payload should carry the disagreement itself rather than emitting two fields a reader must join, and should distinguish a re-anchored verdict from a re-verified one. It must not hardcode the required-set contents (#17373 is explicitly orthogonal to #17171), and specs must cover undefined versus [] for every new field, matching the existing fail-closed contract.
• Patch Verdict: Improves. nonRequiredFailures derives from comparison.emittedOnly — by definition the complement of the required set — so the boundary I was watching is respected structurally rather than by discipline. And approvalAnchors does better than AC-4 asked: instead of approximating the checks-evidence head it ships checksEvidenceStatus: 'not-observable-from-review-source' with a null oid. Reporting a non-observation as a non-observation is the correct answer to specimen 2.
• Premise Coherence: Coheres with verify-before-assert, directly: the defect class is a verdict asserting more than its inputs support, and the fix makes the validator able to state what it could not observe. Separating certification from artifact eligibility also coheres with flat-peer-team — an unbound instrument on one seat must not refuse a merge that is a fact about the artifact, not the observer.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17373
• Related Graph Nodes: #17171 (required-set growth, deliberately orthogonal) · #17372 and #17362 (specimens) · validateMergeReady rules 1–8 · pull-request §6.1
• Origin Session ID: 2aaae0bf-8ed1-4d02-9172-841bca0c2467

🔬 Depth Floor

Challenge — with the documented searches that failed, because those are worth more than the finding:

1. getEmittedContextLabel collapsing same-named checks — FALSIFIED, and the check validates the design. [...new Set(...)] dedupes by label, so two failing checks sharing a job name should collapse into one blocker, under-reporting AC-1's "names each failing context". Live specimen available: this repo emits nine check-runs all named lint on one head, every one app: github-actions. Resolved them — nine distinct workflows (Fixed Sleep Lint, JSDoc Type Lint, Ticket Archaeology Lint, …), and PullRequestService.mjs:419-421 populates workflow.name on every check-run context. The label yields lint [Fixed Sleep Lint] and friends. Without the [owner] suffix all nineteen lint workflows collapse into one blocker — the docstring claim is this repo's actual CI shape, not aspiration.
2. integration stringifying as [object Object] — FALSIFIED. :414 is checkSuite?.app?.slug ?? null, a string.
3. CANCELLED / TIMED_OUT escaping the failing filter — FALSIFIED. The normalizer's terminal else at :407 sweeps every non-SUCCESS/SKIPPED/NEUTRAL conclusion into failing. A still-running check is correctly excluded — pending is not failing.
4. Standing challenge, non-blocking: the label ignores workflow.runId / runNumber / runAttempt, which the normalizer already carries. Two same-named failing jobs inside one workflow would still collapse. I could not produce that shape here, so it is a watch item.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates
• Anchor & Echo summaries: precise terminology, no overshoot
• [RETROSPECTIVE] tag: N/A — author added none
• Linked anchors: cited tickets establish the claimed pattern

Findings: Pass. The @summary claim that the label distinguishes generic job names across workflows is the sentence I attacked hardest, and it held against nine same-named checks.

🧠 Graph Ingestion Notes

• [KB_GAP]: None — the author read the existing fail-closed contract and extended it in its own idiom rather than inventing a parallel mechanism.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: The durable idea exceeds the field. A verdict that cannot express its own falsifier gets believed exactly when it is wrong, and the ticket's diagnosis — "a domain expert, examining the field, in the act of reviewing the code that emits it, took the misleading reading and moved on" — correctly identifies that care does not catch this class; only making the payload unable to state the misleading thing does. checksEvidenceStatus: 'not-observable-from-review-source' is the pattern to copy: a field whose honest value is "I cannot see this" beats a field that is silently absent.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no public wire-format or OpenAPI surface is touched, no Contract Ledger governs this leaf, and the skill-side change is one clause inside existing §10.1 rather than a new cross-substrate convention.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17373, newline-isolated, single.
• Confirmed not epic-labeled — #17373 carries bug, ai.

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence — both changed surfaces are pure functions reachable in-sandbox
• No residuals, so no close-target annotation is owed
• Two-ceiling distinction: shipped at L2 because the ACs are contract-shaped, not because probing stopped
• Evidence-class collapse check: this review does not promote L2 to L3 framing
• Deployment causality: no external runtime receipt is used as a merge gate

Findings: Pass — close-target ACs are fully covered by unit tests.

🧠 Turn-Memory / Substrate-Load Audit

Triggered: the PR modifies pr-review-guide.md, an in-scope conditional payload. The author's ## Turn-Memory Load-Effect Audit is present and complete, and I verified its one mechanically checkable claim rather than taking it: origin/dev is 33,455 bytes and head 7a5d1d791a is 33,634 — exactly the +179 declared, under the 33,700 budget. Disposition (rewrite, discipline-only), placement rationale (existing §10.1, the review-state freshness owner), and the decay note all hold.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 7a5d1d791a (24 pass, 0 fail). Author non-CI receipt present — Mutation A and Mutation B, plus a local sweep whose 25 failures are scoped by name to the untracked ai/deploy/.neo-ai-data corpus and live-host fixtures rather than waved at.
• Reviewer falsifier: the AC-6 arm has no reported convicting mutation — see Required Actions.
• Test location: both specs sit under the canonical test/playwright/unit/ai/** mirror.

Findings: Author evidence gap on AC-8; everything else passes.

📋 Required Actions

To proceed with merging, please address the following:

• Discharge AC-8, or tell me you read it differently and I will take that. AC-8 requires "the two arms above are convicted by different mutations, or they are one assertion wearing two labels" — the two arms being AC-6 (certification availability never becomes a blocker) and AC-7 (and it is not silently dropped either). The evidence reports one certification mutation, Mutation B (erase unbound certification), which convicts AC-7's certification.toEqual. It cannot convict AC-6: erasing the certification block leaves predicate untouched, so JSON.stringify(unbound.predicate) === JSON.stringify(bound.predicate) still passes. The missing mutation is its mirror — make certification unavailability block (push a blocker, or gate strictMergeReady on identityBindingComplete) and show it reddens the byte-identity assertion and not the certification.toEqual one. Both arms currently live in one test, which is exactly why the discrimination is worth showing rather than assuming. Your AC-8 row does establish a real independence — certification arm versus non-required-check specimen — it just is not the pair AC-8 names. If that was your intended reading of "the two arms above", say so and I will accept it with the reasoning recorded; the ambiguity is in the AC's wording, not in your work.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 94 - the new arm extends the existing fail-closed contract in its own idiom (undefined / malformed / unreadable-label each blocking separately) instead of bolting on a parallel mechanism, and nonRequiredFailures derives from emittedOnly so the required-set boundary cannot be hardcoded even accidentally. 6 deducted for the label ignoring run identifiers the normalizer already carries, leaving a same-workflow same-name collapse theoretically reachable.
• [CONTENT_COMPLETENESS]: 96 - rule 8 added to the numbered contract, the fail-closed paragraph updated to include the new field, @param documenting the undefined-vs-[] distinction, and a substrate audit with byte deltas that verify. 4 deducted: the certification object's three outcome values are enumerated in no JSDoc, so a consumer learns the vocabulary only by reading the branch.
• [EXECUTION_QUALITY]: 90 - four falsification attempts against the production surfaces, three of which I expected to land, all held under live data. 10 deducted for the AC-6 arm shipping without a demonstrated convicting mutation.
• [PRODUCTIVITY]: 92 - seven of eight ACs fully discharged with evidence, including the two hardest (AC-4's non-observation, AC-6/7's bound-vs-unbound pair). AC-8's evidence answers an adjacent independence question.
• [IMPACT]: 88 - governs every merge handoff in the organization, and the specimen shows the current shape survives expert review. A validator that certifies what it cannot see is the highest-leverage defect class we have.
• [COMPLEXITY]: 72 - five files across two production surfaces, their specs, and loaded substrate; the joined-predicate semantics require holding required-vs-emitted, artifact-vs-certification, and fetched-vs-empty distinctions simultaneously.
• [EFFORT_PROFILE]: Quick Win - high ROI on a governance-critical path at 217 added lines, with the complexity carried by the specification rather than the implementation.

The one action is cheap, and I would rather it exist than accept the arm on inspection — given the ticket's own thesis is that this class survives careful reading. Re-request me and I will turn it around; I hold no other concern, and the checksEvidenceStatus non-observation is going straight into my own practice.

⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code

[review-budget-managed]

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