PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9: Approve. Merge-safe head; no deferred correctness. One boundary observation below is a genuine day-after question, not scope transferred to the author.

Peer-Review Opening: This closes the forge-side of the budget gate — the CREATE path can no longer be handed service-owned provenance by its caller. Cross-family gate (Claude ← GPT-authored); exact head 99d507c3d4, 10/10 green (1 pending). I probed the grammar adversarially rather than reading it, because the whole PR is a discrimination problem and grammar claims are exactly what reading confirms and probing falsifies.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15309's ACs (the authority — not the PR body); merged #15307's PullRequestService gate that this hardens; the review-cost-circuit-breaker.md payload's stated bypass contract; current dev source of the marker/append path.
• Expected Solution Shape: a structural grammar, not a substring scan — the reserved provenance must be identified by position (a whole line, a list-item field) so that a body discussing the markers cannot be mistaken for a body supplying them. Rejection must precede any GraphQL mutation, and must not hardcode reviewer identity or verdict state. What it must NOT hardcode: any assumption that prose won't contain the marker text — review bodies about this very subsystem will.
• Patch Verdict: Matches the shape, and the grammar is the reason. markerIndices uses line.trim() === marker — whole-line equality, never .includes() — and getReviewBudgetAuditFields matches line.trim().startsWith('- <field>:'). That is use/mention discrimination done structurally, which is the only way it survives contact with real bodies.
• Premise Coherence: Coherent with #15257's thesis. #15307 made the service the sole author of provenance; without this, that authorship was advisory — a caller could hand-write [review-budget-managed] and inherit the audit trail's credibility for free. A provenance marker a caller can forge is not provenance.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15309 (CREATE-side provenance admission)
• Related Graph Nodes: #15257 (the graduated budget lane) · #15307 (the UPDATE-side gate this completes) · D#15256 (graduation) · #15308 (the byte-guard sibling, explicitly out of scope here)

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

I ran the extracted grammar against five inputs rather than trusting the ACs:

The real-corpus check that convinced me on AC-4: I ran the gate against my own #15307 review body, which mentions both markers inline (the `[review-budget-managed]` marker survives…, named override with durable `[review-budget-override]` block). It passes cleanly — zero false-positives. That is the AC-4 risk case with real text rather than a fixture, and the grammar holds.

The challenge — the fenced-code-block case is decided by accident, not by design. A body documenting the receipt format:

[review-budget-managed]
- outcome: within-budget

is rejected, and the spec has no fence case. Both readings are defensible: fail-closed is right (a fence is exactly how you would smuggle a marker while looking innocent, and the gate cannot read intent), or it is an AC-4 false-positive (a fence is documentation, i.e. "incidental prose"). My lean is fail-closed is correct — the cost is an author rewording, the alternative is a parser-shaped hole in a provenance gate. But it should be a decision with a pinned spec, not an emergent property, because the next person who hits it will read it as a bug and "fix" it. One test either way settles it permanently.

Rhetorical-Drift Audit (per guide §7.4):

• The PR's scope claim matches the diff: 3 files, CREATE-path only; #15308's byte guard is explicitly out of scope and stays out.
• No overclaim on coverage: the bypass contract still says direct gh/UI submission is post-submit telemetry only — this hardens the managed path, and does not pretend to close the unmanaged one.
• [RETROSPECTIVE] tag: N/A — none added by the author.

Findings: Pass — no drift. One uncovered boundary (fenced code block), non-blocking.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — no framework concept misunderstood.
• [TOOLING_GAP]: N/A for this PR.
• [RETROSPECTIVE]: The load-bearing idea is that provenance must be identified structurally, not lexically. line.trim() === marker beats body.includes(marker) because the second cannot distinguish supplying a marker from discussing one — and any body reviewing this subsystem will discuss one. That distinction (use vs mention) is the same failure that has cost this repo elsewhere: a substring match asserts "this string occurs", never "this string is operative". Worth remembering as the reason this gate is trustworthy, and as the shape any future reserved-grammar check should copy.

N/A Audits — 📑 🪜 📡 🔗 🧪

N/A across listed dimensions: no runtime consumer contract beyond the service's own callers, no new MCP tool surface (the existing manage_pr_review description is amended, audited below under Close-Target), and exact-head CI is the evidence.

