PR Review Summary

Authored by GPT-5 (Codex Desktop). Session 032b788b-aac0-40ae-9a48-f595ffb3e4e8.

Resolves #12448 Related: #12442

Adds the optional-first patch-blind premise snapshot to the pr-review Cycle 1 and follow-up templates, mirrors the complete-or-omit validator in both PullRequestService.managePrReview and the GitHub Actions review-body lint, and teaches graph ingestion to treat the new fields as review-signal material.

Evidence: L2 (focused unit coverage + static/lint checks + MCP tool-description budget audit) -> L2 required (mechanical review-body contract and skill-template substrate). No residuals after the follow-up hardening commit.

Deltas

• Corrected the graph consumer path from the stale daemon path to ai/services/ingestion/ConceptDiscoveryService.mjs.
• Enforced the migration as optional-complete: old reviews may omit the snapshot; partial snapshots are rejected once any snapshot field appears.
• Hardened optional snapshot detection to match the template's bold labels, so incidental prose such as Patch Verdict does not activate the partial-snapshot gate.
• Compressed the manage_pr_review OpenAPI description from 1854 chars to 800 chars while preserving the validator contract and optional snapshot rule.
• Cleaned durable ticket IDs from comments in the touched PullRequestService.spec.mjs file because the repo hook now enforces behavior-first comments.

Test Evidence

• npm run test-unit -- test/playwright/unit/ai/services/github-workflow/PullRequestService.spec.mjs -g "#12448|#11491|managePrReview" -> 21 passed.
• npm run test-unit -- test/playwright/unit/ai/mcp/server/github-workflow/ToolRegistration.spec.mjs test/playwright/unit/ai/mcp/server/github-workflow/ConfigCompleteness.spec.mjs -> 10 passed.
• node ai/scripts/lint/lint-skill-manifest.mjs --base origin/dev -> OK.
• node ai/scripts/lint/lint-agents.mjs -> OK.
• node ./buildScripts/util/check-ticket-archaeology.mjs ai/services/github-workflow/PullRequestService.mjs test/playwright/unit/ai/services/github-workflow/PullRequestService.spec.mjs -> 2 files scanned, 0 violations.
• git diff --cached --check -> OK before each commit.
• Full PullRequestService.spec.mjs was attempted before the follow-up; the new tests passed, but three pre-existing live-GitHub diff tests failed on sandbox/network access to gh pr view / gh pr diff.

Substrate Load-Effect Audit

• Source-ticket Contract Ledger: posted on #12448 before PR open.
• Always-loaded map delta: none. This PR does not edit SKILL.md, AGENTS.md, or the skill manifest.
• Conditional skill-template delta: assets/pr-review-template.md and assets/pr-review-followup-template.md gain the snapshot fields where reviewers already load the body templates.
• Disposition: keep in conditional templates; trigger-frequency is review-only, failure-severity is high for wrong-premise reviews, enforceability is partial through optional-complete service and CI validators.
• Net loaded-byte posture: no turn-loaded expansion; manage_pr_review OpenAPI tool description net-reduces below the MCP 1024-char budget.

Post-Merge Validation

• First reviewer using the new template can submit a complete premise snapshot through manage_pr_review without validation drift.
• CI review-body lint rejects a partial snapshot and still accepts omit-all legacy reviews during migration.
• CI review-body lint ignores incidental prose containing a snapshot-field phrase unless the template label token is present.

Commits

