fix(supervisor): add missing $SUPERVISOR_DB arg, remove PATH guard (t147.1) #450
Conversation
… HOMEBREW_PREFIX guard (t147.1) - Add $SUPERVISOR_DB as first arg to both db() calls in no_pr retry counter logic (lines 3165, 3183). Without it, sqlite3 treated the SQL as a filename and failed silently — retry counter never persisted, making the 5-attempt threshold unreachable. - Remove unused no_pr_key variable. - Remove HOMEBREW_PREFIX guard around PATH augmentation. The idempotent PATH check already prevents duplicates; the guard was overly restrictive for cron environments where HOMEBREW_PREFIX may be set without all tool paths present. Triaged all 4 unresolved review threads from PR #435: - 3 fixed (critical db bug, unused var, PATH guard) - 1 acknowledged won't-fix (json_extract counter reset is by design) Refs: GH#438, t147.1
|
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. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses several critical issues and improves the robustness and cleanliness of the supervisor script. The primary focus is on fixing a database interaction bug that caused the "no_pr" retry counter to fail silently, leading to infinite retry loops. Additionally, it refines how system PATHs are handled in cron environments and removes an unnecessary variable. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Sat Feb 7 18:10:41 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical bug in the no_pr retry logic by adding the missing $SUPERVISOR_DB argument, which was causing an infinite loop. The cleanup of the unused no_pr_key variable and the removal of the restrictive HOMEBREW_PREFIX guard are also positive changes that improve code clarity and robustness.
My main concern is a potential SQL injection vulnerability in the database queries where $task_id is used directly. I've added specific comments with recommendations to validate this variable to mitigate the risk.
| no_pr_count=$(db "$SUPERVISOR_DB" "SELECT COALESCE( | ||
| (SELECT CAST(json_extract(error, '$.no_pr_retries') AS INTEGER) | ||
| FROM tasks WHERE id='$task_id'), 0);" 2>/dev/null || echo "0") |
There was a problem hiding this comment.
This query appears to be vulnerable to SQL injection. The $task_id variable is interpolated directly into the SQL string. If this variable can be influenced by an external source, it could be manipulated to alter the query's logic, potentially affecting unintended rows or exposing data.
To mitigate this, I recommend validating the $task_id to ensure it conforms to an expected format before it's used in the query. For example:
if [[ ! "$task_id" =~ ^[a-zA-Z0-9_.-]+$ ]]; then
log_error "Invalid task_id format: $task_id"
return 1
fiThis validation should be performed before the database call.
| log_warn "No PR found for $task_id (attempt $no_pr_count/5)" | ||
| # Store retry count in error field as JSON | ||
| db "UPDATE tasks SET error = json_set(COALESCE(error, '{}'), '$.no_pr_retries', $no_pr_count), updated_at = strftime('%Y-%m-%dT%H:%M:%SZ','now') WHERE id='$task_id';" 2>/dev/null || true | ||
| db "$SUPERVISOR_DB" "UPDATE tasks SET error = json_set(COALESCE(error, '{}'), '$.no_pr_retries', $no_pr_count), updated_at = strftime('%Y-%m-%dT%H:%M:%SZ','now') WHERE id='$task_id';" 2>/dev/null || true |
There was a problem hiding this comment.
Similar to the SELECT query, this UPDATE query is also vulnerable to SQL injection via the $task_id variable. An unvalidated task_id could lead to unintended updates across multiple rows in the tasks table.
Please apply the same validation for $task_id before this query is executed to prevent this vulnerability.
1 participant
Summary
$SUPERVISOR_DBargument to bothdb()calls in theno_prretry counter logic. Without it,sqlite3treated the SQL string as a filename and failed silently — the retry counter never persisted, making the 5-attempt threshold unreachable (infinite retry loop).no_pr_keyvariable.HOMEBREW_PREFIXguard around PATH augmentation for cron. The idempotent":$PATH:"check already prevents duplicates; the guard was overly restrictive for cron environments whereHOMEBREW_PREFIXmay be set without all tool paths present.Triage Results (PR #435, 4 threads)
db()missing$SUPERVISOR_DBno_pr_keyvariableHOMEBREW_PREFIXPATH guardjson_extractcounter resetAll 4 threads replied to with evidence and resolved.
Testing
bash -nsyntax check: PASSRefs: GH#438, t147.1