fix: harden VirusTotal error handling, logging, and rate-limit accounting (t147.3)#468
fix: harden VirusTotal error handling, logging, and rate-limit accounting (t147.3)#468marcusquinn merged 2 commits intomainfrom
Conversation
…ting (t147.3) - Redirect all log functions to stderr to prevent stdout pollution in command substitution - Return exit code 2 from vt_request for NotFoundError, allowing callers to distinguish 404 from quota/network errors - Fix cmd_scan_file to propagate real API errors instead of treating all failures as 'not in database' - Count every API request attempt toward rate limit, not just successful ones - Replace O(n²) domain dedup loop with associative array lookup - Add early jq dependency check in main() for clear error messaging - Fix security-helper.sh to scan file directly for file-based skills instead of entire parent directory - Fix add-skill-helper.sh: correct return value comment, use resolved path in log message Addresses 9 review threads (3 high, 3 major, 3 medium) from PR #410. Closes #444
|
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 significantly enhances the robustness and reliability of the VirusTotal integration within the agent scripts. It addresses several critical issues related to error handling, API rate limit management, and scanning logic, ensuring more accurate reporting and preventing unintended side effects during script execution. The changes aim to make the VirusTotal scanning process more resilient and user-friendly by providing clearer feedback and more precise control over API interactions. 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 19:44:59 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 number of excellent fixes and hardening measures for the VirusTotal helper script. The changes to error handling, logging, rate-limiting, and performance are all well-implemented and significantly improve the script's robustness and reliability. I have a couple of suggestions to further improve logging consistency by removing stderr suppression within the cmd_scan_skill function. This would allow detailed API errors to be visible by default, enhancing debuggability. Overall, this is a high-quality pull request.
| response=$(vt_request "GET" "/files/${sha256}" 2>/dev/null) || { | ||
| # Not in VT database -- normal for text files | ||
| local vt_exit=0 | ||
| response=$(vt_request "GET" "/files/${sha256}" 2>/dev/null) || vt_exit=$? |
There was a problem hiding this comment.
The 2>/dev/null here suppresses the detailed API error messages that vt_request logs to stderr. Since a major goal of this PR is to improve logging and error handling, it would be better to let these errors be displayed. This would provide more context when an API error occurs, rather than just the generic "API error" message printed later. The caller can still choose to suppress stderr if needed.
| response=$(vt_request "GET" "/files/${sha256}" 2>/dev/null) || vt_exit=$? | |
| response=$(vt_request "GET" "/files/${sha256}") || vt_exit=$? |
| local response="" | ||
| response=$(vt_request "GET" "/domains/${domain}" 2>/dev/null) || { | ||
| local vt_exit=0 | ||
| response=$(vt_request "GET" "/domains/${domain}" 2>/dev/null) || vt_exit=$? |
There was a problem hiding this comment.
Similar to the file scan, the 2>/dev/null here suppresses detailed error messages from vt_request. Removing it would improve debuggability by showing the actual API error when a domain lookup fails for reasons other than "not found".
| response=$(vt_request "GET" "/domains/${domain}" 2>/dev/null) || vt_exit=$? | |
| response=$(vt_request "GET" "/domains/${domain}") || vt_exit=$? |
1 participant
Summary
vt_requestforNotFoundError, enabling callers to distinguish 404 from quota/network errorscmd_scan_fileto propagate real API errors instead of treating all failures as "not in database"jqdependency check inmain()for clear error messagingsecurity-helper.shto scan file directly for file-based skills instead of entire parent directoryadd-skill-helper.sh: correct return value comment, use resolved path in log messageAnalysis
Addresses 9 unresolved review threads from PR #410 (VirusTotal integration):
virustotal-helper.sh:43virustotal-helper.sh:183vt_requestreturns 1 for all errors - no way to distinguish NotFound from QuotaExceededvirustotal-helper.sh:266cmd_scan_filetreats all API failures as "not in database"security-helper.sh:600dirnamescans entire parent directory for file-based skillsdirname "playwright-skill.md"scans all oftools/browser/virustotal-helper.sh:505request_count++after success onlyvirustotal-helper.sh:266cmd_statusbut not before scan commandsadd-skill-helper.sh:588virustotal-helper.sh:568add-skill-helper.sh:616virustotal-helper.shwithout pathTesting
bash -n: syntax check passes for all 3 filesvt_requestexit code contract: 0=success, 1=general error, 2=not foundCloses #444