fix(secretlint): add rule installation validation and better error handling #74
Conversation
…ndling - Add check_rules_installed() to detect missing rule presets - Add validate_secretlint_setup() for comprehensive validation - Handle exit code 2 (configuration error) separately from secrets found - Add rule validation in show_status() command - Document Failed to load rule module error and fix in secretlint.md - Add system-reminder to markdownlint allowed HTML tags - Fix markdown code block syntax (use text/bash instead of bare blocks) - Update dspyground dependency to ^0.2.0
|
Warning Rate limit exceeded@marcusquinn has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 9 minutes and 35 seconds before requesting another review. ⌛ 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 (1)
WalkthroughIntroduces Secretlint rule validation mechanisms to verify preset installations in shell scripts, extends troubleshooting documentation with remediation guidance, updates a dependency version, and applies formatting standardization across documentation files for consistency. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 user experience of the Secretlint integration by adding comprehensive validation for rule installations and improving error handling. The primary goal is to prevent users from encountering confusing 'scan failed' messages when the underlying issue is a missing rule preset, which was a common cause of false negatives in secret detection. By clearly differentiating between actual secret findings and setup problems, and providing better diagnostic tools and documentation, this change makes the Secretlint setup and troubleshooting process much smoother. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔍 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: Tue Jan 13 22:55:47 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 valuable improvements to the secretlint integration by adding validation for rule installation and enhancing error handling. These changes will significantly improve the user experience by providing clearer diagnostics for configuration issues. The core logic in secretlint-helper.sh is solid, but I've identified a few opportunities to improve maintainability, reduce redundancy, and fix a bug that suppresses important error messages. The accompanying documentation and markdown style updates are well-executed and beneficial.
.agent/scripts/secretlint-helper.sh
Outdated
| if check_rules_installed "$SECRETLINT_CONFIG_FILE" 2>/dev/null; then | ||
| print_success "All configured rules are installed" | ||
| fi |
There was a problem hiding this comment.
There's a bug in how check_rules_installed is called within the show_status function. Redirecting stderr to /dev/null (2>/dev/null) will suppress the detailed error messages that check_rules_installed prints when it detects missing rules.
When a user runs $0 status to diagnose problems, they should see the list of missing packages. By suppressing stderr, they will see no output for a failed check, which can be confusing. Please remove the 2>/dev/null redirection to ensure diagnostic messages are always visible.
| if check_rules_installed "$SECRETLINT_CONFIG_FILE" 2>/dev/null; then | |
| print_success "All configured rules are installed" | |
| fi | |
| if check_rules_installed "$SECRETLINT_CONFIG_FILE"; then | |
| print_success "All configured rules are installed" | |
| fi |
| check_rules_installed() { | ||
| local config_file="${1:-$SECRETLINT_CONFIG_FILE}" | ||
|
|
||
| if [[ ! -f "$config_file" ]]; then | ||
| return 2 | ||
| fi | ||
|
|
||
| # Extract rule IDs from config | ||
| local missing_rules=() | ||
|
|
||
| # Check for preset-recommend (most common) | ||
| if grep -q "secretlint-rule-preset-recommend" "$config_file"; then | ||
| if ! npm list @secretlint/secretlint-rule-preset-recommend &>/dev/null; then | ||
| if ! npm list -g @secretlint/secretlint-rule-preset-recommend &>/dev/null; then | ||
| missing_rules+=("@secretlint/secretlint-rule-preset-recommend") | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| # Check for pattern rule | ||
| if grep -q "secretlint-rule-pattern" "$config_file"; then | ||
| if ! npm list @secretlint/secretlint-rule-pattern &>/dev/null; then | ||
| if ! npm list -g @secretlint/secretlint-rule-pattern &>/dev/null; then | ||
| missing_rules+=("@secretlint/secretlint-rule-pattern") | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| if [[ ${#missing_rules[@]} -gt 0 ]]; then | ||
| print_error "Missing required secretlint rules:" | ||
| for rule in "${missing_rules[@]}"; do | ||
| echo " - $rule" | ||
| done | ||
| print_info "Install with: npm install --save-dev ${missing_rules[*]}" | ||
| return 1 | ||
| fi | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, the check_rules_installed function can be refactored. The current implementation has separate, nearly identical blocks for each rule check.
By using an associative array to map configuration strings to their corresponding npm package names, you can iterate through the rules in a loop. This makes the code cleaner and simplifies adding or removing rule checks in the future.
check_rules_installed() {
local config_file="${1:-$SECRETLINT_CONFIG_FILE}"
if [[ ! -f "$config_file" ]]; then
return 2
fi
local missing_rules=()
# Map of config string to package name
local -A rules_map=(
["secretlint-rule-preset-recommend"]="@secretlint/secretlint-rule-preset-recommend"
["secretlint-rule-pattern"]="@secretlint/secretlint-rule-pattern"
)
for config_string in "${!rules_map[@]}"; do
if grep -q "$config_string" "$config_file"; then
local package_name="${rules_map[$config_string]}"
if ! npm list "$package_name" &>/dev/null && ! npm list -g "$package_name" &>/dev/null; then
missing_rules+=("$package_name")
fi
fi
done
if [[ ${#missing_rules[@]} -gt 0 ]]; then
print_error "Missing required secretlint rules:"
for rule in "${missing_rules[@]}"; do
echo " - $rule"
done
print_info "Install with: npm install --save-dev ${missing_rules[*]}"
return 1
fi
return 0
}| validate_secretlint_setup() { | ||
| local has_issues=0 | ||
|
|
||
| # Check binary | ||
| if ! check_secretlint_installed; then | ||
| has_issues=1 | ||
| fi | ||
|
|
||
| # Check rules if config exists | ||
| if [[ -f "$SECRETLINT_CONFIG_FILE" ]]; then | ||
| if ! check_rules_installed; then | ||
| has_issues=1 | ||
| fi | ||
| fi | ||
|
|
||
| return $has_issues | ||
| } |
There was a problem hiding this comment.
The validate_secretlint_setup function can be simplified for better readability and to remove redundant logic.
The explicit check if [[ -f "$SECRETLINT_CONFIG_FILE" ]] is unnecessary because the called function check_rules_installed already handles the case where the configuration file doesn't exist by returning a specific exit code (2).
You can make the logic clearer by calling check_rules_installed once and then checking its exit code to determine if there's an issue. This function should only flag missing rules (exit code 1) as an issue, not a missing configuration file.
| validate_secretlint_setup() { | |
| local has_issues=0 | |
| # Check binary | |
| if ! check_secretlint_installed; then | |
| has_issues=1 | |
| fi | |
| # Check rules if config exists | |
| if [[ -f "$SECRETLINT_CONFIG_FILE" ]]; then | |
| if ! check_rules_installed; then | |
| has_issues=1 | |
| fi | |
| fi | |
| return $has_issues | |
| } | |
| validate_secretlint_setup() { | |
| local has_issues=0 | |
| # Check binary | |
| if ! check_secretlint_installed; then | |
| has_issues=1 | |
| fi | |
| # Check rules, ignoring "no config" errors (exit code 2) | |
| check_rules_installed | |
| if [[ $? -eq 1 ]]; then | |
| has_issues=1 | |
| fi | |
| return $has_issues | |
| } |
- Remove 2>/dev/null from show_status to preserve diagnostic output - Simplify validate_secretlint_setup to handle exit codes properly
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
🤖 Augment PR SummarySummary: Improves Secretlint integration by validating rule preset installation up-front and making scan failures easier to diagnose. Changes:
Technical Notes: Rule validation uses 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
.agent/scripts/secretlint-helper.sh
Outdated
| # Validate rule installation | ||
| print_info "Rule Installation:" | ||
| if [[ -f "$SECRETLINT_CONFIG_FILE" ]]; then | ||
| if check_rules_installed "$SECRETLINT_CONFIG_FILE" 2>/dev/null; then |
There was a problem hiding this comment.
In show_status, check_rules_installed ... 2>/dev/null suppresses print_error output (stderr), so the “Missing required secretlint rules” header can disappear while the rule list still prints to stdout. Consider not redirecting stderr (or redirecting both streams) so the diagnostics remain clear when rules are missing.
🤖 Was this useful? React with 👍 or 👎
🔍 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: Tue Jan 13 22:58:25 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
1 participant
Summary
Recovers valuable improvements from a stashed bugfix branch that was never merged.
Changes
Secretlint Improvements:
check_rules_installed()to detect missing rule presets before scanningvalidate_secretlint_setup()for comprehensive installation validationshow_status()command for better diagnosticssecretlint.mdMarkdown/Style Fixes:
system-reminderto markdownlint allowed HTML tagstext/bashinstead of bare blocks)Dependencies:
^0.1.0→^0.2.0Why This Matters
The secretlint rule validation prevents confusing "scan failed" errors when users have secretlint installed but are missing the required rule presets. This was a common issue causing false negatives in secret detection.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.