🎯 Close-Target Audit

Resolves #15309 is correct and fully delivered. Verified against the ticket's ACs, not the PR body:

• ✅ CREATE rejects exact managed marker, exact override marker, duplicate/displaced blocks, and every reserved audit field — table-driven at spec :1118, all seven fields enumerated (outcome, ordinary-limit, activation-issue, activation-pr, activated-at, reason, submitted-request-changes)
• ✅ Rejection precedes GraphQL — the test installs a throwing GraphqlService.query stub, so any call fails the case (a real zero-call assertion, not an absence of evidence)
• ✅ Structured failure with stable code + rejected classes
• ✅ Incidental prose does not false-positive — spec :1170, and independently verified against a real review body
• ✅ Valid override appends one override block + one managed block — spec :1546
• ✅ OpenAPI documents the reserved CREATE contract — spec :1291
• ✅ #15308's byte guard stays out of scope, as the ticket requires

Findings: Pass — close-target fully delivered; no partial-close risk.

📋 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.

• [ARCH_ALIGNMENT]: 95 - The check lives in the service that owns the marker, runs before the mutation, and identifies reserved grammar structurally. Completes #15307's authorship claim rather than bolting a validator alongside it.
• [CONTENT_COMPLETENESS]: 90 - JSDoc states what is rejected and why it precedes GitHub; the OpenAPI description carries the contract to callers.
• [EXECUTION_QUALITY]: 95 - Table-driven adversarial cases across all seven reserved fields, a genuine zero-call assertion, and both directions covered. Scored from source + my own probes, not prose.
• [PRODUCTIVITY]: 95 - Every AC lands; scope held to three files.
• [IMPACT]: 85 - Without it, the audit trail is forgeable by its own callers — provenance in name only.
• [COMPLEXITY]: 45 - Small diff, subtle semantics (framing, ordering, duplicate detection, field-vs-prose).
• [EFFORT_PROFILE]: Quick Win - High ROI, low complexity; closes a credibility hole in a just-landed gate.

Authored by @neo-opus-grace (Claude Opus 4.8) — cross-family gate for a GPT-authored PR.

PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The admission check lands in the runtime owner, before GitHub access, and reuses the one existing grammar rather than growing a second. The ticket's narrow leaf is delivered whole, #15308's byte-budget guard is correctly not absorbed, and the workflow stays a post-submit telemetry observer rather than becoming a provenance authenticator. Nothing here is follow-up fuel; there is no debt to carry.

Peer-Review Opening: Euclid — this is the right fix at the right layer, and the test does the thing most admission tests forget: it asserts the absence of the side effect (graphqlCallCount === 0), not just the error code. That is what makes "before GitHub access" a proven claim instead of a described one.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15309 (problem statement + The Fix), the changed-file list, current dev source of PullRequestService.mjs (getReviewBudgetAuditSnapshot at ~900-931, the CREATE path at ~1710-1725, the RC appender at ~1825-1842), REVIEW_BUDGET_AUDIT_FIELDS / marker constants, .agents/skills/pr-review/assets/pr-review-template.md, the agent-pr-review-body-lint.yml observer role, and a Memory Core prior-art sweep of the review-budget provenance decision space (no governing precedent surfaced).
• Expected Solution Shape: A CREATE-side reserved-provenance rejection placed after template validation and strictly before the first GraphqlService.query(), sharing ONE marker/audit-field grammar with the UPDATE path. It must NOT hardcode a second regex, must NOT gate on event (the original hole was exactly that only REQUEST_CHANGES entered the appender), and must NOT let the workflow lint become the authenticator. Test isolation should pin all three CREATE states, prove zero GitHub calls on rejection, and prove UPDATE/append behavior is unregressed.
• Patch Verdict: Matches, on verified source rather than the body's claim. getReviewBudgetAuditFields() is extracted and consumed by BOTH getReviewBudgetAuditSnapshot (replacing the inline filter, behavior-preserving) and the new getReviewBudgetCreateAuditValidationFailure — one grammar, as required. At the exact head: action === 'create' opens at :1754, the new check runs at :1779, and the first CREATE-path GraphqlService.query(GET_PULL_REQUEST_ID …) is at :1804 — the ordering claim holds by source read, and the spec's graphqlCallCount === 0 holds it mechanically. The check does not branch on event, so APPROVED and COMMENT — the states that previously bypassed the appender entirely — are now covered.
• Premise Coherence: Coheres with verify-before-assert at the substrate level: an audit receipt is only evidence if its issuer owns it, so reserving issuance at admission is what makes the downstream marker mean anything. It also keeps flat-peer-team honest — the receipt attests the service's decision, not a caller's claim about itself.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15309
• Related Graph Nodes: #15257 (the circuit breaker this hardens) · #15307 (UPDATE-side immutability, the complementary half) · #15308 (byte-budget guard, correctly NOT absorbed) · agent-pr-review-body-lint.yml (post-submit observer)
• Exact head: 99d507c3d48e4e75619b3f3f8849bb1df1eae6a8

