fix: address critical quality-debt in sanity-check.sh (GH#2866)#2870
fix: address critical quality-debt in sanity-check.sh (GH#2866)#2870alex-solovyev merged 2 commits intomainfrom
Conversation
…eview (GH#2866) - Remove 2>/dev/null from db() calls to surface database errors (critical) - Replace dot-only regex escaping with full metacharacter _escape_regex() helper - Add task_id format validation to reject malformed IDs before regex/sed use - Replace grep 2>/dev/null with explicit file-existence checks - Add prompt injection mitigation: control char stripping, DATA boundary markers, and anti-injection instruction for AI prompt containing DB data Closes #2866
|
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. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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, 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 significantly enhances the robustness and security of the 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: Thu Mar 5 02:13:18 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 introduces a series of excellent quality and security improvements to sanity-check.sh. The changes effectively address several potential issues, including suppressed database errors, incomplete regex escaping, missing input validation, and prompt injection vulnerabilities. Removing error suppression from db() and grep calls will significantly improve debuggability. The new _escape_regex function and task_id validation provide robust protection against injection attacks. The prompt sanitization is also a great step towards hardening the script against malicious inputs. Overall, these are high-quality fixes. I have one minor suggestion to apply the grep error handling pattern consistently throughout the file.
Note: Security Review did not run due to the size of the PR.
| local escaped_oid | ||
| escaped_oid=$(printf '%s' "$oid" | sed 's/\./\\./g') | ||
| escaped_oid=$(_escape_regex "$oid") | ||
| if ! grep -qE "^[[:space:]]*- \[.\] ${escaped_oid}( |$)" "$todo_file" 2>/dev/null; then |
There was a problem hiding this comment.
This grep call still suppresses stderr with 2>/dev/null. While other grep calls in this PR were updated to use [[ -f "$todo_file" ]] guards, this one appears to have been missed. Suppressing stderr can hide important errors like file-not-found or permission issues.
For consistency and to improve robustness, this should be updated. The correct logic for a negative check (! grep) is to use an || condition to check for file existence first.
| if ! grep -qE "^[[:space:]]*- \[.\] ${escaped_oid}( |$)" "$todo_file" 2>/dev/null; then | |
| if [[ ! -f "$todo_file" ]] || ! grep -qE "^[[:space:]]*- \[.[] ${escaped_oid}( |$)" "$todo_file"; then |
References
- Avoid using
2>/dev/nullfor blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging. - Avoid using
2>/dev/nullto suppress errors on file operations if the file's existence has already been verified by a preceding check (e.g.,[[ -f "$file" ]]or an early return). This practice is redundant for 'file not found' errors and can mask other important issues like permissions problems.
…ence guards Address Gemini Code Assist review feedback on PR #2870: the grep at line 342 (orphan detection) still suppressed stderr. Apply the same [[ -f "$todo_file" ]] guard pattern consistently to all remaining grep calls on $todo_file (lines 260, 342, 996-999) so file-not-found and permission errors are visible.
🔍 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: Thu Mar 5 02:23:40 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
2 participants
Summary
Fixes all 5 findings from the PR #2845 CodeRabbit/Gemini review on
.agents/scripts/supervisor-archived/sanity-check.sh:2>/dev/nullfromdb()calls (lines 487, 700, 720, 721, 865) to surface database connection failures, SQL errors, and permission problems instead of silently swallowing themsed 's/\./\\./g') with a full_escape_regex()helper that escapes all BRE/ERE metacharacters (.,/,[,],*,^,$,(,),+,?,{,},|,\), preventing regex/sed injection via crafted task IDs^t[0-9]+(\.[0-9]+)*$) at the entry point of_execute_sanity_action()to reject malformed IDs before they reach any regex/sed operationgrep ... 2>/dev/nullon$todo_filewith explicit[[ -f "$todo_file" ]]guards so file-not-found errors are visibleDATA_START/DATA_ENDboundary markers, and adding an explicit anti-injection instructionShellCheck: zero violations.
Closes #2866