feat(gitleaks): introduce local secret scanning and redaction using Gitleaks#1
Merged
Conversation
📝 WalkthroughWalkthroughThis change adds trusted, optional Gitleaks provisioning and secret redaction across Rust and shell helper paths, introduces installer and release packaging support, expands output-quality evaluation, and updates documentation, skill contracts, CI, and regression tests. ChangesSecret redaction runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Helper as collect_diff_context.sh
participant Sanitizer as collect-diff-context-cli
participant Gitleaks
participant Reviewer as Review output
Helper->>Sanitizer: send captured stdout/stderr
Sanitizer->>Gitleaks: scan input with trusted config
Gitleaks-->>Sanitizer: findings or clean report
Sanitizer-->>Helper: sanitized output and status
Helper-->>Reviewer: redacted output or continued-review status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
junit
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
scripts/bin/collect_diff_context-darwin-arm64 (1)
1-1: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd checksum coverage for
scripts/bin/collect_diff_context-*.
build_all_binaries.shand the release workflow ship these artifacts, but there’s no manifest likescripts/gitleaks-binaries.sha256for consumers to validate them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bin/collect_diff_context-darwin-arm64` at line 1, Add a SHA-256 checksum manifest covering every scripts/bin/collect_diff_context-* artifact shipped by build_all_binaries.sh and the release workflow, following the format and placement of scripts/gitleaks-binaries.sha256. Ensure all platform binaries are included so consumers can validate each downloaded artifact.tests/helper_shadow_mode_test.sh (1)
59-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the fake scanner version from the pinned version file.
Hardcoding
8.30.1makes this test fail wheneverscripts/gitleaks.versionis updated.Proposed fix
fake_gitleaks="$tmp_dir/gitleaks" -cat >"$fake_gitleaks" <<'EOF' +gitleaks_version="$(tr -d '[:space:]' < "$repo_root/scripts/gitleaks.version")" +cat >"$fake_gitleaks" <<EOF #!/usr/bin/env bash set -euo pipefail -if [ "${1:-}" = 'version' ]; then - printf '%s\n' '8.30.1' +if [ "\${1:-}" = 'version' ]; then + printf '%s\n' '$gitleaks_version' exit 0 fi cat >/dev/null printf '[]\n' EOF🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/helper_shadow_mode_test.sh` around lines 59 - 69, Update the fake gitleaks script in the test setup to read and return the version from the pinned scripts/gitleaks.version file instead of hardcoding 8.30.1, while preserving the existing version-command behavior and scan output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@collect-diff-context-cli/src/secret_scan.rs`:
- Around line 319-329: The secret-scan pipeline must fail closed after Gitleaks
confirms a finding but redaction fails. In
collect-diff-context-cli/src/secret_scan.rs lines 319-329, update the
RedactionFailed path to withhold or replace the original content; in
scripts/collect_diff_context.sh lines 145-148, stop retaining or releasing input
for a redaction-failed report; and in tests/secret_gate_test.sh lines 390-397,
assert that the confirmed secret is absent.
In `@install.sh`:
- Around line 546-547: Update the Gitleaks setup around
resolve_gitleaks_platform and gitleaks_binary_name so unsupported OS or
architecture does not terminate installation. Defer platform resolution behind
the download-enabled path, or handle failure as an unavailable optional binary,
allowing --no-download and graceful degradation to continue without bundled
redaction.
In `@scripts/fetch_gitleaks.sh`:
- Around line 68-79: Update the platform-detection logic in the script’s mode
dispatch so host OS and architecture resolution occurs only for current-mode
execution. Ensure --all and --platform proceed using their explicit target
values without exiting on an unsupported host platform, while preserving
validation and errors for current mode.
- Around line 87-109: Update the download function’s curl and wget invocations
to enforce a single overall download timeout, covering stalled response bodies
as well as connection setup. Add the corresponding timeout option to every curl
mode and each wget progress-mode branch, preserving the existing progress and
output behavior.
In `@scripts/lib/gitleaks_integrity.sh`:
- Around line 54-94: Update gitleaks_smoke_scan so the validated configuration
path remains usable after changing into config_dir: resolve the relative config
path to an absolute path before the subshell invokes the executable, while
preserving absolute paths and existing validation behavior.
In `@tests/lib/normalize_parity_output.py`:
- Around line 15-23: Update the scan-metadata removal loop in
normalize_parity_output.py to stop before a line beginning with “unit_id:”,
preserving that adjacent record as the next output record. In
tests/parity_assets_test.sh lines 51-70, add a scan section immediately followed
by unit_id:, and in lines 88-95, assert the adjacent record remains after
normalization.
---
Nitpick comments:
In `@scripts/bin/collect_diff_context-darwin-arm64`:
- Line 1: Add a SHA-256 checksum manifest covering every
scripts/bin/collect_diff_context-* artifact shipped by build_all_binaries.sh and
the release workflow, following the format and placement of
scripts/gitleaks-binaries.sha256. Ensure all platform binaries are included so
consumers can validate each downloaded artifact.
In `@tests/helper_shadow_mode_test.sh`:
- Around line 59-69: Update the fake gitleaks script in the test setup to read
and return the version from the pinned scripts/gitleaks.version file instead of
hardcoding 8.30.1, while preserving the existing version-command behavior and
scan output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc42ecf0-2352-4fc3-97d1-3ace427a7fe2
⛔ Files ignored due to path filters (2)
collect-diff-context-cli/Cargo.lockis excluded by!**/*.lockscripts/bin/collect_diff_context-windows-amd64.exeis excluded by!**/*.exe
📒 Files selected for processing (41)
.github/workflows/lint.yml.github/workflows/release.yml.gitignoreREADME.mdREADME.zh-CN.mdSKILL.mdTHIRD_PARTY_LICENSES/gitleaks-LICENSEcollect-diff-context-cli/Cargo.tomlcollect-diff-context-cli/src/main.rscollect-diff-context-cli/src/secret_scan.rsdocs/gitleaks-quality-evaluation.mddocs/helper-capabilities.mdevals/compare_output_eval_quality.shevals/compare_output_eval_quality_test.shevals/eval_contract_test.shevals/output/advanced-output-eval.jsonevals/output_eval_runner.shevals/output_eval_runner_test.shinstall.shreferences/security/gitleaks.tomlscripts/bin/collect_diff_context-darwin-amd64scripts/bin/collect_diff_context-darwin-arm64scripts/bin/collect_diff_context-linux-amd64scripts/build_all_binaries.shscripts/check_gitleaks.shscripts/collect_diff_context.shscripts/fetch_gitleaks.shscripts/gitleaks-assets.sha256scripts/gitleaks-binaries.sha256scripts/gitleaks.versionscripts/lib/gitleaks_integrity.shtests/control_plane_test.shtests/gitleaks_distribution_test.shtests/helper_shadow_mode_test.shtests/install_gitleaks_test.shtests/install_smoke_test.shtests/lib/normalize_parity_output.pytests/parity_assets_test.shtests/parity_golden_test.shtests/secret_gate_test.shtests/skill_contract_test.sh
👮 Files not reviewed due to content moderation or server errors (2)
- scripts/bin/collect_diff_context-darwin-amd64
- scripts/bin/collect_diff_context-linux-amd64
Comment on lines
+319
to
+329
| Err(error) => { | ||
| let status = if error.is_redaction_failure() { | ||
| SecretScanStatus::RedactionFailed(error.reason_code()) | ||
| } else { | ||
| SecretScanStatus::Unavailable(error.reason_code()) | ||
| }; | ||
| SanitizedOutput { | ||
| content: input.to_string(), | ||
| redactions: Vec::new(), | ||
| status, | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Fail closed once Gitleaks has confirmed a finding.
The current cross-layer contract releases original content after redaction fails, exposing a known secret.
collect-diff-context-cli/src/secret_scan.rs#L319-L329: withhold or replace content forRedactionFailed.scripts/collect_diff_context.sh#L145-L148: do not retain and release input when the report saysredaction-failed.tests/secret_gate_test.sh#L390-L397: require the confirmed secret to be absent.
📍 Affects 3 files
collect-diff-context-cli/src/secret_scan.rs#L319-L329(this comment)scripts/collect_diff_context.sh#L145-L148tests/secret_gate_test.sh#L390-L397
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@collect-diff-context-cli/src/secret_scan.rs` around lines 319 - 329, The
secret-scan pipeline must fail closed after Gitleaks confirms a finding but
redaction fails. In collect-diff-context-cli/src/secret_scan.rs lines 319-329,
update the RedactionFailed path to withhold or replace the original content; in
scripts/collect_diff_context.sh lines 145-148, stop retaining or releasing input
for a redaction-failed report; and in tests/secret_gate_test.sh lines 390-397,
assert that the confirmed secret is absent.
Comment on lines
+546
to
+547
| gitleaks_platform="$(resolve_gitleaks_platform)" | ||
| gitleaks_binary="$(gitleaks_binary_name "$gitleaks_platform")" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not make optional Gitleaks platform support a prerequisite for installation.
resolve_gitleaks_platform terminates installation on an unsupported OS/architecture before --no-download or graceful degradation can apply. Installation should continue without bundled redaction on such hosts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 546 - 547, Update the Gitleaks setup around
resolve_gitleaks_platform and gitleaks_binary_name so unsupported OS or
architecture does not terminate installation. Defer platform resolution behind
the download-enabled path, or handle failure as an unavailable optional binary,
allowing --no-download and graceful degradation to continue without bundled
redaction.
Comment on lines
+68
to
+79
| case "$(uname -s | tr '[:upper:]' '[:lower:]')" in | ||
| darwin) CURRENT_OS='darwin' ;; | ||
| linux) CURRENT_OS='linux' ;; | ||
| msys*|mingw*|cygwin*) CURRENT_OS='windows' ;; | ||
| *) printf 'unsupported operating system\n' >&2; exit 1 ;; | ||
| esac | ||
|
|
||
| case "$(uname -m)" in | ||
| arm64|aarch64) CURRENT_ARCH='arm64' ;; | ||
| x86_64|amd64) CURRENT_ARCH='amd64' ;; | ||
| *) printf 'unsupported architecture\n' >&2; exit 1 ;; | ||
| esac |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Resolve the host platform only in current mode.
--all and --platform are explicit cross-target operations, but an unsupported host OS or architecture exits before either mode runs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/fetch_gitleaks.sh` around lines 68 - 79, Update the
platform-detection logic in the script’s mode dispatch so host OS and
architecture resolution occurs only for current-mode execution. Ensure --all and
--platform proceed using their explicit target values without exiting on an
unsupported host platform, while preserving validation and errors for current
mode.
Comment on lines
+87
to
+109
| download() { | ||
| local url="$1" | ||
| local output="$2" | ||
| if command -v curl >/dev/null 2>&1; then | ||
| local -a curl_args=(-fL --show-error --retry 5 --retry-all-errors --connect-timeout 15) | ||
| if [ "$PROGRESS_MODE" = 'always' ] || { [ "$PROGRESS_MODE" = 'auto' ] && [ -t 2 ]; }; then | ||
| curl_args+=(--progress-bar) | ||
| else | ||
| curl_args+=(--silent) | ||
| fi | ||
| curl "${curl_args[@]}" "$url" -o "$output" | ||
| elif command -v wget >/dev/null 2>&1; then | ||
| case "$PROGRESS_MODE" in | ||
| always) wget --progress=bar:force:noscroll -O "$output" "$url" ;; | ||
| auto) | ||
| if [ -t 2 ]; then | ||
| wget -O "$output" "$url" | ||
| else | ||
| wget -qO "$output" "$url" | ||
| fi | ||
| ;; | ||
| never) wget -qO "$output" "$url" ;; | ||
| esac |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add an overall download timeout.
The connection timeout does not bound a stalled response body, and the wget path has no timeout at all. A network stall can hang installation or release staging indefinitely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/fetch_gitleaks.sh` around lines 87 - 109, Update the download
function’s curl and wget invocations to enforce a single overall download
timeout, covering stalled response bodies as well as connection setup. Add the
corresponding timeout option to every curl mode and each wget progress-mode
branch, preserving the existing progress and output behavior.
Comment on lines
+54
to
+94
| gitleaks_smoke_scan() { | ||
| local executable="$1" | ||
| local config="$2" | ||
| local tmp_dir | ||
| local report_file | ||
| local error_file | ||
| local config_dir | ||
| local compact_report | ||
|
|
||
| [ -x "$executable" ] && [ -f "$config" ] || return 1 | ||
| tmp_dir="$(mktemp -d)" || return 1 | ||
| report_file="$tmp_dir/report.json" | ||
| error_file="$tmp_dir/error.log" | ||
| config_dir="$(CDPATH='' cd -- "$(dirname -- "$config")" && pwd -P)" || { | ||
| rm -rf "$tmp_dir" | ||
| return 1 | ||
| } | ||
|
|
||
| if ! ( | ||
| cd "$config_dir" | ||
| printf '' | "$executable" \ | ||
| --config "$config" \ | ||
| --ignore-gitleaks-allow \ | ||
| --redact=100 \ | ||
| --exit-code=42 \ | ||
| --no-banner \ | ||
| --no-color \ | ||
| --log-level=error \ | ||
| --max-decode-depth=5 \ | ||
| --report-format=json \ | ||
| --report-path=- \ | ||
| stdin > "$report_file" 2> "$error_file" | ||
| ); then | ||
| rm -rf "$tmp_dir" | ||
| return 1 | ||
| fi | ||
|
|
||
| compact_report="$(tr -d '[:space:]' < "$report_file")" | ||
| rm -rf "$tmp_dir" | ||
| [ "$compact_report" = '[]' ] | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve relative configuration overrides during the smoke scan.
A relative PRE_COMMIT_REVIEW_GITLEAKS_CONFIG can pass Line 63, but after changing directories, Line 75 resolves it relative to config_dir again and rejects an otherwise valid configuration.
Proposed fix
local config_dir
+ local config_file
local compact_report
[ -x "$executable" ] && [ -f "$config" ] || return 1
tmp_dir="$(mktemp -d)" || return 1
report_file="$tmp_dir/report.json"
error_file="$tmp_dir/error.log"
config_dir="$(CDPATH='' cd -- "$(dirname -- "$config")" && pwd -P)" || {
rm -rf "$tmp_dir"
return 1
}
+ config_file="$config_dir/$(basename -- "$config")"
if ! (
cd "$config_dir"
printf '' | "$executable" \
- --config "$config" \
+ --config "$config_file" \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| gitleaks_smoke_scan() { | |
| local executable="$1" | |
| local config="$2" | |
| local tmp_dir | |
| local report_file | |
| local error_file | |
| local config_dir | |
| local compact_report | |
| [ -x "$executable" ] && [ -f "$config" ] || return 1 | |
| tmp_dir="$(mktemp -d)" || return 1 | |
| report_file="$tmp_dir/report.json" | |
| error_file="$tmp_dir/error.log" | |
| config_dir="$(CDPATH='' cd -- "$(dirname -- "$config")" && pwd -P)" || { | |
| rm -rf "$tmp_dir" | |
| return 1 | |
| } | |
| if ! ( | |
| cd "$config_dir" | |
| printf '' | "$executable" \ | |
| --config "$config" \ | |
| --ignore-gitleaks-allow \ | |
| --redact=100 \ | |
| --exit-code=42 \ | |
| --no-banner \ | |
| --no-color \ | |
| --log-level=error \ | |
| --max-decode-depth=5 \ | |
| --report-format=json \ | |
| --report-path=- \ | |
| stdin > "$report_file" 2> "$error_file" | |
| ); then | |
| rm -rf "$tmp_dir" | |
| return 1 | |
| fi | |
| compact_report="$(tr -d '[:space:]' < "$report_file")" | |
| rm -rf "$tmp_dir" | |
| [ "$compact_report" = '[]' ] | |
| } | |
| gitleaks_smoke_scan() { | |
| local executable="$1" | |
| local config="$2" | |
| local tmp_dir | |
| local report_file | |
| local error_file | |
| local config_dir | |
| local config_file | |
| local compact_report | |
| [ -x "$executable" ] && [ -f "$config" ] || return 1 | |
| tmp_dir="$(mktemp -d)" || return 1 | |
| report_file="$tmp_dir/report.json" | |
| error_file="$tmp_dir/error.log" | |
| config_dir="$(CDPATH='' cd -- "$(dirname -- "$config")" && pwd -P)" || { | |
| rm -rf "$tmp_dir" | |
| return 1 | |
| } | |
| config_file="$config_dir/$(basename -- "$config")" | |
| if ! ( | |
| cd "$config_dir" | |
| printf '' | "$executable" \ | |
| --config "$config_file" \ | |
| --ignore-gitleaks-allow \ | |
| --redact=100 \ | |
| --exit-code=42 \ | |
| --no-banner \ | |
| --no-color \ | |
| --log-level=error \ | |
| --max-decode-depth=5 \ | |
| --report-format=json \ | |
| --report-path=- \ | |
| stdin > "$report_file" 2> "$error_file" | |
| ); then | |
| rm -rf "$tmp_dir" | |
| return 1 | |
| fi | |
| compact_report="$(tr -d '[:space:]' < "$report_file")" | |
| rm -rf "$tmp_dir" | |
| [ "$compact_report" = '[]' ] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/gitleaks_integrity.sh` around lines 54 - 94, Update
gitleaks_smoke_scan so the validated configuration path remains usable after
changing into config_dir: resolve the relative config path to an absolute path
before the subshell invokes the executable, while preserving absolute paths and
existing validation behavior.
Comment on lines
+15
to
+23
| index += 1 | ||
| while index < len(lines): | ||
| line = lines[index] | ||
| if line.strip() == "": | ||
| index += 1 | ||
| break | ||
| if line.lstrip().startswith("#"): | ||
| break | ||
| index += 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve adjacent split-preview records when removing scan metadata.
tests/lib/normalize_parity_output.py#L15-L23: treatunit_id:as the start of the next output record.tests/parity_assets_test.sh#L51-L70: add a scan section followed immediately byunit_id:.tests/parity_assets_test.sh#L88-L95: assert the adjacent record remains after normalization.
📍 Affects 2 files
tests/lib/normalize_parity_output.py#L15-L23(this comment)tests/parity_assets_test.sh#L51-L70tests/parity_assets_test.sh#L88-L95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lib/normalize_parity_output.py` around lines 15 - 23, Update the
scan-metadata removal loop in normalize_parity_output.py to stop before a line
beginning with “unit_id:”, preserving that adjacent record as the next output
record. In tests/parity_assets_test.sh lines 51-70, add a scan section
immediately followed by unit_id:, and in lines 88-95, assert the adjacent record
remains after normalization.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…itleaks
Summary by CodeRabbit
New Features
Bug Fixes
Documentation