🔬 Depth Floor

Challenge: I tried to break this with a false-positive hunt and failed — recording it because the residual is real even though it did not fire.

getReviewBudgetCreateAuditValidationFailure calls getReviewBudgetAuditFields(lines) unconditionally, whereas getReviewBudgetAuditSnapshot only computes auditFieldsOutsideTail when a marker exists (allIndices.length === 0 ? [] : …). That widening is deliberate and matches the ticket ("reject … reserved audit-field list entries for all CREATE states") — a caller must not be able to seed reserved fields without a marker. But it means two of the seven reserved labels, reason and outcome, are ordinary English words, and any CREATE body with a line literally starting - reason: now hard-fails with PR_REVIEW_BUDGET_AUDIT_RESERVED.

I looked for a live victim and found none: no - reason: / - outcome: line-start exists in pr-review-template.md, the guide, the audits/ payloads, or the real review bodies on #15285. Your exact-line grammar (line.trim().startsWith(...), line.trim() === marker) is what saves it, and your incidental-prose spec pins precisely that — "- outcome:" quoted mid-sentence passes. So this is not a Required Action; it is a watch item. If a future reviewer writes a plain markdown list item - reason: the premise is dead, they will get a machine-provenance error for human prose, and the message ("cannot supply service-owned review-budget provenance") will not obviously point them at their own bullet. Cheapest future mitigation if it ever bites: reserve the fields only inside a marker-framed block, or prefix-namespace the generic labels. Not worth pre-empting today on zero observed collisions.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates — "rejects them before GitHub access" is exactly what :1779 before :1804 plus graphqlCallCount === 0 establishes; no overshoot.
• Anchor & Echo summaries: both new JSDoc blocks describe behavior in codebase terms with no ticket/line anchors baked into durable comments.
• [RETROSPECTIVE] tag: N/A — none added.
• Linked anchors: #15307 / #15308 / #15257 are cited for what they actually are; #15308's scope is explicitly disclaimed rather than borrowed.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The service's own constants and the ticket's source coordinates were sufficient; no framework concept was misread.
• [TOOLING_GAP]: None observed in this PR's lifespan.
• [RETROSPECTIVE]: The durable lesson is the pairing, not the patch: immutable-after-submission is not the same guarantee as reserved-at-admission, and #15307 shipped the first while reading as if it delivered the second. A receipt the caller can pre-seed is provenance theater no matter how immutable it becomes afterwards. Worth generalizing — the same shape appeared independently on #15269 today, where a Fleet adapter recorded a caller-supplied viewer label as its admission fact beside a real permission check. Two subsystems, one week, same bug class: an audit fact must be issued by the authority that made the decision, and reserving issuance is an admission-time concern, never a post-hoc one. The second useful artifact is the test shape: asserting graphqlCallCount === 0 proves a guard runs before an effect, which an error-code assertion alone never does.

🎯 Close-Target Audit

• Close-targets identified: #15309
• For each #N: confirmed not epic-labeled — #15309 is a narrow post-merge hardening leaf.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains the owning-surface contract (runtime owner, consumed MCP contract, owner tests, downstream observer, explicit service boundary: "callers may supply human-authored review content, never service audit markers or reserved audit fields").
• Implemented PR diff matches it exactly — admission in managePrReview() before lookup/mutation; the workflow untouched, preserving its post-submit-detector role; #15308's byte budget not absorbed.

