Skip to content

Review agent: verify diff completeness against PR's stated scope for bulk-change PRs #2580

Description

@fullsend-ai-retro

What happened

On PR #2508, the author stated the goal was to "pin all GitHub Actions to full-length commit SHAs." The fullsend review agent ran 4 times across 3 push cycles. It correctly flagged 3 unpinned actions in renovate.yml (which was in the diff) but missed two categories of omission that a human reviewer caught:

  1. Out-of-diff composite action: .github/actions/setup-gcp/action.yml contained an unpinned google-github-actions/auth@v3 — the same action that was SHA-pinned in e2e.yml within the diff. This composite action is called by 6+ reusable workflows, giving it a wider attack surface. (HIGH finding by waynesun09, review comment)

  2. Missing file from branch: functional-tests.yml existed on main but was absent from the PR branch (merged after branch was cut). It contained 6 unpinned third-party actions including WIF credential handling. (HIGH finding by waynesun09, second review)

Both gaps required the author to push additional fix commits, adding 1 full rework cycle that could have been avoided.

What could go better

The review agent's correctness and intent-coherence sub-agents should detect when a PR's title/description signals a bulk or exhaustive change (patterns like "pin all", "rename all", "migrate all X to Y") and verify that the diff actually covers all matching files in the repo.

In this case, a simple grep -r 'uses:' .github/ internal/scaffold/ filtered against the diff's file list would have revealed both the composite action gap and any workflow files not included in the PR.

I'm fairly confident this is a recurring pattern — bulk-change PRs are inherently scope-sensitive, and the current review agent architecture evaluates the diff in isolation without checking whether the diff fulfills the PR's stated intent. The intent-coherence sub-agent currently checks whether changes are within scope (not out-of-scope), but does not check whether the scope is complete.

Existing issue #1275 (trace workflow_call chains) would partially help by detecting the composite action gap, but would not catch the missing functional-tests.yml or generalize to non-Actions bulk changes. Issue #1774 is about suppressing false positives for incomplete multi-site changes — the inverse of what's needed here.

Proposed change

Add an intent-completeness check to the intent-coherence sub-agent definition at skills/pr-review/sub-agents/intent-coherence.md.

The check should:

  1. Parse the PR title and description for bulk-change intent signals (e.g., "all", "every", "each", "migrate", "rename across", "pin all").
  2. When detected, infer a file pattern or search query from the PR's stated scope (e.g., for "pin all GitHub Actions" → search for uses: in **/*.yml under .github/ and scaffold dirs).
  3. Compare files matching that pattern against the PR's changed-file list.
  4. If files exist in the repo that match the scope but are not in the diff, emit a medium+ finding listing the omitted files.

This should be scoped narrowly to avoid false positives — only trigger when the PR description explicitly claims exhaustive coverage. The finding should be informational ("these files may also need changes") rather than asserting they definitely do, since some omissions may be intentional.

Validation criteria

On the next 3 PRs to this repo that claim to make a bulk/exhaustive change (e.g., "update all X", "pin all Y"), the review agent should identify any files matching the stated scope that are not in the diff. Measure: (1) the review agent flags at least one omitted file that a human would also flag, and (2) no more than 1 false positive per review where the omission was intentional.


Generated by retro agent from #2508

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

    agent/retroRetro agentagent/reviewReview agentcomponent/harnessAgent harness, config, and skills loadingfeatureFeature-category issue awaiting human prioritizationpriority/mediumNormal priority, plan for next cycletriagedTriaged but awaiting human prioritizationtype/featureNew capability request

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions