Repository navigation
OSAC-2145: always post new review comment, review only changed files - #103
Conversation
When a PR has both prd.md and design.md, the design review was overwriting the PRD comment. Now each review only looks for its own marker when checking for existing comments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Always post a new comment instead of updating existing ones - For synchronize events, only review files changed in the push - Each review type uses its own marker independently Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@ItzikEzra-rh: This pull request references OSAC-2145 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughChanges modify GitHub Actions automation scripts: ChangesEP Review Automation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow as ep-review.yml
participant Script as ep_review.py
participant GH as GitHub API
Workflow->>Script: Run with EVENT_NAME, EVENT_ACTION, EVENT_BEFORE_SHA
alt EVENT_ACTION is synchronize
Script->>GH: compare(before_sha, head_sha)
GH-->>Script: incremental changed files
else other event action
Script->>GH: get PR files
GH-->>Script: full changed files list
end
Script->>Script: filter reviewable .md docs
Script->>GH: gh pr comment --body-file (post new review comment)
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eranco74, ItzikEzra-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/ep_hooks.py (1)
60-73: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winScope the pre-gate to the active skill marker.
.github/scripts/ep_hooks.py:60-73still treats any bot comment with the current head SHA as “already reviewed,” while both skills share the samereviewed_label. Sincerun_review()only stores_skill_nameonticketandcheck_pr_state()ignores it, the first review will block the other one as soon as the shared label is set. Filter by the running skill’s marker instead of matching both comment types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/ep_hooks.py around lines 60 - 73, The pre-gate in check_pr_state is too broad because it treats any bot comment with the current head SHA as already reviewed, causing one skill’s review to block the other under the shared reviewed_label. Update check_pr_state to use the active skill marker from ticket["_skill_name"] (set by run_review) when selecting existing comments, so it only matches the comment type for the currently running skill instead of both "AI EP Review:" and "AI Design Review:".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/ep_review.py:
- Around line 43-47: get_incremental_files() currently relies on the compare
API, which can truncate the file list at 300 entries and cause missed
review-triggering changes. Update get_incremental_files() to detect when the
compare result reaches that cap and then fall back to a full-diff source such as
get_changed_files(pr_number) so prd.md/design.md changes are not skipped; use
the existing compare call and the incremental file collection path as the place
to add this fallback.
- Around line 126-130: The synchronize path in get_incremental_files() can still
fail when gh() raises RuntimeError, so the review aborts instead of falling
back. Update the event_action == "synchronize" branch to catch compare failures
around get_incremental_files(before_sha, head_sha) and on error use
get_changed_files(pr_number) as the fallback, keeping the existing print/log
context for the incremental attempt.
---
Outside diff comments:
In @.github/scripts/ep_hooks.py:
- Around line 60-73: The pre-gate in check_pr_state is too broad because it
treats any bot comment with the current head SHA as already reviewed, causing
one skill’s review to block the other under the shared reviewed_label. Update
check_pr_state to use the active skill marker from ticket["_skill_name"] (set by
run_review) when selecting existing comments, so it only matches the comment
type for the currently running skill instead of both "AI EP Review:" and "AI
Design Review:".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6cbd249b-cfb2-4c0d-a9f0-1c03643fc34a
📒 Files selected for processing (3)
.github/scripts/ep_hooks.py.github/scripts/ep_review.py.github/workflows/ep-review.yml
| def get_incremental_files(before_sha, head_sha): | ||
| raw = gh(["api", f"repos/{REPO}/compare/{before_sha}...{head_sha}", | ||
| "--jq", "[.files[].filename]"]) | ||
| return json.loads(raw) if raw.strip() else [] | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the review script and nearby helpers.
git ls-files '.github/scripts/*' '.github/**' | sed -n '1,120p'
echo
echo "=== outline: .github/scripts/ep_review.py ==="
ast-grep outline .github/scripts/ep_review.py --view expanded || true
echo
echo "=== relevant lines in .github/scripts/ep_review.py ==="
cat -n .github/scripts/ep_review.py | sed -n '1,220p'
echo
echo "=== search for get_changed_files / detect_skills / compare usage ==="
rg -n "get_changed_files|get_incremental_files|detect_skills|compare/" .github/scripts .github -SRepository: osac-project/enhancement-proposals
Length of output: 9510
🌐 Web query:
GitHub REST API compare two commits files array capped 300 pagination compare endpoint files not paginated
💡 Result:
The GitHub REST API "Compare two commits" endpoint (/repos/{owner}/{repo}/compare/{basehead}) does not support pagination for the list of changed files [1][2]. While the endpoint itself supports pagination for the list of commits, the files array in the response is capped at a maximum of 300 files and is only returned on the first page of results [1][3][4]. Even when implementing pagination (using the page and per_page parameters to retrieve all commits), the API does not provide a mechanism to fetch additional pages of files beyond the initial 300 [5][2][4]. Consequently, if a comparison involves more than 300 changed files, the REST API cannot be used to retrieve the full list of files [3][2]. To obtain a complete list of changed files for large comparisons, common workarounds include: 1. Using the diff format by setting the Accept header to application/vnd.github.v3.diff [3]. This returns the full diff, from which you can parse the list of files [3]. 2. Fetching commits individually and aggregating the changed files for each commit [6]. Top Results: [1][3][2][4]
Citations:
- 1: https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28
- 2: [Schema Inaccuracy] Compare Endpoint pagination for 'files' list github/rest-api-description#3975
- 3: https://stackoverflow.com/questions/68355441/github-api-compare-2-commits-large-comparison-cant-get-all-changed-files
- 4: [Schema Inaccuracy] Commit compare endpoint limits the number of files returned github/rest-api-description#33
- 5: [Schema Inaccuracy] Comparison API doesn't list all files in first page, per docs github/rest-api-description#3552
- 6: https://stackoverflow.com/questions/72405908/how-can-i-get-a-complete-list-of-changed-files-between-two-commits-in-github
Handle the 300-file cap in compare diffs
get_incremental_files() uses the compare endpoint, which stops at 300 files and does not paginate that list. A large synchronize push can miss prd.md/design.md changes and skip a review; fall back to get_changed_files(pr_number) or another full-diff source once the compare result hits that cap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/ep_review.py around lines 43 - 47, get_incremental_files()
currently relies on the compare API, which can truncate the file list at 300
entries and cause missed review-triggering changes. Update
get_incremental_files() to detect when the compare result reaches that cap and
then fall back to a full-diff source such as get_changed_files(pr_number) so
prd.md/design.md changes are not skipped; use the existing compare call and the
incremental file collection path as the place to add this fallback.
| if event_action == "synchronize" and before_sha and head_sha: | ||
| files = get_incremental_files(before_sha, head_sha) | ||
| print(f"Synchronize: checking incremental diff ({before_sha[:8]}..{head_sha[:8]})") | ||
| else: | ||
| files = get_changed_files(pr_number) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== File map ==\n'
git ls-files .github/scripts/ep_review.py
printf '\n== Outline ==\n'
ast-grep outline .github/scripts/ep_review.py --view expanded || true
printf '\n== Relevant sections ==\n'
nl -ba .github/scripts/ep_review.py | sed -n '1,220p'
printf '\n== Symbols ==\n'
rg -n "def (gh|get_incremental_files|get_changed_files)|event_action|before_sha|head_sha" .github/scripts/ep_review.pyRepository: osac-project/enhancement-proposals
Length of output: 743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== lines 1-180 =='
sed -n '1,180p' .github/scripts/ep_review.py | cat -n
echo
echo '== get_incremental_files / main context =='
sed -n '20,150p' .github/scripts/ep_review.py | cat -nRepository: osac-project/enhancement-proposals
Length of output: 12486
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '20,150p' .github/scripts/ep_review.py | cat -nRepository: osac-project/enhancement-proposals
Length of output: 5431
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '20,150p' .github/scripts/ep_review.py | cat -nRepository: osac-project/enhancement-proposals
Length of output: 5431
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "except RuntimeError|subprocess|raise RuntimeError|get_incremental_files|get_changed_files|def gh" .github/scripts/ep_review.pyRepository: osac-project/enhancement-proposals
Length of output: 483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('.github/scripts/ep_review.py')
text = p.read_text()
for needle in ['def gh(', 'def get_changed_files(', 'def get_incremental_files(', 'def main(']:
idx = text.find(needle)
print(f'## {needle} at {idx}')
if idx != -1:
start = text.rfind('\n', 0, idx - 200)
end = text.find('\n', idx + 800)
snippet = text[max(0, idx-200):min(len(text), idx+1200)]
print(snippet)
print()
PYRepository: osac-project/enhancement-proposals
Length of output: 5890
Fallback to full PR file list on compare failure. get_incremental_files() can still raise here because gh() turns non-zero exits into RuntimeError in CI. A rewritten before_sha or all-zero SHA will abort the review instead of falling back to get_changed_files(pr_number).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/ep_review.py around lines 126 - 130, The synchronize path in
get_incremental_files() can still fail when gh() raises RuntimeError, so the
review aborts instead of falling back. Update the event_action == "synchronize"
branch to catch compare failures around get_incremental_files(before_sha,
head_sha) and on error use get_changed_files(pr_number) as the fallback, keeping
the existing print/log context for the incremental attempt.
3 participants
Summary
Test plan
Test evidence
ItzikEzra-rh#14
Generated with Claude Code
Summary by CodeRabbit