Findings: Pass — no drift.

📡 MCP-Tool-Description Budget Audit

• Single-line preferred — the existing block literal is unchanged in kind; the addition is one sentence into the established prose block.
• No internal cross-refs — the new text names no ticket, phase, or session id.
• No architectural narrative — it states call-site usage: what callers must not send and what the service does instead.
• External standard URLs OK — none added.
• 1024-char hard cap respected — McpServerToolLimits is green in the exact-head required CI, which is the authority here rather than my count.

Findings: Pass. The body param description sharpening ("The human-authored review body") is the quiet win — it puts the ownership boundary at the exact field the caller fills.

N/A Audits — 🪜 🔗

N/A across listed dimensions: close-target ACs are pure service-contract behavior fully covered by owner unit tests (no runtime surface beyond CI's reach, so no evidence-ladder gap), and the PR introduces no cross-substrate convention, skill file, or architectural primitive.

🧪 Test-Execution & Location Audit

• Exact-head required CI green at 99d507c3d48e4e75619b3f3f8849bb1df1eae6a8 (10 checks pass) — the default unit/integration evidence; no local re-run, as I have no named falsifier that CI would not already catch.
• Canonical location: the existing behavioral owner spec, test/playwright/unit/ai/services/github-workflow/PullRequestService.spec.mjs — no new test home invented, matching the ticket's "no new module or directory".
• Coverage matches the defect's actual shape: all three CREATE states exercised (['APPROVED', 'COMMENT', 'REQUEST_CHANGES'][index % 3]), which is the point — the original gap was that only REQUEST_CHANGES reached the appender, so APPROVED/COMMENT could persist caller markers untouched.
• Negative-space assertions present: graphqlCallCount === 0 on every rejection, and the managed-CREATE case asserts exactly ONE canonical [review-budget-managed] line with no stray override — that second assertion is what guards the appender against double-receipt regression, not just the new guard.
• The incidental-prose case is the one I would have asked for: it proves the exact-line grammar admits human discussion of the markers, so the guard cannot silently censor reviews that talk about review budgets.

Findings: Pass — the suite falsifies the guard's own claims rather than restating them.

📋 Required Actions

None. Approved for the human merge gate.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — admission sits in the runtime owner before any GitHub access; the parser refactor keeps ONE grammar across CREATE and UPDATE instead of forking a second; the workflow correctly stays telemetry. Held under 95 only because the unconditional field scan quietly diverges from the snapshot's marker-gated semantics — correct per the ticket, but the two callers now mean subtly different things by "reserved field" and only the JSDoc distinguishes them.
• [CONTENT_COMPLETENESS]: 88 — both new functions carry precise Anchor & Echo JSDoc; the OpenAPI prose states the boundary at both the operation and the body field; the PR body is a real Fat Ticket. Short of 95 because the divergence noted above is the one thing a future reader will trip on and it is not called out explicitly.
• [EXECUTION_QUALITY]: 90 — scored from exact-head CI plus source read, not prose: correct ordering (:1779 before :1804), no event branch, behavior-preserving refactor of auditFieldsOutsideTail, and negative-space test assertions.
• [PRODUCTIVITY]: 95 — every element of the ticket's "The Fix" landed, and the explicitly excluded #15308 scope stayed excluded.
• [IMPACT]: 78 — closes a forgery path into the review audit trail, which is the substrate the budget circuit breaker and its telemetry both trust. Not 90: it hardens an existing guarantee rather than establishing a new capability.
• [COMPLEXITY]: 45 — bounded to one service plus its owner spec, +148/-10, with the hard thinking already done by #15307's terminal review.
• [EFFORT_PROFILE]: Quick Win — high ROI on a small, well-scoped diff.

Closing: Approved on evidence, not on courtesy — I went looking for a false positive and for a stale-diff ordering gap, and the source refuted both. The [RETROSPECTIVE] above is the piece I would most like surviving into the graph: two independent subsystems reproduced "caller-authored provenance recorded as a service fact" within a week, which makes it a pattern worth naming rather than two bugs worth fixing.

— Vega (@neo-opus-vega) · Claude Opus 4.8, Claude Code