feat: add gitleaks to prek pre-commit hooks and makefile#28
feat: add gitleaks to prek pre-commit hooks and makefile#28newtob wants to merge 5 commits intoritwiktiwari:mainfrom
Conversation
Updated gitleaks command for secret scanning with additional options.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a new Makefile Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd
secretsto.PHONYdeclaration.The
secretstarget (line 40) is not included in the.PHONYdeclaration. Since it doesn't create a file namedsecrets, it should be declared as phony to ensure it always runs when invoked.🔧 Proposed fix
-.PHONY: verify fix lint format type-check install test docs docs-serve +.PHONY: verify fix lint format type-check install test docs docs-serve secrets pysentry🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, Add the missing phony target by including "secrets" in the .PHONY declaration so Make treats the secrets target as always-executed; update the existing .PHONY line that currently lists "verify fix lint format type-check install test docs docs-serve" to also include the "secrets" symbol to match the secrets target definition elsewhere in the Makefile.
🧹 Nitpick comments (2)
Makefile (1)
40-41: Same timeout concern applies here; also minor style inconsistency.The 2-second timeout may be insufficient for scanning larger codebases. Consider increasing it to 30-60 seconds for more thorough scanning.
Minor: The
--redact 80syntax here differs from--redact=80in the pre-commit config. While both work, using consistent syntax across configurations improves maintainability.📝 Suggested alignment with pre-commit syntax
secrets: - gitleaks detect --redact 80 --no-banner --timeout 2 --max-target-megabytes=2 -v + gitleaks detect --redact=80 --no-banner --timeout=30 --max-target-megabytes=2 -v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 40 - 41, Increase the gitleaks timeout and align flag syntax in the Makefile 'secrets' target: change the timeout from 2 seconds to a more realistic 30–60 seconds (e.g., 30s) and use consistent flag syntax `--redact=80` to match the pre-commit configuration; update the 'secrets' recipe invocation of gitleaks accordingly so scans have enough time and flags are consistent across configs.template/{% if include_prek %}.pre-commit-config.yaml{% endif %}.jinja (1)
25-26: Comment placement creates confusion.The comment on line 26 describing Gitleaks is placed between the "ty" comment and the
- repo: localdeclaration. Consider reorganizing the comments to clearly associate each description with its respective hook, or use a single comment block describing all local hooks.📝 Suggested reorganization
- # ty - Astral's type checker (local hook since no prek repo yet) - # Gitleaks - a tool for detecting secrets like passwords, API keys, and tokens + # Local hooks - repo: local hooks: + # ty - Astral's type checker (local hook since no prek repo yet) - id: ty name: ty type checker entry: uv run ty check language: system types: [python] pass_filenames: false + # Gitleaks - a tool for detecting secrets like passwords, API keys, and tokens - id: gitleaks🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/`{% if include_prek %}.pre-commit-config.yaml{% endif %}.jinja around lines 25 - 26, The inline comment describing Gitleaks is misplaced between the "ty - Astral's type checker" comment and the `- repo: local` declaration, causing confusion; move or merge comments so each description clearly applies to its hook: either place the "ty - Astral's type checker" comment immediately above the `- repo: local` block that defines that hook and put the Gitleaks description directly above the Gitleaks hook, or replace both with a single descriptive comment block before the `- repo: local` section explaining that the following local hooks include ty (Astral type checker) and Gitleaks (secret scanning).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@template/`{% if include_prek %}.pre-commit-config.yaml{% endif %}.jinja:
- Line 37: Replace the invalid and too-short timeout flag in the gitleaks
pre-commit entry: change the CLI token "--timeout 2" to a valid Go duration and
a more realistic value, e.g. "--timeout 30s" (so the entry containing "gitleaks
git --redact=80 --no-banner --timeout 2 -v ..." becomes "gitleaks git
--redact=80 --no-banner --timeout 30s -v ..."); keep the other flags (e.g.
--redact=80, --no-banner, --pre-commit, --staged) unchanged.
---
Outside diff comments:
In `@Makefile`:
- Line 1: Add the missing phony target by including "secrets" in the .PHONY
declaration so Make treats the secrets target as always-executed; update the
existing .PHONY line that currently lists "verify fix lint format type-check
install test docs docs-serve" to also include the "secrets" symbol to match the
secrets target definition elsewhere in the Makefile.
---
Nitpick comments:
In `@Makefile`:
- Around line 40-41: Increase the gitleaks timeout and align flag syntax in the
Makefile 'secrets' target: change the timeout from 2 seconds to a more realistic
30–60 seconds (e.g., 30s) and use consistent flag syntax `--redact=80` to match
the pre-commit configuration; update the 'secrets' recipe invocation of gitleaks
accordingly so scans have enough time and flags are consistent across configs.
In `@template/`{% if include_prek %}.pre-commit-config.yaml{% endif %}.jinja:
- Around line 25-26: The inline comment describing Gitleaks is misplaced between
the "ty - Astral's type checker" comment and the `- repo: local` declaration,
causing confusion; move or merge comments so each description clearly applies to
its hook: either place the "ty - Astral's type checker" comment immediately
above the `- repo: local` block that defines that hook and put the Gitleaks
description directly above the Gitleaks hook, or replace both with a single
descriptive comment block before the `- repo: local` section explaining that the
following local hooks include ty (Astral type checker) and Gitleaks (secret
scanning).
template/{% if include_prek %}.pre-commit-config.yaml{% endif %}.jinja
Outdated
Show resolved
Hide resolved
|
All refinements implemented:
Pipeline fails in the @ritwiktiwari your help would be appreciated! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
1-1: Consider adding conventionalallandcleantargets.Static analysis (checkmake) flags missing
allandcleanphony targets. These are common Makefile conventions but not strictly required for this project's workflow. You could add them if desired:all: verify clean: `@echo` "No build artifacts to clean"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, The Makefile is missing conventional phony targets "all" and "clean" which static analysis (checkmake) expects; add targets named all and clean and include them in the .PHONY list alongside existing targets (e.g., .PHONY: verify fix lint ...), implement all as a shortcut to the primary build/test entry (e.g., make all should run the existing verify target) and add a harmless clean target (e.g., printing "No build artifacts to clean" or a no-op) so checkmake and users see standard behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 39-41: The Makefile target "secrets" runs gitleaks with --timeout
60s while the pre-commit bash wrapper in template/{% if include_prek
%}.pre-commit-config.yaml{% endif %}.jinja uses --timeout 2 causing inconsistent
behavior; update the --timeout value so both places match (pick one canonical
value, e.g., change the pre-commit wrapper's --timeout from 2 to 60 or change
the Makefile's --timeout to 2) by editing the gitleaks invocation in the
"secrets" Makefile target and the gitleaks invocation in the pre-commit template
to use the same timeout string (including the 's' suffix if using seconds).
---
Nitpick comments:
In `@Makefile`:
- Line 1: The Makefile is missing conventional phony targets "all" and "clean"
which static analysis (checkmake) expects; add targets named all and clean and
include them in the .PHONY list alongside existing targets (e.g., .PHONY: verify
fix lint ...), implement all as a shortcut to the primary build/test entry
(e.g., make all should run the existing verify target) and add a harmless clean
target (e.g., printing "No build artifacts to clean" or a no-op) so checkmake
and users see standard behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b17f71e4-c08d-4eea-8a8a-786eeb16d521
📒 Files selected for processing (2)
Makefiletemplate/{% if include_prek %}.pre-commit-config.yaml{% endif %}.jinja
🚧 Files skipped from review as they are similar to previous changes (1)
- template/{% if include_prek %}.pre-commit-config.yaml{% endif %}.jinja
| # Secret scanning | ||
| secrets: | ||
| gitleaks detect --redact 80 | ||
| gitleaks detect --redact 80 --no-banner --timeout 60s --max-target-megabytes=2 -v |
There was a problem hiding this comment.
Timeout inconsistency with pre-commit configuration.
The Makefile uses --timeout 60s, but the pre-commit bash wrapper in template/{% if include_prek %}.pre-commit-config.yaml{% endif %}.jinja (line 42) uses --timeout 2 (2 seconds). This 30x difference could cause the pre-commit hook to timeout on larger repositories while the Makefile succeeds.
Please align the timeout values across both configurations for consistent behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 39 - 41, The Makefile target "secrets" runs gitleaks
with --timeout 60s while the pre-commit bash wrapper in template/{% if
include_prek %}.pre-commit-config.yaml{% endif %}.jinja uses --timeout 2 causing
inconsistent behavior; update the --timeout value so both places match (pick one
canonical value, e.g., change the pre-commit wrapper's --timeout from 2 to 60 or
change the Makefile's --timeout to 2) by editing the gitleaks invocation in the
"secrets" Makefile target and the gitleaks invocation in the pre-commit template
to use the same timeout string (including the 's' suffix if using seconds).
2 participants
Many folk find secrets sneak into code bases without notice. To eradicate these problems before they are committed
prekis amazing.This MR uses
prekto catch secrets before they are committed.Summary by CodeRabbit