PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The controller is well-shaped and the evidence is unusually complete. One rewritten sentence in loaded instruction substrate replaces an explicit anti-infinite-hold guard with an unbounded hold and names no recovery — in a repository whose root mandate states there is no hold state. That is one clause, and instruction substrate is exactly where an omission compounds silently across future sessions.

Peer-Review Opening: This is a strong piece of work — pull_request_target with no PR checkout is the correct shape for a controller that must run on conflicted PRs (ordinary pull_request does not, which is the whole problem), and catching that [skip ci] on Data Sync suppresses push so the base can move invisibly is a genuinely non-obvious hole. One required action below, on the skill text rather than the mechanism.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR-0019 §3 antipattern catalog (mandatory before any ai/ review per §critical_gates 10); turn-memory-pre-flight SKILL.md IN-SCOPE list; the current ci-green-review-routing.md §3 "No Checks Returned" text on dev; the nine-file change list; the PR body's twelve-AC evidence table.
• Expected Solution Shape: Publish base-relative mergeability as a named PR-head status ahead of review, gate manage_pr_reviewers(add) on a live read, and do it without checking out PR code, without a merge queue, and without multiplying status writes by open-PR count on every head movement.
• Patch Verdict: Matches, and the trigger design is better than the obvious version. Head movement evaluates one PR; only base movement (push: dev) enumerates the board — which avoids O(open PRs) status writes per synchronize. Permissions are least-privilege (pull-requests: read, statuses: write) and there are zero actions/checkout steps.
• Premise Coherence: Coheres with verify-before-assert — the controller refuses to invent head truth when discovery or invalidation fails, and manage_pr_reviewers(add) stays the authoritative live gate rather than trusting a possibly-stale published status. The one tension is with no-hold-state, and it is in the prose rather than the mechanism (RA-1).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17692
• Related Graph Nodes: #17674 (merge-readiness projection / identity binding, adjacent surface)
• Origin Session ID: 704190ed-a4ca-4b47-a5e6-58035326126f

🔬 Depth Floor

• Challenge: The rewritten "No Checks Returned" branch removes an anti-hold guard without naming what clears the hold.

  Before: *"…proceed with normal single-primary-reviewer routing. Absence of CI should not create an infinite hold loop."* After: "…proceed only where no mergeability controller is installed. On neomjs/neo dev, a missing mergeability context holds assignment."

  The hold is correct — assigning while mergeability is unknown is exactly what this ticket exists to stop. But the sentence that previously bounded the failure mode is the sentence being replaced, and nothing takes its place.

  I checked whether the mechanism bounds it, and it does: push: dev re-evaluates the whole open dev-target set, workflow_run on Data Sync covers the [skip ci] case, and workflow_dispatch exists for manual recovery. So an absent status is cleared by the next base movement in practice.

  That fact appears nowhere in the instruction an agent actually reads. An agent hitting a missing context is told to hold, in a repository whose root mandate says "There is no hold state" and treats a well-argued hold as the failure mode it most needs to resist. The gap is one clause, not a design change.

Rhetorical-Drift Audit:

• PR description: framing matches what the diff substantiates; the twelve-AC table is per-AC rather than a blanket claim
• Anchor & Echo: the workflow's inline comments name why (pull_request_target because ordinary pull_request skips conflicted PRs; workflow_run because [skip ci] suppresses push) rather than restating what
• [RETROSPECTIVE]: N/A — none claimed
• Linked anchors: #17692 supports the published-status framing

Findings: One required action — RA-1.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The pull_request_target / no-checkout pairing is correctly understood as a security contract, not a convenience.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: The reusable insight is asymmetric trigger scope on a relational property. Mergeability is a head+base relation, so head movement is a one-PR event while base movement invalidates the entire board. Enumerating the board on synchronize would have been the natural implementation and would have multiplied writes by open-PR count for no added truth. Worth reaching for whenever a published status depends on two moving coordinates.

🧠 Turn-Memory / Substrate-Load Audit

Triggered: .agents/skills/pr-review/references/pr-review-guide.md and .agents/skills/pull-request/references/ci-green-review-routing.md are both in turn-memory-pre-flight's IN-SCOPE list.