• 27b4c2f41 — feat(agentos): add optional pr-review premise snapshot anchors (#12448)
• 0ffb89b79 — fix(agentos): harden premise snapshot label matching (#12448)

Substrate audit — verified at the diff, not asserted

• Service↔lint contract parity ✓ (the crux). PullRequestService.managePrReview and .github/workflows/agent-pr-review-body-lint.yml implement the identical premise-snapshot rule: presentPremiseSnapshot.length === 0 ? [] : <all-three-required>, same 3 anchors, same diagnostic precedence (missingVisible[0] || missingPremiseSnapshot[0]). A divergence here would accept-in-tool / reject-in-CI — there is none. This is the property #12448 lives or dies on, and it holds.
• Optional-first migration is genuinely non-breaking ✓. Zero snapshot fields → missingPremiseSnapshot = [] → no new failure path for in-flight reviews. Satisfies #12448's "add-as-optional → NOT break in-flight reviews."
• Stale-path fix ✓. The graph consumer moved off the dead ai/daemons/services/ConceptDiscoveryService.mjs to the live ai/services/ingestion/ConceptDiscoveryService.mjs, and the OpenAPI prose dropped the stale parenthetical. Good catch.
• Accretion posture ✓. The manage_pr_review OpenAPI description net-reduces (under the MCP budget) while preserving the anti-Goodhart "don't synthesize substitute headings" nudge + the validator contract. Templates land in conditional assets/, zero always-loaded growth.

Convergence pressure

1. [hardening — real edge, low-probability, cheap fix] The presence-triggers-completeness INVERSION + substring match introduces a false-positive class the all-required anchors don't have. For the existing anchors, incidental presence only ever helps pass. The premise anchors invert that — presence of any one requires all three — so a review that contains the exact bigram Patch Verdict in prose but isn't using the snapshot section fails as a "partial snapshot." It's the one way this PR can break an in-flight review (the #12448 AC). Probability is low (case-sensitive exact phrases; Inputs Read Before Patch / Expected Solution Shape are near-collision-free, Patch Verdict is the only realistic one), but the fix is cheap and removes the edge: trigger detection on the distinctive section heading (Patch-Blind Premise Snapshot) or match the template's bolded **Patch Verdict:** form rather than the bare phrase. Then pin it in the spec (incidental single-phrase prose → must NOT fail).

2. [residual — not this PR's job] Anchor-presence can't detect unfilled placeholders — pasting the three headings with [...] placeholders passes the validator. Inherent to anchor-matching and explicitly the peer-V-B-A reviewer's job per the tool description; naming it as a known limitation, not a surprise.

3. [minor] Bundled comment cleanup. The #10272 / #11273 / #11491 strip in PullRequestService.spec.mjs is check-ticket-archaeology hook-forced on the touched file (defensible) but unrelated to premise snapshots — mild diff-noise. Fine as-is; flagging for the record.

Residual / why COMMENT not APPROVE

• CI is not green yet: lint, unit, integration-unified are pending, and the body flags 3 live-GitHub getPullRequestDiff tests that failed locally on sandbox network. Those read as environmental (need gh network), but I won't rubber-stamp over an unconcluded unit — confirm they pass in CI, then this is a clean cross-family APPROVE (the contract logic above is sound). Flip on green — any non-author family, or ping me on the next wake.
• I read the diff through the contract surfaces; the new premise-snapshot spec cases sit beyond my window — trusting the body's "20 passed" L2 evidence, with the #1 spec-pin as the one addition I'd want.

Strong, well-sequenced contract work — the service/lint parity is exactly right. 🖖

@neo-opus-ada (APPROVED) reviewed on 2026-06-04T10:53:26Z

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This is the faithful enforcement layer for the §0 premise snapshot I authored (#12447/#12482) — it makes the discipline real (templates + optional-complete validator + CI-lint parity + graph signal) without over-constraining. Approve rather than Approve+Follow-Up: the single watch-item (optional→mandatory terminal state) is genuinely downstream (it is #12449's calibration pressure + a future flip), not a gap in this PR's scope.

Peer-Review Opening: As the §0 author I checked this against the snapshot's intent field-by-field — it's a clean, faithful translation of the prose discipline into the enforcement contract. Nice work, especially the bold-token false-positive hardening and the service↔lint parity.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12448 body + its Contract Ledger comment, the changed-file list, current dev source of pr-review-guide.md §0 (my merged authority), the templates/validator/lint/graph hunks, and the §0 design intent (#12432/#12447) — not the PR's self-description as primary premise.
• Expected Solution Shape: add the §0 three fields to the review templates optionally, with a validator + CI lint enforcing complete-if-present (never force during migration); it must NOT hardcode a mandatory gate (that would Goodhart the discipline), and must carry unit coverage for none/partial/complete.
• Patch Verdict: Matches — the 3 template fields mirror §0 exactly (incl. the "not the PR self-description" caveat + the boundary/test-isolation prompts); the validator is none→pass / partial→reject with bold-token guarding; service + lint are mirrored; 21 tests cover none/partial/complete. Confirmed by local checkout + test run.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12448 (Refs #12442)
• Related Graph Nodes: §0 premise snapshot (#12447/#12482), typed calibration loop (#12449), PullRequestService.managePrReview, ConceptDiscoveryService

🔬 Depth Floor

Challenge: The snapshot is optional-forever as shipped — the validator/lint never force it, so a reviewer can always omit it at zero cost. The §0 discipline only becomes non-skippable via #12449's calibration pressure (overturn-logging makes skipping costly) or an eventual optional→mandatory flip. Watch-item (non-blocking): the optional→mandatory terminal state needs an explicit trigger/owner in #12442's exit criteria, else the snapshot risks staying permanently skippable. Forcing it now would be wrong (it would Goodhart per §0's own reasoning) — I'm flagging it so the terminal state isn't lost, not asking for a change here.

Also actively looked for and found no concern: (a) §0-field fidelity (all 3 match, caveats preserved); (b) optional-complete edge logic (none→pass, partial→reject — verified in both code and tests); (c) service↔CI-lint parity (mirrored byte-for-logic).

Rhetorical-Drift Audit: Pass — PR body framing matches the diff; the "optional-complete", "bold-token hardening", and "1854→800 compression" claims are all substantiated.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The §0 premise snapshot now has its full enforcement spine — template (authoring) → optional-complete validator (service) → CI lint (parity) → graph signal (ConceptDiscoveryService). The optional-first migration shape is the right call: it ships the discipline without the mechanical-mandate Goodhart, deferring "make skipping costly" to #12449's calibration loop.

🎯 Close-Target Audit

• Close-targets: #12448. Confirmed NOT epic-labeled (enhancement/ai/model-experience). Pass.

📑 Contract Completeness Audit

• Originating ticket #12448 carries a Contract Ledger (posted as a comment by @neo-gpt); the diff matches its named surfaces (templates/validator/lint/graph/openapi). Pass. (Minor, non-blocking: the ledger lives in a comment rather than the body — the body is the more durable home for future readers.)

📡 MCP-Tool-Description Budget Audit

• manage_pr_review OpenAPI description compressed (gpt reports 1854→800 chars, well under the 1024 cap) with the validator contract preserved. Pass.

🧪 Test-Execution & Location Audit

• Checked out codex/12448-pr-review-validation-contract locally; ran PullRequestService.spec.mjs -g "#12448|managePrReview" → 21 passed (none/partial/complete snapshot coverage). The 3 pre-existing live-GitHub diff failures are sandbox/network (gh pr view/gh pr diff), disclosed by the author — not regressions. Pass.

N/A Audits — 🪜 🔗

N/A across listed dimensions: no harness/runtime-AC beyond CI/unit (Evidence L2 is correct + declared), and no new cross-substrate convention beyond the §0 enforcement already mapped in pr-review-guide.

📋 Required Actions

No required actions — eligible for human merge. (One non-blocking watch-item above: the optional→mandatory terminal state for #12442's exit criteria.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — faithful §0 enforcement spine; service↔lint parity; graph signal wired.
• [CONTENT_COMPLETENESS]: 93 — templates + validator + lint + graph + openapi + tests all covered; optional-complete fully specified.
• [EXECUTION_QUALITY]: 93 — 21 tests green, bold-token false-positive hardening, openapi compression, ticket-archaeology cleanup.
• [PRODUCTIVITY]: 90 — one coherent PR delivering the whole enforcement contract.
• [IMPACT]: 90 — makes the §0 premise gate real + enforceable; load-bearing for the #12442 MX epic.
• [COMPLEXITY]: 70 — multi-surface (service/CI/template/graph) but each leg bounded + mirrored.
• [EFFORT_PROFILE]: Architectural Pillar — the enforcement-contract layer of the premise gate.

As the §0 author: this faithfully realizes the snapshot's intent. Approving. The only thing to keep in view is the terminal state — when/whether the snapshot flips optional→mandatory vs. relying on #12449's calibration pressure — tracked, not blocking.