Skip to content

fix(review): make reviewers read the head's check runs, never the description's claims, and name make verify unconditionally #248

Description

@bketelsen

Problem

A reviewer produced a false block on frostyard/core#118 round 2 (2026-08-24 23:41Z, model gpt-5.3-codex, item 6db977a4…). Its evidence:

  • "Attempted repository non-mutating gate make verify; repository has no verify target (make: *** No rule to make target 'verify')" — core is a Node repository whose gate was npm run verify.
  • "PR body evidence states npm run check:fleet fails for … clix and firn … so the PR's own fleet-conventions criterion is not green on this head."

The PR body was the author's claim from the previous head; on the reviewed head a6b39f4 the fleet-conventions check run had completed success 33 seconds before the verdict. The reviewer never read the head's check runs. The minted instructions make this possible:

  • src/queue/pull-request-review.ts:644 tells the reviewer to "read the pull request's description and its diff … run the repository's non-mutating gate (make verify) when it exists" — nothing tells it to read the head's check runs, and "when it exists" turns a missing target into silence rather than a finding.
  • src/queue/pull-request-review.ts:698 (the pr-review-fix instructions) has the same "make verify where it exists".
  • .agents/skills/review-snowcat-queue/SKILL.md lines ~54 and ~92 name make verify "where it exists" and ask for "the checks you ran" in evidence, but never for the head's CI conclusions.
  • docs/specs/work-queue.md rule 53 (~line 764) requires the minted instructions to name make verify and forbid make check; it says nothing about check runs.

Core is deciding that every repository exposes make verify/check/ci (core ADR-0044, proposed 2026-08-24, widening ADR-0043 from Go repositories to all); once accepted, "where it exists" is wrong twice over — a missing target is a conformance gap, not a reason to skip.

Change

  1. src/queue/pull-request-review.ts — the pr-review instructions (line ~644): add a sentence before reading the description: read the head's check runs (gh pr checks <n> --repo <owner/repo> or gh api repos/<owner/repo>/commits/<head>/check-runs) and treat their conclusions as the evidence for any CI-anchored acceptance criterion; the description is the author's claim and never evidence of a check's state; a run still in progress means wait or return unable-to-review, never infer from the body. Replace "(make verify) when it exists" with "(make verify; every fleet repository exposes it — its absence is itself a blocker, defect:makefile:verify-missing)". Same replacement in the pr-review-fix instructions (line ~698), and add "check the head's check runs after your push" there.
  2. .agents/skills/review-snowcat-queue/SKILL.md: the "Judge" step gains the same read-the-check-runs rule and the unconditional make verify; the "Finish" step's evidence bullet requires the head SHA, each check run's name and conclusion, and the local make verify result.
  3. docs/specs/work-queue.md rule 53: the minted instructions MUST direct the reviewer to the head's check runs as the evidence for CI criteria and MUST name make verify unconditionally (no "where it exists"). Name the rule in the PR template.
  4. Tests: test/pull-request-review.test.ts already asserts on minted instruction text — extend the assertion to the new sentence (check runs) and to the absence of "when it exists".

Acceptance criteria

  • On the PR's own check job log, test/pull-request-review.test.ts passes with an assertion that the minted pr-review instructions contain check-runs (or gh pr checks) and do not contain "when it exists", and the job is green on the PR head.
  • grep -c 'when it exists\|where it exists' src/queue/pull-request-review.ts .agents/skills/review-snowcat-queue/SKILL.md prints 0 for both.
  • npm run check passes (paste the coverage-floor lines).

Constraints

src/queue/pull-request-review.ts, the skill, the spec rule, and the test only; no store, MCP, or schema change. Risk tier 2 (src/queue/** is not a governance boundary here, but the instructions steer every reviewer). Reference: this session's finding on core#118 rounds 1–2 and the snowcat rollout plan's "Provider reliability" item.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    snowcatQueued for the Snowcat fleet

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions