[feature/bots] Add CI failure bot for automated responses #524#562
[feature/bots] Add CI failure bot for automated responses #524#562Srinath0916 wants to merge 5 commits intoopenwisp:masterfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
WalkthroughAdds a CI Failure Bot and related integration: a new GitHub Actions workflow triggers on failed workflow runs and executes openwisp_utils.bots.ci_failure.bot.CIFailureBot. The bot authenticates to GitHub (GITHUB_TOKEN) and optionally Gemini (GEMINI_API_KEY), retrieves workflow run jobs and logs, fetches PR diffs, classifies failures, requests AI analysis (with a deterministic fallback), and posts or updates a deduplicated comment on the PR. Also adds package extras for GitHub Actions, documentation, unit tests, a Django AppConfig, package initializers, test settings registration, and updates .coveragerc and runtests. Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant GHApi as GitHub API
participant Bot as CI Failure Bot
participant Gemini as Gemini API
participant PR as Pull Request
GHA->>Bot: Trigger on CI failure (workflow_run)
Bot->>GHApi: Get workflow run & jobs
GHApi-->>Bot: Workflow run data (jobs, actor)
Bot->>Bot: Evaluate skip conditions (dependabot, fork, deleted)
alt Proceed with analysis
Bot->>GHApi: Fetch failed job logs (zip/plain)
GHApi-->>Bot: Job logs
Bot->>Bot: Parse logs & classify failure
Bot->>GHApi: Fetch PR diff
GHApi-->>Bot: PR diff
Bot->>Gemini: Send context (logs + diff + metadata)
Gemini-->>Bot: Diagnosis + remediation
alt Gemini available
Bot->>Bot: Use AI analysis
else Gemini unavailable
Bot->>Bot: Use fallback remediation
end
Bot->>GHApi: Post or update single PR comment
GHApi->>PR: Comment created/updated
else Skip
Bot->>Bot: Exit without posting
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @.github/scripts/ci_failure_bot.py:
- Around line 109-125: The method generate_commit_message_response is dead code
(never called); either remove it or call it from run() where other validations
happen: locate the run() method and after fetching the commit message (e.g.,
commit_message variable or the payload field you already parse), call
self.generate_commit_message_response(commit_message) and use its return value
to post a comment or append to the bot response flow, or simply delete the
generate_commit_message_response function if you don't want commit message
validation now; ensure you refer to the exact symbol
generate_commit_message_response when removing or invoking it and update any
tests/uses accordingly.
- Around line 134-136: The code currently builds comment_body with an automatic
mention via the variables author and comment_body and always calls
pr.create_issue_comment, which can spam the PR author; update this by removing
the "@{author}" mention or making mentions optional via a config flag (e.g.,
mention_author) and by adding rate-limiting/deduplication before calling
pr.create_issue_comment: record the last_mention timestamp per PR (in-memory
cache or persistent store) and only include the mention or post a comment if the
last mention is older than a configured cooldown, or skip adding the mention
entirely and post a non-mention comment instead; ensure you reference and modify
the creation site that constructs comment_body and the call to
pr.create_issue_comment to enforce the new logic.
In @.github/workflows/ci-failure-bot.yml:
- Around line 28-34: The step setting PR_NUMBER reads
github.event.workflow_run.pull_requests[0].number without checking for an empty
array; update the env for PR_NUMBER in the "Run CI Failure Bot" step to a
conditional expression that yields the PR number when pull_requests exists and
has items, otherwise an empty string (use a GitHub Actions expression checking
github.event.workflow_run.pull_requests and its length before indexing). This
prevents failures for workflow runs that have no associated PR while keeping the
Python script behavior unchanged.
🧹 Nitpick comments (4)
.github/scripts/ci_failure_bot.py (3)
18-20: Improve error message specificity for missing environment variables.The current error message "Missing env vars" doesn't indicate which variables are missing, making debugging harder.
🔎 Proposed fix for more descriptive error message
if not all([self.github_token, self.workflow_run_id, self.repository_name]): - print("Missing env vars") + missing = [] + if not self.github_token: missing.append('GITHUB_TOKEN') + if not self.workflow_run_id: missing.append('WORKFLOW_RUN_ID') + if not self.repository_name: missing.append('REPOSITORY') + print(f"Missing required environment variables: {', '.join(missing)}") sys.exit(1)
25-43: Refine exception handling for better error visibility.The broad
Exceptioncatch on line 41 can mask specific API errors. Consider catchingGithubExceptionspecifically or re-raising after logging.Based on static analysis hints, catching blind exceptions makes debugging harder.
🔎 Proposed fix for more specific exception handling
+from github import GithubException + class CIFailureBot: # ... existing code ... def get_workflow_logs(self): try: workflow_run = self.repo.get_workflow_run(int(self.workflow_run_id)) jobs = workflow_run.jobs() failed_jobs = [] for job in jobs: if job.conclusion == 'failure': for step in job.steps: if step.conclusion == 'failure': failed_jobs.append({ 'job_name': job.name, 'step_name': step.name, 'step_number': step.number }) return failed_jobs - except Exception as e: + except GithubException as e: print(f"Error getting logs: {e}") return []
127-139: Validate PR number and improve exception handling.Two concerns:
- Line 128 only checks truthiness of
pr_number; it should validate it's a non-empty string that can be converted to an integer.- Line 138 catches blind
Exception, which can mask specific API errors.🔎 Proposed fix for validation and exception handling
+from github import GithubException + class CIFailureBot: # ... existing code ... def post_comment(self, message): - if not self.pr_number: + if not self.pr_number or self.pr_number.strip() == '': print("No PR number, skipping comment") return try: + pr_num = int(self.pr_number) - pr = self.repo.get_pull(int(self.pr_number)) + pr = self.repo.get_pull(pr_num) author = pr.user.login comment_body = f"@{author} {message}" pr.create_issue_comment(comment_body) - print(f"Posted comment to PR #{self.pr_number}") + print(f"Posted comment to PR #{pr_num}") - except Exception as e: + except (ValueError, GithubException) as e: print(f"Error posting comment: {e}").github/workflows/ci-failure-bot.yml (1)
24-26: Pin dependency versions for reproducibility and stability in CI workflows.Installing packages without version constraints means each workflow run could install different versions, potentially introducing breaking changes. Consider pinning to specific stable versions.
🔎 Proposed fix with current stable versions
- name: Install dependencies run: | - pip install requests PyGithub + pip install requests==2.32.5 PyGithub==2.8.1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/ci_failure_bot.py.github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
.github/scripts/ci_failure_bot.py
40-40: Consider moving this statement to an else block
(TRY300)
41-41: Do not catch blind exception: Exception
(BLE001)
138-138: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
🔇 Additional comments (2)
.github/workflows/ci-failure-bot.yml (1)
1-13: LGTM! Workflow trigger and condition are properly configured.The workflow correctly triggers on the completion of the "OpenWISP Utils CI Build" workflow and only runs when the conclusion is 'failure'. The setup is appropriate for this use case.
.github/scripts/ci_failure_bot.py (1)
141-183: LGTM! The orchestration flow is well-structured.The
run()method properly orchestrates the workflow: retrieves logs, analyzes failure types, generates appropriate responses, includes a sensible fallback for unrecognized failures, and posts the final comment. The main execution block follows standard Python conventions.
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
4d35151 to
55627fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @.github/scripts/ci_failure_bot.py:
- Line 9: The project imports github.Github in .github/scripts/ci_failure_bot.py
but PyGithub is not declared as a dependency; update the package manifest (e.g.,
setup.py) to add PyGithub (suggestion: 'PyGithub>=2.0.0') to the appropriate
extras_require/dev/ci dependency group so local installs and CI installs include
it, and ensure any CI/dev docs or requirements files reference the same version
constraint.
♻️ Duplicate comments (2)
.github/scripts/ci_failure_bot.py (2)
114-130: Unused method:generate_commit_message_responseis never called.This method is defined but not invoked anywhere in the
run()method. Either integrate commit message validation into the bot's flow or remove this dead code.Would you like me to suggest how to integrate commit message validation into the workflow, or would you prefer to remove this method for now?
139-141: Remove or make the automatic PR author mention optional and rate-limited.GitHub's automation policies explicitly forbid mass-mentioning and repeatedly pinging users. The current implementation automatically mentions the PR author in every CI failure comment (line 140), which could violate GitHub's anti-spam guidelines if failures occur frequently. Either remove the automatic @mention entirely or implement rate-limiting and deduplication to avoid repeated pings on the same PR.
🔎 Remove automatic mention
try: pr = self.repo.get_pull(int(self.pr_number)) - author = pr.user.login - comment_body = f"@{author} {message}" - pr.create_issue_comment(comment_body) + pr.create_issue_comment(message) print(f"Posted comment to PR #{self.pr_number}")
🧹 Nitpick comments (3)
.github/scripts/ci_failure_bot.py (3)
19-21: Improve error message to specify which environment variables are missing.The error message "Missing env vars" doesn't indicate which specific variables are missing, making debugging harder for users.
🔎 More descriptive error message
if not all([self.github_token, self.workflow_run_id, self.repository_name]): - print("Missing env vars") + missing = [] + if not self.github_token: + missing.append("GITHUB_TOKEN") + if not self.workflow_run_id: + missing.append("WORKFLOW_RUN_ID") + if not self.repository_name: + missing.append("REPOSITORY") + print(f"Missing required environment variables: {', '.join(missing)}") sys.exit(1)
44-46: Make exception handling more specific or enhance error logging.Catching
Exceptionis too broad and may hide important issues like authentication failures, network errors, or API rate limits. At minimum, log the exception type to aid debugging.🔎 Enhanced error logging
except Exception as e: - print(f"Error getting logs: {e}") + print(f"Error getting logs ({type(e).__name__}): {e}") return []Or, better yet, catch specific exceptions:
- except Exception as e: - print(f"Error getting logs: {e}") + except (GithubException, ValueError) as e: + print(f"Error getting workflow logs: {e}") return []Note: You'll need to import
GithubExceptionfromgithubif using the specific approach.
143-144: Enhance exception handling specificity.Similar to the
get_workflow_logsmethod, catching broadExceptioncan hide important API errors. Consider catching specific exceptions or improving error logging.🔎 More specific exception handling
- except Exception as e: - print(f"Error posting comment: {e}") + except (GithubException, ValueError) as e: + print(f"Error posting comment ({type(e).__name__}): {e}")Note: Import
GithubExceptionfromgithubif using this approach.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/ci_failure_bot.py.github/workflows/ci-failure-bot.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
.github/scripts/ci_failure_bot.py
43-43: Consider moving this statement to an else block
(TRY300)
44-44: Do not catch blind exception: Exception
(BLE001)
143-143: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (4)
.github/scripts/ci_failure_bot.py (4)
48-63: LGTM! Failure classification logic is sound.The method correctly categorizes failures based on step names with case-insensitive matching and proper deduplication.
65-112: LGTM! Response templates provide clear, actionable guidance.All three response generators (
generate_qa_response,generate_test_response,generate_setup_response) offer specific commands and helpful troubleshooting steps with appropriate documentation links.
146-185: LGTM! Main orchestration logic is well-structured.The
run()method properly coordinates workflow log retrieval, failure analysis, response generation, and comment posting. The fallback to a generic message for unrecognized failures ensures users always receive guidance.
188-190: LGTM! Standard entry point pattern.The script entry point follows Python best practices.
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
55627fb to
2baccea
Compare
|
Hi @nemesifier , Got the CI failure bot working! It catches QA failures, test issues, and setup problems, then posts helpful responses with the fix commands and links to contributing docs. Tested it on my own repo first like you suggested - triggers correctly when builds fail and generates the right responses. This covers the first part i mentioned. Let me know how it looks and I can work on the auto-assignment and stalled PR stuff next. |
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @.github/scripts/ci_failure_bot.py:
- Around line 114-124: post_comment currently always creates a new PR comment
which causes duplicates; modify post_comment to fetch existing PR comments via
pr.get_issue_comments(), determine the bot's username via
self.github.get_user().login (or self.github.login), scan comments for ones
authored by the bot and matching the failure signature (e.g., a stable substring
from message like "CI Build Failed" or exact message match), and if found either
update that comment with comment.edit(message) or skip creating a new one; only
call pr.create_issue_comment(message) when no matching bot comment exists and
keep existing try/except behavior for errors.
- Around line 26-46: The code calls int(self.workflow_run_id) in
get_workflow_logs which can raise ValueError if the env var is non-numeric; add
validation in the class __init__ (where self.workflow_run_id is set) to parse
and store it as an integer (or raise/exit with a clear message) and reject
non-integer values, and update get_workflow_logs to use the validated integer
attribute (e.g., self.workflow_run_id_int) instead of converting at call time;
ensure __init__ handles missing/invalid input gracefully so get_workflow_logs no
longer needs to guard against ValueError.
♻️ Duplicate comments (1)
.github/scripts/ci_failure_bot.py (1)
9-9: PyGithub dependency still needs to be added to project manifest.This was flagged in a previous review: the script imports PyGithub but it's not declared in the project's dependency manifest (e.g.,
setup.py). While the workflow installs it directly, it should be tracked in the project's dependencies for consistency and local development support.Based on past review comments.
🧹 Nitpick comments (1)
.github/scripts/ci_failure_bot.py (1)
100-112: Consider making version numbers configurable.Lines 108-109 hardcode Python and Django version numbers. As new versions are released or old ones deprecated, this will require manual script updates. Consider moving these to environment variables, a config file, or reading from the project's CI matrix configuration.
💡 Example: Use environment variables
def generate_setup_response(self): + python_versions = os.environ.get("SUPPORTED_PYTHON_VERSIONS", "3.10, 3.11, 3.12, 3.13") + django_versions = os.environ.get("SUPPORTED_DJANGO_VERSIONS", "4.2, 5.0, 5.1, 5.2") return f""" ## Setup Failed There was an issue with dependencies or environment setup. Check if you added new dependencies to requirements-test.txt. Verify Python/Django version compatibility: -- Python: 3.10, 3.11, 3.12, 3.13 -- Django: 4.2, 5.0, 5.1, 5.2 +- Python: {python_versions} +- Django: {django_versions} The CI will retry automatically, or push a small change to trigger rebuild. """
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/ci_failure_bot.py.github/workflows/ci-failure-bot.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
.github/scripts/ci_failure_bot.py
43-43: Consider moving this statement to an else block
(TRY300)
44-44: Do not catch blind exception: Exception
(BLE001)
123-123: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (5)
.github/scripts/ci_failure_bot.py (5)
48-63: LGTM!The failure classification logic is clear and handles the main categories appropriately. Using
set()to deduplicate failure types is a good approach.
65-80: LGTM!The QA response provides clear, actionable guidance with the appropriate command and documentation link.
82-98: LGTM!The test failure response provides helpful debugging steps and covers common issues appropriately.
126-165: LGTM!The orchestration logic is well-structured. The bot correctly handles multiple failure types, provides fallback messaging, and gracefully handles the case when no failure logs are found.
168-170: LGTM!Standard main execution pattern is correctly implemented.
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
2baccea to
8d3d6bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/ci-failure-bot.yml (1)
28-34: The empty pull_requests array issue has been addressed.Line 33 correctly uses the
|| ''fallback to handle cases wherepull_requestsis empty (e.g., manual workflow runs or direct pushes). The Python script'spost_commentmethod gracefully handles an emptyPR_NUMBER..github/scripts/ci_failure_bot.py (1)
1-11: PyGithub dependency handling noted in previous review.The import of PyGithub (line 9) without declaring it in the project's
setup.pyhas already been flagged in a previous review. While the workflow explicitly installs it, adding it to the project's dependency manifest would improve local development support.
🧹 Nitpick comments (2)
.github/workflows/ci-failure-bot.yml (1)
24-26: Consider pinning dependency versions for reproducibility.The workflow installs
requestsandPyGithubwithout version constraints. While this ensures you get the latest versions, it could lead to unexpected breakage if major version changes introduce breaking changes.🔎 Example: Pin dependency versions
- name: Install dependencies run: | - pip install requests PyGithub + pip install requests>=2.31.0 PyGithub>=2.0.0.github/scripts/ci_failure_bot.py (1)
32-53: Consider catching more specific exceptions.Line 50 catches the broad
Exceptiontype. While this makes the bot resilient to API failures, catching more specific exceptions (e.g.,GithubException,RateLimitExceededException) would provide better error diagnosis and allow you to handle different failure modes appropriately.🔎 Example: Catch specific exceptions
from github import GithubException, RateLimitExceededException def get_workflow_logs(self): try: workflow_run = self.repo.get_workflow_run(self.workflow_run_id) jobs = workflow_run.jobs() # ... rest of logic except RateLimitExceededException as e: print(f"Rate limit exceeded: {e}") return [] except GithubException as e: print(f"GitHub API error: {e}") return []
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/ci_failure_bot.py.github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
.github/scripts/ci_failure_bot.py
49-49: Consider moving this statement to an else block
(TRY300)
50-50: Do not catch blind exception: Exception
(BLE001)
144-144: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
🔇 Additional comments (11)
.github/workflows/ci-failure-bot.yml (3)
1-9: LGTM: Workflow trigger configuration is correct.The
workflow_runtrigger on the "OpenWISP Utils CI Build" workflow with thecompletedevent type is the appropriate way to respond to CI failures. The additional failure check on line 13 ensures the bot only runs when needed.
10-17: LGTM: Job configuration and checkout step are appropriate.The failure condition and ubuntu-latest runner are correctly configured. Using
actions/checkout@v6is current best practice.
19-22: LGTM: Python setup is appropriate.Python 3.11 is a stable, well-supported version suitable for this automation script.
.github/scripts/ci_failure_bot.py (8)
12-31: LGTM: Input validation has been properly implemented.The
__init__method correctly validates that required environment variables are present (line 19) and thatworkflow_run_idis numeric (lines 23-27) before attempting to use it. This addresses the previous concern about potentialValueErrorexceptions during runtime.
54-69: LGTM: Failure type analysis logic is sound.The method correctly categorizes failures by analyzing step names with case-insensitive matching and returns unique failure types. The keyword-based classification appropriately handles QA checks, tests, and setup failures.
71-118: LGTM: Response generation methods provide helpful guidance.The response templates offer clear, actionable guidance for QA failures, test failures, and setup issues. The commands and links to contributing guidelines will help contributors resolve issues quickly.
Note: The Python and Django versions on lines 114-115 are hardcoded and may need periodic updates as new versions are released.
120-123: LGTM: Early return for missing PR number is appropriate.The check on line 121 correctly handles cases where the bot is triggered by workflow runs without an associated PR (e.g., direct pushes or manual runs).
128-146: Deduplication logic successfully addresses previous concerns.The implementation correctly checks for existing bot comments (lines 128-139) and updates them rather than creating duplicates. This addresses the major issue raised in a previous review.
The search strings on lines 133-136 ("CI Build Failed" and "QA Checks Failed") should continue to work as the fallback message on line 172 and the QA response on line 73 contain these markers.
147-187: LGTM: Main orchestration logic is well-structured.The
runmethod effectively coordinates log retrieval, failure analysis, response generation, and comment posting. The fallback message (lines 169-181) ensures helpful guidance is provided even when specific failure types aren't detected.
189-191: LGTM: Standard script execution pattern.The main execution block follows Python best practices.
124-146: Add validation before converting PR_NUMBER to int.Line 126 converts
self.pr_numbertoint()without validation. IfPR_NUMBERis an empty string (which can happen when the workflow fallback on line 33 of the YAML file triggers), this will raise aValueErrorand crash the bot. The early return on line 121 checks for falsy values, but an empty string passes the truthy check.🔎 Proposed fix to validate PR_NUMBER
def post_comment(self, message): - if not self.pr_number: + if not self.pr_number or self.pr_number == '': print("No PR number, skipping comment") return try: - pr = self.repo.get_pull(int(self.pr_number)) + try: + pr_num = int(self.pr_number) + except ValueError: + print(f"Invalid PR_NUMBER: {self.pr_number}") + return + + pr = self.repo.get_pull(pr_num)Likely an incorrect or invalid review comment.
Implements automated bot responses for common CI failures: - Parses QA check failures (flake8, black, isort) - Detects test failures and setup issues - Provides specific fix suggestions with links - Removes automatic mentions to prevent spam - Handles edge cases for workflow runs without PRs Tested on personal repository before submission. Closes openwisp#524
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
8d3d6bd to
939a127
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/scripts/ci_failure_bot.py (1)
9-9: PyGithub dependency still not declared in project manifest.This was flagged in a previous review and remains unaddressed. The script imports
github.Githubbut PyGithub is not insetup.py. While the workflow installs it explicitly, adding it toextras_requireensures consistency for local development.
🧹 Nitpick comments (4)
.github/scripts/ci_failure_bot.py (4)
19-21: Consider logging which environment variables are missing.The current message "Missing env vars" doesn't indicate which specific variables are absent, making debugging harder when the workflow fails during initialization.
🔎 Suggested improvement
- if not all([self.github_token, self.workflow_run_id, self.repository_name]): - print("Missing env vars") + missing = [] + if not self.github_token: + missing.append("GITHUB_TOKEN") + if not self.workflow_run_id: + missing.append("WORKFLOW_RUN_ID") + if not self.repository_name: + missing.append("REPOSITORY") + if missing: + print(f"Missing required env vars: {', '.join(missing)}") sys.exit(1)
50-52: Avoid catching bareException.Catching
Exceptionwill swallow all errors includingKeyboardInterruptandSystemExit. Use more specific exception types from PyGithub (e.g.,GithubException) to handle API errors while allowing unexpected exceptions to propagate.🔎 Suggested fix
+from github import Github, GithubException + # In get_workflow_logs: - except Exception as e: - print(f"Error getting logs: {e}") + except GithubException as e: + print(f"Error getting workflow logs: {e}") return []
134-136: Cache the bot login to avoid repeated API calls.
self.github.get_user().loginmakes a GitHub API call each timepost_commentis invoked. Consider caching this value in__init__to reduce API usage and improve performance.🔎 Suggested approach
# In __init__, after line 30: + self.bot_login = self.github.get_user().login # In post_comment, replace line 135: - bot_login = self.github.get_user().login + bot_login = self.bot_login
150-151: Use specific exception types instead of bareException.Similar to
get_workflow_logs, this catch-all will swallow unexpected errors. Consider catchingGithubExceptionfor API errors andValueErrorfor the PR number conversion (which is already handled separately).🔎 Suggested fix
- except Exception as e: + except GithubException as e: print(f"Error posting comment: {e}")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/ci_failure_bot.py.github/workflows/ci-failure-bot.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
.github/scripts/ci_failure_bot.py
49-49: Consider moving this statement to an else block
(TRY300)
50-50: Do not catch blind exception: Exception
(BLE001)
150-150: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (4)
.github/scripts/ci_failure_bot.py (4)
54-69: LGTM!The failure type classification logic is clear and the deduplication using
set()is appropriate.
71-118: LGTM!The response templates are clear, actionable, and provide helpful links to documentation.
153-192: LGTM!The orchestration logic is clean with appropriate fallback handling for unclassified failures.
195-197: LGTM!Standard entry point pattern.
There was a problem hiding this comment.
I am not seeing any response in Srinath0916/ci-bot-test#1. Is it really working?
Would this work as a reusable github actions?
Docs and tests are completely missing but I guess we can spend time on that once we are sure this is going in the right direction.
PS: please let's exclude dependabot PRs.
| - Python: 3.10, 3.11, 3.12, 3.13 | ||
| - Django: 4.2, 5.0, 5.1, 5.2 | ||
|
|
||
| The CI will retry automatically, or push a small change to trigger rebuild. |
There was a problem hiding this comment.
I don't understand the point of this. Given my previous comment, do you think this is still needed?
There was a problem hiding this comment.
This static message approach isn't needed given the Gemini integration direction.
The static responses were just to validate the workflow mechanics, but the AI analysis will provide much better, context-specific guidance.
I'll focus on the Gemini approach instead.
|
Hi @nemesifier , I’ve implemented a Gemini-based version locally following the approach and system prompt you outlined (using build logs, PR diff, workflow context, and a direct tone). Before pushing it, I wanted to check if you’re happy with proceeding in this direction and with introducing a GEMINI_API_KEY dependency at the org level. Let me know and I’ll move forward accordingly. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@Srinath0916 before merging it here we need to test this somewhere else.
I do not see any output in Srinath0916/ci-bot-test#1, how are you testing this?
|
Hi @nemesifier , I can show you the working bot demo here: srinath-testing/gemini-ci-bot-demo#1 You can see the bot posted: "🤖 Demo CI Failure Bot (AI-powered)" with structured analysis. The reason you don't see output in Srinath0916/ci-bot-test#1 is that personal repos have token permission limitations with workflow_run triggers. The demo repo I created specifically for this has the right setup. The Gemini integration is ready locally - I can push it once you confirm the demo proves the concept works. Would you like me to proceed with the full AI implementation? |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
It's encouraging @Srinath0916, please push your progress and then focus on adding automated tests, docs and following up reviews.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @.github/scripts/ci_failure_bot.py:
- Line 71: The requests.get call using variables logs_url and headers has no
timeout and can hang; modify the call to pass a timeout (e.g., timeout=10) so it
becomes requests.get(logs_url, headers=headers, timeout=<seconds>), and add a
surrounding try/except to catch requests.exceptions.Timeout (or
requests.exceptions.RequestException) to log the timeout/error and handle
failure gracefully where response is expected.
- Around line 113-123: requests.get call may hang and the except catches all
exceptions; add a timeout argument to requests.get (e.g., requests.get(diff_url,
headers=headers, timeout=10)) and replace the broad except Exception with except
requests.RequestException as e to only catch network-related errors, then log or
print the error variable as before.
- Around line 16-53: The constructor currently hardcodes self.model =
genai.GenerativeModel("gemini-1.5-flash") which is deprecated; update __init__
to read a GEMINI_MODEL env var (e.g., model_name =
os.environ.get("GEMINI_MODEL", "gemini-2.5-flash")) and pass that into
genai.GenerativeModel so the model can be changed without code edits, and ensure
genai.configure(api_key=...) remains before instantiating self.model; optionally
validate that model_name is non-empty and include its variable name in error
messages.
🧹 Nitpick comments (3)
.github/scripts/ci_failure_bot.py (3)
134-136: Narrow the exception type for better debuggability.Catching bare
Exceptioncan mask unexpected errors. Consider catchingGithubExceptionspecifically.🔎 Proposed fix
- except Exception as e: + except GithubException as e: print(f"Error getting workflow YAML: {e}") return None
154-188: Remove ineffective# noqacomments from within the string literal.The
# noqa: E501comments on lines 154, 157, 169, 173-175, 182, 185 are inside the multi-line string and have no effect on linter behavior. They just clutter the prompt sent to Gemini.🔎 Proposed fix
Remove all
# noqa: E501suffixes from within the context string. If you need to suppress E501 for this file, add a single# noqa: E501comment on line 155 (the opening of the f-string), or configure the linter to ignore this file/section.- # noqa: E501 (ignore line length for multiline string) context = f""" ### ROLE -You are the "Automated Maintainer Gatekeeper." Your goal is to analyze Pull Request (PR) build failures and provide direct, technically accurate, and no-nonsense feedback to contributors. # noqa: E501 +You are the "Automated Maintainer Gatekeeper." Your goal is to analyze Pull Request (PR) build failures and provide direct, technically accurate, and no-nonsense feedback to contributors.Apply similar cleanup to lines 169, 173, 174, 175, 182, and 185.
246-253: Narrow exception handling for actor check.The dependabot check catches bare
Exception. Since this is a non-critical check (bot continues anyway), consider catchingGithubExceptionspecifically or at minimumAttributeErrorfor the case whereactormight be None.🔎 Proposed fix
try: workflow_run = self.repo.get_workflow_run(self.workflow_run_id) - actor_login = workflow_run.actor.login.lower() - if "dependabot" in actor_login: + if workflow_run.actor and "dependabot" in workflow_run.actor.login.lower(): print(f"Skipping dependabot PR from {workflow_run.actor.login}") return - except Exception as e: + except (GithubException, AttributeError) as e: print(f"Warning: Could not check actor: {e}")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/ci_failure_bot.py.github/workflows/ci-failure-bot.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
.github/scripts/ci_failure_bot.py
71-71: Probable use of requests call without timeout
(S113)
93-93: Consider moving this statement to an else block
(TRY300)
113-113: Probable use of requests call without timeout
(S113)
120-120: Do not catch blind exception: Exception
(BLE001)
134-134: Do not catch blind exception: Exception
(BLE001)
154-154: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
192-192: Consider moving this statement to an else block
(TRY300)
193-193: Do not catch blind exception: Exception
(BLE001)
252-252: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (4)
.github/scripts/ci_failure_bot.py (4)
1-13: Imports and setup look appropriate.The imports are well-organized and appropriate for the script's functionality. Note that a past review flagged adding
PyGithubandgoogle-generativeaito the project's dependency manifest for local development support—ensure this is tracked.
212-239: Comment deduplication is well-implemented.The logic correctly checks for existing bot comments and updates them rather than creating duplicates. This addresses previous review feedback about preventing comment spam.
197-210: Fallback response is appropriate.The static fallback provides useful guidance when Gemini is unavailable, with actionable commands and a link to documentation.
76-78: Token limits are not a constraint for these truncations.Gemini 1.5 Flash has a 1,048,576-token context window (~4 million characters). With logs truncated to 5000 chars and diff to 8000 chars, plus the prompt template, the total message size (~15-20K characters) is negligible. The truncations are sensible for practical reasons—API response speed, cost optimization, and filtering noise—not because of Gemini's context limitations.
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
294b7f6 to
2e7b8f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @.github/workflows/ci-failure-bot.yml:
- Around line 29-32: The comment "Pin dependency versions for reproducibility"
is inaccurate relative to the pip install line under the "Install dependencies"
step; either change the version specifiers in the pip install command to exact
pins using == (e.g., requests==2.31.0 PyGithub==2.0.0
google-generativeai==0.3.0) to match the comment, or update the comment to
something like "Specify minimum dependency versions" to reflect the current >=
constraints—modify the pip install line or the comment accordingly in the
"Install dependencies" step.
🧹 Nitpick comments (4)
.github/scripts/ci_failure_bot.py (4)
125-136: Narrow exception type for better error handling.Line 134 catches bare
Exception, which can mask unexpected errors. Consider catching specific GitHub API exceptions.🔎 Proposed fix
workflow_file = self.repo.get_contents(workflow_path) return workflow_file.decoded_content.decode("utf-8") - except Exception as e: + except (GithubException, ValueError) as e: print(f"Error getting workflow YAML: {e}") return None
154-154: Remove unusednoqadirective.Line 154 has an unused
noqadirective that can be removed.🔎 Proposed fix
- # noqa: E501 (ignore line length for multiline string) context = f"""
190-195: Narrow exception type in Gemini API call.Line 193 catches bare
Exception. Consider catching specific API exceptions from thegoogle.generativeailibrary for better error diagnostics.🔎 Proposed fix
try: response = self.model.generate_content(context) return response.text - except Exception as e: + except (ValueError, ConnectionError) as e: print(f"Error calling Gemini API: {e}") return self.fallback_response()
246-253: Narrow exception type in actor check.Line 252 catches bare
Exception. Consider catching specific GitHub API exceptions.🔎 Proposed fix
try: workflow_run = self.repo.get_workflow_run(self.workflow_run_id) actor_login = workflow_run.actor.login.lower() if "dependabot" in actor_login: print(f"Skipping dependabot PR from {workflow_run.actor.login}") return - except Exception as e: + except (GithubException, AttributeError) as e: print(f"Warning: Could not check actor: {e}")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/ci_failure_bot.py.github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
.github/scripts/ci_failure_bot.py
71-71: Probable use of requests call without timeout
(S113)
93-93: Consider moving this statement to an else block
(TRY300)
113-113: Probable use of requests call without timeout
(S113)
120-120: Do not catch blind exception: Exception
(BLE001)
134-134: Do not catch blind exception: Exception
(BLE001)
154-154: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
192-192: Consider moving this statement to an else block
(TRY300)
193-193: Do not catch blind exception: Exception
(BLE001)
252-252: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
🔇 Additional comments (3)
.github/scripts/ci_failure_bot.py (1)
212-239: Comment deduplication logic looks good.The implementation correctly checks for existing bot comments and updates them instead of creating duplicates. The exception handling is appropriately narrow.
.github/workflows/ci-failure-bot.yml (2)
1-18: Workflow trigger and permissions configured correctly.The workflow appropriately:
- Triggers only on CI build failures
- Excludes dependabot PRs at the job level
- Grants minimal required permissions (issues/PR write, contents read)
34-41: Verify GEMINI_API_KEY secret is configured.Line 37 references
secrets.GEMINI_API_KEY. Based on the PR comments, this requires organization-level secret configuration. Ensure the secret is added to the OpenWISP organization before merging, or the workflow will fail.Would you like me to generate a verification script to check if the secret is accessible, or would you prefer to confirm this manually with the maintainers?
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
2e7b8f1 to
6ab0337
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/scripts/ci_failure_bot.py (2)
188-188: Remove unusednoqadirective.The
# noqa: E501directive is unnecessary as E501 (line length) is not enabled in the linter configuration.🔎 Proposed fix
-""" # noqa: E501 +"""
190-195: Consider narrowing the exception type for Gemini API errors.Catching bare
Exceptioncan mask unexpected errors. Thegoogle.generativeailibrary typically raisesgoogle.api_core.exceptions.GoogleAPIErroror similar for API failures. However, given the fallback behavior is appropriate and the bot should be resilient, this is a minor concern.🔎 Proposed fix to catch more specific exceptions
try: response = self.model.generate_content(context) return response.text - except Exception as e: + except (genai.types.GenerateContentError, ValueError, ConnectionError) as e: print(f"Error calling Gemini API: {e}") return self.fallback_response()Note: You may need to verify the exact exception types from the
google-generativeailibrary.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/ci_failure_bot.py.github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
.github/scripts/ci_failure_bot.py
94-94: Consider moving this statement to an else block
(TRY300)
188-188: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
192-192: Consider moving this statement to an else block
(TRY300)
193-193: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (10)
.github/scripts/ci_failure_bot.py (9)
1-12: LGTM!Imports are well-organized with standard library imports first, followed by third-party packages.
15-54: LGTM!The initialization logic is well-structured with comprehensive environment variable validation, proper integer conversion with error handling, and a configurable Gemini model name via environment variable.
56-97: LGTM!The method correctly fetches failed job logs with a timeout, truncates to avoid token limits, and handles exceptions with specific types.
99-124: LGTM!Previous review feedback was addressed: timeout is set, and exception handling uses specific types.
126-137: LGTM!Simple and focused method with appropriate exception handling.
197-210: LGTM!The fallback response provides useful guidance when AI analysis is unavailable.
212-239: LGTM!The deduplication logic correctly checks for existing bot comments and updates them instead of creating duplicates. This addresses the previous review feedback.
241-271: LGTM!The orchestration flow is clear and includes a defense-in-depth dependabot check. Error handling at each step ensures graceful degradation.
274-276: LGTM!Standard entry point pattern.
.github/workflows/ci-failure-bot.yml (1)
22-27: The action versions are valid. Bothactions/checkout@v6andactions/setup-python@v6are publicly available releases (checkout v6.0.1 and setup-python v6.1.0). The workflow will not fail due to these action versions.Likely an incorrect or invalid review comment.
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
6ab0337 to
ab7cd1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @.github/scripts/ci_failure_bot.py:
- Around line 212-239: The comment deduplication in post_comment is brittle
because it looks for specific phrases; update post_comment to use a consistent
hidden HTML marker (e.g., "<!-- ci-failure-bot -->") when creating or editing
comments and check for that marker in existing_comments instead of matching "CI
Build Failed" or "build failed"; ensure the marker is appended to the message
passed to pr.create_issue_comment and comment.edit, and then change the loop
that scans pr.get_issue_comments() to detect comment.body containing the marker
(while preserving the existing bot_login check).
- Around line 99-124: The PR diff extraction in get_pr_diff currently slices
response.text[-8000:], dropping the beginning of the diff where imports and
initial definitions live; change this to capture the start of the diff (e.g.,
use the first 8000 characters of response.text) or, if you want both head and
tail context, concatenate response.text[:4000] + "\n...\n" +
response.text[-4000:] so the function returns the leading context instead of
only the tail.
🧹 Nitpick comments (1)
.github/scripts/ci_failure_bot.py (1)
139-195: Gemini integration is functional but could be refined.The context construction is comprehensive and the system prompt matches the maintainer's requirements. Two observations:
Optional refactor: Line 193 catches bare
Exception, which static analysis flags. While this provides robustness for external API calls, consider catching specificgenaiexceptions if available for clearer error handling.Context size awareness: The combined context (build logs, PR diff, workflow YAML, prompt template) could grow quite large with multiple failed jobs. Gemini 2.5-flash has a generous context window, so this should work, but monitor for potential token limit issues in practice.
🔎 Optional: Narrow exception handling
try: response = self.model.generate_content(context) return response.text - except (ValueError, ConnectionError, Exception) as e: + except (ValueError, ConnectionError, TimeoutError) as e: print(f"Error calling Gemini API: {e}") return self.fallback_response()Note: You may want to check the
google.generativeaidocumentation for specific exception types to catch instead of the bareException.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/ci_failure_bot.py.github/workflows/ci-failure-bot.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
.github/scripts/ci_failure_bot.py
94-94: Consider moving this statement to an else block
(TRY300)
192-192: Consider moving this statement to an else block
(TRY300)
193-193: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
🔇 Additional comments (3)
.github/scripts/ci_failure_bot.py (3)
16-54: Initialization and environment validation is solid.The constructor properly validates all required environment variables with specific error messages, ensures
workflow_run_idis numeric, and makes the Gemini model configurable. The setup logic is clean and maintainable.
56-97: Build log collection is well-implemented.The method correctly fetches logs from failed jobs with appropriate timeout, limits log size to avoid token overflow, and includes both job-level and step-level failure details. Exception handling is appropriate.
241-271: Main execution flow is well-structured.The
runmethod orchestrates the bot's operations logically: validates actor (skips dependabot), gathers context, analyzes with Gemini, and posts results. The early exit when no build logs are found prevents unnecessary API calls. Good defensive programming throughout.
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
2 times, most recently
from
8e5d108 to
5d3181a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @.github/scripts/ci_failure_bot.py:
- Line 189: Remove the unnecessary inline noqa directive from the trailing
docstring terminator (the '""" # noqa: E501' token) so the line ends simply
with '"""'; update the closing triple-quote in the module-level string in
ci_failure_bot.py to remove the "# noqa: E501" comment.
🧹 Nitpick comments (2)
.github/scripts/ci_failure_bot.py (2)
56-97: Log truncation approach is reasonable for error context.The method correctly fetches logs from failed jobs and steps. Taking the last 5000 characters of logs (line 78) is a pragmatic choice since error messages typically appear at the end of build logs. However, if early setup or environment context is needed for debugging, consider capturing both the first and last portions (e.g., first 2000 + last 3000 chars).
191-196: Consider narrowing the exception catch for better debugging.While catching
Exceptionensures the bot always falls back gracefully, it can mask unexpected errors (e.g., programming bugs). Consider catching only API-related exceptions likegoogle.generativeai.types.GenerationExceptionor logging unexpected exceptions before falling back.🔎 Optional improvement
try: response = self.model.generate_content(context) return response.text - except (ValueError, ConnectionError, Exception) as e: + except (ValueError, ConnectionError, AttributeError) as e: print(f"Error calling Gemini API: {e}") return self.fallback_response() + except Exception as e: + print(f"Unexpected error in Gemini analysis: {e}") + return self.fallback_response()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/ci_failure_bot.py.github/workflows/ci-failure-bot.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.10)
.github/scripts/ci_failure_bot.py
94-94: Consider moving this statement to an else block
(TRY300)
189-189: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
193-193: Consider moving this statement to an else block
(TRY300)
194-194: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (7)
.github/scripts/ci_failure_bot.py (7)
1-14: LGTM! Clean imports and documentation.The module docstring and imports are well-organized and appropriate for the bot's functionality.
16-54: LGTM! Robust initialization with comprehensive validation.The environment variable validation is thorough with clear error messages,
workflow_run_idis properly validated as numeric, and the Gemini model is configurable. The initialization sequence is correct.
126-137: LGTM! Clean workflow YAML retrieval.The method correctly fetches and decodes the workflow file content with appropriate error handling.
198-211: LGTM! Appropriate fallback message.The static fallback provides useful guidance when AI analysis is unavailable.
242-277: LGTM! Well-structured main execution flow.The run method orchestrates the bot's workflow logically: validates it's not a dependabot PR, gathers context, performs AI analysis, and posts results. The dependabot check prevents unnecessary processing.
119-119: Diff truncation still loses critical early context.This issue was previously flagged: taking the last 8000 characters discards the beginning of the diff where imports, class definitions, and initial functions typically appear. For large diffs, Gemini will miss crucial context.
Likely an incorrect or invalid review comment.
227-230: Comment deduplication remains brittle.This issue was previously flagged: the string matching for "CI Build Failed" or "build failed" is fragile. If Gemini generates a response without these phrases, duplicate comments will be created. A consistent HTML marker (e.g.,
<!-- ci-failure-bot -->) would be more reliable.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/developer/reusable-github-utils.rst`:
- Around line 141-158: Change the RST heading hierarchy so "Repository Secrets"
and "Environment Variables" are subsections of "Configuration": update the
underline adornments for the "Repository Secrets" and "Environment Variables"
headings (currently using +++) to a different character (e.g., ^^^^^) to make
them lower-level headings under the "Configuration" section; keep the
"Configuration" heading as-is so "Repository Secrets" and "Environment
Variables" become its child subsections.
In `@openwisp_utils/bots/ci_failure/bot.py`:
- Around line 52-59: Guard against self.repo being None before using it in
get_build_logs and get_pr_diff: check if self.repo is truthy at the top of each
method (get_build_logs and get_pr_diff) and if not, log or print a clear message
and return a safe default (e.g., [] for get_build_logs and '' or None for
get_pr_diff) instead of dereferencing self.repo; update any early returns to
avoid AttributeError and keep existing behavior when workflow_run_id or other
preconditions fail.
In `@openwisp_utils/tests/test_ci_failure_bot.py`:
- Around line 340-356: Update the test_get_build_logs_request_exception to match
get_build_logs behavior: when requests.get raises RequestException the bot
catches it and appends a job_entry with empty logs, so assert that logs equals
[{"job_name": "test-job", "logs": ""}] (or construct an equivalent dict); locate
the CIFailureBot.get_build_logs behavior and the
test_get_build_logs_request_exception function to change the expected value
accordingly, referencing the job_entry keys "job_name" and "logs".
🧹 Nitpick comments (3)
openwisp_utils/bots/ci_failure/bot.py (2)
165-199: Classification logic: minor style nit on set iteration.The classification logic is well-structured with separate
ifblocks allowing a single log entry to contribute to multiple failure types (enabling "mixed" detection).Line 197:
list(failure_types)[0]creates an unnecessary intermediate list. Usenext(iter(failure_types))as suggested by the linter.♻️ Suggested fix
if not failure_types: return "unknown" elif len(failure_types) == 1: - return list(failure_types)[0] + return next(iter(failure_types)) else: return "mixed"
231-340: Prompt is well-structured but the Gemini call has no timeout or safety check on response.The prompt template is thorough with clear rules, examples, and failure-type routing.
Two concerns:
No timeout on
generate_content(line 336): If Gemini hangs, the bot blocks indefinitely. While GitHub Actions has a job-level timeout, adding ageneration_configwith a timeout or usingasynciowith a timeout wrapper would be more robust.
response.text(line 337): If Gemini returns a blocked/filtered response (e.g., safety filters), accessing.textmay raise aValueError. Theexcept Exceptionon line 338 catches this, but it would be clearer to checkresponse.candidatesor handleStopCandidateExceptionexplicitly.openwisp_utils/tests/test_ci_failure_bot.py (1)
162-189: Gemini analysis tests have weak assertions due to conditional guards.Line 176:
if bot.model:makes the success-path assertions conditional. SincesetUpmocksGenerativeModelsuccessfully,bot.modelshould always be truthy in this test. The guard makes the test potentially vacuous if mocking changes. Consider removing it and asserting directly.Similarly, line 185:
if bot.model:intest_analyze_with_gemini_api_error— theside_effectis only set conditionally, so ifbot.modelisNone, the test doesn't actually test the API error path at all, just the fallback.♻️ Suggested strengthening
def test_analyze_with_gemini_success(self): bot = CIFailureBot() + self.assertIsNotNone(bot.model, "Gemini model should be mocked in setUp") mock_response = Mock() ... result = bot.analyze_with_gemini(build_logs, pr_diff) - if bot.model: - self.assertIn("PEP 8", result) - self.assertIn("pip install", result) - self.mock_model.generate_content.assert_called_once() - else: - self.assertIn("Automated analysis", result) + self.assertIn("PEP 8", result) + self.assertIn("pip install", result) + self.mock_model.generate_content.assert_called_once()def test_analyze_with_gemini_api_error(self): bot = CIFailureBot() - if bot.model: - self.mock_model.generate_content.side_effect = Exception("API Error") + self.assertIsNotNone(bot.model, "Gemini model should be mocked in setUp") + self.mock_model.generate_content.side_effect = Exception("API Error") result = bot.analyze_with_gemini([], None)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci-failure-bot.ymldocs/developer/reusable-github-utils.rstopenwisp_utils/bots/__init__.pyopenwisp_utils/bots/ci_failure/__init__.pyopenwisp_utils/bots/ci_failure/bot.pyopenwisp_utils/tests/test_ci_failure_bot.py
✅ Files skipped from review due to trivial changes (1)
- openwisp_utils/bots/ci_failure/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci-failure-bot.yml
🧰 Additional context used
🪛 Ruff (0.14.14)
openwisp_utils/bots/ci_failure/bot.py
[warning] 30-30: Do not catch blind exception: Exception
(BLE001)
[warning] 45-45: Do not catch blind exception: Exception
(BLE001)
[warning] 120-120: Consider moving this statement to an else block
(TRY300)
[error] 140-140: subprocess call: check for execution of untrusted input
(S603)
[error] 141-141: Starting a process with a partial executable path
(S607)
[warning] 197-197: Prefer next(iter(failure_types)) over single element slice
Replace with next(iter(failure_types))
(RUF015)
[warning] 338-338: Do not catch blind exception: Exception
(BLE001)
[warning] 441-441: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (10)
openwisp_utils/bots/__init__.py (1)
1-1: LGTM!Simple package marker file. No concerns.
openwisp_utils/bots/ci_failure/bot.py (4)
14-50: Initialization is robust with graceful degradation — looks good.The
__init__properly handles missing env vars and failed GitHub/Gemini initialization without crashing.The broad
Exceptioncatches (lines 30, 45) flagged by Ruff are acceptable here since these guard external API initialization where the exact exception types are unpredictable.
82-98: ZIP log extraction and truncation logic looks correct.The
PKmagic-byte check,.txtfile filtering, and truncation strategy (first 2000 + last 3000 chars) are sound. Exception handling forBadZipFileis appropriately scoped.
395-449:run()method: nested try/except structure is functional but complex.The skip-logic handling (dependabot, forks, deleted forks) is thorough. A few observations:
- Line 432:
returnon skip exits before the "completed successfully" print on line 449. This is inconsistent but harmless — the skip message on line 431 is sufficient.- Line 437: When both
build_logsandpr_diffare empty, the bot still callsanalyze_with_gemini, which will send an empty context to Gemini. This is acceptable since Gemini would return something and the fallback exists, but logging could note this more clearly.Overall the flow is sound.
358-393: Comment deduplication approach is clean.The HTML comment marker pattern (
<!-- ci-failure-bot-comment -->) for finding and updating existing comments is a reliable deduplication strategy. The method properly guards against missing PR number and uninitialized GitHub client.openwisp_utils/tests/test_ci_failure_bot.py (4)
1-40: Test setup and teardown are well-structured.The
setUpproperly mocks all external dependencies (GitHub, Gemini, env vars) andtearDownuseshasattrchecks before stopping patchers. The conditional import withskipTestfallback on line 14-15 is a good defensive pattern.
70-103: Classification tests provide good coverage across all failure types.Tests cover QA, tests, setup, mixed, unknown, and empty-logs scenarios. Assertions are straightforward and correct.
285-319: ZIP format log test is thorough and well-constructed.Creates an in-memory ZIP file, properly mocks the response chain, and verifies both the job log content and step-level failure info. Good test.
321-338: Fork and deleted-fork skip tests are comprehensive.Both tests verify the skip message is printed AND that
create_issue_commentis not called, covering both the control flow and the side-effect contract.Also applies to: 387-406
docs/developer/reusable-github-utils.rst (1)
100-107: No action needed—bothactions/checkout@v6andactions/setup-python@v6are current and valid as of February 2026.As of early 2026, v6 is the latest major version for both actions. The code snippet correctly references these versions and will not cause workflow errors.
Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| **Configuration** | ||
| +++++++++++++++++ | ||
|
|
||
| Repository Secrets | ||
| ++++++++++++++++++ | ||
|
|
||
| The following secrets can be configured in the repository for enhanced | ||
| functionality: | ||
|
|
||
| - ``GEMINI_API_KEY``: Google Gemini API key for AI analysis (optional - | ||
| fallback responses used if not provided) | ||
|
|
||
| Environment Variables | ||
| +++++++++++++++++++++ | ||
|
|
||
| Optional environment variables for customization: | ||
|
|
||
| - ``GEMINI_MODEL``: Gemini model to use (default: ``gemini-2.5-flash``) |
There was a problem hiding this comment.
RST heading hierarchy: "Configuration", "Repository Secrets", and "Environment Variables" are siblings, not parent-child.
All three headings use +++ underlines, making them the same level in RST. "Repository Secrets" and "Environment Variables" should be sub-sections of "Configuration" and need a different underline character (e.g., ^^^^^).
📝 Suggested fix
**Configuration**
+++++++++++++++++
Repository Secrets
-++++++++++++++++++
+^^^^^^^^^^^^^^^^^^
The following secrets can be configured in the repository for enhanced
functionality:
- ``GEMINI_API_KEY``: Google Gemini API key for AI analysis (optional -
fallback responses used if not provided)
Environment Variables
-+++++++++++++++++++++
+^^^^^^^^^^^^^^^^^^^^^
Optional environment variables for customization:🤖 Prompt for AI Agents
In `@docs/developer/reusable-github-utils.rst` around lines 141 - 158, Change the
RST heading hierarchy so "Repository Secrets" and "Environment Variables" are
subsections of "Configuration": update the underline adornments for the
"Repository Secrets" and "Environment Variables" headings (currently using +++)
to a different character (e.g., ^^^^^) to make them lower-level headings under
the "Configuration" section; keep the "Configuration" heading as-is so
"Repository Secrets" and "Environment Variables" become its child subsections.
| @patch("openwisp_utils.bots.ci_failure.bot.requests.get") | ||
| def test_get_build_logs_request_exception(self, mock_requests): | ||
| """Test network error during log fetch""" | ||
| import requests | ||
|
|
||
| bot = CIFailureBot() | ||
| mock_workflow_run = Mock() | ||
| mock_job = Mock() | ||
| mock_job.name = "test-job" | ||
| mock_job.conclusion = "failure" | ||
| mock_job.logs_url = "https://api.github.com/logs/123" | ||
| mock_job.steps = [] | ||
| mock_workflow_run.jobs.return_value = [mock_job] | ||
| self.mock_repo.get_workflow_run.return_value = mock_workflow_run | ||
| mock_requests.side_effect = requests.RequestException("Network error") | ||
| logs = bot.get_build_logs() | ||
| self.assertEqual(logs, []) |
There was a problem hiding this comment.
Bug: test_get_build_logs_request_exception has an incorrect assertion — test will fail.
When requests.get() raises RequestException, the exception is caught by the inner try/except on line 101 of bot.py, which sets job_entry["logs"] = "". The job_entry is still appended to build_logs on line 107. So the return value is [{"job_name": "test-job", "logs": ""}], not [].
Line 356's assertion self.assertEqual(logs, []) will fail.
🐛 Proposed fix
logs = bot.get_build_logs()
- self.assertEqual(logs, [])
+ self.assertEqual(len(logs), 1)
+ self.assertEqual(logs[0]["job_name"], "test-job")
+ self.assertEqual(logs[0]["logs"], "")📝 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.
| @patch("openwisp_utils.bots.ci_failure.bot.requests.get") | |
| def test_get_build_logs_request_exception(self, mock_requests): | |
| """Test network error during log fetch""" | |
| import requests | |
| bot = CIFailureBot() | |
| mock_workflow_run = Mock() | |
| mock_job = Mock() | |
| mock_job.name = "test-job" | |
| mock_job.conclusion = "failure" | |
| mock_job.logs_url = "https://api.github.com/logs/123" | |
| mock_job.steps = [] | |
| mock_workflow_run.jobs.return_value = [mock_job] | |
| self.mock_repo.get_workflow_run.return_value = mock_workflow_run | |
| mock_requests.side_effect = requests.RequestException("Network error") | |
| logs = bot.get_build_logs() | |
| self.assertEqual(logs, []) | |
| `@patch`("openwisp_utils.bots.ci_failure.bot.requests.get") | |
| def test_get_build_logs_request_exception(self, mock_requests): | |
| """Test network error during log fetch""" | |
| import requests | |
| bot = CIFailureBot() | |
| mock_workflow_run = Mock() | |
| mock_job = Mock() | |
| mock_job.name = "test-job" | |
| mock_job.conclusion = "failure" | |
| mock_job.logs_url = "https://api.github.com/logs/123" | |
| mock_job.steps = [] | |
| mock_workflow_run.jobs.return_value = [mock_job] | |
| self.mock_repo.get_workflow_run.return_value = mock_workflow_run | |
| mock_requests.side_effect = requests.RequestException("Network error") | |
| logs = bot.get_build_logs() | |
| self.assertEqual(len(logs), 1) | |
| self.assertEqual(logs[0]["job_name"], "test-job") | |
| self.assertEqual(logs[0]["logs"], "") |
🤖 Prompt for AI Agents
In `@openwisp_utils/tests/test_ci_failure_bot.py` around lines 340 - 356, Update
the test_get_build_logs_request_exception to match get_build_logs behavior: when
requests.get raises RequestException the bot catches it and appends a job_entry
with empty logs, so assert that logs equals [{"job_name": "test-job", "logs":
""}] (or construct an equivalent dict); locate the CIFailureBot.get_build_logs
behavior and the test_get_build_logs_request_exception function to change the
expected value accordingly, referencing the job_entry keys "job_name" and
"logs".
d6ce5b6 to
c975a09
Compare
|
@coderabbitai , Please point out only the currently uncovered branches/lines in openwisp_utils/bots/ci_failure/bot.py that are responsible for the coverage drop, and suggest the minimal set of unit tests required to raise overall coverage back above the project threshold (~97%). Avoid suggesting redundant tests. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 4487
Uncovered Critical Branches
Minimal Test Set (6 Tests)Add these to `@patch.dict`(os.environ, BASE_ENV)
`@patch`('openwisp_utils.bots.ci_failure.bot.Github')
`@patch`('openwisp_utils.bots.ci_failure.bot.subprocess.run')
def test_get_pr_diff_git_nonzero_returncode(self, mock_subprocess, mock_github_class):
"""Test git diff returns non-zero exit code (Line 150)"""
mock_github = MagicMock()
mock_github_class.return_value = mock_github
mock_repo = MagicMock()
mock_github.get_repo.return_value = mock_repo
mock_repo.default_branch = 'main'
mock_pr = MagicMock()
mock_pr.title = 'Test'
mock_pr.body = 'Desc'
mock_repo.get_pull.return_value = mock_pr
mock_subprocess.return_value.returncode = 1
mock_subprocess.return_value.stdout = "some output"
bot = CIFailureBot()
diff = bot.get_pr_diff()
self.assertIsNone(diff)
`@patch.dict`(os.environ, BASE_ENV)
`@patch`('openwisp_utils.bots.ci_failure.bot.Github')
`@patch`('openwisp_utils.bots.ci_failure.bot.requests.get')
def test_get_build_logs_bad_zip_file(self, mock_requests, mock_github_class):
"""Test BadZipFile exception handling (Line 101)"""
mock_github = MagicMock()
mock_github_class.return_value = mock_github
mock_repo = MagicMock()
mock_github.get_repo.return_value = mock_repo
mock_run = MagicMock()
mock_run.status = 'completed'
mock_run.conclusion = 'failure'
mock_repo.get_workflow_run.return_value = mock_run
mock_job = MagicMock()
mock_job.name = 'test-job'
mock_job.conclusion = 'failure'
mock_job.logs_url = 'https://api.github.com/logs/123'
mock_job.steps = []
mock_run.jobs.return_value = [mock_job]
# Return PK header but corrupted ZIP data
mock_response = MagicMock()
mock_response.content = b"PK\x03\x04corrupted_data"
mock_requests.return_value = mock_response
bot = CIFailureBot()
logs = bot.get_build_logs()
self.assertEqual(len(logs), 1)
self.assertEqual(logs[0]['logs'], '')
`@patch.dict`(os.environ, BASE_ENV)
`@patch`('openwisp_utils.bots.ci_failure.bot.Github')
def test_get_pr_diff_invalid_pr_number_value_error(self, mock_github_class):
"""Test ValueError for invalid PR number (Lines 131, 372)"""
mock_github = MagicMock()
mock_github_class.return_value = mock_github
mock_repo = MagicMock()
mock_github.get_repo.return_value = mock_repo
bot = CIFailureBot()
bot.pr_number = "invalid_string"
# Test get_pr_diff path
diff = bot.get_pr_diff()
self.assertIsNone(diff)
# Test post_comment path
bot.post_comment("Test message")
`@patch.dict`(os.environ, {"GITHUB_TOKEN": "token", "REPOSITORY": "repo"})
`@patch`('openwisp_utils.bots.ci_failure.bot.Github')
def test_init_github_exception(self, mock_github_class):
"""Test GitHub initialization exception (Line 30)"""
from github import GithubException
mock_github_class.side_effect = GithubException(401, "Unauthorized")
bot = CIFailureBot()
self.assertIsNone(bot.github)
self.assertIsNone(bot.repo)
`@patch.dict`(os.environ, {"GITHUB_TOKEN": "token", "REPOSITORY": "repo", "GEMINI_API_KEY": "key"})
`@patch`('openwisp_utils.bots.ci_failure.bot.Github')
`@patch`('openwisp_utils.bots.ci_failure.bot.genai')
def test_init_gemini_exception(self, mock_genai, mock_github_class):
"""Test Gemini initialization exception (Line 45)"""
mock_genai.configure.side_effect = Exception("API key invalid")
bot = CIFailureBot()
self.assertIsNone(bot.model)
`@patch.dict`(os.environ, BASE_ENV)
`@patch`('openwisp_utils.bots.ci_failure.bot.Github')
def test_analyze_with_gemini_exception_fallback(self, mock_github_class):
"""Test exception in analyze_with_gemini falls back (Line 338)"""
mock_github = MagicMock()
mock_github_class.return_value = mock_github
mock_repo = MagicMock()
mock_github.get_repo.return_value = mock_repo
bot = CIFailureBot()
bot.model = MagicMock()
bot.model.generate_content.side_effect = Exception("API error")
result = bot.analyze_with_gemini([], None)
# Should return fallback response
self.assertIn("OpenWISP QA workflow", result)Why these 6 tests:
These tests directly target the largest uncovered exception handlers and conditional branches, avoiding redundant coverage of already-tested paths. Should restore coverage to ~97%. |
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
c975a09 to
a51dc2f
Compare
|
@coderabbitai Please list the exact uncovered line numbers in openwisp_utils/bots/ci_failure/bot.py based on current coverage, and propose the minimal set of tests that would raise project coverage above the threshold. Avoid redundant or already-covered branches. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2001 🏁 Script executed: Length of output: 2081 🏁 Script executed: Length of output: 1333 🏁 Script executed: Length of output: 533 🏁 Script executed: Length of output: 5038 🏁 Script executed: Length of output: 2643
Uncovered Lines (7 Exception Handlers)
These 7 exception handlers (~14-20 lines) account for the 15% coverage drop. Minimal Test Set (7 Tests)Add these to def test_get_build_logs_github_exception(self):
"""Test GithubException in get_build_logs (lines 121-122)"""
from github import GithubException
bot = CIFailureBot()
self.mock_repo.get_workflow_run.side_effect = GithubException(404, "Not found")
logs = bot.get_build_logs()
self.assertEqual(logs, [])
def test_get_pr_diff_github_exception(self):
"""Test GithubException when fetching PR in get_pr_diff (lines 136-137)"""
from github import GithubException
bot = CIFailureBot()
self.mock_repo.get_pull.side_effect = GithubException(404, "PR not found")
diff = bot.get_pr_diff()
self.assertIsNone(diff)
def test_post_comment_github_exception_fetch_pr(self):
"""Test GithubException when fetching PR in post_comment (lines 377-378)"""
from github import GithubException
bot = CIFailureBot()
self.mock_repo.get_pull.side_effect = GithubException(403, "Forbidden")
bot.post_comment("Test message")
# Should handle gracefully without crashing
def test_post_comment_github_exception_get_comments(self):
"""Test GithubException when getting comments (lines 387-388)"""
from github import GithubException
bot = CIFailureBot()
mock_pr = Mock()
self.mock_repo.get_pull.return_value = mock_pr
mock_pr.get_issue_comments.side_effect = GithubException(500, "Server error")
bot.post_comment("Test message")
# Should still attempt to create comment
mock_pr.create_issue_comment.assert_called_once()
def test_post_comment_github_exception_create_comment(self):
"""Test GithubException when creating comment (lines 392-393)"""
from github import GithubException
bot = CIFailureBot()
mock_pr = Mock()
self.mock_repo.get_pull.return_value = mock_pr
mock_pr.get_issue_comments.return_value = []
mock_pr.create_issue_comment.side_effect = GithubException(403, "Forbidden")
bot.post_comment("Test message")
# Should handle gracefully
def test_run_github_exception_fork_check(self):
"""Test GithubException in fork check in run() (lines 422-423)"""
from github import GithubException
bot = CIFailureBot()
mock_run = Mock()
mock_run.actor.login = "user"
self.mock_repo.get_workflow_run.return_value = mock_run
self.mock_repo.get_pull.side_effect = GithubException(404, "Not found")
bot.run()
# Should handle gracefully and continue
def test_run_github_exception_actor_check(self):
"""Test GithubException in actor check in run() (lines 424-425)"""
from github import GithubException
bot = CIFailureBot()
self.mock_repo.get_workflow_run.side_effect = GithubException(401, "Unauthorized")
bot.run()
# Should handle gracefully and continueThese 7 tests directly target the uncovered exception handlers and should restore coverage to ~97%. |
|
@coderabbitai please use the latest Coveralls report for this PR and list the exact uncovered line numbers in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1772 🏁 Script executed: Length of output: 2796
Uncovered Exception Handlers (8 lines causing the coverage drop)
Minimal Test Set (8 Tests)Add these to def test_get_build_logs_github_exception(self):
"""Test GithubException in get_build_logs (lines 121-122)"""
from github import GithubException
bot = CIFailureBot()
self.mock_repo.get_workflow_run.side_effect = GithubException(404, "Not found")
logs = bot.get_build_logs()
self.assertEqual(logs, [])
def test_get_pr_diff_github_exception(self):
"""Test GithubException in get_pr_diff (lines 136-137)"""
from github import GithubException
bot = CIFailureBot()
self.mock_repo.get_pull.side_effect = GithubException(404, "PR not found")
diff = bot.get_pr_diff()
self.assertIsNone(diff)
def test_post_comment_invalid_pr_number(self):
"""Test ValueError for invalid PR number in post_comment (line 372)"""
bot = CIFailureBot()
bot.pr_number = "not_a_number"
# Should handle ValueError gracefully
bot.post_comment("Test message")
def test_post_comment_github_exception_fetch_pr(self):
"""Test GithubException when fetching PR in post_comment (lines 377-378)"""
from github import GithubException
bot = CIFailureBot()
self.mock_repo.get_pull.side_effect = GithubException(403, "Forbidden")
bot.post_comment("Test message")
def test_post_comment_github_exception_get_comments(self):
"""Test GithubException when getting comments (lines 387-388)"""
from github import GithubException
bot = CIFailureBot()
mock_pr = Mock()
self.mock_repo.get_pull.return_value = mock_pr
mock_pr.get_issue_comments.side_effect = GithubException(500, "Server error")
bot.post_comment("Test message")
# Should still attempt to create comment
mock_pr.create_issue_comment.assert_called_once()
def test_post_comment_github_exception_create_comment(self):
"""Test GithubException when creating comment (lines 392-393)"""
from github import GithubException
bot = CIFailureBot()
mock_pr = Mock()
self.mock_repo.get_pull.return_value = mock_pr
mock_pr.get_issue_comments.return_value = []
mock_pr.create_issue_comment.side_effect = GithubException(403, "Forbidden")
bot.post_comment("Test message")
def test_run_github_exception_fork_check(self):
"""Test GithubException in fork check (lines 422-423)"""
from github import GithubException
bot = CIFailureBot()
mock_run = Mock()
mock_run.actor.login = "user"
self.mock_repo.get_workflow_run.return_value = mock_run
self.mock_repo.get_pull.side_effect = GithubException(404, "Not found")
bot.run()
def test_run_github_exception_actor_check(self):
"""Test GithubException in actor check (lines 424-425)"""
from github import GithubException
bot = CIFailureBot()
self.mock_repo.get_workflow_run.side_effect = GithubException(401, "Unauthorized")
bot.run()These 8 tests directly target the 8 uncovered exception handlers and should restore coverage to ~97% with minimal changes. No redundant tests, only what's needed to cover the missing lines. |
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
a51dc2f to
ff40f0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@openwisp_utils/bots/ci_failure/bot.py`:
- Line 9: The unconditional top-level import "import google.generativeai as
genai" causes ImportError when the Gemini package is missing; change it to a
lazy/conditional import (e.g., import inside the initializer or a helper) and
ensure all references to genai are guarded in __init__ so the bot only attempts
to import/initialize google.generativeai when GEMINI_API_KEY is present; update
usage sites in the class (references to genai) to either use the lazily imported
module or skip Gemini setup when the import fails, raising/logging a clear
message if needed.
In `@openwisp_utils/tests/test_ci_failure_bot.py`:
- Around line 162-181: The test_analyze_with_gemini_success uses a conditional
if bot.model: ... else: ... which weakens the test; remove the conditional and
assert the expected success behavior unconditionally (i.e., assert the result
contains "PEP 8" and "pip install" and that
self.mock_model.generate_content.assert_called_once()), ensuring the test relies
on the setUp patch of genai.GenerativeModel that provides self.mock_model and
that CIFailureBot().model is truthy; update the test body in
test_analyze_with_gemini_success to directly perform these assertions against
CIFailureBot and mock_model.generate_content.
- Around line 183-189: The test test_analyze_with_gemini_api_error doesn't
guarantee the Gemini model path is exercised because it conditionally sets
bot.model.generate_content.side_effect only if bot.model is truthy; always force
a model mock to simulate the API error regardless of CIFailureBot internals by
ensuring bot.model is a mock (or replacing it) and then setting
bot.model.generate_content.side_effect = Exception("API Error") before calling
analyze_with_gemini; update the test to explicitly assign a mock to bot.model
(or assert/skip if model is absent) so the exception-handling branch in
CIFailureBot.analyze_with_gemini is actually executed and the assertions for
fallback text remain meaningful.
🧹 Nitpick comments (5)
openwisp_utils/bots/ci_failure/bot.py (4)
401-455:self.repocan beNoneat line 411 — the control flow relies on exception catching for normal logic.When
self.repoisNone(e.g., missingGITHUB_TOKEN), line 411 (self.repo.get_workflow_run(...)) raisesAttributeError, caught by the outerexcepton line 430. While this doesn't crash the bot, using exception handling for expected control flow is fragile and makes the logic hard to follow.♻️ Suggested guard
try: - if self.workflow_run_id: + if self.workflow_run_id and self.repo: workflow_run = self.repo.get_workflow_run(int(self.workflow_run_id))
200-205: Minor: prefernext(iter(…))overlist(…)[0]for single-element set extraction.♻️ Suggested fix
if not failure_types: return "unknown" elif len(failure_types) == 1: - return list(failure_types)[0] + return next(iter(failure_types)) else: return "mixed"
237-340: Very large inline prompt string — consider extracting to a constant or template file.The ~100-line Gemini prompt embedded in
analyze_with_geminidominates the method and makes it hard to review the actual logic. Extracting it to a module-level constant (e.g.,GEMINI_PROMPT_TEMPLATE) or a separate file would improve readability and make prompt tuning easier without touching method logic.
85-101: ZIP log extraction reads all.txtfiles but doesn't limit total size before concatenation.If the ZIP contains many large
.txtfiles, the concatenation on line 93 could consume significant memory before the truncation on line 96 kicks in. Consider tracking cumulative size during extraction.openwisp_utils/tests/test_ci_failure_bot.py (1)
442-448:test_get_pr_diff_invalid_pr_number_value_erroralso testspost_comment— split or rename.Line 448 calls
bot.post_comment("Test message")with the invalid PR number, which is unrelated to validatingget_pr_diff. This makes the test name misleading and conflates two test concerns.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/ci-failure-bot.ymldocs/developer/reusable-github-utils.rstopenwisp_utils/bots/__init__.pyopenwisp_utils/bots/ci_failure/__init__.pyopenwisp_utils/bots/ci_failure/bot.pyopenwisp_utils/tests/test_ci_failure_bot.pytests/test_project/tests/test_test_utils.py
✅ Files skipped from review due to trivial changes (1)
- docs/developer/reusable-github-utils.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_utils/bots/ci_failure/init.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_utils/tests/test_ci_failure_bot.py (1)
openwisp_utils/bots/ci_failure/bot.py (7)
classify_failure(171-205)get_build_logs(52-126)get_pr_diff(128-169)analyze_with_gemini(218-346)post_comment(364-399)run(401-455)get_failed_jobs_summary(207-216)
🪛 Ruff (0.14.14)
openwisp_utils/bots/ci_failure/bot.py
[warning] 30-30: Do not catch blind exception: Exception
(BLE001)
[warning] 45-45: Do not catch blind exception: Exception
(BLE001)
[warning] 123-123: Consider moving this statement to an else block
(TRY300)
[error] 146-146: subprocess call: check for execution of untrusted input
(S603)
[error] 147-147: Starting a process with a partial executable path
(S607)
[warning] 203-203: Prefer next(iter(failure_types)) over single element slice
Replace with next(iter(failure_types))
(RUF015)
[warning] 344-344: Do not catch blind exception: Exception
(BLE001)
[warning] 447-447: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/tests/test_ci_failure_bot.py
[warning] 469-469: Unused method argument: mock_github_class
(ARG002)
[warning] 492-492: Unused method argument: mock_github_class
(ARG002)
[warning] 509-509: Unused method argument: mock_github_class
(ARG002)
[warning] 563-563: Unused method argument: mock_requests
(ARG002)
🔇 Additional comments (6)
openwisp_utils/bots/__init__.py (1)
1-1: LGTM!Package initializer is minimal and appropriate.
.github/workflows/ci-failure-bot.yml (1)
15-18: Theifcondition duplicates the trigger filter — consider simplifying.The
workflow_runtrigger on lines 4-8 already fires only oncompleted, and theifon line 18 re-checksconclusion == 'failure'and the dependabot exclusion. While theifis needed (the trigger doesn't filter onconclusion), the dependabot check is also duplicated in the Python bot'srun()method. This is defensive and acceptable, just noting the redundancy.openwisp_utils/tests/test_ci_failure_bot.py (3)
6-9: Good defensive import guard for the optional dependency.The
try/except ImportErrorpattern correctly handles the case where the bot's dependencies aren't installed, allowing the test suite to skip gracefully.
13-40: Well-structured setUp/tearDown with proper patcher lifecycle management.The pattern of starting patchers in
setUpand conditionally stopping them intearDown(viahasattrchecks) is solid and prevents leaking mocks between tests.
285-319: Thorough ZIP-format log test — good coverage of thePKmagic-byte detection path.This test exercises an important edge case with a real in-memory ZIP file. The assertions properly validate both the log content extraction and step-level failure info.
tests/test_project/tests/test_test_utils.py (1)
101-101: Loosened retry count assertions — verify this isn't masking a regression.Changing from exact count assertions (
assertEqual(..., 4)) to minimum bounds (assertGreaterEqual(..., 3)) is reasonable for accommodating urllib3 retry behavior variations across versions, but confirm that the retry configuration inretryable_requeststill defaults to the expected number of retries. If this change was made solely to fix a flaky test, it's worth documenting why.#!/bin/bash # Check the default retry count in retryable_request rg -n "def retryable_request" --type py -A 20 | head -30Also applies to: 115-115
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
2 times, most recently
from
fee7333 to
4b408bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openwisp_utils/bots/ci_failure/bot.py`:
- Around line 222-232: The analyze_with_gemini method currently uses
self.repository_name.split("/") without checking for None; update
analyze_with_gemini to defensively handle a missing repository_name by checking
if self.repository_name is truthy before splitting and, if not, returning
self.fallback_response() (or raising a clear ValueError) with an explanatory
message; reference the analyze_with_gemini function and the repository_name
attribute so the fix is applied where project_name and repo_url are computed.
In `@tests/openwisp2/settings.py`:
- Line 33: Remove the invalid app entry "openwisp_utils.bots" from
INSTALLED_APPS in the tests settings or convert the package into a proper Django
app: either delete the "openwisp_utils.bots" entry from the INSTALLED_APPS list,
or add an apps.py defining a subclass of django.apps.AppConfig (e.g., class
BotsConfig) in the openwisp_utils/bots package and reference its dotted path (or
set default_app_config in the package __init__.py) so that INSTALLED_APPS points
to a valid AppConfig; locate the INSTALLED_APPS list and the string
"openwisp_utils.bots" to apply the change.
🧹 Nitpick comments (7)
openwisp_utils/bots/ci_failure/tests/test_bot.py (3)
79-89:clear=Truemay cause the Gemini mock assertion to be fragile.The
patch.dict(os.environ, env_vars_no_gemini, clear=True)context on line 86 clears all env vars, including any that the outersetUppatcher added. This is intentional here to simulate a missingGEMINI_API_KEY. However,self.mock_genai.configure.assert_not_called()on line 89 relies on the fact thatsetUpdoesn't itself triggergenai.configure— which is true today sincesetUpdoesn't instantiateCIFailureBot. If that ever changes, this assertion would break for unclear reasons.A small improvement would be to reset the mock before the assertion or scope the check within the
withblock:self.mock_genai.reset_mock() with patch.dict(os.environ, env_vars_no_gemini, clear=True): bot = CIFailureBot() self.assertIsNone(bot.model) self.mock_genai.configure.assert_not_called()
271-276: Test asserts onprint()output which is a fragile coupling to implementation detail.Multiple tests (
test_run_skips_dependabot,test_run_skips_fork_pr,test_run_skips_deleted_fork_pr) verify behavior by asserting specificprint()strings. If the log messages change even slightly, these tests break. Consider using Python'sloggingmodule instead ofprint()in the bot (which would also be better practice for a library), and then asserting on log records — or at minimum, assert on observable behavior (e.g.,create_issue_comment.assert_not_called()) rather than exact print strings.The fork/deleted-fork tests already do check
mock_pr.create_issue_comment.assert_not_called(), which is good. The dependabot test on line 276 does not, so it relies solely on the print assertion.
278-292:test_run_full_workflowdoesn't setself.mock_repo.full_name, relying on Mock auto-attribute.In
bot.py'srun()method, the fork check comparespr.head.repo.full_name != self.repository_name. The_mock_pr()helper defaultsfull_nameto"openwisp/openwisp-utils"which matches the env var, so the test passes. However,self.mock_repo.full_nameis never explicitly set in this test (unliketest_run_skips_fork_prwhich sets it on line 298). While this works because the bot compares againstself.repository_name(the env var string), it would be clearer to setself.mock_repo.full_nameexplicitly for consistency.openwisp_utils/bots/ci_failure/bot.py (4)
206-207: Prefernext(iter(failure_types))overlist(...)[0].Minor efficiency and clarity improvement flagged by Ruff (RUF015).
next(iter(failure_types))avoids allocating an intermediate list.♻️ Proposed fix
elif len(failure_types) == 1: - return list(failure_types)[0] + return next(iter(failure_types))
405-459:run()has deeply nested try/except blocks — consider restructuring for clarity.The
run()method has three levels of nested exception handling (lines 410, 413, 423) plus a top-level catch-all on line 451. The control flow throughshould_skip,skip_reason, earlyreturnon line 442, and the post-trycomment logic (lines 455–458) is hard to follow.A few specific concerns:
Line 415:
self.repo.get_workflow_run(...)will raiseAttributeErrorifself.repoisNone. This is caught by the outer except on line 434, but silently swallowed as a "could not check actor" warning — hiding a more fundamental configuration problem.Line 442:
returnexits before printing "CI Failure Bot completed successfully" (line 459), creating inconsistent end-of-run logging.Lines 447–449: When both
build_logsandpr_diffare empty, the bot prints a message but still callsanalyze_with_gemini, which will hit the Gemini API (or fallback) with no useful context.♻️ Suggested improvement for concern `#3`
if not build_logs and not pr_diff: print("No build logs or PR diff found, using fallback analysis") - print("Analyzing failure with Gemini AI...") - message = self.analyze_with_gemini(build_logs, pr_diff) + message = self.fallback_response() + else: + print("Analyzing failure with Gemini AI...") + message = self.analyze_with_gemini(build_logs, pr_diff)
368-403:post_commentidentifies existing bot comments by marker string only, not by author.Line 393 checks
if marker in comment.bodyto find an existing bot comment to update. This means if any user or bot copies the marker (<!-- ci-failure-bot-comment -->) into a comment, the bot will silently edit their comment instead of creating a new one. While the marker is obscure enough that accidental inclusion is unlikely, adding an author check (comment.user.login) would make this more robust.Additionally, the method calls
self.repo.get_pull()and parsesself.pr_numberagain — duplicating logic already performed inrun(). Consider passing the PR object directly.
241-344: The Gemini prompt embeds full build logs and PR diffs — be mindful of token limits and cost.
build_logs_jsoncan contain up to ~5000 chars per job, andpr_diff_jsonup to ~8000 chars, plus the lengthy prompt template itself. For workflows with multiple failed jobs, the total context could exceed model token limits (especially for older Gemini models). There's no check on the total prompt size before sending to the API.Consider adding a total size cap or summarizing when the combined context exceeds a threshold.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.coveragerc.github/workflows/ci-failure-bot.ymldocs/developer/reusable-github-utils.rstopenwisp_utils/bots/__init__.pyopenwisp_utils/bots/ci_failure/__init__.pyopenwisp_utils/bots/ci_failure/bot.pyopenwisp_utils/bots/ci_failure/tests/__init__.pyopenwisp_utils/bots/ci_failure/tests/test_bot.pytests/openwisp2/settings.pytests/test_project/tests/test_test_utils.py
✅ Files skipped from review due to trivial changes (2)
- docs/developer/reusable-github-utils.rst
- openwisp_utils/bots/ci_failure/tests/init.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci-failure-bot.yml
- openwisp_utils/bots/init.py
- .coveragerc
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_utils/bots/ci_failure/tests/test_bot.py (1)
openwisp_utils/bots/ci_failure/bot.py (6)
CIFailureBot(18-459)classify_failure(175-209)get_build_logs(56-130)get_pr_diff(132-173)analyze_with_gemini(222-350)run(405-459)
🪛 Ruff (0.14.14)
openwisp_utils/bots/ci_failure/bot.py
[warning] 34-34: Do not catch blind exception: Exception
(BLE001)
[warning] 49-49: Do not catch blind exception: Exception
(BLE001)
[warning] 127-127: Consider moving this statement to an else block
(TRY300)
[error] 150-150: subprocess call: check for execution of untrusted input
(S603)
[error] 151-151: Starting a process with a partial executable path
(S607)
[warning] 207-207: Prefer next(iter(failure_types)) over single element slice
Replace with next(iter(failure_types))
(RUF015)
[warning] 348-348: Do not catch blind exception: Exception
(BLE001)
[warning] 451-451: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
🔇 Additional comments (4)
openwisp_utils/bots/ci_failure/__init__.py (1)
1-1: LGTM!Minimal
__init__.pyfor package namespace — no concerns.tests/test_project/tests/test_test_utils.py (1)
101-101: Relaxing exact retry count assertions — reasonable but verify the motivation.Changing from
assertEqual(..., 4)toassertGreaterEqual(..., 3)makes the tests less brittle against minor retry-count variations. However, this also means the tests won't catch regressions where retries silently decrease.Is this change needed because adding
openwisp_utils.botstoINSTALLED_APPSsomehow altered the mock call count, or was it to fix pre-existing flakiness? If the former, that would be worth understanding further.Also applies to: 115-115
openwisp_utils/bots/ci_failure/bot.py (2)
12-15: LGTM — conditional import forgoogle.generativeaiproperly addresses the "Gemini is optional" requirement.The
try/except ImportErrorpattern with aNonefallback is the standard approach. This was correctly addressed from a prior review.
56-130: Build log retrieval is well-structured with good error handling.The ZIP detection via magic bytes, per-job log truncation, step-level failure extraction, and graceful handling of missing
logs_urlare all solid. Thegetattr(job, "steps", [])on line 116 is a good defensive pattern.One minor note: only
.txtfiles are extracted from ZIP archives (line 93). If GitHub ever changes the log file extension, logs would silently be lost. Consider logging when no.txtfiles are found in the ZIP.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
4b408bd to
d14bd9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openwisp_utils/bots/ci_failure/bot.py`:
- Around line 44-54: The current initializer calls genai.configure and
genai.GenerativeModel when self.gemini_api_key is set but doesn't check whether
genai (the google-generativeai import) is None, causing an AttributeError to be
caught and conflated with real configuration errors; update the block that
checks self.gemini_api_key to first assert genai is not None (e.g., if
self.gemini_api_key and genai is not None) and if genai is None log a clear
warning and set self.model = None, otherwise proceed to call
genai.configure(...) and genai.GenerativeModel(self.model_name) and catch only
exceptions from those calls (or rephrase the except to preserve original
exception info) so missing package vs runtime config errors are distinguishable
(refer to genai, genai.configure, genai.GenerativeModel, self.gemini_api_key,
self.model).
In `@tests/test_project/tests/test_test_utils.py`:
- Line 101: The test weakened the assertion on mocked__get_conn call count from
an exact check (4) to a lower-bound (>=3), which hides under-retry or
infinite-retry bugs; change the assertion to explicitly allow the known
acceptable counts (e.g., assert that len(mocked__get_conn.mock_calls) is in
{3,4}) so the test still tolerates the observed flakiness but preserves
verification of retry behavior, and while updating the test also inspect the
retryable_request default retry configuration in the retryable_request function
to understand why the call count can be 3 vs 4.
🧹 Nitpick comments (7)
.coveragerc (1)
7-10: Test files underopenwisp_utils/are no longer omitted from coverage.The old config omitted
*/test*paths. Now that the bot's test suite lives insideopenwisp_utils/bots/ci_failure/tests/, removing that omit pattern means test files themselves inflate the reported coverage percentage. Typically test code should be excluded from the coverage metric.Consider restoring a test omit pattern:
Proposed fix
[report] omit = */migrations/* */__init__.py + */tests/*.github/workflows/ci-failure-bot.yml (1)
1-18: Workflow triggers on all failures, not just PR-related ones.The
workflow_runtrigger fires for any run of "OpenWISP Utils CI Build" that fails, including branch pushes without PRs. The bot handles this gracefully (no PR → no comment), but the job still spins up a runner, checks out code, and installs dependencies for nothing. Consider adding an additional condition to short-circuit when no PR is associated:ci-failure-bot: runs-on: ubuntu-latest - if: ${{ github.event.workflow_run.conclusion == 'failure' && !contains(github.event.workflow_run.actor.login, 'dependabot') }} + if: ${{ github.event.workflow_run.conclusion == 'failure' && !contains(github.event.workflow_run.actor.login, 'dependabot') && github.event.workflow_run.pull_requests[0] }}This avoids wasting CI minutes on non-PR failures.
openwisp_utils/bots/ci_failure/tests/test_bot.py (3)
78-88: Missing test:genaipackage not installed +GEMINI_API_KEYpresent.When the
google-generativeaiimport fails,genaiisNone. IfGEMINI_API_KEYis set,bot.pyline 46 callsgenai.configure(...)which hitsAttributeErrorcaught by the broadexcept Exception. This path isn't tested. Consider adding:def test_init_genai_import_failed(self): """When google-generativeai is not installed but key is provided.""" with patch("openwisp_utils.bots.ci_failure.bot.genai", None): bot = CIFailureBot() self.assertIsNone(bot.model)
270-275: Test relies on short-circuit order inrun()for correctness.This test doesn't call
_mock_pr(), soself.mock_repo.get_pull()returns a bareMock. The test only passes because the dependabot check short-circuits before the fork check. If the order inrun()is ever swapped, this test would break or produce confusing results. Consider addingself._mock_pr()for robustness.
183-197:default_branchnot set on mock repo.
get_pr_diffcallsself.repo.default_branchin the git diff command. Unliketest_get_pr_diff_error(line 209) which explicitly setsself.mock_repo.default_branch = "main", this test leaves it as an auto-createdMockattribute. The test still passes becausesubprocess.runis mocked, but the git command argument would contain a stringified Mock object. Setdefault_branchexplicitly for a more realistic test.openwisp_utils/bots/ci_failure/bot.py (2)
204-209: Prefernext(iter(failure_types))overlist(...)[0].When
failure_typeshas exactly one element,next(iter(failure_types))avoids creating a temporary list.Proposed fix
- elif len(failure_types) == 1: - return list(failure_types)[0] + elif len(failure_types) == 1: + return next(iter(failure_types))
405-459:run()prints "completed successfully" even after error/fallback paths.Line 459 always executes regardless of whether the bot hit the
except Exceptionon line 451 and used a fallback. This makes log inspection misleading. Consider differentiating success from fallback/error completion.Proposed fix
if message: self.post_comment(message) else: print("No PR context available, no comment posted (expected)") - print("CI Failure Bot completed successfully") + print("CI Failure Bot completed")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.coveragerc.github/workflows/ci-failure-bot.ymldocs/developer/reusable-github-utils.rstopenwisp_utils/bots/__init__.pyopenwisp_utils/bots/ci_failure/__init__.pyopenwisp_utils/bots/ci_failure/bot.pyopenwisp_utils/bots/ci_failure/tests/__init__.pyopenwisp_utils/bots/ci_failure/tests/test_bot.pytests/openwisp2/settings.pytests/test_project/tests/test_test_utils.py
✅ Files skipped from review due to trivial changes (1)
- openwisp_utils/bots/init.py
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/developer/reusable-github-utils.rst
- openwisp_utils/bots/ci_failure/init.py
- tests/openwisp2/settings.py
- openwisp_utils/bots/ci_failure/tests/init.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_utils/bots/ci_failure/tests/test_bot.py (1)
openwisp_utils/bots/ci_failure/bot.py (6)
classify_failure(175-209)get_build_logs(56-130)get_pr_diff(132-173)analyze_with_gemini(222-350)post_comment(368-403)run(405-459)
🪛 Ruff (0.14.14)
openwisp_utils/bots/ci_failure/bot.py
[warning] 34-34: Do not catch blind exception: Exception
(BLE001)
[warning] 49-49: Do not catch blind exception: Exception
(BLE001)
[warning] 127-127: Consider moving this statement to an else block
(TRY300)
[error] 150-150: subprocess call: check for execution of untrusted input
(S603)
[error] 151-151: Starting a process with a partial executable path
(S607)
[warning] 207-207: Prefer next(iter(failure_types)) over single element slice
Replace with next(iter(failure_types))
(RUF015)
[warning] 348-348: Do not catch blind exception: Exception
(BLE001)
[warning] 451-451: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
🔇 Additional comments (8)
openwisp_utils/bots/ci_failure/tests/test_bot.py (3)
8-34: LGTM — setUp/tearDown structure is solid.The defensive
hasattrchecks intearDowncorrectly handle partialsetUpfailures. Environment patching and mock setup are clean.
90-118: Classification tests are clear and cover the key categories well.Each failure type (qa, tests, setup, mixed, unknown) has a dedicated test with appropriate input data.
141-166: Good coverage of ZIP-encoded log extraction.Creating a real in-memory ZIP file makes this test realistic and exercises the actual decompression path.
openwisp_utils/bots/ci_failure/bot.py (4)
12-15: Conditional import ofgoogle.generativeai— good.Gracefully handles the missing package case with a
try/except ImportErrorand setsgenai = None.
56-130: Build log retrieval is well-structured with proper error handling.Guards for
self.repoandself.workflow_run_id, handles both ZIP and plain-text logs, truncates long output, and collects step-level failure details. Thegetattr(job, "steps", [])on line 116 is a nice defensive pattern.
149-156: Subprocess security: list-formsubprocess.runis safe here.Ruff flags S603/S607, but since
subprocess.runis invoked with a list (notshell=True),self.repo.default_branchfrom the GitHub API cannot cause command injection. The flags are false positives in this context.
368-403: Comment deduplication logic is correct and clean.The HTML marker approach (
<!-- ci-failure-bot-comment -->) ensures only one bot comment exists per PR, and updating vs. creating is handled properly with separate try/except blocks for each GitHub API call..github/workflows/ci-failure-bot.yml (1)
22-22: No action needed.actions/checkout@v6is the current latest major version as of February 2026, confirmed by the official repository and GitHub Marketplace documentation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
d14bd9f to
2e1527b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_utils/bots/ci_failure/bot.py`:
- Around line 390-403: When get_issue_comments() raises GithubException while
checking for existing bot comments, the code currently logs the error and falls
through to pr.create_issue_comment(), risking duplicate posts; change the error
handling in the block that calls pr.get_issue_comments() so that on
GithubException you either re-raise or return immediately (i.e., do not proceed
to pr.create_issue_comment()), ensuring the deduplication check around marker
and comment.edit(message_with_marker) is respected; update the try/except around
get_issue_comments() (referencing get_issue_comments, GithubException, marker,
message_with_marker, pr, pr_num, and create_issue_comment) to stop execution
after the listing failure or explicitly document the intent if you purposely
want to post when listing fails.
🧹 Nitpick comments (5)
openwisp_utils/bots/ci_failure/bot.py (3)
447-450: Misleading log message: "using fallback analysis" but Gemini is still invoked.Line 448 prints "No build logs or PR diff found, using fallback analysis", but execution continues to line 450 which calls
analyze_with_gemini(potentially invoking the Gemini API with empty context). Either callself.fallback_response()directly when there's no data, or reword the message to "No build logs or PR diff found".Proposed fix — skip Gemini when there's nothing to analyze
if not build_logs and not pr_diff: print("No build logs or PR diff found, using fallback analysis") - print("Analyzing failure with Gemini AI...") - message = self.analyze_with_gemini(build_logs, pr_diff) + message = self.fallback_response() + else: + print("Analyzing failure with Gemini AI...") + message = self.analyze_with_gemini(build_logs, pr_diff)
175-209: Minor: prefernext(iter(failure_types))overlist(failure_types)[0].Line 207 creates a temporary list just to get one element from the set.
next(iter(failure_types))is more idiomatic and avoids the allocation.- return list(failure_types)[0] + return next(iter(failure_types))
405-459:run()accessesself.repoat line 415 without an explicit guard.If
self.repoisNone(GitHub init failed),self.repo.get_workflow_run(...)raisesAttributeError, caught by theexcept (GithubException, AttributeError, ValueError)on line 434. This works but means anyAttributeErrorin those ~20 lines is silently swallowed—including potential programming errors. Consider adding an earlyif not self.repocheck before the skip-conditions block.openwisp_utils/bots/ci_failure/tests/test_bot.py (2)
277-291:test_run_full_workflowdoes not mocksubprocess.run, relying on it failing in the test environment.
bot.run()→get_pr_diff()→subprocess.run(["git", "diff", ...])is not mocked here. The test implicitly depends on the subprocess call failing (no git checkout), which makespr_diffsilently becomeNone. This is fragile—if tests ever run inside a git repo where the command succeeds, the behavior changes.Consider adding
@patch("openwisp_utils.bots.ci_failure.bot.subprocess.run")for determinism.
469-479: Themock_requestsparameter is unused — patch is unnecessary whenlogs_urlisNone.Since the mock job has
logs_url=None, therequests.getcall is never reached. The@patchdecorator and parameter are superfluous. Removing it simplifies the test.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.coveragerc.github/workflows/ci-failure-bot.ymldocs/developer/reusable-github-utils.rstopenwisp_utils/bots/__init__.pyopenwisp_utils/bots/ci_failure/__init__.pyopenwisp_utils/bots/ci_failure/bot.pyopenwisp_utils/bots/ci_failure/tests/__init__.pyopenwisp_utils/bots/ci_failure/tests/test_bot.pytests/openwisp2/settings.pytests/test_project/tests/test_test_utils.py
✅ Files skipped from review due to trivial changes (2)
- docs/developer/reusable-github-utils.rst
- tests/openwisp2/settings.py
🚧 Files skipped from review as they are similar to previous changes (3)
- openwisp_utils/bots/init.py
- .github/workflows/ci-failure-bot.yml
- tests/test_project/tests/test_test_utils.py
🧰 Additional context used
🪛 Ruff (0.14.14)
openwisp_utils/bots/ci_failure/bot.py
[warning] 34-34: Do not catch blind exception: Exception
(BLE001)
[warning] 49-49: Do not catch blind exception: Exception
(BLE001)
[warning] 127-127: Consider moving this statement to an else block
(TRY300)
[error] 150-150: subprocess call: check for execution of untrusted input
(S603)
[error] 151-151: Starting a process with a partial executable path
(S607)
[warning] 207-207: Prefer next(iter(failure_types)) over single element slice
Replace with next(iter(failure_types))
(RUF015)
[warning] 348-348: Do not catch blind exception: Exception
(BLE001)
[warning] 451-451: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/ci_failure/tests/test_bot.py
[warning] 470-470: Unused method argument: mock_requests
(ARG002)
[warning] 538-538: Unused method argument: mock_github_class
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (9)
openwisp_utils/bots/ci_failure/__init__.py (1)
1-1: LGTM!Package initializer is fine.
openwisp_utils/bots/ci_failure/bot.py (2)
56-130: LGTM —get_build_logshas proper guards and error handling.The
self.repoandself.workflow_run_idguards are in place. ZIP detection via magic bytes, log truncation, and per-step failure collection all look correct. Exception handling covers bothGithubExceptionandValueError.
132-173: LGTM —get_pr_diffhas layered error handling.Guards for
self.repo,self.pr_number, invalid PR number, GitHub fetch errors, and subprocess errors are all present. Diff truncation logic is clean.openwisp_utils/bots/ci_failure/tests/test_bot.py (5)
8-34: LGTM — setUp/tearDown properly manage patchers.The environment, GitHub, and Gemini mocks are cleanly started and stopped. The
hasattrguards intearDownare a good safety net.
68-77: LGTM — init success test verifies all env vars and mock interactions.Good coverage of the happy-path initialization.
90-118: LGTM — classification tests cover all failure types including edge cases.QA, tests, setup, mixed, and unknown are all exercised.
270-275: LGTM — dependabot skip test correctly asserts the skip message.
293-314: LGTM — fork PR and deleted fork skip tests verify both the print output and that no comment is posted..coveragerc (1)
7-10: The review comment is based on an incorrect premise. The.coveragercfile was newly created in commit 2e1527b, not refactored from an existing configuration. No*/test*omit pattern was dropped—it never existed in this file. The current omit configuration is the original design choice for this new file, not an accidental deletion. No action needed.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
2e1527b to
11e767e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openwisp_utils/bots/ci_failure/bot.py`:
- Around line 243-259: The prompt currently embeds untrusted data verbatim
(build_logs_json, failed_jobs_json, pr_diff_json) into the context f-string
which creates a prompt-injection risk; fix it by sanitizing and truncating those
inputs before json.dumps (escape or remove suspicious sequences like "###",
"SYSTEM:", model-instruction keywords, control chars, and limit to a safe max
length), and append a clear AI-generated disclaimer to the context string (e.g.,
mention that the analysis is automated and may be inaccurate) so that the posted
comment includes the disclaimer; update the variables build_logs_json,
pr_diff_json (and where context is assembled) to use the sanitized/truncated
values.
In `@openwisp_utils/bots/ci_failure/tests/test_bot.py`:
- Around line 384-388: The test's expected print message is wrong: update the
assertion in test_bot.py that calls bot.run() (the mock_print.assert_any_call)
to expect the exact string the bot prints ("No build logs or PR diff found,
using fallback response") to match the output from the Bot.run/print in bot.py,
or alternatively change the print in Bot.run to the phrase the test expects;
reference the bot.run() call and the print statement in bot.py to ensure both
strings match exactly.
🧹 Nitpick comments (4)
openwisp_utils/bots/ci_failure/bot.py (1)
156-168: Sanitizedefault_branchbefore passing to subprocess.
self.repo.default_branchis sourced from the GitHub API, which is generally trusted. However, as a defense-in-depth measure against injection via a compromised or malicious repo name, consider validating that the branch name contains only expected characters (alphanumeric, hyphens, slashes, dots) before interpolating it into the subprocess command.🛡️ Proposed validation
+ import re + default_branch = self.repo.default_branch + if not re.match(r'^[\w.\-/]+$', default_branch): + print(f"Suspicious default branch name: {default_branch}") + return None try: result = subprocess.run( - ["git", "diff", f"origin/{self.repo.default_branch}"], + ["git", "diff", f"origin/{default_branch}"], capture_output=True, text=True, timeout=30, check=False, )openwisp_utils/bots/ci_failure/tests/test_bot.py (3)
543-558: Double-patching ofGithub— consider usingself.mock_github.side_effectinstead.
setUpalready patchesopenwisp_utils.bots.ci_failure.bot.Github. This test adds a second@patchfor the same target, which works but is confusing and triggers the unused-argument lint warning. You can simplify by setting the side effect on the existing mock.♻️ Proposed simplification
- `@patch`("openwisp_utils.bots.ci_failure.bot.Github") - def test_init_github_exception(self, mock_github_class): + def test_init_github_exception(self): """Test GitHub initialization exception""" from github import GithubException - mock_github_class.side_effect = GithubException(401, "Unauthorized") + self.mock_github.side_effect = GithubException(401, "Unauthorized") env_vars = { "GITHUB_TOKEN": "token", "REPOSITORY": "repo",
560-574: Same double-patching pattern astest_init_github_exception.Both
Githubandgenaiare already patched insetUp. Prefer settingside_effectonself.mock_genaidirectly. Themock_github_classparameter is unused (Ruff ARG002).♻️ Proposed simplification
- `@patch`("openwisp_utils.bots.ci_failure.bot.genai") - `@patch`("openwisp_utils.bots.ci_failure.bot.Github") - def test_init_gemini_exception(self, mock_github_class, mock_genai): + def test_init_gemini_exception(self): """Test Gemini initialization exception""" - mock_genai.configure.side_effect = Exception("API key invalid") + self.mock_genai.configure.side_effect = Exception("API key invalid") env_vars = { "GITHUB_TOKEN": "token", "GEMINI_API_KEY": "key",
443-454: Consider adding explicit assertions to no-op tests.These tests verify that
post_commentdoesn't crash when prerequisites are missing, which is good. However, adding assertions (e.g.,mock_pr.create_issue_comment.assert_not_called()or checking print output) would make the test intent more explicit and guard against silent regressions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.coveragerc.github/workflows/ci-failure-bot.ymldocs/developer/reusable-github-utils.rstopenwisp_utils/bots/__init__.pyopenwisp_utils/bots/ci_failure/__init__.pyopenwisp_utils/bots/ci_failure/bot.pyopenwisp_utils/bots/ci_failure/tests/__init__.pyopenwisp_utils/bots/ci_failure/tests/test_bot.pytests/openwisp2/settings.pytests/test_project/tests/test_test_utils.py
✅ Files skipped from review due to trivial changes (1)
- docs/developer/reusable-github-utils.rst
🚧 Files skipped from review as they are similar to previous changes (6)
- openwisp_utils/bots/ci_failure/init.py
- .coveragerc
- openwisp_utils/bots/init.py
- tests/openwisp2/settings.py
- tests/test_project/tests/test_test_utils.py
- .github/workflows/ci-failure-bot.yml
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_utils/bots/ci_failure/tests/test_bot.py (1)
openwisp_utils/bots/ci_failure/bot.py (8)
CIFailureBot(18-478)classify_failure(182-216)get_build_logs(63-137)get_pr_diff(139-180)analyze_with_gemini(229-360)post_comment(378-414)run(416-478)get_failed_jobs_summary(218-227)
🪛 Ruff (0.14.14)
openwisp_utils/bots/ci_failure/bot.py
[warning] 34-34: Do not catch blind exception: Exception
(BLE001)
[warning] 49-49: Do not catch blind exception: Exception
(BLE001)
[warning] 134-134: Consider moving this statement to an else block
(TRY300)
[error] 157-157: subprocess call: check for execution of untrusted input
(S603)
[error] 158-158: Starting a process with a partial executable path
(S607)
[warning] 358-358: Do not catch blind exception: Exception
(BLE001)
[warning] 470-470: Do not catch blind exception: Exception
(BLE001)
openwisp_utils/bots/ci_failure/tests/test_bot.py
[warning] 562-562: Unused method argument: mock_github_class
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (10)
openwisp_utils/bots/ci_failure/bot.py (5)
19-61: LGTM!Initialization logic handles missing env vars, GitHub client failures, and Gemini unavailability gracefully. The conditional
genaiimport andNoneguards from past reviews are properly addressed.
63-137: LGTM!Build log retrieval with ZIP support, truncation, and step-level failure extraction is well-structured. The
Noneguards and error handling are solid.
182-216: LGTM!Keyword-based classification logic is straightforward and correctly aggregates multiple failure types into "mixed".
378-414: LGTM!Comment deduplication and error handling are solid. The fix to return early when listing fails (preventing duplicate comments) is correct.
416-478: LGTM!The orchestration flow correctly handles skip conditions, fallback scenarios, and the single-comment decision point. Exception handling is appropriately layered.
openwisp_utils/bots/ci_failure/tests/test_bot.py (5)
8-34: LGTM!Test setup and teardown properly isolate external dependencies via patching.
36-66: LGTM!Helper methods provide clean, reusable mock setups.
78-88: LGTM!Init tests correctly verify both success and missing-key scenarios.
270-310: LGTM!Skip-condition tests (dependabot, fork, deleted fork) correctly verify the expected behavior and print output.
90-118: LGTM!Failure classification tests cover all categories (QA, tests, setup, mixed, unknown) with appropriate log fixtures.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| build_logs_json = json.dumps(build_logs, indent=2) | ||
| failed_jobs_json = json.dumps(failed_jobs, indent=2) | ||
|
|
||
| if pr_diff: | ||
| pr_diff_json = json.dumps(pr_diff, indent=2) | ||
| else: | ||
| pr_diff_json = "No PR associated" | ||
|
|
||
| context = f""" | ||
| ### ROLE | ||
| You are analyzing CI build failures for OpenWISP. Provide diagnosis AND remediation advice. | ||
|
|
||
| ### INPUT CONTEXT | ||
| 1. **Failure Type:** {failure_type} | ||
| 2. **Failed Jobs:** {failed_jobs_json} | ||
| 3. **Build Logs:** {build_logs_json} | ||
| 4. **PR Diff:** {pr_diff_json} |
There was a problem hiding this comment.
Prompt injection risk: untrusted build logs and PR diffs are embedded directly into the Gemini prompt.
Build logs and PR diffs may contain attacker-controlled content (e.g., crafted commit messages, file contents, or test output). This content is serialized verbatim into the prompt, which could manipulate the model's response. Since the response is posted as a PR comment, a successful injection could surface misleading guidance or unintended content.
Consider adding a disclaimer to the posted comment that the analysis is AI-generated and may be inaccurate, or sanitizing/escaping the inputs before embedding them in the prompt.
🤖 Prompt for AI Agents
In `@openwisp_utils/bots/ci_failure/bot.py` around lines 243 - 259, The prompt
currently embeds untrusted data verbatim (build_logs_json, failed_jobs_json,
pr_diff_json) into the context f-string which creates a prompt-injection risk;
fix it by sanitizing and truncating those inputs before json.dumps (escape or
remove suspicious sequences like "###", "SYSTEM:", model-instruction keywords,
control chars, and limit to a safe max length), and append a clear AI-generated
disclaimer to the context string (e.g., mention that the analysis is automated
and may be inaccurate) so that the posted comment includes the disclaimer;
update the variables build_logs_json, pr_diff_json (and where context is
assembled) to use the sanitized/truncated values.
49f2236 to
c93f87c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
runtests.py (1)
8-19:⚠️ Potential issue | 🔴 CriticalPyGithub must be installed before testing
openwisp_utils.bots.Line 10 of
openwisp_utils/bots/ci_failure/bot.pyhas an unconditional import:from github import Github, GithubException. When runtests.py adds"openwisp_utils.bots"to the test suite (line 19), the test runner will attempt to import this module and fail immediately because PyGithub is only included in thegithub_actionsextra (setup.py lines 66-70), which is not installed during normal test runs. Note that the module does have conditional handling forgenai(try/except), but not forgithub.To fix, choose one:
- Add
PyGithubto base or test dependencies- Make the
githubimport conditional (wrap in try/except likegenai)- Ensure the test environment installs
pip install -e .[github_actions]
🤖 Fix all issues with AI agents
In @.github/workflows/ci-failure-bot.yml:
- Around line 20-42: The workflow_run checkout yields the default branch so
local git diff is empty; update get_pr_diff() in
openwisp_utils.bots.ci_failure.bot to fall back to retrieving the PR diff via
the GitHub API (use PR number from env PR_NUMBER and GITHUB_TOKEN, call the PR's
diff_url with the appropriate Accept header and return the diff text) instead of
relying solely on local git, ensuring the bot supplies the real PR diff to
Gemini when running under the workflow_run event.
In `@openwisp_utils/bots/ci_failure/bot.py`:
- Around line 416-478: The final unconditional print("CI Failure Bot completed
successfully") in run() is misleading because the outer except block may handle
errors and post a fallback; change run() to track success (e.g., introduce a
local boolean success=True, set success=False in the outer except block that
calls fallback_response()) and replace the unconditional "completed
successfully" print with a conditional that prints success only when success is
True (otherwise print a failure/handled message or omit); ensure references to
run(), fallback_response(), and post_comment() remain consistent so the
comment-posting behavior is unchanged.
- Around line 156-163: The subprocess call using subprocess.run with ["git",
"diff", f"origin/{self.repo.default_branch}"] passes an API-derived branch name
(self.repo.default_branch) directly and can be misinterpreted as flags; update
the invocation to guard against that by either validating/sanitizing
self.repo.default_branch (allow only safe ref chars) or by using the git
separator form (e.g., pass "--", "origin/<branch>") before the branch ref when
calling subprocess.run in the code around the subprocess.run call to ensure git
treats the value as a ref, not an option.
- Around line 18-61: The constructor of CIFailureBot sets self.model_name only
on successful Gemini init, causing AttributeError later; update __init__ to
always define self.model_name alongside self.model so it exists even when Gemini
is skipped or initialization fails: ensure CIFailureBot.__init__ assigns a
sensible default (e.g., the same default string "gemini-2.5-flash" or None) to
self.model_name in the branches where genai is missing or gemini init raises an
exception, and keep the existing assignment self.model_name =
os.environ.get("GEMINI_MODEL", "gemini-2.5-flash") in the successful path for
consistency.
🧹 Nitpick comments (3)
openwisp_utils/bots/ci_failure/tests/test_bot.py (3)
8-34: Consider usingaddCleanupinstead of manualhasattrchecks intearDown.The
hasattrguards intearDownare unnecessary sincesetUpalways creates these attributes. More importantly, ifsetUpfails partway through (e.g., afterenv_patcher.start()but beforegithub_patcher.start()),tearDownwould still run but the later patchers wouldn't exist. Usingself.addCleanup(patcher.stop)right after eachpatcher.start()insetUpis more robust and idiomatic.♻️ Proposed refactor
def setUp(self): self.env_vars = { "GITHUB_TOKEN": "test_token", "GEMINI_API_KEY": "test_gemini_key", "WORKFLOW_RUN_ID": "12345", "REPOSITORY": "openwisp/openwisp-utils", "PR_NUMBER": "1", } self.env_patcher = patch.dict(os.environ, self.env_vars) self.env_patcher.start() + self.addCleanup(self.env_patcher.stop) self.github_patcher = patch("openwisp_utils.bots.ci_failure.bot.Github") self.genai_patcher = patch("openwisp_utils.bots.ci_failure.bot.genai") self.mock_github = self.github_patcher.start() + self.addCleanup(self.github_patcher.stop) self.mock_genai = self.genai_patcher.start() + self.addCleanup(self.genai_patcher.stop) self.mock_repo = Mock() self.mock_github.return_value.get_repo.return_value = self.mock_repo self.mock_model = Mock() self.mock_genai.GenerativeModel.return_value = self.mock_model - def tearDown(self): - if hasattr(self, "env_patcher"): - self.env_patcher.stop() - if hasattr(self, "github_patcher"): - self.github_patcher.stop() - if hasattr(self, "genai_patcher"): - self.genai_patcher.stop()
270-297:test_run_full_workflowdoesn't verify the content of the posted comment.The test asserts that
create_issue_commentis called once but doesn't check that the comment contains the Gemini analysis text ("Analysis: Build failed due to syntax error"), the bot marker, or the bot header. This weakens the test's ability to catch regressions in message composition.
443-454: Edge-case tests forpost_commentandget_pr_diffwithNonevalues lack assertions on side-effects.Tests like
test_post_comment_no_pr_number(line 443),test_post_comment_no_github_client(line 449) only call the method without asserting anything (no exception = pass). Consider asserting thatcreate_issue_commentwas not called and/or that the expected warning was printed, to verify the guard logic rather than just absence of a crash.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.coveragerc.github/workflows/ci-failure-bot.ymldocs/developer/reusable-github-utils.rstopenwisp_utils/bots/__init__.pyopenwisp_utils/bots/apps.pyopenwisp_utils/bots/ci_failure/__init__.pyopenwisp_utils/bots/ci_failure/bot.pyopenwisp_utils/bots/ci_failure/tests/__init__.pyopenwisp_utils/bots/ci_failure/tests/test_bot.pyruntests.pytests/openwisp2/settings.pytests/test_project/tests/test_test_utils.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/openwisp2/settings.py
- openwisp_utils/bots/init.py
- .coveragerc
- docs/developer/reusable-github-utils.rst
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_utils/bots/ci_failure/tests/test_bot.py (1)
openwisp_utils/bots/ci_failure/bot.py (7)
classify_failure(182-216)get_build_logs(63-137)get_pr_diff(139-180)analyze_with_gemini(229-360)post_comment(378-414)run(416-478)get_failed_jobs_summary(218-227)
🪛 GitHub Actions: OpenWISP Utils CI Build
openwisp_utils/bots/ci_failure/tests/test_bot.py
[error] 5-5: ImportError: Failed to import CIFailureBot due to missing 'github' module.
openwisp_utils/bots/ci_failure/bot.py
[error] 1-1: ModuleNotFoundError: No module named 'github' during test import. The CIFailureBot test module requires PyGithub; ensure 'github' (PyPI: PyGithub) is installed and added to dependencies.
🪛 Ruff (0.14.14)
openwisp_utils/bots/ci_failure/tests/test_bot.py
[warning] 562-562: Unused method argument: mock_github_class
(ARG002)
openwisp_utils/bots/ci_failure/bot.py
[warning] 34-34: Do not catch blind exception: Exception
(BLE001)
[warning] 49-49: Do not catch blind exception: Exception
(BLE001)
[warning] 134-134: Consider moving this statement to an else block
(TRY300)
[error] 157-157: subprocess call: check for execution of untrusted input
(S603)
[error] 158-158: Starting a process with a partial executable path
(S607)
[warning] 358-358: Do not catch blind exception: Exception
(BLE001)
[warning] 470-470: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (4)
openwisp_utils/bots/ci_failure/__init__.py (1)
1-1: LGTM!Minimal package initializer is appropriate here.
openwisp_utils/bots/apps.py (1)
1-7: LGTM!Standard Django
AppConfigdefinition.openwisp_utils/bots/ci_failure/bot.py (1)
1-15: LGTM on the conditional import.The
try/except ImportErrorpattern forgoogle.generativeaicorrectly implements the "Gemini is optional" contract, addressing the previous review feedback..github/workflows/ci-failure-bot.yml (1)
22-22:actions/checkout@v6is valid and currently points to v6.0.2.The floating major version tag is the recommended approach to automatically track the latest v6.x release.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Set up Python | ||
| uses: actions/setup-python@v6 | ||
| with: | ||
| python-version: "3.11" | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| pip install -e .[github_actions] | ||
|
|
||
| - name: Run CI Failure Bot | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| GEMINI_API_KEY: ${{ secrets.GEMINI_API_KEY }} | ||
| WORKFLOW_RUN_ID: ${{ github.event.workflow_run.id }} | ||
| REPOSITORY: ${{ github.repository }} | ||
| PR_NUMBER: ${{ github.event.workflow_run.pull_requests[0].number || '' }} | ||
| run: | | ||
| echo "Starting CI Failure Bot..." | ||
| python -m openwisp_utils.bots.ci_failure.bot |
There was a problem hiding this comment.
git diff will always be empty: workflow_run checks out the default branch, not the PR branch.
The workflow_run event triggers on the repository's default branch. Line 22 checks out the default branch, and then the bot runs git diff origin/{default_branch} — comparing the default branch against itself, which always yields an empty diff. get_pr_diff() will always return None, so Gemini never receives PR diff context.
To get the actual PR diff, either:
- Fetch the diff via the GitHub API (e.g.,
requests.get(pr.diff_url, ...)) instead of relying on local git, or - Checkout the PR's head ref before running the bot:
Option 2: Checkout PR head
- name: Checkout repository
uses: actions/checkout@v6
+ with:
+ fetch-depth: 0
+
+ - name: Fetch PR branch
+ if: ${{ github.event.workflow_run.pull_requests[0].number }}
+ run: |
+ PR_HEAD=${{ github.event.workflow_run.head_sha }}
+ git fetch origin "$PR_HEAD"
+ git checkout "$PR_HEAD"Option 1 (API-based diff) is more robust for workflow_run events and was previously suggested.
🤖 Prompt for AI Agents
In @.github/workflows/ci-failure-bot.yml around lines 20 - 42, The workflow_run
checkout yields the default branch so local git diff is empty; update
get_pr_diff() in openwisp_utils.bots.ci_failure.bot to fall back to retrieving
the PR diff via the GitHub API (use PR number from env PR_NUMBER and
GITHUB_TOKEN, call the PR's diff_url with the appropriate Accept header and
return the diff text) instead of relying solely on local git, ensuring the bot
supplies the real PR diff to Gemini when running under the workflow_run event.
| class CIFailureBot: | ||
| def __init__(self): | ||
| self.github_token = os.environ.get("GITHUB_TOKEN") | ||
| self.gemini_api_key = os.environ.get("GEMINI_API_KEY") | ||
| self.workflow_run_id = os.environ.get("WORKFLOW_RUN_ID") | ||
| self.repository_name = os.environ.get("REPOSITORY") | ||
| self.pr_number = os.environ.get("PR_NUMBER") | ||
|
|
||
| # Initialize with None values if missing - bot will still try to comment | ||
| self.github = None | ||
| self.repo = None | ||
|
|
||
| if self.github_token and self.repository_name: | ||
| try: | ||
| self.github = Github(self.github_token) | ||
| self.repo = self.github.get_repo(self.repository_name) | ||
| except Exception as e: | ||
| print(f"Warning: Could not initialize GitHub client: {e}") | ||
| else: | ||
| missing = [] | ||
| if not self.github_token: | ||
| missing.append("GITHUB_TOKEN") | ||
| if not self.repository_name: | ||
| missing.append("REPOSITORY") | ||
| print(f"Warning: Missing environment variables: {', '.join(missing)}") | ||
|
|
||
| if self.gemini_api_key and genai is not None: | ||
| try: | ||
| genai.configure(api_key=self.gemini_api_key) | ||
| self.model_name = os.environ.get("GEMINI_MODEL", "gemini-2.5-flash") | ||
| self.model = genai.GenerativeModel(self.model_name) | ||
| except Exception as e: | ||
| print(f"Warning: Could not initialize Gemini: {e}") | ||
| self.model = None | ||
| else: | ||
| if not self.gemini_api_key: | ||
| print( | ||
| "Warning: GEMINI_API_KEY not provided, will use fallback responses" | ||
| ) | ||
| else: | ||
| print( | ||
| "Warning: google-generativeai not installed, will use fallback responses" | ||
| ) | ||
| self.model = None |
There was a problem hiding this comment.
self.model_name is only defined inside the Gemini success path.
If Gemini initialization is skipped or fails, self.model_name is never set. Any future code (or debugging) that accesses bot.model_name will raise AttributeError. Initialize it alongside self.model in the fallback branches.
🛡️ Proposed fix
if self.gemini_api_key and genai is not None:
try:
genai.configure(api_key=self.gemini_api_key)
self.model_name = os.environ.get("GEMINI_MODEL", "gemini-2.5-flash")
self.model = genai.GenerativeModel(self.model_name)
except Exception as e:
print(f"Warning: Could not initialize Gemini: {e}")
+ self.model_name = None
self.model = None
else:
if not self.gemini_api_key:
print(
"Warning: GEMINI_API_KEY not provided, will use fallback responses"
)
else:
print(
"Warning: google-generativeai not installed, will use fallback responses"
)
+ self.model_name = None
self.model = None🧰 Tools
🪛 Ruff (0.14.14)
[warning] 34-34: Do not catch blind exception: Exception
(BLE001)
[warning] 49-49: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@openwisp_utils/bots/ci_failure/bot.py` around lines 18 - 61, The constructor
of CIFailureBot sets self.model_name only on successful Gemini init, causing
AttributeError later; update __init__ to always define self.model_name alongside
self.model so it exists even when Gemini is skipped or initialization fails:
ensure CIFailureBot.__init__ assigns a sensible default (e.g., the same default
string "gemini-2.5-flash" or None) to self.model_name in the branches where
genai is missing or gemini init raises an exception, and keep the existing
assignment self.model_name = os.environ.get("GEMINI_MODEL", "gemini-2.5-flash")
in the successful path for consistency.
| try: | ||
| result = subprocess.run( | ||
| ["git", "diff", f"origin/{self.repo.default_branch}"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=30, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
Subprocess invocation uses API-sourced branch name without validation.
self.repo.default_branch is fetched from the GitHub API and passed directly to git diff. While the list-based subprocess.run prevents shell injection, a malformed branch name (e.g., containing --) could be interpreted as a git flag. Prefix with -- separator or validate the value.
🛡️ Proposed fix
result = subprocess.run(
- ["git", "diff", f"origin/{self.repo.default_branch}"],
+ ["git", "diff", "--", f"origin/{self.repo.default_branch}"],
capture_output=True,
text=True,
timeout=30,
check=False,
)🧰 Tools
🪛 Ruff (0.14.14)
[error] 157-157: subprocess call: check for execution of untrusted input
(S603)
[error] 158-158: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
In `@openwisp_utils/bots/ci_failure/bot.py` around lines 156 - 163, The subprocess
call using subprocess.run with ["git", "diff",
f"origin/{self.repo.default_branch}"] passes an API-derived branch name
(self.repo.default_branch) directly and can be misinterpreted as flags; update
the invocation to guard against that by either validating/sanitizing
self.repo.default_branch (allow only safe ref chars) or by using the git
separator form (e.g., pass "--", "origin/<branch>") before the branch ref when
calling subprocess.run in the code around the subprocess.run call to ensure git
treats the value as a ref, not an option.
| def run(self): | ||
| """Main execution flow - adapted for workflow_run""" | ||
| message = None | ||
| should_skip = False | ||
| skip_reason = "" | ||
| try: | ||
| print("CI Failure Bot starting - AI-powered analysis") | ||
|
|
||
| # Early guard for repo | ||
| if not self.repo: | ||
| print("GitHub client not initialized, cannot proceed") | ||
| return | ||
|
|
||
| # Check for skip conditions (but don't return early) | ||
| try: | ||
| if self.workflow_run_id: | ||
| workflow_run = self.repo.get_workflow_run(int(self.workflow_run_id)) | ||
| if ( | ||
| workflow_run.actor | ||
| and "dependabot" in workflow_run.actor.login.lower() | ||
| ): | ||
| should_skip = True | ||
| skip_reason = f"dependabot PR from {workflow_run.actor.login}" | ||
| if self.pr_number and not should_skip: | ||
| try: | ||
| pr_num = int(self.pr_number) | ||
| pr = self.repo.get_pull(pr_num) | ||
| if pr.head.repo is None: | ||
| should_skip = True | ||
| skip_reason = "PR with deleted head repository" | ||
| elif pr.head.repo.full_name != self.repository_name: | ||
| should_skip = True | ||
| skip_reason = f"fork PR from {pr.head.repo.full_name}" | ||
| except (GithubException, ValueError) as e: | ||
| print(f"Warning: Could not check fork status: {e}") | ||
| except (GithubException, AttributeError, ValueError) as e: | ||
| print(f"Warning: Could not check actor: {e}") | ||
| # Determine message based on context | ||
| if not self.pr_number: | ||
| print("No PR context available - workflow_run without PR") | ||
| message = None | ||
| elif should_skip: | ||
| print(f"Skipping: {skip_reason}") | ||
| return | ||
| else: | ||
| # We have PR context, proceed with analysis | ||
| build_logs = self.get_build_logs() | ||
| pr_diff = self.get_pr_diff() | ||
| if not build_logs and not pr_diff: | ||
| print("No build logs or PR diff found, using fallback response") | ||
| message = self.fallback_response() | ||
| else: | ||
| print("Analyzing failure with Gemini AI...") | ||
| message = self.analyze_with_gemini(build_logs, pr_diff) | ||
| except Exception as e: | ||
| print(f"Error in analysis: {e}") | ||
| message = self.fallback_response() | ||
| # Single comment decision point | ||
| if message: | ||
| self.post_comment(message) | ||
| else: | ||
| print("No PR context available, no comment posted (expected)") | ||
| print("CI Failure Bot completed successfully") |
There was a problem hiding this comment.
run() prints "completed successfully" even on error paths.
Line 478 (print("CI Failure Bot completed successfully")) executes unconditionally after both the normal and exception paths. When the outer except on line 470 catches an error and posts a fallback, the "completed successfully" message is misleading.
🛡️ Proposed fix
# Single comment decision point
if message:
self.post_comment(message)
else:
print("No PR context available, no comment posted (expected)")
- print("CI Failure Bot completed successfully")
+ print("CI Failure Bot completed")📝 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 run(self): | |
| """Main execution flow - adapted for workflow_run""" | |
| message = None | |
| should_skip = False | |
| skip_reason = "" | |
| try: | |
| print("CI Failure Bot starting - AI-powered analysis") | |
| # Early guard for repo | |
| if not self.repo: | |
| print("GitHub client not initialized, cannot proceed") | |
| return | |
| # Check for skip conditions (but don't return early) | |
| try: | |
| if self.workflow_run_id: | |
| workflow_run = self.repo.get_workflow_run(int(self.workflow_run_id)) | |
| if ( | |
| workflow_run.actor | |
| and "dependabot" in workflow_run.actor.login.lower() | |
| ): | |
| should_skip = True | |
| skip_reason = f"dependabot PR from {workflow_run.actor.login}" | |
| if self.pr_number and not should_skip: | |
| try: | |
| pr_num = int(self.pr_number) | |
| pr = self.repo.get_pull(pr_num) | |
| if pr.head.repo is None: | |
| should_skip = True | |
| skip_reason = "PR with deleted head repository" | |
| elif pr.head.repo.full_name != self.repository_name: | |
| should_skip = True | |
| skip_reason = f"fork PR from {pr.head.repo.full_name}" | |
| except (GithubException, ValueError) as e: | |
| print(f"Warning: Could not check fork status: {e}") | |
| except (GithubException, AttributeError, ValueError) as e: | |
| print(f"Warning: Could not check actor: {e}") | |
| # Determine message based on context | |
| if not self.pr_number: | |
| print("No PR context available - workflow_run without PR") | |
| message = None | |
| elif should_skip: | |
| print(f"Skipping: {skip_reason}") | |
| return | |
| else: | |
| # We have PR context, proceed with analysis | |
| build_logs = self.get_build_logs() | |
| pr_diff = self.get_pr_diff() | |
| if not build_logs and not pr_diff: | |
| print("No build logs or PR diff found, using fallback response") | |
| message = self.fallback_response() | |
| else: | |
| print("Analyzing failure with Gemini AI...") | |
| message = self.analyze_with_gemini(build_logs, pr_diff) | |
| except Exception as e: | |
| print(f"Error in analysis: {e}") | |
| message = self.fallback_response() | |
| # Single comment decision point | |
| if message: | |
| self.post_comment(message) | |
| else: | |
| print("No PR context available, no comment posted (expected)") | |
| print("CI Failure Bot completed successfully") | |
| def run(self): | |
| """Main execution flow - adapted for workflow_run""" | |
| message = None | |
| should_skip = False | |
| skip_reason = "" | |
| try: | |
| print("CI Failure Bot starting - AI-powered analysis") | |
| # Early guard for repo | |
| if not self.repo: | |
| print("GitHub client not initialized, cannot proceed") | |
| return | |
| # Check for skip conditions (but don't return early) | |
| try: | |
| if self.workflow_run_id: | |
| workflow_run = self.repo.get_workflow_run(int(self.workflow_run_id)) | |
| if ( | |
| workflow_run.actor | |
| and "dependabot" in workflow_run.actor.login.lower() | |
| ): | |
| should_skip = True | |
| skip_reason = f"dependabot PR from {workflow_run.actor.login}" | |
| if self.pr_number and not should_skip: | |
| try: | |
| pr_num = int(self.pr_number) | |
| pr = self.repo.get_pull(pr_num) | |
| if pr.head.repo is None: | |
| should_skip = True | |
| skip_reason = "PR with deleted head repository" | |
| elif pr.head.repo.full_name != self.repository_name: | |
| should_skip = True | |
| skip_reason = f"fork PR from {pr.head.repo.full_name}" | |
| except (GithubException, ValueError) as e: | |
| print(f"Warning: Could not check fork status: {e}") | |
| except (GithubException, AttributeError, ValueError) as e: | |
| print(f"Warning: Could not check actor: {e}") | |
| # Determine message based on context | |
| if not self.pr_number: | |
| print("No PR context available - workflow_run without PR") | |
| message = None | |
| elif should_skip: | |
| print(f"Skipping: {skip_reason}") | |
| return | |
| else: | |
| # We have PR context, proceed with analysis | |
| build_logs = self.get_build_logs() | |
| pr_diff = self.get_pr_diff() | |
| if not build_logs and not pr_diff: | |
| print("No build logs or PR diff found, using fallback response") | |
| message = self.fallback_response() | |
| else: | |
| print("Analyzing failure with Gemini AI...") | |
| message = self.analyze_with_gemini(build_logs, pr_diff) | |
| except Exception as e: | |
| print(f"Error in analysis: {e}") | |
| message = self.fallback_response() | |
| # Single comment decision point | |
| if message: | |
| self.post_comment(message) | |
| else: | |
| print("No PR context available, no comment posted (expected)") | |
| print("CI Failure Bot completed") |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 470-470: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@openwisp_utils/bots/ci_failure/bot.py` around lines 416 - 478, The final
unconditional print("CI Failure Bot completed successfully") in run() is
misleading because the outer except block may handle errors and post a fallback;
change run() to track success (e.g., introduce a local boolean success=True, set
success=False in the outer except block that calls fallback_response()) and
replace the unconditional "completed successfully" print with a conditional that
prints success only when success is True (otherwise print a failure/handled
message or omit); ensure references to run(), fallback_response(), and
post_comment() remain consistent so the comment-posting behavior is unchanged.
| import os | ||
| from unittest.mock import Mock, patch | ||
|
|
||
| from django.test import TestCase | ||
| from openwisp_utils.bots.ci_failure.bot import CIFailureBot |
There was a problem hiding this comment.
Pipeline failure: from github import Github will fail when PyGithub is not installed.
The import on line 5 unconditionally imports CIFailureBot, which transitively imports from github import Github, GithubException. The CI pipeline confirms this fails with ModuleNotFoundError: No module named 'github'. This is the direct symptom of the root cause flagged in runtests.py — PyGithub must be available in the test environment.
🧰 Tools
🪛 GitHub Actions: OpenWISP Utils CI Build
[error] 5-5: ImportError: Failed to import CIFailureBot due to missing 'github' module.
…ailure handling openwisp#524 - Move bot under openwisp_utils/bots/ci_failure - Classify failures (QA/tests/setup) from jobs and logs - Let the LLM suggest only relevant fixes (no hardcoded remediation) - Remove dead code and tighten error handling - Update tests and workflow for the new structure Closes openwisp#524
Srinath0916
force-pushed
the
issues/524-ci-failure-bot
branch
from
c93f87c to
520ede6
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
It seems you me that you're going off track @Srinath0916, I see code that doesn't make any sense. You should stop abusing AI tools and go back to the drawing board. I left a couple of comments below.
| from django.apps import AppConfig | ||
|
|
||
|
|
||
| class BotsConfig(AppConfig): |
There was a problem hiding this comment.
What are you doing? This is totally out of place! This is not a django app!
| args.insert(1, "test") | ||
| args.insert(2, "test_project") | ||
| args.insert(3, "openwisp_utils.metric_collection") | ||
| args.insert(4, "openwisp_utils.bots") |
There was a problem hiding this comment.
this is wrong, do tests with pytests, look at tests added in #560, look also at the tests for the releaser.
|
This has stalled for the moment and the PR has become huge with many irrelevant changes. I will close this for now to free this issue for other contributors if they want to pick it up. It's a shame we did a lot of work on this and was almost working so if you want to finish it you're totally welcome but please open a new, cleaner, pull request. |
github-project-automation
Bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
5 participants
Implements automated bot responses for common CI failures:
Analyzes QA check failures and provides OpenWISP-aligned guidance
Detects test failures and setup issues
Provides specific fix suggestions with links
Posts helpful guidance comments automatically
Checklist
Reference to Existing Issue
Addresses #524
Description of Changes
This PR introduces a CI Failure Bot that automatically analyzes failed CI builds and posts helpful guidance comments on pull requests.
What the bot does:
Analyzes CI failure logs and provides factual diagnosis
Posts a single deduplicated comment with clear next steps
Always recommends the official OpenWISP QA workflow commands
Never suggests raw linters (black, flake8, isort)
Gracefully handles edge cases (missing API keys, fork PRs, etc.)
Skips dependabot PRs automatically
Recommended commands (always consistent):
pip install -e .[qa]
openwisp-qa-format
./run-qa-checks
Demo repository & real-world testing
The CI failure bot has been tested using a dedicated demo repository:
https://github.com/srinath-testing/gemini-ci-bot-demo
The following real-world failure scenarios are covered using actual pull requests:
Demo PR – QA / formatting failures → srinath-testing/gemini-ci-bot-demo#14
Demo PR – Unit test failures → srinath-testing/gemini-ci-bot-demo#15
Demo PR – Mixed issues (imports + formatting) → srinath-testing/gemini-ci-bot-demo#16
Each example demonstrates:
CI failure occurs
Bot posts automatic comment
Correct OpenWISP-aligned guidance provided
No duplicate or misleading output
Notes
The bot is intentionally designed as a conservative first-line helper and does not attempt perfect root-cause classification in all complex scenarios. It focuses on reliability and consistency with OpenWISP standards.
AI integration (Gemini) is optional and never blocks posting feedback to contributors.