Skip to content

fix: filter inline review comments to files in the PR diff - #1040

Merged
waynesun09 merged 4 commits into
mainfrom
fix-post-review-422
May 18, 2026
Merged

fix: filter inline review comments to files in the PR diff#1040
waynesun09 merged 4 commits into
mainfrom
fix-post-review-422

Conversation

@waynesun09

Copy link
Copy Markdown
Member

Summary

  • Adds ListPullRequestFiles to the forge client interface to fetch the changed file list from a PR
  • Filters inline review comments in submitFormalReview to only include files present in the PR diff
  • Findings on files outside the diff still appear in the sticky comment body — nothing is lost

Root cause: The review agent flagged docs-currency findings in docs/guides/admin/installation.md and docs/ADRs/0033-per-repo-installation-md for PR #1035, but those files weren't changed by the PR. When fullsend post-review tried to submit them as inline review comments, GitHub returned 422 Unprocessable Entity because the API only allows inline comments on files in the diff.

Run: https://github.com/fullsend-ai/.fullsend/actions/runs/25942695364/job/76263838470

Test plan

  • go vet ./internal/cli/ ./internal/forge/... — clean
  • go test ./internal/cli/ — all pass, including new TestFindingsToReviewComments_FiltersByDiffFiles
  • go test ./internal/forge/... — all pass
  • make lint — clean

The review agent can flag findings in files not changed by the PR
(e.g. stale docs). When these are posted as inline review comments,
GitHub returns 422 Unprocessable Entity because the API only allows
comments on files in the diff.

Fetch the PR's changed files before submitting the review and drop
inline comments that reference files outside the diff. The findings
still appear in the sticky comment body, so nothing is lost.

Fixes the 422 failure seen in fullsend-ai/.fullsend run 25942695364
where docs-currency findings targeted docs files outside PR #1035's
diff.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown

Site preview

Preview: https://3d95a784-site.fullsend-ai.workers.dev

Commit: c00c29f9678a1331c17cf7cd6fdf9af4a9ae72e6

@fullsend-ai-review

fullsend-ai-review Bot commented May 15, 2026

Copy link
Copy Markdown

Review

Findings

No findings.

Previous run

Review

Findings

No findings.

Previous run (2)

Review

Findings

Low

  • [correctness] internal/forge/github/github.go — The GitHub API caps PR file listings at 3,000 files. For very large PRs exceeding this limit, files beyond the cap would not appear in diffFiles, causing their inline comments to be silently filtered. The current behavior (filtered rather than 422 error) is still an improvement, but a doc comment noting the limit would help future maintainers.
    Remediation: Add a comment on ListPullRequestFiles noting the 3,000-file API cap.

Info

  • [style] internal/forge/github/github.go — The pagination safety bound (page <= 100) is reasonable but the magic number could benefit from a brief comment explaining it exists as an infinite-loop guard.
Previous run (3)

Review

Findings

No findings.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label May 15, 2026
Add TestSubmitFormalReview_FiltersByPRFiles to verify the full pipeline
(ListPullRequestFiles → build diffFiles map → filter → submit review)
excludes inline comments on files not in the PR diff.

Add TestSubmitFormalReview_ListPRFilesErrorFallsBack to verify graceful
degradation when the file list API fails — all comments pass through
unfiltered.

Include the actual error in the degradation log message for debugging.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review

Copy link
Copy Markdown

Review follow-ups

Created follow-up issues for actionable non-blocking review findings:

  • #1041 — GitHub API caps PR file listings at 3,000 files. For very large PRs exceeding this limit, files beyond the cap would not appear in diffFiles, causing their inline comments to be silently filtered. Current behavior is still an improvement over 422 errors.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 15, 2026
@ralphbean
ralphbean requested a review from ben-alkov May 15, 2026 22:20
Comment thread internal/cli/postreview.go

@ben-alkov ben-alkov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Log a StepWarn with the count of filtered findings so CI logs show
when agents are generating comments on files outside the PR diff.

Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 18, 2026
Eliminates the second O(n) pass over findings to compute the warning
count. The function now returns ([]ReviewComment, int) where int is the
number of findings dropped because their file was not in the PR diff,
removing the coupling between the warning logic and the function's
internal filter criteria.

Also:
- Log a StepInfo when the PR file list is empty (non-error empty slice),
  so operators can see when filtering is silently disabled
- Add a doc comment to ListPullRequestFiles in the forge.Client interface
- Note GitHub's 3000-file cap in the LiveClient implementation
- Add TestSubmitFormalReview_EmptyPRFileListFallsBack
- Update TestFindingsToReviewComments* to assert filtered counts

Signed-off-by: Wayne Sun <gsun@redhat.com>
@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels May 18, 2026
@waynesun09
waynesun09 added this pull request to the merge queue May 18, 2026
Merged via the queue into main with commit a1e6fbc May 18, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants