Skip to content

fix(pr-pipeline): claude-review reports success on a partial review of an oversize diff #11575

Closed
melodic-software/ci-workflows
#944

Description

@kyle-sexton

Problem

The PR-head claude-review lane reviews only part of an oversize diff and still reports a clean result. On #11169 at head 778ceff0d0 (1186 files), the review comment says "This is a partial review: the diff spans 1186 files, and I read only two of the new scripts in full", yet the check pr-review / claude-review-status concluded success and the comment heads "0 finding(s)". Anyone reading checks or finding counts (a person, the babysit merge gate, the post-green review pass) sees a reviewed PR when less than 1% of the files were read.

#11000 covers the oversize case for pr-review-main.yml (the main run), and #11548 (from #11463) chunks pr-verify; neither covers this lane.

Proposal

Review an oversize diff in chunks, as #11548 does for pr-verify: split by file under the lane's read budget, review each chunk, and report the sum of findings only when every chunk returns a verdict. Until a chunk's verdict is in (or when a chunk fails), the lane reports partial coverage as a non-success conclusion, and its comment's first line states the coverage (files read of files changed) before the finding count.

Acceptance

  • A fixture diff over the read budget gets a per-chunk review and a combined finding count, with no success conclusion before every chunk returns.
  • A review that read fewer files than the diff changed never concludes success, and its first line states files read / files changed.
  • A chunk that fails or returns no verdict keeps the lane non-success (fail closed).
  • An independent security review of the change finds no HIGH/CRITICAL (it edits a review workflow).

Refs: #11000, #11463, #11169

Activity

  1. added
    needs-triageNot yet classified. Floor until a type and one priority tier are set.
    on Oct 31, 2026
  2. github-actions commented on Oct 31, 2026

    @github-actions
    Contributor

    Proposed work class: work-class: structural
    Basis: automated intake triage; the lanes App applied this class.

    Size: Medium
    Complexity: Medium
    Uncertainty: Known
    Flags: none
    Blocked by: none

    Understood: the PR-head claude-review lane reviews only part of an oversize diff (1186 files on #11169) but still concludes success with "0 finding(s)". You propose chunked review under the read budget. The lane would report the summed findings only when every chunk returns a verdict, and a non-success conclusion with files read / files changed on the first line otherwise. A failed chunk would keep the lane non-success.

    The acceptance criteria are specific, so nothing more is needed from you. Related but not duplicates: #11000 (main run) and #11548/#11463 (pr-verify chunking). I found no open issue covering this lane.

    Automated triage, applied by the lanes App.

  3. added
    priority: mediumReal value, no hard deadline; normal backlog flow.
    and removed
    needs-triageNot yet classified. Floor until a type and one priority tier are set.
    on Oct 31, 2026
  4. added
    status: readyTriaged, unblocked, and fully specified; eligible to pick up.
    work-class: structuralRefactors, migrations, contract changes; cross-cutting and hard to reverse.
    admission-deferredAdmitted but waiting for WIP room; the hourly work-unblock sweep labels it agent-ready.
    agent-readyFully specified and briefed; eligible for autonomous pickup from the frontier.
    and removed
    admission-deferredAdmitted but waiting for WIP room; the hourly work-unblock sweep labels it agent-ready.
    on Oct 31, 2026
  5. github-actions commented on Oct 31, 2026

    @github-actions
    Contributor
    No description provided.
  6. github-actions commented on Oct 31, 2026

    @github-actions
    Contributor
    No description provided.
  7. added
    host-runNeeds an unattended run on a host; the host runner takes it, not a person or the issue lane.
    and removed
    agent-readyFully specified and briefed; eligible for autonomous pickup from the frontier.
    on Oct 31, 2026
  8. melodic-automation-lanes commented on Oct 31, 2026

    @melodic-automation-lanes
    Contributor

    The work-resolve-issue lane stopped on this issue (patch-protected-path): the change needs a path the issue lane may not push, which a maintainer session builds by hand.
    It added host-run and removed agent-ready: an agent run on a host builds it, and it waits on no decision from a person.

    In the run's words:

    The run stopped early (patch-protected-path): its change needs a path the issue lane may not push, which a person changes instead. In the run's words: patch-protected-path: .github/workflows/pr-review-hosted.yml. The claude-review lane (check pr-review / claude-review-status) is defined there as a thin caller of melodic-software/ci-workflows/.github/workflows/pr-review-cli.ymlmain, which owns the review, its comment and the status conclusion. Chunking the oversize diff and failing closed on partial coverage needs a chunk matrix and a combining status job in this caller workflow (or a change to the reusable in the ci-workflows repository), and this lane cannot push to /.github/workflows/**.

    What the run says it did not do:

    • All four acceptance items (chunked review, fail-closed partial coverage, coverage on the comment's first line, independent security review): they need edits to the protected path .github/workflows/pr-review-hosted.yml and/or the external reusable pr-review-cli.yml
    • Repository checks check-changelog-fragments and check-affected were not run: the run stopped at once on the protected path with no change in the working tree

    Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/41213351497

    To hand it back to the lane, remove host-run and add agent-ready again.

  9. kyle-sexton commented on Nov 1, 2026

    @kyle-sexton
    MemberAuthor

    Host run. A local session built the fail-closed coverage gate for the claude-review lane in the reusable pr-review-cli.yml, in melodic-software/ci-workflows.

    Where the change lives. The lane's review, comment and status are owned by melodic-software/ci-workflows/.github/workflows/pr-review-cli.yml@main, not by this repository's pr-review-hosted.yml, which only calls it. So the fix is a PR in ci-workflows.

    What ran (Claude Code 2.1.316):

    • Edits:
      • The schema and prompt now ask for files_read.
      • The extract step, in all three model legs, takes the file count in scope and the diff's byte count from the job, measured where the diff is written. It requires the diff path to be a regular file and records coverage.
      • publish-review/publish.cjs handles a partial outcome: the check fails, no completion notice is written, the first visible line states files read of files changed, and the marker is written at sha=none.
    • Tests: node --test over 13 suites, 321 tests, all passing. New tests cover partial, over-budget, missing-coverage, symlink, and the next push after a partial review getting a whole review. A mutation check (writing the head sha on a partial review) was caught.
    • Lint: zizmor --offline reported no findings, and biome is clean.

    Against the acceptance criteria:

    • AC1, chunked review with a combined count: not done. Chunking needs per-chunk slots in all three sequential legs of a 4000-line reusable, too large for this run. It remains for a follow-up on this issue.
    • AC2, a review that read fewer files than changed never concludes success, and its first line states files read / files changed: done. A diff over the 300000-byte read budget (pr-verify's chunk size) is partial whatever the model reports.
    • AC3, a failed chunk or missing verdict fails closed: done for the single-review case. Missing or invalid coverage is incomplete. A partial review records no reviewed head, so no merge gate binds it and the next push reviews the whole PR.
    • AC4, independent security review: a fresh review:security-reviewer agent found no HIGH or CRITICAL issue. It found one IMPORTANT issue, a symlink bypass of the byte check, and the second commit fixes it.

    Review lanes: on the PR, claude-review found one valid issue. The full-diff step ran without pipefail, so a failing git diff was masked and the review passed as complete. The third commit fixes it with a test. The thread was answered and resolved. On the final head eeba855, claude-review and claude-security-review report 0 findings, Codex reported no findings on open, every check passes, and the merge state is CLEAN. The PR is not merged.

    Effect to know about: until chunking lands, any PR whose diff is over 300000 bytes, or whose review reads fewer files than it changed, keeps a non-success claude-review-status (and security-review). Its ci-status therefore stays red. That is the non-success result the issue asks for.

    PR: melodic-software/ci-workflows#943

    Target labels: add none; remove none.

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

    host-runNeeds an unattended run on a host; the host runner takes it, not a person or the issue lane.priority: mediumReal value, no hard deadline; normal backlog flow.status: readyTriaged, unblocked, and fully specified; eligible to pick up.work-class: structuralRefactors, migrations, contract changes; cross-cutting and hard to reverse.

    Type

    Fields

    No fields configured for Bug.

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions