Skip to content

Rebalance PR test evidence between CI, authors, and reviewers #15122

Description

@neo-gpt

Context

Neo's PR-review execution rule was created after the static-diff hallucination in #10501: a reviewer asserted that tests were green without executing them. That correction was right for its time, but it predates the current CI matrix and later CI-first review controls.

The substrate has since changed materially:

  • #10897 added the unit + integration CI matrix.
  • #11465 made green CI a precondition for spending a formal review.
  • #11467 delayed actionable author-side review requests until current-head CI is green.
  • #11441 and #11885 established review-cost reduction as an explicit MX concern.

The remaining rule still requires every code reviewer to check out the branch and discover/run related tests locally. On 2026-07-13, operator review-pressure analysis challenged that rule: reviewers generally cannot know which tests a change affects without scanning the test tree and reconstructing dependency reach. That discovery can cost more than the duplicate execution it is meant to justify.

Live latest-open sweep: checked the latest 20 open issues plus scoped live search and recent A2A claims immediately before creation; no equivalent ticket or in-flight claim exists.

The Problem

The current pr-review contract conflates three different responsibilities:

  1. CI executing routine unit/integration coverage.
  2. Authors proving behavior that hosted CI does not execute.
  3. Reviewers falsifying concrete behavioral concerns.

As a result, reviewers are told to rediscover related tests and rerun them even when exact-head CI already executed the canonical suite. The inverse shortcut is also unsafe: author evidence cannot trigger only when a PR claims whitebox E2E or another non-CI evidence class, because an author could simply omit the claim. A change to an application or feature with existing non-CI coverage can affect that coverage even when the test file is untouched.

The current cold-review guide + Cycle-1 template measure 50,160 bytes before PR-specific context. Adding a new decision table, static NEO_TEST_SKIP_CI file list, classifier ritual, or audit payload would increase the very review pressure this ticket must reduce.

The Architectural Reality

This is one test-evidence field-class correction across two existing skills:

  • .agents/skills/pr-review/references/pr-review-guide.md owns reviewer execution behavior.
  • .agents/skills/pr-review/assets/pr-review-template.md and pr-review-followup-template.md own review evidence shape.
  • .agents/skills/pull-request/references/pull-request-workflow.md already requires every agent PR body to contain ## Test Evidence; it owns author evidence.
  • Required exact-head CI owns routine unit/integration execution.
  • Existing non-CI coverage of a touched surface—not author prose—triggers author-side evidence.
  • Reviewers own targeted falsifiers when the review surfaces a named behavioral concern.
  • Test-location review remains relevant only when a test is added or moved.

Progressive Disclosure outcome: modify and compress existing payloads. Do not add a skill, audit payload, static skip registry, or always-loaded rule.

Structure-map gate: no new .mjs file or novel code placement is involved; the owning substrate is the existing pr-review and pull-request skill payloads.

The Fix

Replace the unconditional reviewer-local-rerun rule with this boundary:

Required exact-head CI is the default execution evidence for routine unit/integration behavior. Reviewers do not search for or rerun related unit/integration tests solely to duplicate green CI. A reviewer runs a targeted test only when the review identifies a named behavior not established by CI, and records that falsifier. Authors execute existing non-CI coverage for every touched surface and record the receipt in ## Test Evidence.

Apply it coherently:

  1. Rewrite pr-review-guide.md §2.2 and §7.5 to preserve exact-head source inspection while removing mandatory reviewer discovery/reruns.
  2. Replace the Cycle-1 template's local-run checklist with terse exact-head CI + optional named-falsifier fields.
  3. Align the follow-up template with the same boundary.
  4. Extend author-side ## Test Evidence guidance: if a touched application/feature has existing whitebox E2E, component, visual, local-model, embedding, host-native, or other non-CI coverage, the author runs the relevant existing coverage even when no test file changed and no evidence class is claimed.
  5. Keep test-placement validation only for added/moved tests.
  6. Rewrite the §7.7 anti-pattern so the violation is claiming execution without exact-head CI/author evidence/falsifier—not failing to duplicate the author's command.
  7. Net-reduce the loaded review surface; do not create another payload.

Contract Ledger Matrix

Target Surface Source of Authority Proposed Behavior Fallback Docs Evidence
Reviewer routine execution evidence Current required CI + #10501 anti-hallucination lineage + #11465 CI-first review gate Exact-head green required CI is sufficient; no default local rerun or test-tree discovery Named reviewer falsifier when a concrete behavior remains uncertain pr-review-guide.md §2.2/§7.5 Exact wording audit + byte measurement + skill lint
Reviewer formal evidence shape pr-review-template.md / follow-up template Record exact-head CI and optional targeted falsifier; test location only for added/moved tests Full evidence note when CI does not establish the challenged behavior Existing templates Template validation / representative dry run
Author non-CI evidence pull-request-workflow.md Test Evidence contract Existing non-CI coverage for every touched surface is executed and named, independent of PR claims or test-file changes Explicit environment-bound residual with post-merge/live validation; never silent omission pull-request-workflow.md PR-body example + source audit
Skill loaded surface ADR 0008 Map/Atlas + substrate-accretion defense Compress existing payloads; no new audit/list/classifier prose None Existing payloads only wc -c before/after + lint-skill-manifest.mjs

Acceptance Criteria

  • pr-review-guide.md states that exact-head green required CI is the default execution evidence for routine unit/integration behavior.
  • Reviewers are explicitly told not to search for or rerun related unit/integration tests solely to duplicate green CI.
  • Reviewer-local execution is retained only as a targeted falsifier for a named behavioral concern.
  • The Cycle-1 review template records exact-head CI plus Reviewer falsifier: N/A / <command + named concern> instead of mandatory local test execution.
  • The follow-up template uses the same boundary without reintroducing changed-surface test discovery.
  • Test-location review remains required only when tests are added or moved.
  • pull-request-workflow.md makes existing non-CI coverage of the touched surface the author-side trigger; wording must not depend on whether the PR claims E2E/model/visual evidence.
  • The author rule explicitly covers an app/feature change whose existing whitebox E2E can regress even when the E2E spec itself is unchanged.
  • ## Test Evidence includes a compact Existing non-CI coverage for touched surfaces receipt or an honest None found.
  • Environment-bound residuals are explicit and tied to the evidence ladder/post-merge validation rather than silently omitted.
  • The §7.7 anti-pattern is reframed around unsupported execution claims, not failure to duplicate author execution.
  • No new skill, audit payload, static NEO_TEST_SKIP_CI list, dependency classifier, or always-loaded AGENTS rule is added.
  • The reviewer cold surface (pr-review-guide.md + Cycle-1 template) is smaller than the 50,160-byte filing baseline.
  • The combined modified skill payload surface does not exceed its pre-change baseline; any author-side addition is offset by reviewer-side deletion.
  • /turn-memory-pre-flight load-effect evidence and the source-ticket Contract Ledger are carried into the PR body/review gate.
  • node ai/scripts/lint/lint-skill-manifest.mjs --base origin/dev passes.

Decision Record impact

Aligned with ADR 0008 (existing payload compression and decision-atom refinement; no new primitive or ADR amendment).

Out of Scope

  • Adding whitebox E2E, component, or visual suites to hosted CI; #15110 is the adjacent CI-coverage problem.
  • Resolving or reclassifying current NEO_TEST_SKIP_CI guards.
  • Building a test dependency graph or reviewer-side coverage classifier.
  • Weakening patch-blind premise review, architecture review, security review, or targeted behavioral falsification.
  • Treating green CI as approval.
  • Changing cross-family review or human-only merge authority.

Avoided Traps

  • Reviewer test-tree archaeology: rejected because the reviewer lacks implementation-local knowledge and can spend more context discovering tests than the rerun adds.
  • Evidence-by-author-claim: rejected because omitting the claim would bypass existing non-CI coverage.
  • Static skip-file list: rejected because NEO_TEST_SKIP_CI mixes environment requirements, state pollution, races, and missing fixtures; it is neither a safe-run registry nor dependency map.
  • New audit payload: rejected because the rule is a compact responsibility boundary and must net-reduce review substrate.
  • Green CI equals correctness: rejected; CI is execution evidence, while reviewers still own premise, architecture, diff correctness, and named falsifiers.

Related

Origin Session ID: 837ad74b-c2d2-413d-9aab-b7165a93a82a

Retrieval Hint: "PR review pressure CI default execution evidence author existing non-CI coverage touched surface targeted reviewer falsifier"

Metadata

Metadata

Assignees

Labels

aienhancementNew feature or requestmodel-experienceIssues related to Model Experience (MX), agent workflows, and AI frictiontesting

Type

No type

Fields

No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions