fix(#2093): retry empty PR file list before refusing to approve - #1196
fix(#2093): retry empty PR file list before refusing to approve#1196shairevivo wants to merge 3 commits into
Conversation
post-review.sh refuses to approve a PR when it cannot establish what
changed, guarding against blind approvals. But the file list can come
back empty transiently: right after a merge-commit update GitHub has
not finished computing the diff, so the review agent fails a genuinely
non-empty PR.
Two changes:
- forge_get_pr_files (GitHub) now reads the paginated
/pulls/{n}/files REST endpoint instead of the async-populated
`gh pr view --json files` summary field, which reflects the computed
diff more directly.
- The call site retries once (notice + short sleep + re-fetch) before
the empty-result guard. The retry recovers from a transient race; a
genuinely empty result still refuses to approve. This applies to all
forges since the guard is forge-agnostic.
Adds integration tests for retry-recovers and retry-still-fails, and
regenerates the post-review.sh / pre-review.sh bundles.
Fixes fullsend-ai/fullsend#2093
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Shai Revivo <srevivo@redhat.com>
Functional tests are runningAuthorization passed for this commit. See the Functional Tests workflow for results. |
PR Summary by QodoRetry transient empty PR file lists before blocking approval
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Protected scripts need human review
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
The empty-PR-files retry branch calls `sleep 10` before re-fetching. The integration tests prepend only MOCK_BIN to PATH and provided no `sleep` mock, so the real sleep ran in all three empty-list cases (retry-recovers, retry-still-fails, and the pre-existing safety-net-independent test), adding ~30s to every serial suite run. Add a no-op `sleep` mock to MOCK_BIN. The retry logic doesn't depend on real elapsed time, so stubbing it keeps the tests deterministic and fast without changing what they assert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only findings from an automated PR sweep.
Treat non-zero forge file-list fetches like empty results so the retry and fail-closed guard run for GitHub and GitLab. Add regression coverage for both adapters and describe the async-diff explanation as a hypothesis rather than an API guarantee. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 8:37 PM UTC · Completed 8:54 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.43 |
|
Risk Assessment: moderate (2/5) DetailsTargeted edge-case bug fix in high-churn, multi-author core review scripts (5 protected paths, heavy regression history on post-review.sh) offset by narrow, well-tested scope with no security, dependency, or CI concerns and strong issue-to-implementation alignment. |
ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| # enabled — only the pattern-matching loop below is gated on a | ||
| # non-empty REVIEW_ACTIVE_PROTECTED_PATHS. | ||
| PR_FILES=$(forge_get_pr_files) | ||
| PR_FILES=$(forge_get_pr_files || true) |
There was a problem hiding this comment.
[low] fail-open
The || true appended to forge_get_pr_files suppresses non-zero exit codes from gh api --paginate. If the API fails mid-pagination, filenames from earlier pages are captured in PR_FILES. Because PR_FILES is non-empty, the empty-string guard does not trigger, and a partial file list is used for protected-path enforcement. A protected file on a later, un-fetched page would not be matched. The practical likelihood is low (requires 30+ changed files, a mid-pagination API error, and the protected file on a later page), but the previous code was unconditionally safe. The same pattern appears in bundled post-review.sh at line 651.
Suggested fix: Capture both stdout and the exit code from forge_get_pr_files separately. If the exit code is non-zero, treat the result as unreliable regardless of whether stdout was non-empty.
| # asynchronous diff computation, but GitHub does not document that as | ||
| # an API contract. The files endpoint reflects the computed diff more | ||
| # directly. | ||
| GH_TOKEN="${REVIEW_TOKEN}" gh api \ |
There was a problem hiding this comment.
[low] error-handling-idiom
forge_get_pr_files does not suppress errors internally; || true appears at every call site instead. All other query functions in this file (forge_get_pr_state, forge_get_pr_author, forge_get_pr_info) handle error suppression inside their function bodies, keeping call sites clean.
Suggested fix: Add error suppression inside the function body so call sites can use the plain PR_FILES=$(forge_get_pr_files) form.
3 participants
What
Fixes the review agent failing genuinely non-empty PRs when the forge returns an empty changed-files list.
post-review.shrefuses to approve a PR when it cannot establish what changed (a safety net against blind approvals). But the file list can come back empty transiently: right after a merge-commit update, GitHub has not finished computing the diff, soforge_get_pr_filesreturns nothing and the agent aborts on a PR that actually has changes.Changes
scripts/lib/github-review-ops.lib.sh—forge_get_pr_filesnow reads the paginated/pulls/{n}/filesREST endpoint instead of the asynchronously-populatedgh pr view --json filessummary field. The files endpoint reflects the computed diff more directly.scripts/post-review.src.sh— the empty-result guard now retries once (::notice::+ shortsleep+ re-fetch) before refusing to approve. The retry recovers from the transient race; a genuinely empty result still refuses to approve. This lives at the call site, so it protects every forge (GitHub + GitLab) regardless of whichforge_get_pr_filesimplementation runs.scripts/post-review-test.sh— mockghupdated for the new endpoint; addedempty-pr-files-retry-recoversandempty-pr-files-retry-still-failsintegration tests.post-review.shandpre-review.shbundles (make script-build).Verification
make script-build+make check-bundle— bundles in syncbash scripts/post-review-test.sh— 106 pass, incl. both new testsmake lint(skillsaw) — Grade A, 0 errors/warningspre-commit runon changed files — shellcheck + secret scan passThe remaining
harness-jira-test.shfailures undermake script-testare pre-existing and env-dependent (unsetJIRA_*vars); they reproduce on a cleanmainand are unrelated to this change.Note: the tracking issue lives in
fullsend-ai/fullsend#2093, but the bug and fix are in this repo (the review scripts).🤖 Generated with Claude Code