Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughWalkthroughAdds crash-resume and persistent loop state to PR review tooling, hardens git/gh invocation and error handling, introduces interruptible sleep and backoff, expands rules/docs for local→global discovery and push-before-trigger, updates skill/workflow paths and adds tests and a .gitignore entry. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as Workflow
participant CLI as pr_skill CLI
participant Git as Local Git
participant GH as GitHub API
participant Rules as Rules/Standards
rect rgba(200,230,255,0.5)
Workflow->>CLI: invoke `status` / `trigger_review` / `resume` (PATH_TO_SKILLS)
end
CLI->>Git: verify repo, pull/rebase (Freshness)
Git-->>CLI: status / errors
CLI->>Rules: consult `pr-standards` (deployment, rate-limits, prohibitions)
CLI->>GH: query/create reviews via GitHub API
GH-->>CLI: responses / rate-limit
loop poll for main reviewer (with backoff & heartbeat)
CLI->>GH: poll reviewers/status
GH-->>CLI: reviewer assigned OR rate-limit/no-assignment
CLI->>CLI: save loop state (LOOP_STATE_FILENAME)
CLI->>CLI: interruptible sleep (heartbeat)
end
alt reviewer assigned
CLI-->>Workflow: success payload (checked_at_utc, reviewer, next_step)
else rate-limited or timeout
CLI-->>Workflow: stop with RATE_LIMIT_INSTRUCTION / next steps
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello @sheepdestroyer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the operational guidelines and associated tooling for autonomous agents involved in the PR review process. The primary goal is to enforce more robust and predictable agent behaviors by clarifying rules around review cycles, code freshness, rate limit management, and critical prohibitions. These updates ensure agents operate more autonomously and effectively within the defined review workflow, while also providing clearer instructions through tool outputs. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 4d4a0f3 in 27 seconds. Click for details.
- Reviewed
224lines of code in5files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_Bxun4R9u8LnP9m5k
You can customize 
|
/gemini review |
|
@coderabbitai review |
|
@sourcery-ai review |
|
Here's the code health analysis summary for commits Analysis Summary
|
There was a problem hiding this comment.
Skipped PR review on 4df283c because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
|
/review |
|
@ellipsis review this |
|
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters. Please try again later or upgrade to continue using Sourcery |
✅ Actions performedReview triggered.
|
PR Reviewer Guide 🔍(Review updated until commit 02b64ee)Here are some key observations to aid the review process:
|
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
|
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters. Please try again later or upgrade to continue using Sourcery |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the PR review skill and its associated documentation to enforce stricter behaviors for autonomous agents. The changes introduce new rules for freshness, rate limits, and clarify prohibitions. The skill's Python code is updated to provide more detailed next_step instructions.
My review focuses on improving the maintainability of the Python code and correcting minor grammatical issues in the documentation. The main suggestion is to refactor the heavily duplicated instruction strings in pr_skill.py into constants to improve readability and ease of maintenance. I've also pointed out several typos and grammatical errors in the markdown files.
.agent/rules/pr-standards.md
Outdated
| * **No Idling**: Agents must actively monitor PR status. Do NOT exit/notify the user just to wait for a bot or a long process. | ||
| * **Polling Strategy**: Use GitHub MCP tools (`mcp_github_pull_request_read`) for reliable, non-blocking status polling. Wait ~3 minutes after triggering before first check, then poll every 2 minutes. | ||
| * **Autonomous Action**: Agents are AUTHORIZED and REQUIRED to `git commit` and `git push` fixes autonomously if tests pass. Do not ask the user to push for you. | ||
| * **Freshness**: Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may since have pushed formatting fixes to your previous changes. |
There was a problem hiding this comment.
The phrasing "may since have pushed" is slightly awkward. For better clarity, consider rephrasing to "may have since pushed".
| * **Freshness**: Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may since have pushed formatting fixes to your previous changes. | |
| * **Freshness**: Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may have since pushed formatting fixes to your previous changes. |
.agent/rules/pr-standards.md
Outdated
| **ABSOLUTE RULE**: Agents must **NEVER** merge a PR, close a PR, or delete a PR's branch. | ||
| * **Reason**: Merging, closing, and branch deletion are privileged actions reserved for human maintainers or specific CI/CD pipelines. | ||
| * **Action**: If you believe a PR is ready to be merged or should be closed, you must notify the user and await instructions. **Do NOT** execute commands to merge, close, or delete the branch. | ||
| * **Action**: If you believe a PR is ready to be merged or should be closed, you must notify the user and await instructions. **Do NOT** execute commands to merge, close, or delete the branch. Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review. |
There was a problem hiding this comment.
There is a typo in this line. "an branch" should be "a branch".
| * **Action**: If you believe a PR is ready to be merged or should be closed, you must notify the user and await instructions. **Do NOT** execute commands to merge, close, or delete the branch. Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review. | |
| * **Action**: If you believe a PR is ready to be merged or should be closed, you must notify the user and await instructions. **Do NOT** execute commands to merge, close, or delete the branch. Never merge or delete a branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review. |
.agent/skills/pr_review/SKILL.md
Outdated
|
|
||
| ### Mandatory Behavior Rules (Enforced by Tool Output) | ||
| 1. **Autonomy**: "Be autonomous, don't stop the cycle. you must regularly poll for the main-reviewer's review until it eventually gets posted, then continue" | ||
| 2. **Freshness**: "Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may since have pushed formatting fixes to your previous changes" |
There was a problem hiding this comment.
The phrasing "may since have pushed" is a bit awkward. For better readability, I suggest changing it to "may have since pushed".
| 2. **Freshness**: "Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may since have pushed formatting fixes to your previous changes" | |
| 2. **Freshness**: "Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may have since pushed formatting fixes to your previous changes" |
.agent/skills/pr_review/SKILL.md
Outdated
| ### Mandatory Behavior Rules (Enforced by Tool Output) | ||
| 1. **Autonomy**: "Be autonomous, don't stop the cycle. you must regularly poll for the main-reviewer's review until it eventually gets posted, then continue" | ||
| 2. **Freshness**: "Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may since have pushed formatting fixes to your previous changes" | ||
| 3. **Completeness**: "Be sure to address every comments and code reviews from all reviewers, ensure CI passes" |
There was a problem hiding this comment.
There's a grammatical error here. "every comments" should be either "every comment" or "all comments". I'd suggest "all comments" for consistency.
| 3. **Completeness**: "Be sure to address every comments and code reviews from all reviewers, ensure CI passes" | |
| 3. **Completeness**: "Be sure to address all comments and code reviews from all reviewers, ensure CI passes" |
.agent/skills/pr_review/SKILL.md
Outdated
| 3. **Completeness**: "Be sure to address every comments and code reviews from all reviewers, ensure CI passes" | ||
| 4. **Quality**: "Be sure to run and fix all available tests and Linting before pushing your next changes" | ||
| 5. **Rate Limits**: "If main reviewer says it just became rate-limited, address remaining code reviews then stop there" | ||
| 6. **Prohibitions**: "Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" |
There was a problem hiding this comment.
There is a typo in this line. "an branch" should be "a branch".
| 6. **Prohibitions**: "Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" | |
| 6. **Prohibitions**: "Never merge or delete a branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" |
| poll_interval=None, | ||
| ): | ||
| """ | ||
| Polls until the main reviewer has provided feedback since the given timestamp. |
There was a problem hiding this comment.
This string has a trailing space, which is likely unintentional. It's best to remove it for code cleanliness.
| Polls until the main reviewer has provided feedback since the given timestamp. | |
| "next_step": status_data.get("next_step", "Run 'status' to check for feedback. Be autonomous, don't stop the cycle. you must regularly poll for the main-reviewer's review until it eventually gets posted, then continue") |
There was a problem hiding this comment.
Code Review
This pull request updates the PR review skill and related documentation to enforce stricter autonomous agent behaviors. The changes introduce new rules for freshness, rate limits, and prohibitions in the documentation. The Python skill script is updated to reflect these new rules in its next_step messages and has been auto-formatted for better readability. My review focuses on improving the maintainability of the new instructional strings and correcting some minor typos in the documentation and code. All original comments were kept as they did not contradict any provided rules.
.agent/rules/pr-standards.md
Outdated
| **ABSOLUTE RULE**: Agents must **NEVER** merge a PR, close a PR, or delete a PR's branch. | ||
| * **Reason**: Merging, closing, and branch deletion are privileged actions reserved for human maintainers or specific CI/CD pipelines. | ||
| * **Action**: If you believe a PR is ready to be merged or should be closed, you must notify the user and await instructions. **Do NOT** execute commands to merge, close, or delete the branch. | ||
| * **Action**: If you believe a PR is ready to be merged or should be closed, you must notify the user and await instructions. **Do NOT** execute commands to merge, close, or delete the branch. Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review. |
There was a problem hiding this comment.
There's a small grammatical error here. 'an branch' should be 'a branch'.
| * **Action**: If you believe a PR is ready to be merged or should be closed, you must notify the user and await instructions. **Do NOT** execute commands to merge, close, or delete the branch. Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review. | |
| * **Action**: If you believe a PR is ready to be merged or should be closed, you must notify the user and await instructions. **Do NOT** execute commands to merge, close, or delete the branch. Never merge or delete a branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review. |
.agent/skills/pr_review/SKILL.md
Outdated
| 3. **Completeness**: "Be sure to address every comments and code reviews from all reviewers, ensure CI passes" | ||
| 4. **Quality**: "Be sure to run and fix all available tests and Linting before pushing your next changes" | ||
| 5. **Rate Limits**: "If main reviewer says it just became rate-limited, address remaining code reviews then stop there" | ||
| 6. **Prohibitions**: "Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" |
There was a problem hiding this comment.
There's a small grammatical error here. 'an branch' should be 'a branch'.
| 6. **Prohibitions**: "Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" | |
| 6. **Prohibitions**: "Never merge or delete a branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" |
.agent/skills/pr_review/pr_skill.py
Outdated
| "next_step": status_data.get("next_step", "Run 'status' to check for feedback.") | ||
| "next_step": status_data.get( | ||
| "next_step", | ||
| "Run 'status' to check for feedback. Be autonomous, don't stop the cycle. you must regularly poll for the main-reviewer's review until it eventually gets posted, then continue ", |
There was a problem hiding this comment.
The default next_step message appears to have a typo. It ends with a space and a comma ( ,) inside the string, which is likely unintentional. This should probably be a period or removed entirely for clarity.
| "Run 'status' to check for feedback. Be autonomous, don't stop the cycle. you must regularly poll for the main-reviewer's review until it eventually gets posted, then continue ", | |
| "Run 'status' to check for feedback. Be autonomous, don't stop the cycle. you must regularly poll for the main-reviewer's review until it eventually gets posted, then continue.", |
.agent/skills/pr_review/pr_skill.py
Outdated
| if has_changes_requested: | ||
| next_step = "CRITICAL: Changes requested by reviewer. ANALYZE feedback -> FIX code -> SAFE_PUSH. DO NOT STOP." | ||
| next_step = "CRITICAL: Changes requested by reviewer. ANALYZE feedback -> FIX code -> SAFE_PUSH. DO NOT STOP. Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may since have pushed formatting fixes to your previous changes. Be sure to address every comments and code reviews from all reviewers, ensure CI passes. Be sure to run and fix all available tests and Linting before pushing your next changes. If main reviewer says it just became rate-limited, address remaining code reviews then stop there." | ||
| elif has_new_main_reviewer_comments: | ||
| next_step = f"New comments from {validation_reviewer} after approval. ANALYZE feedback -> FIX code -> SAFE_PUSH." | ||
| next_step = f"New comments from {validation_reviewer} after approval. ANALYZE feedback -> FIX code -> SAFE_PUSH. Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may since have pushed formatting fixes to your previous changes. Be sure to address every comments and code reviews from all reviewers, ensure CI passes. Be sure to run and fix all available tests and Linting before pushing your next changes. If main reviewer says it just became rate-limited, address remaining code reviews then stop there." | ||
| elif main_reviewer_state == "APPROVED": | ||
| # Check if there's any OTHER feedback besides the main reviewer's approval | ||
| other_feedback = [ | ||
| item for item in new_feedback | ||
| if not (item.get("user") == validation_reviewer | ||
| and item.get("type") == "review_summary" | ||
| and item.get("state") == "APPROVED") | ||
| ] | ||
| if other_feedback: | ||
| next_step = "New feedback received. ANALYZE items -> FIX issues -> SAFE_PUSH. DO NOT STOP." | ||
| else: | ||
| next_step = "Validation Complete (STOP LOOP - DO NOT MERGE AUTONOMOUSLY). Notify User." | ||
| # Check if there's any OTHER feedback besides the main reviewer's approval | ||
| other_feedback = [ | ||
| item | ||
| for item in new_feedback | ||
| if not ( | ||
| item.get("user") == validation_reviewer | ||
| and item.get("type") == "review_summary" | ||
| and item.get("state") == "APPROVED" | ||
| ) | ||
| ] | ||
| if other_feedback: | ||
| next_step = "New feedback received. ANALYZE items -> FIX issues -> SAFE_PUSH. DO NOT STOP. Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may since have pushed formatting fixes to your previous changes. Be sure to address every comments and code reviews from all reviewers, ensure CI passes. Be sure to run and fix all available tests and Linting before pushing your next changes." | ||
| else: | ||
| next_step = "Validation Complete (STOP LOOP - DO NOT MERGE AUTONOMOUSLY). Notify User. Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" | ||
| elif new_feedback: | ||
| next_step = "New feedback received. ANALYZE items -> FIX issues -> SAFE_PUSH. DO NOT STOP." | ||
| next_step = "New feedback received. ANALYZE items -> FIX issues -> SAFE_PUSH. DO NOT STOP. Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may since have pushed formatting fixes to your previous changes. Be sure to address every comments and code reviews from all reviewers, ensure CI passes. Be sure to run and fix all available tests and Linting before pushing your next changes." | ||
| else: | ||
| next_step = f"Waiting for approval from {validation_reviewer} (Current: {main_reviewer_state}). Poll again." | ||
| next_step = f"Waiting for approval from {validation_reviewer} (Current: {main_reviewer_state}). Poll again. Be autonomous, don't stop the cycle. you must regularly poll for the main-reviewer's review until it eventually gets posted, then continue" |
There was a problem hiding this comment.
The long instructional strings are duplicated across several next_step assignments (e.g., lines 733, 735, 748, 752). This duplication makes the code harder to read and maintain. If these instructions need to be updated, the change has to be made in multiple places, which is error-prone.
Consider extracting the common instructional text into a constant and building the final next_step strings dynamically. For example:
ACTION_INSTRUCTIONS = (" Pull and merge latest changes..."
" ensure CI passes..."
" run and fix all tests...")
# ...
if has_changes_requested:
next_step = f"CRITICAL: Changes requested...{ACTION_INSTRUCTIONS}"
elif new_feedback:
next_step = f"New feedback received...{ACTION_INSTRUCTIONS}"
# ...
.agent/skills/pr_review/pr_skill.py
Outdated
| if other_feedback: | ||
| next_step = "New feedback received. ANALYZE items -> FIX issues -> SAFE_PUSH. DO NOT STOP. Pull and merge latest changes from the remote branch before starting addressing code reviews, as bots may since have pushed formatting fixes to your previous changes. Be sure to address every comments and code reviews from all reviewers, ensure CI passes. Be sure to run and fix all available tests and Linting before pushing your next changes." | ||
| else: | ||
| next_step = "Validation Complete (STOP LOOP - DO NOT MERGE AUTONOMOUSLY). Notify User. Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" |
There was a problem hiding this comment.
There's a small grammatical error in this instructional string. 'an branch' should be 'a branch'.
| next_step = "Validation Complete (STOP LOOP - DO NOT MERGE AUTONOMOUSLY). Notify User. Never merge or delete an branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" | |
| next_step = "Validation Complete (STOP LOOP - DO NOT MERGE AUTONOMOUSLY). Notify User. Never merge or delete a branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed bb67257 in 24 seconds. Click for details.
- Reviewed
67lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_0FDxWrfi4TF8FKGH
You can customize
|
/gemini review |
|
@coderabbitai review |
|
@sourcery-ai review |
|
/review |
|
@ellipsis review this |
|
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters. Please try again later or upgrade to continue using Sourcery |
|
@coderabbitai review |
|
@sourcery-ai review |
|
/review |
|
@ellipsis review this |
✅ Actions performedReview triggered.
|
|
Persistent review updated to latest commit bbcf651 |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed bbcf651 in 38 seconds. Click for details.
- Reviewed
85lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_NpHCEyCoekRtL2Ss
You can customize
Reviewer's GuideStrengthens the PR review skill to be a non‑blocking, crash‑resilient loop controller with stricter autonomous‑agent guidance, and updates rules/workflow docs to enforce autonomy, freshness, rate‑limit handling, and prohibitions against autonomous merges/branch operations. Sequence diagram for PR review loop with crash recovery and resume commandsequenceDiagram
actor Agent
participant PRSkill as PRSkill_ReviewManager
participant GitHubAPI
participant FileSystem
Agent->>PRSkill: safe_push()
PRSkill->>PRSkill: _verify_clean_git()
PRSkill->>GitHubAPI: git_status_and_push_via_run_git_cmd
GitHubAPI-->>PRSkill: push_result
PRSkill-->>Agent: {status: success, next_step}
Agent->>PRSkill: trigger_review(pr_number)
PRSkill->>GitHubAPI: trigger_review_bots
GitHubAPI-->>PRSkill: bots_triggered
PRSkill-->>Agent: {status: triggered, next_step: "WAIT then run status"}
loop Polling_loop
Agent->>PRSkill: status(pr_number, since_iso)
PRSkill->>GitHubAPI: fetch_issue_comments
PRSkill->>GitHubAPI: fetch_review_comments
PRSkill->>GitHubAPI: fetch_reviews
PRSkill->>PRSkill: _analyze_main_reviewer()
PRSkill->>PRSkill: _determine_next_step()
PRSkill-->>Agent: {status: success, items, main_reviewer, next_step}
alt main_reviewer_not_finished
Agent->>Agent: WAIT per next_step then call status again
end
end
%% Crash or interruption during internal polling helper
Agent->>PRSkill: internal_poll_via_poll_for_main_reviewer
PRSkill->>FileSystem: _save_loop_state(loop_state.json)
PRSkill->>GitHubAPI: check_status_during_poll
GitHubAPI-->>PRSkill: status_data
alt crash_or_interrupt
PRSkill->>FileSystem: loop_state.json remains on disk
PRSkill-->>Agent: {status: interrupted, next_step: "Run 'resume'"}
end
Agent->>PRSkill: resume()
PRSkill->>FileSystem: _load_loop_state(loop_state.json)
FileSystem-->>PRSkill: saved_state
PRSkill->>PRSkill: compute_remaining_attempts()
PRSkill->>PRSkill: _poll_for_main_reviewer(start_attempt)
PRSkill->>FileSystem: _save_loop_state(updated_attempt)
PRSkill->>GitHubAPI: check_status()
GitHubAPI-->>PRSkill: status_data
alt main_reviewer_completed
PRSkill->>FileSystem: _clear_loop_state()
end
PRSkill-->>Agent: {status: resumed, initial_status, next_step}
Agent->>Agent: Follow next_step (ANALYZE -> FIX -> SAFE_PUSH, never merge/delete)
Updated class diagram for ReviewManager and PR review skill helpersclassDiagram
class ReviewManager {
- token: str
- repo
- workspace: str
- loop_state_file: str
+ __init__()
+ _ensure_workspace() void
+ _detect_repo() void
+ _verify_clean_git() tuple[bool, str]
+ _check_local_state() tuple[bool, str]
+ safe_push() dict
+ trigger_review(pr_number: int, wait_seconds: int, validation_reviewer: str) dict
+ check_status(pr_number: int, since_iso: str, return_data: bool, validation_reviewer: str) dict
+ _poll_for_main_reviewer(pr_number: int, since_iso: str, validation_reviewer: str, max_attempts: int, poll_interval: int, start_attempt: int) dict
+ resume_loop() dict
+ _save_loop_state(pr_number: int, since_iso: str, validation_reviewer: str, poll_attempt: int) void
+ _load_loop_state() dict
+ _clear_loop_state() void
+ _interruptible_sleep(total_seconds: int, heartbeat_interval: int) void
+ _get_since_dt(since_iso: str) datetime
+ _analyze_main_reviewer(reviews, validation_reviewer: str) tuple[str, datetime]
+ _check_new_main_reviewer_comments(new_feedback: list, validation_reviewer: str, last_approval_dt: datetime) bool
+ _determine_next_step(new_feedback: list, validation_reviewer: str, main_reviewer_state: str, has_changes_requested: bool, has_new_main_comments: bool) str
+ _fetch_issue_comments(pr_number: int, since_dt: datetime) list
+ _fetch_review_comments(pr, since_dt: datetime) list
+ _fetch_reviews(pr, since_dt: datetime) tuple[list, list]
+ _log(message: str) void
+ _run_git_cmd(args: list, timeout: int, check: bool)
+ _run_gh_cmd(args: list, timeout: int)
}
class PRSkillModuleGlobals {
<<module>>
+ GIT_PATH: str
+ GH_PATH: str
+ POLL_INTERVAL_SECONDS: int
+ POLL_MAX_ATTEMPTS: int
+ LOOP_STATE_FILENAME: str
+ ACTION_INSTRUCTIONS: str
+ RATE_LIMIT_INSTRUCTION: str
+ DEFAULT_VALIDATION_REVIEWER: str
+ print_json(data)
+ print_error(message: str)
}
PRSkillModuleGlobals --> ReviewManager : configures
ReviewManager --> PRSkillModuleGlobals : uses_constants
ReviewManager --> PRSkillModuleGlobals : uses_ACTION_INSTRUCTIONS
ReviewManager --> PRSkillModuleGlobals : uses_RATE_LIMIT_INSTRUCTION
Flow diagram for rules and skill deployment locations and tool usageflowchart LR
subgraph Locations
A[Project_root]
B[Local_agent_dir<br/>.agent/]
C[Global_rules_dir<br/>~/.gemini/rules]
D[Global_skills_dir<br/>~/.gemini/antigravity/skills]
end
subgraph Rules_and_docs
R0[rules_pr_standards]
R1[rules_rules]
W1[workflow_pr_review_cycle]
K1[knowledge_base]
S1[skill_doc_SKILL]
end
subgraph Skill_binary
P1[pr_skill_py]
G1[Git_binary_resolved]
H1[gh_binary_resolved]
end
A --> B
B --> R0
B --> R1
B --> W1
B --> K1
B --> S1
C --> R0
C --> R1
D --> P1
S1 -->|Path_examples| P1
W1 -->|Uses_tool| P1
K1 -->|References| R0
R0 -->|Defines| Rloop[Loop_rule]
R0 -->|Defines| Rfresh[Freshness]
R0 -->|Defines| Rrate[Rate_limits]
R0 -->|Defines| Rprohib[Prohibitions]
R1 -->|Enforces| R0
P1 -->|Implements| Rloop
P1 -->|Implements| Rfresh
P1 -->|Implements| Rrate
P1 -->|Implements| Rprohib
P1 --> G1
P1 --> H1
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 security issues, 1 other issue, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
trigger_review, thewait_secondsparameter is now effectively ignored and thenext_stepstring hardcodesWAIT 180 seconds; consider either wiring the message to the actualwait_secondsvalue or deprecating the argument to avoid confusing callers. - There are duplicate
self._log(f"State verified: {msg}")calls intrigger_review; you can drop one to avoid redundant audit logging noise. - Resolving
GIT_PATHandGH_PATHat import time and exiting ifgitis missing can make the module unusable in contexts that only need non-git functionality (e.g., help/metadata); consider deferring these checks to first use or tomain()to keep import side effects minimal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `trigger_review`, the `wait_seconds` parameter is now effectively ignored and the `next_step` string hardcodes `WAIT 180 seconds`; consider either wiring the message to the actual `wait_seconds` value or deprecating the argument to avoid confusing callers.
- There are duplicate `self._log(f"State verified: {msg}")` calls in `trigger_review`; you can drop one to avoid redundant audit logging noise.
- Resolving `GIT_PATH` and `GH_PATH` at import time and exiting if `git` is missing can make the module unusable in contexts that only need non-git functionality (e.g., help/metadata); consider deferring these checks to first use or to `main()` to keep import side effects minimal.
## Individual Comments
### Comment 1
<location> `tests/test_pr_skill.py:2-7` </location>
<code_context>
+
+import unittest
+
+class TestPlaceholder(unittest.TestCase):
+ def test_ci_pass(self):
+ """Dummy test to ensure pytest collects at least one item and exits with 0."""
+ self.assertTrue(True)
</code_context>
<issue_to_address>
**issue (testing):** Replace placeholder test with focused tests that cover the new PR loop, crash recovery, and next_step behavior.
The current dummy test only satisfies CI and doesn’t validate any of the new behavior in `pr_skill.py`. Given the complexity of the loop logic and `next_step` messaging, this file should include targeted tests, e.g.:
- **Loop state & resume**: Unit-test `_save_loop_state`, `_load_loop_state`, `_clear_loop_state` using a temp workspace, and `resume_loop()` paths for: no state, state with remaining attempts (correct `start_attempt`/`max_attempts`), and exhausted attempts (expected `resumed` response and guidance).
- **Polling & interruption**: Test `_poll_for_main_reviewer` by mocking `check_status` and `_interruptible_sleep` to avoid real waits. Cover approval/changes-requested paths (loop exits and `_clear_loop_state` called), timeout (correct `polling_timeout` structure and `next_step`), and `KeyboardInterrupt` (sets `polling_interrupted` and “resume” guidance).
- **`check_status` & `next_step`**: With a fake/mocked repo/PR, assert detection of `CHANGES_REQUESTED`, approved-with-vs-without-extra-feedback, comments after approval, and the no-new-feedback case, each yielding the correct `next_step` text.
- **Git/GH helpers**: Test `_run_git_cmd` / `_run_gh_cmd` by mocking `subprocess.run` and `GIT_PATH`/`GH_PATH`, including the error path when `GH_PATH` is missing.
These will ensure the crash-resilient loop behavior and nuanced autonomy-related `next_step` messages are covered by tests, rather than relying on a placeholder.
</issue_to_address>
### Comment 2
<location> `.agent/skills/pr_review/pr_skill.py:142-148` </location>
<code_context>
return subprocess.run(
[GIT_PATH] + args,
capture_output=True,
text=True,
check=check,
timeout=timeout,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 3
<location> `.agent/skills/pr_review/pr_skill.py:155-161` </location>
<code_context>
return subprocess.run(
[GH_PATH] + args,
capture_output=True,
text=True,
check=True,
timeout=timeout,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tests/test_pr_skill.py
Outdated
| import unittest | ||
|
|
||
| class TestPlaceholder(unittest.TestCase): | ||
| def test_ci_pass(self): | ||
| """Dummy test to ensure pytest collects at least one item and exits with 0.""" | ||
| self.assertTrue(True) |
There was a problem hiding this comment.
issue (testing): Replace placeholder test with focused tests that cover the new PR loop, crash recovery, and next_step behavior.
The current dummy test only satisfies CI and doesn’t validate any of the new behavior in pr_skill.py. Given the complexity of the loop logic and next_step messaging, this file should include targeted tests, e.g.:
-
Loop state & resume: Unit-test
_save_loop_state,_load_loop_state,_clear_loop_stateusing a temp workspace, andresume_loop()paths for: no state, state with remaining attempts (correctstart_attempt/max_attempts), and exhausted attempts (expectedresumedresponse and guidance). -
Polling & interruption: Test
_poll_for_main_reviewerby mockingcheck_statusand_interruptible_sleepto avoid real waits. Cover approval/changes-requested paths (loop exits and_clear_loop_statecalled), timeout (correctpolling_timeoutstructure andnext_step), andKeyboardInterrupt(setspolling_interruptedand “resume” guidance). -
check_status&next_step: With a fake/mocked repo/PR, assert detection ofCHANGES_REQUESTED, approved-with-vs-without-extra-feedback, comments after approval, and the no-new-feedback case, each yielding the correctnext_steptext. -
Git/GH helpers: Test
_run_git_cmd/_run_gh_cmdby mockingsubprocess.runandGIT_PATH/GH_PATH, including the error path whenGH_PATHis missing.
These will ensure the crash-resilient loop behavior and nuanced autonomy-related next_step messages are covered by tests, rather than relying on a placeholder.
| return subprocess.run( | ||
| [GIT_PATH] + args, | ||
| capture_output=True, | ||
| text=True, | ||
| check=check, | ||
| timeout=timeout, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| return subprocess.run( | ||
| [GH_PATH] + args, | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| timeout=timeout, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the PR review skill by making it non-blocking, adding crash-resilience, and enforcing stricter autonomous agent behaviors through updated documentation and more detailed instructions. The refactoring of the check_status method is a notable improvement in code structure and readability. I've provided a few specific comments on minor issues related to configuration consistency, logging clarity, and code duplication. Overall, these changes are a great step forward for the agent's capabilities.
| try: | ||
| POLL_INTERVAL_SECONDS = max( | ||
| int(os.environ.get("PR_REVIEW_POLL_INTERVAL", "30")), 1) | ||
| except ValueError: | ||
| POLL_INTERVAL_SECONDS = 30 | ||
| POLL_INTERVAL_SECONDS = 120 | ||
|
|
||
| try: | ||
| POLL_MAX_ATTEMPTS = max( | ||
| int(os.environ.get("PR_REVIEW_POLL_MAX_ATTEMPTS", "20")), 1) | ||
| int(os.environ.get("PR_REVIEW_POLL_MAX_ATTEMPTS", "12")), 1) | ||
| except ValueError: | ||
| POLL_MAX_ATTEMPTS = 20 |
There was a problem hiding this comment.
The default values for polling configuration read from environment variables are inconsistent with the fallback values in the except blocks.
PR_REVIEW_POLL_INTERVAL: The default fromos.environ.getis '30', but the fallback is120.PR_REVIEW_POLL_MAX_ATTEMPTS: The default is '12', but the fallback is20.
To ensure predictable default behavior, the default values in os.environ.get should match their corresponding fallback values.
| try: | |
| POLL_INTERVAL_SECONDS = max( | |
| int(os.environ.get("PR_REVIEW_POLL_INTERVAL", "30")), 1) | |
| except ValueError: | |
| POLL_INTERVAL_SECONDS = 30 | |
| POLL_INTERVAL_SECONDS = 120 | |
| try: | |
| POLL_MAX_ATTEMPTS = max( | |
| int(os.environ.get("PR_REVIEW_POLL_MAX_ATTEMPTS", "20")), 1) | |
| int(os.environ.get("PR_REVIEW_POLL_MAX_ATTEMPTS", "12")), 1) | |
| except ValueError: | |
| POLL_MAX_ATTEMPTS = 20 | |
| try: | |
| POLL_INTERVAL_SECONDS = max( | |
| int(os.environ.get("PR_REVIEW_POLL_INTERVAL", "120")), 1) | |
| except ValueError: | |
| POLL_INTERVAL_SECONDS = 120 | |
| try: | |
| POLL_MAX_ATTEMPTS = max( | |
| int(os.environ.get("PR_REVIEW_POLL_MAX_ATTEMPTS", "20")), 1) | |
| except ValueError: | |
| POLL_MAX_ATTEMPTS = 20 |
| self._log( | ||
| f"Poll attempt {attempt} (resume+{i + 1}) / max {POLL_MAX_ATTEMPTS}..." | ||
| ) |
There was a problem hiding this comment.
The log message format is a bit confusing, especially the (resume+{i + 1}) part. A simpler format showing the current attempt out of the total maximum attempts would be clearer and more maintainable.
self._log(f"Poll attempt {attempt}/{POLL_MAX_ATTEMPTS}...")
.agent/skills/pr_review/pr_skill.py
Outdated
|
|
||
| # Capture start time for status check | ||
| start_time = datetime.now(timezone.utc) | ||
| self._log(f"State verified: {msg}") |
There was a problem hiding this comment.
This log message on line 613 is a duplicate of the one on line 611. This appears to be a copy-paste error from the refactoring and should be removed for code clarity.
| if created_at_val.endswith("Z"): | ||
| created_at_val = created_at_val[:-1] + "+00:00" | ||
| dt_val = datetime.fromisoformat(created_at_val) |
There was a problem hiding this comment.
This date parsing logic for handling the 'Z' suffix is also present in _get_since_dt. To improve maintainability, consider extracting it into a shared helper function.
Additionally, using replace('Z', '+00:00') is more robust and was used in the previous version of the code. The current endswith('Z') check is a slight regression.
dt_val = datetime.fromisoformat(
created_at_val.replace("Z", "+00:00")
)
|
/gemini review |
|
@coderabbitai review |
|
@sourcery-ai review |
|
/review |
|
@ellipsis review this |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 02b64ee in 17 seconds. Click for details.
- Reviewed
156lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_ixzJhbwqH93JSU4y
You can customize
|
Persistent review updated to latest commit 02b64ee |
Reviewer's GuideStrengthens the PR review skill’s autonomy loop, safety, and crash‑recovery behavior, and aligns standards/workflow/docs with new mandatory rules for autonomy, freshness, completeness, rate limits, and prohibitions on autonomous merges/branch operations. Class diagram for updated ReviewManager and helper behaviorclassDiagram
class ReviewManager {
- token : str
- repo : object
- workspace : str
- loop_state_file : str
+ __init__()
+ _mask_token(text : str) str
+ _ensure_workspace() void
+ _detect_repo() void
+ _verify_clean_git() tuple
+ _check_local_state() tuple
+ safe_push() dict
+ _poll_for_main_reviewer(pr_number : int, since_iso : str, validation_reviewer : str, max_attempts : int, poll_interval : int, start_attempt : int) dict
+ resume_loop() dict
+ trigger_review(pr_number : int, wait_seconds : int, validation_reviewer : str) dict
+ check_status(pr_number : int, since_iso : str, return_data : bool, validation_reviewer : str) dict
+ _save_loop_state(pr_number : int, since_iso : str, validation_reviewer : str, poll_attempt : int) void
+ _load_loop_state() dict
+ _clear_loop_state() void
+ _interruptible_sleep(total_seconds : int, heartbeat_interval : int) void
+ _get_since_dt(since_iso : str) datetime
+ _analyze_main_reviewer(reviews : list, validation_reviewer : str) tuple
+ _check_new_main_reviewer_comments(new_feedback : list, validation_reviewer : str, last_approval_dt : datetime) bool
+ _determine_next_step(new_feedback : list, validation_reviewer : str, main_reviewer_state : str, has_changes_requested : bool, has_new_main_comments : bool) str
+ _fetch_issue_comments(pr_number : int, since_dt : datetime) list
+ _fetch_review_comments(pr : object, since_dt : datetime) list
+ _fetch_reviews(pr : object, since_dt : datetime) tuple
+ _log(message : str) void
+ _run_git_cmd(args : list, timeout : int, check : bool) CompletedProcess
+ _run_gh_cmd(args : list, timeout : int) CompletedProcess
+ _get_aware_utc_datetime(dt_obj : datetime) datetime
}
class GitHubAPI {
<<external>>
+ get_pull(pr_number : int) object
+ get_issue(pr_number : int) object
+ get_reviews() list
+ get_review_comments() list
+ get_comments() list
}
class GitCLI {
<<external>>
+ rev_parse_show_toplevel()
+ status_porcelain()
+ fetch()
+ push()
+ rev_list_counts()
}
class GhCLI {
<<external>>
+ auth_token()
+ repo_view()
}
class FileSystem {
<<external>>
+ write_loop_state()
+ read_loop_state()
+ delete_loop_state()
}
ReviewManager --> GitHubAPI : uses
ReviewManager --> GitCLI : uses via _run_git_cmd
ReviewManager --> GhCLI : uses via _run_gh_cmd
ReviewManager --> FileSystem : uses for loop_state.json
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 security issues, 3 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The new
tests/test_pr_skill.pyhas a couple of issues that will likely cause it to fail to run: the callmgr._save_loop_state(8, "polling")doesn’t match the method signature (needspr_number, since_iso, validation_reviewer, poll_attempt), and the innerwith patch("os.open")...block is mis-indented, which will raise anIndentationError. - In
knowledge-base.mdyou now referencerules/pr-standards.mdas the path while elsewhere (e.g.,pr-standards.md,rules.md,pr-review-cycle.md) you normalize on.agent/...vs~/.gemini/...; consider making this reference consistent with the deployment-location rules you just introduced. - In
SKILL.mdthe Loop Resilience section still describes the skill as "non-blocking and stateless" even though loop state is now persisted inloop_state.json; it would be clearer to adjust this wording to reflect the new persistence behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `tests/test_pr_skill.py` has a couple of issues that will likely cause it to fail to run: the call `mgr._save_loop_state(8, "polling")` doesn’t match the method signature (needs `pr_number, since_iso, validation_reviewer, poll_attempt`), and the inner `with patch("os.open")...` block is mis-indented, which will raise an `IndentationError`.
- In `knowledge-base.md` you now reference `rules/pr-standards.md` as the path while elsewhere (e.g., `pr-standards.md`, `rules.md`, `pr-review-cycle.md`) you normalize on `.agent/...` vs `~/.gemini/...`; consider making this reference consistent with the deployment-location rules you just introduced.
- In `SKILL.md` the Loop Resilience section still describes the skill as "non-blocking and stateless" even though loop state is now persisted in `loop_state.json`; it would be clearer to adjust this wording to reflect the new persistence behavior.
## Individual Comments
### Comment 1
<location> `.agent/skills/pr_review/pr_skill.py:318` </location>
<code_context>
- )
+ # Fetch latest state from remote for accurate comparison
+ # Suppress stdout to avoid polluting structured output; inherit stderr so prompts/hangs remain visible
+ self._run_git_cmd(["fetch"], timeout=GIT_FETCH_TIMEOUT)
# Get current branch
</code_context>
<issue_to_address>
**issue (bug_risk):** Using _run_git_cmd for `git fetch` now captures stderr instead of inheriting it, which changes interactive behavior.
Previously `git fetch` inherited stderr, so prompts and errors were visible, whereas `_run_git_cmd` uses `capture_output=True`, suppressing both stdout and stderr. This can hide auth prompts or failures and make hangs hard to debug, which conflicts with the intent to keep stderr visible. Consider adding a way for `_run_git_cmd` to control `capture_output`/`stderr`, or special-casing `git fetch` to preserve stderr inheritance.
</issue_to_address>
### Comment 2
<location> `tests/test_pr_skill.py:53-62` </location>
<code_context>
+ )
+ self.assertIn("CRITICAL: Changes requested", step)
+
+ def test_save_loop_state(self):
+ """Test state saving logic."""
+ mgr = ReviewManager(self.mock_repo)
+ with patch("builtins.open", mock_open()) as mock_file:
+ # We also need to mock os.open for the secure write
+ with patch("os.open") as mock_os_open, \
+ patch("os.fdopen") as mock_fdopen:
+ mgr._save_loop_state(8, "polling")
+ # Verification is tricky due to secure write implementation details,
+ # but we ensure it doesn't crash.
+ pass
+
+ def test_trigger_review_next_step(self):
</code_context>
<issue_to_address>
**issue (testing):** Fix indentation and argument mismatch in `test_save_loop_state`, and add an assertion so the test meaningfully validates behavior.
This test has three issues that prevent it from being useful:
1. **Indentation**: Remove the extra leading space before `with patch("os.open")` so it aligns under the `with patch("builtins.open", ...)` block and avoids an `IndentationError`.
2. **Argument mismatch**: `_save_loop_state` expects `(self, pr_number, since_iso, validation_reviewer, poll_attempt)`, but the test calls `mgr._save_loop_state(8, "polling")`. Update the call to pass all required arguments (e.g. `mgr._save_loop_state(8, "2024-01-01T00:00:00Z", DEFAULT_VALIDATION_REVIEWER, 1)`) so the secure write path is actually exercised.
3. **Missing assertion**: Replace the `pass` with at least one concrete assertion, such as verifying the mocks were called (`mock_os_open.assert_called_once()` or checking a mocked `json.dump` call), so the test checks behavior rather than just running code.
</issue_to_address>
### Comment 3
<location> `tests/test_pr_skill.py:28-37` </location>
<code_context>
+ def test_determine_next_step_waiting(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for additional `next_step` decision paths (approval with feedback, comments after approval, rate-limit instructions).
Current `_determine_next_step` tests only cover waiting for feedback and a generic `CHANGES_REQUESTED` case. Given the branching and critical `ACTION_INSTRUCTIONS` / `RATE_LIMIT_INSTRUCTION` text, please add coverage for at least:
- Main reviewer approved, no other feedback: assert the `next_step` text includes the explicit STOP / “DO NOT MERGE AUTONOMOUSLY” and no-merge/no-delete instructions.
- Main reviewer approved with additional feedback from others: ensure `next_step` emphasizes addressing remaining feedback rather than stopping.
- New comments from main reviewer after approval: assert the “New comments from {validation_reviewer} after approval” wording and that the full `ACTION_INSTRUCTIONS` + `RATE_LIMIT_INSTRUCTION` content remains.
- Changes requested: beyond the existing substring check, verify the rate-limit instruction appears when applicable.
This will better lock in the expected autonomous-agent behavior and protect these instructions during future refactors.
Suggested implementation:
```python
class TestReviewManager(unittest.TestCase):
def setUp(self):
self.mock_repo = MagicMock()
self.mock_gh = MagicMock()
self.mock_gh.get_repo.return_value = self.mock_repo
# Mock global vars GIT_PATH/GH_PATH in pr_skill module if needed,
# but since we mock subprocss it might be fine.
def test_determine_next_step_waiting(self):
"""Test next_step when waiting for feedback."""
mgr = ReviewManager(self.mock_repo)
step = mgr._determine_next_step(
new_feedback=[],
validation_reviewer="gemini-bot",
main_reviewer_state="PENDING",
has_changes_requested=False,
has_new_main_comments=False
)
self.assertIn("WAIT 30 seconds", step)
def test_determine_next_step_main_approved_no_other_feedback(self):
"""Main reviewer approved, no other feedback: enforce STOP / do-not-merge instructions."""
mgr = ReviewManager(self.mock_repo)
step = mgr._determine_next_step(
new_feedback=[],
validation_reviewer="gemini-bot",
main_reviewer_state="APPROVED",
has_changes_requested=False,
has_new_main_comments=False,
)
# Explicit STOP / do-not-merge autonomy and no-merge/no-delete guard rails.
self.assertIn("DO NOT MERGE AUTONOMOUSLY", step)
self.assertIn("do not merge this PR yourself", step.lower())
self.assertIn("do not delete the branch", step.lower())
def test_determine_next_step_main_approved_with_additional_feedback(self):
"""
Main reviewer approved but there is additional feedback from others:
next_step should emphasize addressing remaining feedback rather than stopping.
"""
mgr = ReviewManager(self.mock_repo)
step = mgr._determine_next_step(
new_feedback=["Please update the docs before merging."],
validation_reviewer="gemini-bot",
main_reviewer_state="APPROVED",
has_changes_requested=False,
has_new_main_comments=False,
)
# Should prioritize addressing remaining feedback.
self.assertIn("feedback", step.lower())
self.assertIn("address", step.lower())
# When there is still feedback, we should not be in the hard STOP / autonomous-merge guard rail flow.
self.assertNotIn("DO NOT MERGE AUTONOMOUSLY", step)
def test_determine_next_step_new_comments_from_main_after_approval(self):
"""
New comments from the main reviewer after approval:
ensure specific wording and that full action / rate-limit instructions are preserved.
"""
mgr = ReviewManager(self.mock_repo)
step = mgr._determine_next_step(
new_feedback=["Follow-up comment from main reviewer."],
validation_reviewer="gemini-bot",
main_reviewer_state="APPROVED",
has_changes_requested=False,
has_new_main_comments=True,
)
# Wording should call out new comments from the validation reviewer after approval.
self.assertIn("New comments from gemini-bot after approval", step)
# Full ACTION_INSTRUCTIONS and RATE_LIMIT_INSTRUCTION must still be present.
self.assertIn(ACTION_INSTRUCTIONS.strip(), step)
self.assertIn(RATE_LIMIT_INSTRUCTION.strip(), step)
def test_determine_next_step_changes_requested_includes_rate_limit(self):
"""
When changes are requested, ensure the rate-limit instruction is present
in addition to the generic CHANGES_REQUESTED messaging.
"""
mgr = ReviewManager(self.mock_repo)
step = mgr._determine_next_step(
new_feedback=["Please refactor this function."],
validation_reviewer="gemini-bot",
main_reviewer_state="CHANGES_REQUESTED",
has_changes_requested=True,
has_new_main_comments=False,
)
# Existing CHANGES_REQUESTED behavior plus explicit rate-limit instructions.
self.assertIn("CHANGES_REQUESTED", step)
self.assertIn(RATE_LIMIT_INSTRUCTION.strip(), step)
```
To fully wire these tests in, you may also need to:
1. Ensure `ACTION_INSTRUCTIONS` and `RATE_LIMIT_INSTRUCTION` are imported into `tests/test_pr_skill.py`, for example:
- `from pr_skill import ReviewManager, ACTION_INSTRUCTIONS, RATE_LIMIT_INSTRUCTION`
If `ReviewManager` is already imported elsewhere in this file, extend that import rather than duplicating it.
2. Confirm the exact substrings used in `_determine_next_step`:
- If the "DO NOT MERGE AUTONOMOUSLY", "do not merge this PR yourself", or "do not delete the branch" texts differ slightly in the implementation, adjust the asserted substrings to match the real messages while preserving the intent (explicit STOP and no-merge/no-delete instructions).
- Likewise, if the implementation’s wording for “New comments from {validation_reviewer} after approval” varies, update the expected string accordingly (but keep the test verifying that this reviewer-specific text is present).
3. If `_determine_next_step` normalizes whitespace (e.g., multi-line constants), the `.strip()` around `ACTION_INSTRUCTIONS` / `RATE_LIMIT_INSTRUCTION` might need to be updated (e.g., by asserting on a distinctive phrase from each constant instead of the full string).
</issue_to_address>
### Comment 4
<location> `.agent/skills/pr_review/pr_skill.py:137-143` </location>
<code_context>
return subprocess.run(
[GIT_PATH] + args,
capture_output=True,
text=True,
check=check,
timeout=timeout,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 5
<location> `.agent/skills/pr_review/pr_skill.py:150-156` </location>
<code_context>
return subprocess.run(
[GH_PATH] + args,
capture_output=True,
text=True,
check=True,
timeout=timeout,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ) | ||
| # Fetch latest state from remote for accurate comparison | ||
| # Suppress stdout to avoid polluting structured output; inherit stderr so prompts/hangs remain visible | ||
| self._run_git_cmd(["fetch"], timeout=GIT_FETCH_TIMEOUT) |
There was a problem hiding this comment.
issue (bug_risk): Using _run_git_cmd for git fetch now captures stderr instead of inheriting it, which changes interactive behavior.
Previously git fetch inherited stderr, so prompts and errors were visible, whereas _run_git_cmd uses capture_output=True, suppressing both stdout and stderr. This can hide auth prompts or failures and make hangs hard to debug, which conflicts with the intent to keep stderr visible. Consider adding a way for _run_git_cmd to control capture_output/stderr, or special-casing git fetch to preserve stderr inheritance.
| def test_save_loop_state(self): | ||
| """Test state saving logic.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| with patch("builtins.open", mock_open()) as mock_file: | ||
| # We also need to mock os.open for the secure write | ||
| with patch("os.open") as mock_os_open, \ | ||
| patch("os.fdopen") as mock_fdopen: | ||
| mgr._save_loop_state(8, "polling") | ||
| # Verification is tricky due to secure write implementation details, | ||
| # but we ensure it doesn't crash. |
There was a problem hiding this comment.
issue (testing): Fix indentation and argument mismatch in test_save_loop_state, and add an assertion so the test meaningfully validates behavior.
This test has three issues that prevent it from being useful:
-
Indentation: Remove the extra leading space before
with patch("os.open")so it aligns under thewith patch("builtins.open", ...)block and avoids anIndentationError. -
Argument mismatch:
_save_loop_stateexpects(self, pr_number, since_iso, validation_reviewer, poll_attempt), but the test callsmgr._save_loop_state(8, "polling"). Update the call to pass all required arguments (e.g.mgr._save_loop_state(8, "2024-01-01T00:00:00Z", DEFAULT_VALIDATION_REVIEWER, 1)) so the secure write path is actually exercised. -
Missing assertion: Replace the
passwith at least one concrete assertion, such as verifying the mocks were called (mock_os_open.assert_called_once()or checking a mockedjson.dumpcall), so the test checks behavior rather than just running code.
| def test_determine_next_step_waiting(self): | ||
| """Test next_step when waiting for feedback.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| step = mgr._determine_next_step( | ||
| new_feedback=[], | ||
| validation_reviewer="gemini-bot", | ||
| main_reviewer_state="PENDING", | ||
| has_changes_requested=False, | ||
| has_new_main_comments=False | ||
| ) |
There was a problem hiding this comment.
suggestion (testing): Add tests for additional next_step decision paths (approval with feedback, comments after approval, rate-limit instructions).
Current _determine_next_step tests only cover waiting for feedback and a generic CHANGES_REQUESTED case. Given the branching and critical ACTION_INSTRUCTIONS / RATE_LIMIT_INSTRUCTION text, please add coverage for at least:
- Main reviewer approved, no other feedback: assert the
next_steptext includes the explicit STOP / “DO NOT MERGE AUTONOMOUSLY” and no-merge/no-delete instructions. - Main reviewer approved with additional feedback from others: ensure
next_stepemphasizes addressing remaining feedback rather than stopping. - New comments from main reviewer after approval: assert the “New comments from {validation_reviewer} after approval” wording and that the full
ACTION_INSTRUCTIONS+RATE_LIMIT_INSTRUCTIONcontent remains. - Changes requested: beyond the existing substring check, verify the rate-limit instruction appears when applicable.
This will better lock in the expected autonomous-agent behavior and protect these instructions during future refactors.
Suggested implementation:
class TestReviewManager(unittest.TestCase):
def setUp(self):
self.mock_repo = MagicMock()
self.mock_gh = MagicMock()
self.mock_gh.get_repo.return_value = self.mock_repo
# Mock global vars GIT_PATH/GH_PATH in pr_skill module if needed,
# but since we mock subprocss it might be fine.
def test_determine_next_step_waiting(self):
"""Test next_step when waiting for feedback."""
mgr = ReviewManager(self.mock_repo)
step = mgr._determine_next_step(
new_feedback=[],
validation_reviewer="gemini-bot",
main_reviewer_state="PENDING",
has_changes_requested=False,
has_new_main_comments=False
)
self.assertIn("WAIT 30 seconds", step)
def test_determine_next_step_main_approved_no_other_feedback(self):
"""Main reviewer approved, no other feedback: enforce STOP / do-not-merge instructions."""
mgr = ReviewManager(self.mock_repo)
step = mgr._determine_next_step(
new_feedback=[],
validation_reviewer="gemini-bot",
main_reviewer_state="APPROVED",
has_changes_requested=False,
has_new_main_comments=False,
)
# Explicit STOP / do-not-merge autonomy and no-merge/no-delete guard rails.
self.assertIn("DO NOT MERGE AUTONOMOUSLY", step)
self.assertIn("do not merge this PR yourself", step.lower())
self.assertIn("do not delete the branch", step.lower())
def test_determine_next_step_main_approved_with_additional_feedback(self):
"""
Main reviewer approved but there is additional feedback from others:
next_step should emphasize addressing remaining feedback rather than stopping.
"""
mgr = ReviewManager(self.mock_repo)
step = mgr._determine_next_step(
new_feedback=["Please update the docs before merging."],
validation_reviewer="gemini-bot",
main_reviewer_state="APPROVED",
has_changes_requested=False,
has_new_main_comments=False,
)
# Should prioritize addressing remaining feedback.
self.assertIn("feedback", step.lower())
self.assertIn("address", step.lower())
# When there is still feedback, we should not be in the hard STOP / autonomous-merge guard rail flow.
self.assertNotIn("DO NOT MERGE AUTONOMOUSLY", step)
def test_determine_next_step_new_comments_from_main_after_approval(self):
"""
New comments from the main reviewer after approval:
ensure specific wording and that full action / rate-limit instructions are preserved.
"""
mgr = ReviewManager(self.mock_repo)
step = mgr._determine_next_step(
new_feedback=["Follow-up comment from main reviewer."],
validation_reviewer="gemini-bot",
main_reviewer_state="APPROVED",
has_changes_requested=False,
has_new_main_comments=True,
)
# Wording should call out new comments from the validation reviewer after approval.
self.assertIn("New comments from gemini-bot after approval", step)
# Full ACTION_INSTRUCTIONS and RATE_LIMIT_INSTRUCTION must still be present.
self.assertIn(ACTION_INSTRUCTIONS.strip(), step)
self.assertIn(RATE_LIMIT_INSTRUCTION.strip(), step)
def test_determine_next_step_changes_requested_includes_rate_limit(self):
"""
When changes are requested, ensure the rate-limit instruction is present
in addition to the generic CHANGES_REQUESTED messaging.
"""
mgr = ReviewManager(self.mock_repo)
step = mgr._determine_next_step(
new_feedback=["Please refactor this function."],
validation_reviewer="gemini-bot",
main_reviewer_state="CHANGES_REQUESTED",
has_changes_requested=True,
has_new_main_comments=False,
)
# Existing CHANGES_REQUESTED behavior plus explicit rate-limit instructions.
self.assertIn("CHANGES_REQUESTED", step)
self.assertIn(RATE_LIMIT_INSTRUCTION.strip(), step)To fully wire these tests in, you may also need to:
-
Ensure
ACTION_INSTRUCTIONSandRATE_LIMIT_INSTRUCTIONare imported intotests/test_pr_skill.py, for example:from pr_skill import ReviewManager, ACTION_INSTRUCTIONS, RATE_LIMIT_INSTRUCTION
IfReviewManageris already imported elsewhere in this file, extend that import rather than duplicating it.
-
Confirm the exact substrings used in
_determine_next_step:- If the "DO NOT MERGE AUTONOMOUSLY", "do not merge this PR yourself", or "do not delete the branch" texts differ slightly in the implementation, adjust the asserted substrings to match the real messages while preserving the intent (explicit STOP and no-merge/no-delete instructions).
- Likewise, if the implementation’s wording for “New comments from {validation_reviewer} after approval” varies, update the expected string accordingly (but keep the test verifying that this reviewer-specific text is present).
-
If
_determine_next_stepnormalizes whitespace (e.g., multi-line constants), the.strip()aroundACTION_INSTRUCTIONS/RATE_LIMIT_INSTRUCTIONmight need to be updated (e.g., by asserting on a distinctive phrase from each constant instead of the full string).
| return subprocess.run( | ||
| [GIT_PATH] + args, | ||
| capture_output=True, | ||
| text=True, | ||
| check=check, | ||
| timeout=timeout, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| return subprocess.run( | ||
| [GH_PATH] + args, | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| timeout=timeout, | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agent/skills/pr_review/pr_skill.py (1)
437-505:⚠️ Potential issue | 🟠 MajorResume polling skips waits due to an off-by-context check.
After resume,
attemptcan exceedmax_attempts, soattempt < max_attemptsbecomes false early and the loop can hammer the API without sleeping.✅ Suggested fix
- if attempt < max_attempts: + if i < max_attempts - 1: self._log( f"Main reviewer has not responded yet. Waiting {poll_interval}s before next poll..." ) self._interruptible_sleep(poll_interval)
🤖 Fix all issues with AI agents
In @.agent/skills/pr_review/pr_skill.py:
- Around line 628-641: The returned next_step message is hardcoded to "WAIT 180
seconds" instead of using the actual wait_seconds value; update the return in
the method containing the self._log(...) and the dict response (the block that
returns status "triggered") to interpolate or format the wait_seconds variable
(used for the --wait argument) into next_step so it instructs "WAIT
{wait_seconds} seconds (for bots to run), then run 'status'..." and ensure the
variable referenced matches the name wait_seconds used earlier in this scope.
🧹 Nitpick comments (3)
.agent/skills/pr_review/pr_skill.py (3)
317-323: Remove the duplicated comment to reduce noise.The “Fetch latest state…” comment is repeated twice back-to-back.
♻️ Suggested edit
- # Fetch latest state from remote for accurate comparison - # Suppress stdout to avoid polluting structured output; inherit stderr so prompts/hangs remain visible - # Fetch latest state from remote for accurate comparison - # Suppress stdout to avoid polluting structured output; inherit stderr so prompts/hangs remain visible + # Fetch latest state from remote for accurate comparison + # Suppress stdout to avoid polluting structured output; inherit stderr so prompts/hangs remain visible
611-613: Remove the duplicated “State verified” log.This line is logged twice in a row, which adds noise.
♻️ Suggested edit
- self._log(f"State verified: {msg}") - - self._log(f"State verified: {msg}") + self._log(f"State verified: {msg}")
740-778: Use the configured poll interval in the WAIT instruction.The message currently hardcodes 30 seconds, which can diverge from the configured poll interval.
♻️ Suggested tweak
- return f"Waiting for approval from {validation_reviewer} (Current: {main_reviewer_state}). WAIT 30 seconds, then run 'status' to check again. Be autonomous, don't stop the cycle. You must regularly poll for the main-reviewer's review until it eventually gets posted, then continue." + return f"Waiting for approval from {validation_reviewer} (Current: {main_reviewer_state}). WAIT {POLL_INTERVAL_SECONDS} seconds, then run 'status' to check again. Be autonomous, don't stop the cycle. You must regularly poll for the main-reviewer's review until it eventually gets posted, then continue."
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)
.agent/skills/pr_review/pr_skill.py (1)
401-402:⚠️ Potential issue | 🟡 MinorAdd
next_stepto timeout error response for schema consistency.The
TimeoutExpiredhandler returns a dict withoutnext_step, while other error paths include it. This forces callers to handle missing keys.🧩 Suggested fix
except subprocess.TimeoutExpired: - return {"status": "error", "message": "Git upstream check timed out."} + return { + "status": "error", + "message": "Git upstream check timed out.", + "next_step": "Check git connectivity and retry safe_push.", + }
🤖 Fix all issues with AI agents
In `@tests/test_pr_skill.py`:
- Around line 37-48: The test calls ReviewManager(self.mock_repo) but
ReviewManager.__init__ takes no repo argument; remove the arg and either set up
required internals on the instance after construction or patch init helpers:
instantiate with mgr = ReviewManager(), then assign or mock attributes (e.g.
mgr.repo = self.mock_repo) or patch/patch.object the initialization helpers
(_detect_repo, _ensure_workspace, GitHub client creation) so tests don't hit
real initialization; update test_determine_next_step_waiting to construct
ReviewManager without args and configure needed attributes/mocks before calling
_determine_next_step.
- Around line 62-71: The test calls ReviewManager._save_loop_state with two args
but the method requires four; update the test to call _save_loop_state with the
full signature: pass pr_number (8), since_iso (a valid ISO string, e.g.
"2020-01-01T00:00:00Z"), validation_reviewer (None or an empty string as
appropriate for your code), and poll_attempt (an int like 1), so call
ReviewManager._save_loop_state(8, "2020-01-01T00:00:00Z", None, 1) in the test
to match the required parameters of ReviewManager._save_loop_state.
🧹 Nitpick comments (4)
.agent/skills/pr_review/pr_skill.py (2)
44-54: Align poll constant fallbacks with their defaults.The normal path uses
"30"/"12"as defaults, but theValueErrorfallback uses120/20. A malformed environment variable will silently switch to very different cadence values rather than using the same defaults.♻️ Suggested fix (single source of truth)
+DEFAULT_POLL_INTERVAL_SECONDS = 30 +DEFAULT_POLL_MAX_ATTEMPTS = 12 + # Polling constants for review feedback # Configurable via environment variables try: POLL_INTERVAL_SECONDS = max( - int(os.environ.get("PR_REVIEW_POLL_INTERVAL", "30")), 1) + int(os.environ.get("PR_REVIEW_POLL_INTERVAL", str(DEFAULT_POLL_INTERVAL_SECONDS))), 1) except ValueError: - POLL_INTERVAL_SECONDS = 120 + POLL_INTERVAL_SECONDS = DEFAULT_POLL_INTERVAL_SECONDS try: POLL_MAX_ATTEMPTS = max( - int(os.environ.get("PR_REVIEW_POLL_INTERVAL", "12")), 1) + int(os.environ.get("PR_REVIEW_POLL_MAX_ATTEMPTS", str(DEFAULT_POLL_MAX_ATTEMPTS))), 1) except ValueError: - POLL_MAX_ATTEMPTS = 20 + POLL_MAX_ATTEMPTS = DEFAULT_POLL_MAX_ATTEMPTS
973-976: Remove redundanttracebackimport.
tracebackis already imported at Line 17, so the import on Line 974 is redundant.🔧 Suggested fix
except Exception as e: # Catch-all for unhandled exceptions to prevent raw tracebacks in JSON output # Log full traceback to stderr for debugging sys.stderr.write(f"CRITICAL ERROR: {e!s}\n") - import traceback - traceback.print_exc(file=sys.stderr)tests/test_pr_skill.py (2)
8-25: Consider using proper package structure instead ofsys.pathmanipulation.The
sys.path.appendapproach is fragile and can break in different execution contexts. Consider:
- Adding an
__init__.pyto make it a proper package- Using
conftest.pywithpytestto handle path setup- Installing the package in editable mode (
pip install -e .)The CI failure (
ModuleNotFoundError: No module named 'github') originates frompr_skill.pyimportingPyGithubat module level, which isn't installed in CI.
65-71: Unused mock variables and missing assertions.
mock_file,mock_os_open, andmock_fdopenare assigned but never used (Ruff F841). The test only verifies "doesn't crash" which provides limited value. Consider adding assertions to verify the state file is written correctly.
| def test_determine_next_step_waiting(self): | ||
| """Test next_step when waiting for feedback.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| step = mgr._determine_next_step( | ||
| new_feedback=[], | ||
| validation_reviewer="gemini-bot", | ||
| main_reviewer_state="PENDING", | ||
| has_changes_requested=False, | ||
| has_new_main_comments=False, | ||
| ) | ||
| self.assertIn("WAIT 30 seconds", step) | ||
| self.assertIn("Run 'status'", step) |
There was a problem hiding this comment.
ReviewManager.__init__ doesn't accept a repo argument.
ReviewManager() takes no constructor arguments (see pr_skill.py Line 88). Passing self.mock_repo will raise TypeError. You need to mock the initialization internals or patch the class attributes post-construction.
🐛 Suggested approach
+ `@patch.object`(ReviewManager, '__init__', lambda self: None)
def test_determine_next_step_waiting(self):
"""Test next_step when waiting for feedback."""
- mgr = ReviewManager(self.mock_repo)
+ mgr = ReviewManager()
+ mgr.repo = self.mock_repo
+ mgr.token = "fake-token"
+ mgr.workspace = "/tmp/test"
+ mgr.loop_state_file = "/tmp/test/loop_state.json"
step = mgr._determine_next_step(Or use @patch decorators to mock _detect_repo, _ensure_workspace, and the GitHub client initialization.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_determine_next_step_waiting(self): | |
| """Test next_step when waiting for feedback.""" | |
| mgr = ReviewManager(self.mock_repo) | |
| step = mgr._determine_next_step( | |
| new_feedback=[], | |
| validation_reviewer="gemini-bot", | |
| main_reviewer_state="PENDING", | |
| has_changes_requested=False, | |
| has_new_main_comments=False, | |
| ) | |
| self.assertIn("WAIT 30 seconds", step) | |
| self.assertIn("Run 'status'", step) | |
| `@patch.object`(ReviewManager, '__init__', lambda self: None) | |
| def test_determine_next_step_waiting(self): | |
| """Test next_step when waiting for feedback.""" | |
| mgr = ReviewManager() | |
| mgr.repo = self.mock_repo | |
| mgr.token = "fake-token" | |
| mgr.workspace = "/tmp/test" | |
| mgr.loop_state_file = "/tmp/test/loop_state.json" | |
| step = mgr._determine_next_step( | |
| new_feedback=[], | |
| validation_reviewer="gemini-bot", | |
| main_reviewer_state="PENDING", | |
| has_changes_requested=False, | |
| has_new_main_comments=False, | |
| ) | |
| self.assertIn("WAIT 30 seconds", step) | |
| self.assertIn("Run 'status'", step) |
🤖 Prompt for AI Agents
In `@tests/test_pr_skill.py` around lines 37 - 48, The test calls
ReviewManager(self.mock_repo) but ReviewManager.__init__ takes no repo argument;
remove the arg and either set up required internals on the instance after
construction or patch init helpers: instantiate with mgr = ReviewManager(), then
assign or mock attributes (e.g. mgr.repo = self.mock_repo) or patch/patch.object
the initialization helpers (_detect_repo, _ensure_workspace, GitHub client
creation) so tests don't hit real initialization; update
test_determine_next_step_waiting to construct ReviewManager without args and
configure needed attributes/mocks before calling _determine_next_step.
| def test_save_loop_state(self): | ||
| """Test state saving logic.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| with patch("builtins.open", mock_open()) as mock_file: | ||
| # We also need to mock os.open for the secure write | ||
| with patch("os.open") as mock_os_open, patch("os.fdopen") as mock_fdopen: | ||
| mgr._save_loop_state(8, "polling") | ||
| # Verification is tricky due to secure write implementation details, | ||
| # but we ensure it doesn't crash. | ||
| pass |
There was a problem hiding this comment.
_save_loop_state signature mismatch.
Line 68 calls _save_loop_state(8, "polling") but the method signature requires 4 arguments: (pr_number, since_iso, validation_reviewer, poll_attempt).
🐛 Suggested fix
def test_save_loop_state(self):
"""Test state saving logic."""
- mgr = ReviewManager(self.mock_repo)
- with patch("builtins.open", mock_open()) as mock_file:
+ with patch.object(ReviewManager, '__init__', lambda self: None):
+ mgr = ReviewManager()
+ mgr.workspace = "/tmp/test"
+ mgr.loop_state_file = "/tmp/test/loop_state.json"
# We also need to mock os.open for the secure write
with patch("os.open") as mock_os_open, patch("os.fdopen") as mock_fdopen:
- mgr._save_loop_state(8, "polling")
- # Verification is tricky due to secure write implementation details,
- # but we ensure it doesn't crash.
- pass
+ mock_fdopen.return_value.__enter__ = MagicMock()
+ mock_fdopen.return_value.__exit__ = MagicMock()
+ mgr._save_loop_state(
+ pr_number=8,
+ since_iso="2026-01-30T00:00:00Z",
+ validation_reviewer="gemini-bot",
+ poll_attempt=1
+ )
+ mock_os_open.assert_called_once()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_save_loop_state(self): | |
| """Test state saving logic.""" | |
| mgr = ReviewManager(self.mock_repo) | |
| with patch("builtins.open", mock_open()) as mock_file: | |
| # We also need to mock os.open for the secure write | |
| with patch("os.open") as mock_os_open, patch("os.fdopen") as mock_fdopen: | |
| mgr._save_loop_state(8, "polling") | |
| # Verification is tricky due to secure write implementation details, | |
| # but we ensure it doesn't crash. | |
| pass | |
| def test_save_loop_state(self): | |
| """Test state saving logic.""" | |
| with patch.object(ReviewManager, '__init__', lambda self: None): | |
| mgr = ReviewManager() | |
| mgr.workspace = "/tmp/test" | |
| mgr.loop_state_file = "/tmp/test/loop_state.json" | |
| # We also need to mock os.open for the secure write | |
| with patch("os.open") as mock_os_open, patch("os.fdopen") as mock_fdopen: | |
| mock_fdopen.return_value.__enter__ = MagicMock() | |
| mock_fdopen.return_value.__exit__ = MagicMock() | |
| mgr._save_loop_state( | |
| pr_number=8, | |
| since_iso="2026-01-30T00:00:00Z", | |
| validation_reviewer="gemini-bot", | |
| poll_attempt=1 | |
| ) | |
| mock_os_open.assert_called_once() |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 65-65: Local variable mock_file is assigned to but never used
Remove assignment to unused variable mock_file
(F841)
[error] 67-67: Local variable mock_os_open is assigned to but never used
Remove assignment to unused variable mock_os_open
(F841)
[error] 67-67: Local variable mock_fdopen is assigned to but never used
Remove assignment to unused variable mock_fdopen
(F841)
🤖 Prompt for AI Agents
In `@tests/test_pr_skill.py` around lines 62 - 71, The test calls
ReviewManager._save_loop_state with two args but the method requires four;
update the test to call _save_loop_state with the full signature: pass pr_number
(8), since_iso (a valid ISO string, e.g. "2020-01-01T00:00:00Z"),
validation_reviewer (None or an empty string as appropriate for your code), and
poll_attempt (an int like 1), so call ReviewManager._save_loop_state(8,
"2020-01-01T00:00:00Z", None, 1) in the test to match the required parameters of
ReviewManager._save_loop_state.
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the PR review skill by introducing stricter autonomous agent behaviors, crash recovery mechanisms, and improved documentation. The pr_skill.py file has been refactored to be more modular and robust, with new helper methods for Git/GitHub command execution and state management. The trigger_review command is now non-blocking, providing clear next_step instructions for the agent to continue the review cycle. Documentation across various .md files has been updated to reflect these new rules and guidelines, including deployment locations, freshness requirements, and rate limit awareness. Overall, these changes contribute to a more resilient, autonomous, and maintainable PR review process.
| step = mgr._determine_next_step( | ||
| new_feedback=[], | ||
| validation_reviewer="gemini-bot", | ||
| main_reviewer_state="CHANGES_REQUESTED", | ||
| has_changes_requested=True, | ||
| has_new_main_comments=False, | ||
| ) | ||
| self.assertIn("CRITICAL: Changes requested", step) | ||
|
|
||
| def test_save_loop_state(self): | ||
| """Test state saving logic.""" |
There was a problem hiding this comment.
The comment "Verification is tricky due to secure write implementation details, but we ensure it doesn't crash" suggests that test_save_loop_state might not be fully asserting the correctness of the saved state. While the current implementation prevents crashes, it would be beneficial to add assertions to verify the content of the loop_state.json file after _save_loop_state is called, ensuring that the correct data is being persisted. This would make the test more robust and provide better confidence in the crash recovery mechanism.
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)
.agent/skills/pr_review/SKILL.md (1)
61-67:⚠️ Potential issue | 🟡 MinorTrigger_review polling description no longer matches code.
The tool is now non-blocking and returns anext_stepinstead of polling until response.✏️ Suggested edit
-* **Polling Behavior**: After the initial wait, the tool **polls until the main reviewer responds** (up to ~10 minutes). This enforces the Loop Rule - preventing premature exit before feedback is received. +* **Polling Behavior**: After the initial wait, the tool returns immediately with a `next_step` telling the agent to wait and poll via `status`. The script itself does not block or poll.
🤖 Fix all issues with AI agents
In @.agent/skills/pr_review/pr_skill.py:
- Around line 730-770: The waiting message in _determine_next_step currently
hard-codes "WAIT 30 seconds" — update it to use the configured poll interval
(POLL_INTERVAL_SECONDS or the constant used elsewhere) so the message reflects
actual behavior; locate the return clause in _determine_next_step that builds
the waiting string and replace the literal "30" with the poll interval variable
(formatted into the string), ensuring the message still instructs to wait and
then run 'status' while preserving existing text and any surrounding constants
like validation_reviewer and main_reviewer_state.
In `@tests/test_pr_skill.py`:
- Around line 37-85: Tests create ReviewManager with a repo arg and call
_save_loop_state with wrong arity; update tests to instantiate ReviewManager()
with no arguments (use mgr.repo = self.mock_repo to attach the mock repo if
needed), and call _save_loop_state with the four parameters its signature
requires (pass the pull number, state string, and the two additional args the
method expects) so the calls match ReviewManager.__init__ and
ReviewManager._save_loop_state; also ensure mocks for os.open/os.fdopen remain
in place around the secure write call in test_save_loop_state.
| @staticmethod | ||
| def _determine_next_step( | ||
| new_feedback, | ||
| validation_reviewer, | ||
| main_reviewer_state, | ||
| has_changes_requested, | ||
| has_new_main_comments, | ||
| ): | ||
| """Generate the next_step instruction string.""" | ||
| if has_changes_requested: | ||
| return f"CRITICAL: Changes requested by reviewer. {ACTION_INSTRUCTIONS}{RATE_LIMIT_INSTRUCTION}" | ||
| elif has_new_main_comments: | ||
| return f"New comments from {validation_reviewer} after approval. {ACTION_INSTRUCTIONS}{RATE_LIMIT_INSTRUCTION}" | ||
| elif main_reviewer_state == "APPROVED": | ||
| other_feedback = [ | ||
| item | ||
| for item in new_feedback | ||
| if not ( | ||
| item.get("user") == validation_reviewer | ||
| and item.get("type") == "review_summary" | ||
| and item.get("state") == "APPROVED" | ||
| ) | ||
| ] | ||
| if other_feedback: | ||
| return f"New feedback received. {ACTION_INSTRUCTIONS}" | ||
| else: | ||
| return "Validation Complete (STOP LOOP - DO NOT MERGE AUTONOMOUSLY). Notify User. Never merge or delete a branch on your own, if you believe the main reviewer said that the PR is ready, just stop and ask for Human review" | ||
| elif new_feedback: | ||
| # Count explicitly addressable comments | ||
| comment_count = sum( | ||
| 1 | ||
| for item in new_feedback | ||
| if item.get("type") in ["inline_comment", "issue_comment"] | ||
| ) | ||
| if comment_count > 0: | ||
| return f"{comment_count} new comments received. {ACTION_INSTRUCTIONS}" | ||
| else: | ||
| return f"New feedback received. {ACTION_INSTRUCTIONS}" | ||
| else: | ||
| return f"Waiting for approval from {validation_reviewer} (Current: {main_reviewer_state}). WAIT 30 seconds, then run 'status' to check again. Be autonomous, don't stop the cycle. You must regularly poll for the main-reviewer's review until it eventually gets posted, then continue." | ||
|
|
There was a problem hiding this comment.
Wait instruction should reflect configured poll interval.
Line 769 hard-codes “WAIT 30 seconds,” which can diverge from POLL_INTERVAL_SECONDS.
🛠️ Suggested fix
- return f"Waiting for approval from {validation_reviewer} (Current: {main_reviewer_state}). WAIT 30 seconds, then run 'status' to check again. Be autonomous, don't stop the cycle. You must regularly poll for the main-reviewer's review until it eventually gets posted, then continue."
+ return f"Waiting for approval from {validation_reviewer} (Current: {main_reviewer_state}). WAIT {POLL_INTERVAL_SECONDS} seconds, then run 'status' to check again. Be autonomous, don't stop the cycle. You must regularly poll for the main-reviewer's review until it eventually gets posted, then continue."🤖 Prompt for AI Agents
In @.agent/skills/pr_review/pr_skill.py around lines 730 - 770, The waiting
message in _determine_next_step currently hard-codes "WAIT 30 seconds" — update
it to use the configured poll interval (POLL_INTERVAL_SECONDS or the constant
used elsewhere) so the message reflects actual behavior; locate the return
clause in _determine_next_step that builds the waiting string and replace the
literal "30" with the poll interval variable (formatted into the string),
ensuring the message still instructs to wait and then run 'status' while
preserving existing text and any surrounding constants like validation_reviewer
and main_reviewer_state.
| def test_determine_next_step_waiting(self): | ||
| """Test next_step when waiting for feedback.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| step = mgr._determine_next_step( | ||
| new_feedback=[], | ||
| validation_reviewer="gemini-bot", | ||
| main_reviewer_state="PENDING", | ||
| has_changes_requested=False, | ||
| has_new_main_comments=False, | ||
| ) | ||
| self.assertIn("WAIT 30 seconds", step) | ||
| self.assertIn("Run 'status'", step) | ||
|
|
||
| def test_determine_next_step_changes_requested(self): | ||
| """Test next_step when changes are requested.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| step = mgr._determine_next_step( | ||
| new_feedback=[], | ||
| validation_reviewer="gemini-bot", | ||
| main_reviewer_state="CHANGES_REQUESTED", | ||
| has_changes_requested=True, | ||
| has_new_main_comments=False, | ||
| ) | ||
| self.assertIn("CRITICAL: Changes requested", step) | ||
|
|
||
| def test_save_loop_state(self): | ||
| """Test state saving logic.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| with patch("builtins.open", mock_open()) as mock_file: | ||
| # We also need to mock os.open for the secure write | ||
| with patch("os.open") as mock_os_open, patch("os.fdopen") as mock_fdopen: | ||
| mgr._save_loop_state(8, "polling") | ||
| # Verification is tricky due to secure write implementation details, | ||
| # but we ensure it doesn't crash. | ||
| pass | ||
|
|
||
| def test_trigger_review_next_step(self): | ||
| """Test trigger_review returns correct wait instruction.""" | ||
| # This requires mocking _check_local_state and repo.get_pull | ||
| mgr = ReviewManager(self.mock_repo) | ||
| mgr._check_local_state = MagicMock(return_value=(True, "Clean")) | ||
| mgr._log = MagicMock() | ||
|
|
||
| mock_pr = MagicMock() | ||
| self.mock_repo.get_pull.return_value = mock_pr | ||
|
|
||
| result = mgr.trigger_review(8, wait_seconds=42) | ||
| self.assertIn("WAIT 42 seconds", result["next_step"]) | ||
| self.assertIn("Run 'status'", result["next_step"]) |
There was a problem hiding this comment.
Tests instantiate ReviewManager incorrectly and pass wrong args to _save_loop_state.
ReviewManager.__init__ takes no repo argument, and _save_loop_state expects four parameters. These tests will raise TypeError before assertions.
✅ Suggested fix
- mgr = ReviewManager(self.mock_repo)
- step = mgr._determine_next_step(
+ step = ReviewManager._determine_next_step(
new_feedback=[],
validation_reviewer="gemini-bot",
main_reviewer_state="PENDING",
has_changes_requested=False,
has_new_main_comments=False,
)
@@
- mgr = ReviewManager(self.mock_repo)
- step = mgr._determine_next_step(
+ step = ReviewManager._determine_next_step(
new_feedback=[],
validation_reviewer="gemini-bot",
main_reviewer_state="CHANGES_REQUESTED",
has_changes_requested=True,
has_new_main_comments=False,
)
@@
- mgr = ReviewManager(self.mock_repo)
- with patch("builtins.open", mock_open()) as mock_file:
- # We also need to mock os.open for the secure write
- with patch("os.open") as mock_os_open, patch("os.fdopen") as mock_fdopen:
- mgr._save_loop_state(8, "polling")
- # Verification is tricky due to secure write implementation details,
- # but we ensure it doesn't crash.
- pass
+ mgr = ReviewManager.__new__(ReviewManager)
+ mgr.loop_state_file = "loop_state.json"
+ mgr._log = MagicMock()
+ with patch("os.open", return_value=3) as mock_os_open, \
+ patch("os.fdopen", mock_open()) as mock_fdopen:
+ mgr._save_loop_state(8, "2024-01-01T00:00:00Z", "gemini-bot", 1)
+ mock_os_open.assert_called()
@@
- mgr = ReviewManager(self.mock_repo)
+ mgr = ReviewManager.__new__(ReviewManager)
+ mgr.repo = self.mock_repo
mgr._check_local_state = MagicMock(return_value=(True, "Clean"))
mgr._log = MagicMock()🧰 Tools
🪛 Ruff (0.14.14)
[error] 65-65: Local variable mock_file is assigned to but never used
Remove assignment to unused variable mock_file
(F841)
[error] 67-67: Local variable mock_os_open is assigned to but never used
Remove assignment to unused variable mock_os_open
(F841)
[error] 67-67: Local variable mock_fdopen is assigned to but never used
Remove assignment to unused variable mock_fdopen
(F841)
🤖 Prompt for AI Agents
In `@tests/test_pr_skill.py` around lines 37 - 85, Tests create ReviewManager with
a repo arg and call _save_loop_state with wrong arity; update tests to
instantiate ReviewManager() with no arguments (use mgr.repo = self.mock_repo to
attach the mock repo if needed), and call _save_loop_state with the four
parameters its signature requires (pass the pull number, state string, and the
two additional args the method expects) so the calls match
ReviewManager.__init__ and ReviewManager._save_loop_state; also ensure mocks for
os.open/os.fdopen remain in place around the secure write call in
test_save_loop_state.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/test_pr_skill.py`:
- Around line 62-71: The test calls ReviewManager._save_loop_state with wrong
arity and leaves unused mocks; update the call to pass all required parameters
(pr_number, since_iso, validation_reviewer, poll_attempt) when invoking
_save_loop_state (referencing _save_loop_state) and remove the unused mock
variables (mock_file, mock_os_open, mock_fdopen) or actually use them to emulate
secure file write behavior; ensure the test only patches what it needs and that
the call matches the method signature so Ruff F841 unused-mock warnings are
eliminated.
- Around line 37-83: The tests construct ReviewManager which runs its real
__init__ (hitting env/gh/git/GitHub); change tests to avoid that by creating a
test-safe instance: patch ReviewManager.__init__ to a no-op (or use
ReviewManager.__new__ to allocate an object) and then manually set any
attributes the tests use (e.g., _determine_next_step, _save_loop_state,
_check_local_state, _log, mock_repo) so the methods under test can run without
performing real initialization; update test_determine_next_step*,
test_save_loop_state, and test_trigger_review to apply this patch or manual
instantiation before calling the methods.
- Around line 15-26: The test module imports pr_skill which requires the
external PyGithub package, but tests don't declare this dependency; add PyGithub
(e.g., PyGithub>=2.8.1) to the project's dev/test requirements so importing
pr_skill (and symbols DEFAULT_VALIDATION_REVIEWER and ReviewManager) succeeds in
CI; alternatively, if you cannot modify requirements, modify the test to
re-raise the ImportError to fail fast or wrap imports and apply unittest.skipIf
on specific test methods instead of raising unittest.SkipTest at module scope.
| try: | ||
| from pr_skill import DEFAULT_VALIDATION_REVIEWER, ReviewManager | ||
| except ImportError: | ||
| # Handle the case where the import path might be different in CI | ||
| sys.path.append( | ||
| os.path.abspath( | ||
| os.path.join(os.path.dirname(__file__), | ||
| "../../.agent/skills/pr_review") | ||
| ) | ||
| ) | ||
| from pr_skill import DEFAULT_VALIDATION_REVIEWER, ReviewManager | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for PyGithub in dependency declarations
rg -n "PyGithub|github" -g "requirements*.txt" -g "pyproject.toml" -g "setup.cfg" -g "setup.py" -A 2 -B 2Repository: sheepdestroyer/agent-tools
Length of output: 128
🏁 Script executed:
# Also check for any GitHub-related imports in the repo
git ls-files | head -20Repository: sheepdestroyer/agent-tools
Length of output: 410
🏁 Script executed:
# Get context around the test file
fd "test_pr_skill.py" --type fRepository: sheepdestroyer/agent-tools
Length of output: 92
🏁 Script executed:
# Read the test file to understand its structure
cat -n tests/test_pr_skill.pyRepository: sheepdestroyer/agent-tools
Length of output: 3919
🏁 Script executed:
# Check the pr_skill.py to understand what it imports
cat -n .agent/skills/pr_review/pr_skill.py | head -50Repository: sheepdestroyer/agent-tools
Length of output: 1552
PyGithub is not declared in test dependencies; add to dev/test requirements.
pr_skill imports github at module level (line 20 of pr_skill.py), which requires PyGithub. While it's declared in .agent/skills/pr_review/requirements.txt, it must also be added to the test environment's requirements so tests can import the module.
The proposed fix using unittest.SkipTest at module level is incorrect—this exception is for skipping individual test methods, not module-level imports. Instead, either:
- Add
PyGithub>=2.8.1to test/dev requirements (preferred) - Re-raise the
ImportErrorto fail fast and signal the missing dependency - Use a conditional import with
skipIfdecorator on affected test methods
🤖 Prompt for AI Agents
In `@tests/test_pr_skill.py` around lines 15 - 26, The test module imports
pr_skill which requires the external PyGithub package, but tests don't declare
this dependency; add PyGithub (e.g., PyGithub>=2.8.1) to the project's dev/test
requirements so importing pr_skill (and symbols DEFAULT_VALIDATION_REVIEWER and
ReviewManager) succeeds in CI; alternatively, if you cannot modify requirements,
modify the test to re-raise the ImportError to fail fast or wrap imports and
apply unittest.skipIf on specific test methods instead of raising
unittest.SkipTest at module scope.
| def test_determine_next_step_waiting(self): | ||
| """Test next_step when waiting for feedback.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| step = mgr._determine_next_step( | ||
| new_feedback=[], | ||
| validation_reviewer="gemini-bot", | ||
| main_reviewer_state="PENDING", | ||
| has_changes_requested=False, | ||
| has_new_main_comments=False, | ||
| ) | ||
| self.assertIn("WAIT 30 seconds", step) | ||
| self.assertIn("Run 'status'", step) | ||
|
|
||
| def test_determine_next_step_changes_requested(self): | ||
| """Test next_step when changes are requested.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| step = mgr._determine_next_step( | ||
| new_feedback=[], | ||
| validation_reviewer="gemini-bot", | ||
| main_reviewer_state="CHANGES_REQUESTED", | ||
| has_changes_requested=True, | ||
| has_new_main_comments=False, | ||
| ) | ||
| self.assertIn("CRITICAL: Changes requested", step) | ||
|
|
||
| def test_save_loop_state(self): | ||
| """Test state saving logic.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| with patch("builtins.open", mock_open()) as mock_file: | ||
| # We also need to mock os.open for the secure write | ||
| with patch("os.open") as mock_os_open, patch("os.fdopen") as mock_fdopen: | ||
| mgr._save_loop_state(8, "polling") | ||
| # Verification is tricky due to secure write implementation details, | ||
| # but we ensure it doesn't crash. | ||
| pass | ||
|
|
||
| def test_trigger_review_next_step(self): | ||
| """Test trigger_review returns correct wait instruction.""" | ||
| # This requires mocking _check_local_state and repo.get_pull | ||
| mgr = ReviewManager(self.mock_repo) | ||
| mgr._check_local_state = MagicMock(return_value=(True, "Clean")) | ||
| mgr._log = MagicMock() | ||
|
|
||
| mock_pr = MagicMock() | ||
| self.mock_repo.get_pull.return_value = mock_pr | ||
|
|
||
| result = mgr.trigger_review(8, wait_seconds=42) |
There was a problem hiding this comment.
ReviewManager construction is invalid and triggers real initialization.
ReviewManager takes no args and its __init__ hits env/gh/git/GitHub. These tests will error or reach out to external systems.
🔧 Suggested test-safe instantiation
- mgr = ReviewManager(self.mock_repo)
+ mgr = ReviewManager.__new__(ReviewManager)
+ mgr.repo = self.mock_repo
+ mgr._log = MagicMock()🧰 Tools
🪛 Ruff (0.14.14)
[error] 65-65: Local variable mock_file is assigned to but never used
Remove assignment to unused variable mock_file
(F841)
[error] 67-67: Local variable mock_os_open is assigned to but never used
Remove assignment to unused variable mock_os_open
(F841)
[error] 67-67: Local variable mock_fdopen is assigned to but never used
Remove assignment to unused variable mock_fdopen
(F841)
🤖 Prompt for AI Agents
In `@tests/test_pr_skill.py` around lines 37 - 83, The tests construct
ReviewManager which runs its real __init__ (hitting env/gh/git/GitHub); change
tests to avoid that by creating a test-safe instance: patch
ReviewManager.__init__ to a no-op (or use ReviewManager.__new__ to allocate an
object) and then manually set any attributes the tests use (e.g.,
_determine_next_step, _save_loop_state, _check_local_state, _log, mock_repo) so
the methods under test can run without performing real initialization; update
test_determine_next_step*, test_save_loop_state, and test_trigger_review to
apply this patch or manual instantiation before calling the methods.
| def test_save_loop_state(self): | ||
| """Test state saving logic.""" | ||
| mgr = ReviewManager(self.mock_repo) | ||
| with patch("builtins.open", mock_open()) as mock_file: | ||
| # We also need to mock os.open for the secure write | ||
| with patch("os.open") as mock_os_open, patch("os.fdopen") as mock_fdopen: | ||
| mgr._save_loop_state(8, "polling") | ||
| # Verification is tricky due to secure write implementation details, | ||
| # but we ensure it doesn't crash. | ||
| pass |
There was a problem hiding this comment.
_save_loop_state called with wrong arity + unused mocks.
The method expects (pr_number, since_iso, validation_reviewer, poll_attempt). Also, mock_file, mock_os_open, mock_fdopen are unused (Ruff F841).
🛠️ Proposed fix
- with patch("builtins.open", mock_open()) as mock_file:
- # We also need to mock os.open for the secure write
- with patch("os.open") as mock_os_open, patch("os.fdopen") as mock_fdopen:
- mgr._save_loop_state(8, "polling")
+ with patch("os.open"), patch("os.fdopen"):
+ mgr._save_loop_state(
+ pr_number=8,
+ since_iso="2026-01-01T00:00:00+00:00",
+ validation_reviewer=DEFAULT_VALIDATION_REVIEWER,
+ poll_attempt=1,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_save_loop_state(self): | |
| """Test state saving logic.""" | |
| mgr = ReviewManager(self.mock_repo) | |
| with patch("builtins.open", mock_open()) as mock_file: | |
| # We also need to mock os.open for the secure write | |
| with patch("os.open") as mock_os_open, patch("os.fdopen") as mock_fdopen: | |
| mgr._save_loop_state(8, "polling") | |
| # Verification is tricky due to secure write implementation details, | |
| # but we ensure it doesn't crash. | |
| pass | |
| def test_save_loop_state(self): | |
| """Test state saving logic.""" | |
| mgr = ReviewManager(self.mock_repo) | |
| with patch("os.open"), patch("os.fdopen"): | |
| mgr._save_loop_state( | |
| pr_number=8, | |
| since_iso="2026-01-01T00:00:00+00:00", | |
| validation_reviewer=DEFAULT_VALIDATION_REVIEWER, | |
| poll_attempt=1, | |
| ) |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 65-65: Local variable mock_file is assigned to but never used
Remove assignment to unused variable mock_file
(F841)
[error] 67-67: Local variable mock_os_open is assigned to but never used
Remove assignment to unused variable mock_os_open
(F841)
[error] 67-67: Local variable mock_fdopen is assigned to but never used
Remove assignment to unused variable mock_fdopen
(F841)
🤖 Prompt for AI Agents
In `@tests/test_pr_skill.py` around lines 62 - 71, The test calls
ReviewManager._save_loop_state with wrong arity and leaves unused mocks; update
the call to pass all required parameters (pr_number, since_iso,
validation_reviewer, poll_attempt) when invoking _save_loop_state (referencing
_save_loop_state) and remove the unused mock variables (mock_file, mock_os_open,
mock_fdopen) or actually use them to emulate secure file write behavior; ensure
the test only patches what it needs and that the call matches the method
signature so Ruff F841 unused-mock warnings are eliminated.
2 participants
User description
Updating PR Review Skill and documentation to enforced strict autonomous agent behaviors (The Loop, Freshness, Prohibitions).
PR Type
Enhancement, Documentation
Description
Enhanced PR review skill with stricter autonomous agent behavior enforcement
Added mandatory behavior rules for autonomy, freshness, completeness, and quality
Expanded next_step messages with detailed instructions for code review handling
Updated documentation paths and added new sections on freshness and rate limits
Clarified prohibitions against autonomous merging and branch deletion
Diagram Walkthrough
File Walkthrough
pr_skill.py
Enhanced next_step messages with stricter behavior enforcement.agent/skills/pr_review/pr_skill.py
trigger_reviewnext_step message to enforce autonomouspolling behavior
instructions for code review handling
running tests, and handling rate limits
ask for human review
pr-standards.md
Added freshness and rate limit rules with path corrections.agent/rules/pr-standards.md
.agent/skills/pr_review/pr_skill.pyto
skills/pr_review/pr_skill.pyresolution requirements
strategies
explicit instruction against autonomous branch operations
rules.md
Added compliance section enforcing PR standards.agent/rules/rules.md
standards and prohibitions
SKILL.md
Updated paths and added mandatory behavior rules documentation.agent/skills/pr_review/SKILL.md
.agent/rules/pr-standards.mdto
~/.gemini/rules/pr-standards.mdbehaviors
~/.gemini/antigravity/skills/pr_review/pr_skill.pyinstructions
pr-review-cycle.md
Updated workflow paths and added freshness requirements.agent/workflows/pr-review-cycle.md
~/.gemini/antigravity/skills/pr_review/pr_skill.pyformatImplement)
~/.gemini/rules/pr-standards.mdImportant
Enhances PR review skill with stricter autonomous agent behavior enforcement and updates documentation for clarity and compliance.
pr_skill.pywith enhancednext_stepmessages and mandatory behavior rules.pr-standards.mdwith new sections on freshness and rate limits, and corrects path references.SKILL.mdwith mandatory behavior rules and consistent path references.rules.mdto emphasize compliance with PR standards.test_pr_skill.pyto ensure CI test suite runs successfully.This description was created by
for 02b64ee. You can customize this summary. It will automatically update as commits are pushed.
Summary by Sourcery
Strengthen the PR review skill’s autonomous agent behavior, add crash-resilient polling, and align documentation and rules with new loop, freshness, and safety standards.
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores