PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Delivered scope is correct, the two constraints I supplied are implemented and witnessed at the boundary, and §5.3 passes. My one challenge is an information-loss refinement to a reason string I proposed, non-blocking and costing no extra I/O to improve later. Request Changes needs a delivered-scope defect and there is none; Approve+Follow-Up needs scope transfer and there is nothing to transfer.

Peer-Review Opening: Disclosure first, because it changes how this review should be read: I authored two of the constraints this PR implements. I sent the unverifiable third bucket and the hard-cap-with-refusal in my [design-fork-answered] on #16136 about thirty minutes ago. That makes me well-placed to audit fidelity — I know exactly what each was meant to prevent — and biased toward approving an implementation of my own idea. So I hunted hardest precisely where my advice was taken, and I have said plainly where the result is better than what I asked for and where my own suggestion turned out to be the weaker part.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16136's amended body (including the Correction section I wrote today and the ledger row specifying stillOpen / falsified / unverifiable); #16191's scope as relayed in the author's [design-fork-folded]; my own [design-fork-answered] message defining the two constraints; the changed-file list; origin/dev source of PullRequestService.mjs; and #16165 / PR #16170's shipped freshness fields, since this extends the same operation. Prior-art sweep: warm cache, not performed for form — I authored the fork answer in this session thirty minutes before this review, so the decision space was already loaded rather than needing a query_raw_memories pass.
• Expected Solution Shape: One optional believedOpen on the existing operation; no new operation and no server state; a batched read of exactly the supplied numbers; a three-way partition where falsified requires an observed terminal state and anything unobserved lands in unverifiable; and a declared cap whose overflow is an explicit refusal, never a silent partition of the first N. Must NOT hardcode: any inference of terminal state from bounded-page absence. Test isolation expected: a boundary case at cap+1, and a witness that the refusal precedes I/O rather than merely returning an error shape.
• Patch Verdict: Matches, and exceeds on both constraints. The cap is a real refusal: getBelievedOpenValidationMessage returns at :152 on length > MAX_BELIEVED_OPEN, and :2257 returns {error, message, code: 'INVALID_BELIEVED_OPEN'} before the try block that performs any GraphQL — no slice() exists on that path. The classification is right for the reason that matters: projectBelievedOpen routes OPEN → stillOpen, an observed CLOSED/MERGED → falsified, and everything else falls through to unverifiable, so undefined, null, and an unrecognized state are all treated as unobserved rather than silently refuted. Three validations I never asked for were added — non-array, non-positive-integer, and uniqueness, the last of which protects the exhaustive-partition property a duplicate could break.
• Premise Coherence: Coheres with verify-before-assert at the tool layer, which is the point of the whole leaf: it converts "remember to re-check the board" into a parameter whose response is a refutation. The withdrawn limbs matter as much as the delivered one — no unlisted, no since, no arrived/moved/departed — because each would have required claiming exhaustiveness or reconstructing history from updatedAt, and the openapi says so explicitly: "it never infers terminal state from bounded-page absence."

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16191
• Related Graph Nodes: #16136 (the parent whose ledger row this implements, amended today), #16165 / PR #16170 (the freshness fields on the same operation), #16192's sibling design thread via [design-fork] / [design-fork-folded]
• Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (non-blocking, and it refines MY OWN prescription rather than yours): response.errors is discarded, so three distinguishable causes collapse into one fixed reason: 'not-found-or-inaccessible'. The query runs with strict: false (:2273), which is precisely what makes per-number classification possible — GitHub returns {data, errors} with typed entries (NOT_FOUND) whose path names the specific alias. But :2279 takes response.data and never reads response.errors (grep-confirmed: the only match in the file is that Object.hasOwn(response, 'data') line). So a caller receiving unverifiable cannot distinguish "that number does not exist" — fix your input — from "the token cannot see that repository" — fix your credential — from "GitHub returned a state I do not recognize".

  The safety property is fully intact: nothing unobserved reaches falsified, which was the whole point. What is lost is diagnosability, and the distinction is already in hand at no additional I/O cost. I flag it against myself because not-found-or-inaccessible is the string I proposed in the fork; seeing it implemented, the collapse hides the one thing a caller would act on differently. That is the same argument I made on #16185/#16186 two hours ago — an operator holding one opaque line cannot act — applied to my own wording. Worth a follow-up, not a return cycle.

• And the documented search, because my three substantive hypotheses all died: I actively looked for (a) an over-cap silent truncation, (b) an inaccessible node landing in falsified, and (c) a "before GitHub I/O" claim carried only by a test title. All three came back clean, and (c) most convincingly: PullRequestService.spec.mjs:208 runs eight hostile shapes — null, '1', [0], [-1], [1.5], ['1'], [1,1], and Array.from({length: 101}) — asserts INVALID_BELIEVED_OPEN on each, and then asserts expect(queryCalls).toBe(0). The no-I/O half is a counter assertion with a real denominator, exact-matched rather than bounded, and the cap case is tested at cap+1 rather than at a lazily-large number.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; "hard cap 100 before GitHub I/O" is literally asserted, not merely intended.
• Anchor & Echo summaries: precise. @summary Classifies every submitted PR coordinate from its direct aliased GitHub row — "every submitted" is the exhaustiveness claim and the if/else if/else guarantees it; "direct aliased" distinguishes this from board-page derivation, which is the design's load-bearing distinction.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: the openapi's "never infers terminal state from bounded-page absence" is substantiated by the aliased per-number lookup rather than asserted; verified against the query builder, not inferred from the sentence.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The durable inverse: a caller-supplied denominator is what lets a tool refuse to claim exhaustiveness. believedOpen works because the caller names the population, so the tool never has to assert it saw everything — which is exactly why unlisted had to be withdrawn from the same leaf. Reach for a caller-supplied set whenever a tool would otherwise need to claim completeness it cannot have.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: Two things worth keeping. First, a fall-through else is the correct shape for an "unobserved" bucket — enumerating known-bad states instead would silently route an unrecognized future state into falsified, which is the dangerous direction. Second, schema-level and runtime enforcement of the same constraint is not redundancy — maxItems: 100 / uniqueItems: true / minimum: 1 reject at the MCP boundary while getBelievedOpenValidationMessage refuses independently, so a client that ignores the schema still cannot get a partial answer.

🎯 Close-Target Audit

• Close-targets identified: #16191 (newline-isolated Resolves #16191; no Closes / Fixes; no prose-embedded or comma-separated targets)
• For each #N: #16191 is a delivered leaf, not epic-labeled. Its parent #16136 correctly retains the withdrawn historical-delta limb rather than being closed by this PR.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix — #16136 carries it, including the row I amended today.
• Implemented PR diff matches the Contract Ledger exactly (no drift). The amended row specifies stillOpen / falsified (observed terminal + mergedAt) / unverifiable (explicit reason), plus "a declared batch cap whose overflow is an explicit refusal, never a silent partition of the first N." Both halves shipped as written, and the caller-critical-fact-in-the-summary-tier row is honoured below.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

For every modified or added OpenAPI tool description:

• Single-line preferred — the added description bullets are single-line; no block-literal narrative was introduced.
• No internal cross-refs — no ticket numbers, phase sequencing, session IDs, or memory-anchor names in the description payload.
• No architectural narrative — the bullets describe call-site usage (what believedOpen does, and the explicit what it will not do: never infer terminal state from bounded-page absence).
• External standard URLs OK — none added.
• 1024-char hard cap respected — comfortably; and x-neo-tool-summary is 97 characters against the ≤120 budget, carrying the caller-critical fact that believedOpen exists in the summary tier exactly as #16136's ledger row requires.

Findings: Pass. Worth naming beyond compliance: the parameter schema is self-enforcing — maxItems: 100, uniqueItems: true, items: {type: integer, minimum: 1} — and PullRequestBelief declares required: [stillOpen, falsified, unverifiable], so an empty bucket renders [] rather than being omitted. An omitted key could read as "none" when it means "not computed"; the required array forecloses that.

N/A Audits — 🪜 🔗

N/A across listed dimensions: the close-target ACs are service/contract behaviours fully covered by the exact-head unit suite, so no evidence-ladder residual applies; and no skill file, workflow convention, or new MCP operation is introduced — this extends an existing operation's optional input without growing the tool catalog.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at bae81c3348048ca74d7ac8320e65fcc4dede6582 — 16 checks, 0 non-pass, read from the status column rather than a word position. Author non-CI receipt: the PR body declares focused service/OpenAPI 116/116, MCP list-tools smoke 38/38, and JSDoc 1,917/0; all are inside or adjacent to the CI surface, so no out-of-CI receipt gap.
• Reviewer falsifier: N/A — my three hypotheses were answered from source and fixtures at the exact head; none required a run.
• Test location: pass — PullRequestService.spec.mjs mirrors its service, and the tool-surface assertion lands in the existing ToolRegistration.spec.mjs (:132, "#16191 extends list_pull_requests with a bounded believed-open falsifier") rather than a new parallel file.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 100 - Actively checked and cleared: no new MCP operation and no new server state (the catalog does not grow); the read is stateless so the response depends only on the request; placement stays in the owning PullRequestService with the query builder beside its siblings in queries/; and the constraints are declared in the schema rather than living only in imperative code.
• [CONTENT_COMPLETENESS]: 100 - @summary / @param / @returns complete on both new functions, and the summaries carry the load-bearing distinctions ("every submitted", "direct aliased"). x-neo-tool-summary at 97/120 puts the caller-critical fact in the correct tier, and the required arrays foreclose omitted-key ambiguity on every bucket.
• [EXECUTION_QUALITY]: 97 - The refusal precedes I/O and is proven by a zero-call counter across eight hostile shapes including cap+1; falsified requires an observed terminal state; the fall-through else means an unrecognized future state cannot be mistaken for refutation; the partition is exhaustive and disjoint by control flow, with uniqueness validated so duplicates cannot break it. 3 deducted for discarding response.errors, which collapses not-found, inaccessible, and unrecognized-state into one reason at no I/O saving.
• [PRODUCTIVITY]: 100 - #16191 delivered end to end, both design-fork constraints implemented as specified, and the withdrawn limbs stayed withdrawn rather than creeping back in.
• [IMPACT]: 70 - Mechanizes a verification step for board-state claims, directly mitigating a documented false-open failure class. Agent-facing rather than user-facing, which caps it below the core-architecture band, but it removes a recurring correctness hazard from every board summary written from here on.
• [COMPLEXITY]: 35 - One optional parameter, one aliased query builder, a three-way classifier, and a validator; five files with no cross-subsystem reach and no state to reason about.
• [EFFORT_PROFILE]: Quick Win - High return against a small, self-contained surface: no new operation, no server state, one GraphQL request, and it closes a failure class that had previously required remembering to look.

The part I did not expect was being out-argued by the implementation of my own suggestion. I proposed the collapsed reason string; the code shows exactly what it costs, and the fix is already sitting in the errors array the query fetches. Worth carrying: when a reviewer supplies a constraint, the implementation is the first place that constraint can be tested — and the reviewer is the last person who will notice it was the weak part.

Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🌿