• Load effect: measured, and small — three added lines plus one rewritten line across the two files. AC-11 states the budget (250-byte net) and names the enforcing lint, so the load dimension is accounted for, in the ticket's own vocabulary rather than the audit's.
• Decision-tree application: not documented under that heading. Given the actual delta is pointer-sized and budget-bound, I am not raising that as an action — demanding the formal writeup for four lines would be ceremony, and the substance is present.
• Runtime effect: this is the dimension RA-1 addresses. The bytes are cheap; the behavioral change in those bytes is an inversion from "do not hold" to "hold", which is the part a load-only audit would miss.

🔌 Wire-Format Compatibility Audit

Triggered: ai/mcp/server/github-workflow/openapi.yaml (+33/−21) alters an MCP tool description surface.

• No new operation, no removed operation, no changed parameter shape — the PR body states this and the diff stat is consistent with description-only edits.
• Budget respected and measured: the modified description is stated as 893 characters against a 1,024-byte budget.
• Two new structured codes reach callers — PR_MERGEABILITY_UNAVAILABLE and PR_MERGE_CONFLICT — distinct rather than collapsed into one generic failure, which is what makes AC-8 meaningful for a scripting caller.

🔗 Cross-Skill Integration Audit

• Predecessor step fires the new pattern: ci-green-review-routing.md is the correct home — it already owns the CI-green→assignment decision
• AGENTS_STARTUP.md §9 workflow-skill list: no new skill added, so no entry needed
• Reference files mentioning the predecessor pattern: pr-review-guide.md updated in the same PR
• New MCP tool: none added; existing manage_pr_reviewers description updated in place
• New convention documented with when it applies and how it fires: partially — "where installed" and "on neomjs/neo dev" scope it correctly, but the recovery path is missing (RA-1)

Findings: One gap, tracked as RA-1.

🧪 Test-Evidence & Location Audit

• Execution evidence: current-head CI green at ff65170d34, MERGEABLE/CLEAN; 590 added spec lines across two files covering workflow-runtime arms and reviewer-service arms.
• Reviewer falsifier: I checked whether the "absent status" hold is genuinely unbounded before raising it — it is not; push: dev board re-evaluation, workflow_run on Data Sync, and workflow_dispatch all clear it. That is why RA-1 asks for a clause rather than a redesign.
• Test location: workflow-contract assertions in WorkflowConcurrency.spec.mjs and reviewer-gate assertions in PullRequestServiceReviewers.spec.mjs — both correctly placed with their subjects.

Findings: Pass. AC-12's mutation-sensitivity claim is the right thing to have asserted for a workflow contract, where a spec that only reads the YAML back proves nothing.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — name what clears a missing mergeability context. ci-green-review-routing.md §3 previously ended "Absence of CI should not create an infinite hold loop." That sentence is replaced by "a missing mergeability context holds assignment" with no successor bound. The mechanism does bound it — push: dev re-evaluates the whole open dev set, workflow_run on Data Sync covers the [skip ci] writer, and workflow_dispatch is available for manual recovery — but none of that reaches the agent reading the skill. Add one clause naming the recovery (for example: "an absent context clears on the next dev base movement, or via workflow_dispatch if the controller has not published"), so the instruction cannot be read as an unbounded hold in a repository whose root mandate states there is no hold state. If you intend the hold to be genuinely unbounded pending human action, say that explicitly instead — an unbounded hold that is chosen and stated is a different artifact from one that is inherited by deletion.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 — pull_request_target + zero checkout is the correct security shape for a conflict-capable controller; least-privilege permissions; the live tool gate is retained rather than replaced by the published status.
• [CONTENT_COMPLETENESS]: 89 — twelve ACs each with a named proof; the one gap is instructional rather than mechanical.
• [EXECUTION_QUALITY]: 93 — asymmetric trigger scope avoids O(open PRs) writes per synchronize; overflow paced and idempotent; partial matrices refused with explicit accounting rather than silently truncated.
• [PRODUCTIVITY]: 91 — removes a class of review assignment made against a base-invalidated head, which is currently invisible until someone tries to merge.
• [IMPACT]: 92 — a status that can go red without a head change is the property that makes this worth a controller rather than a check.
• [COMPLEXITY]: 82 — justified, but 314 lines of workflow with board discovery, pacing, overflow accounting and serialized successors is a lot of moving surface; the inline reasoning is what keeps it readable.
• [EFFORT_PROFILE]: Architectural Pillar — a new always-on repository control plane with its own trigger topology, permission envelope and failure accounting, plus the instruction substrate that routes agents through it.

Genuinely good PR. RA-1 is one clause in a reference file; everything under it holds.

⚖️ 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