test: centralize scrubber fixtures + add regression meta-test - #2
Merged
Merged
Conversation
Consolidates 6 fake-key/fake-PII placeholder strings into a single `src/test_fixtures.rs` module so the scrubber test suite has one source of truth. Previously the same literals lived in 3 files (`src/lib.rs`, `src/pipeline/scrubber.rs`, `tests/pipeline_e2e.rs`), which is what triggered KeyScout issue #1 on the Anthropic placeholder. Strings are obviously not real secrets (Amazon publishes AKIAIOSFODNN7EXAMPLE as their documented 'fake' key; the rest are alphabet+digits patterns). See `src/test_fixtures.rs` module doc for full rationale and scanner caveats. Also adds `tests/scrubber_fixtures_meta.rs` with two meta-tests: - every_fixture_is_redacted_by_the_scrubber: catches future fixtures added without a matching regex in with_safe_defaults - fixture_strings_match_their_declared_regex_pattern: catches fixture/regex drift (e.g., tightened regex no longer matching the declared literal) All 63 tests pass. No public API changes.
See CHANGELOG.md for full notes. Yanks v0.6.1 post-publish via `cargo yank --version 0.6.1` (see MANIFEST-2026-07-28).
📝 WalkthroughWalkthroughThe release updates the crate to 0.6.2, centralizes non-secret scrubber fixtures, migrates existing tests to use them, and adds meta-tests verifying every fixture is redacted and matches its configured regex. ChangesScrubber fixture maintenance
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/lib.rs`:
- Around line 11-14: Restrict the test_fixtures module declaration in src/lib.rs
to test-only compilation, preferably with #[cfg(test)], so it is not exposed
through the public crate API; no direct change is required in CHANGELOG.md
because the public API change should be removed.
In `@src/pipeline/scrubber.rs`:
- Around line 122-129: Update the redacts_email_address test to assert that the
scrubbed message no longer contains the shared FAKE_EMAIL fixture, rather than
checking the hardcoded `@b.com` suffix. Keep the existing redaction-kind assertion
unchanged.
In `@tests/scrubber_fixtures_meta.rs`:
- Around line 1-15: Replace the separately maintained fixture-name and
fixture-value arrays in the meta-test with one authoritative table containing
each fixture’s name, SecretKind, and value. Update both test paths, including
the sections around the existing arrays and the additional referenced ranges, to
iterate over this shared table so adding or removing a fixture cannot leave the
lists out of sync.
🪄 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 Plus
Run ID: 0e4eab56-23eb-48e1-a01d-da08312739b7
📒 Files selected for processing (7)
CHANGELOG.mdCargo.tomlsrc/lib.rssrc/pipeline/scrubber.rssrc/test_fixtures.rstests/pipeline_e2e.rstests/scrubber_fixtures_meta.rs
Comment on lines
+11
to
+14
| // Test-only fixtures. `#[doc(hidden)]` keeps this out of rustdoc; | ||
| // `pub` is required so integration tests under `tests/` can import it. | ||
| #[doc(hidden)] | ||
| pub mod test_fixtures; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching lib.rs / CHANGELOG =="
git ls-files | grep -E '(^|/)src/lib\.rs$|(^|/)CHANGELOG\.md$' || true
echo
echo "== src/lib.rs outline =="
ast-grep outline src/lib.rs --view expanded 2>/dev/null || true
echo
echo "== src/lib.rs relevant lines =="
cat -n src/lib.rs | sed -n '1,80p'
echo
echo "== CHANGELOG relevant lines =="
cat -n CHANGELOG.md | sed -n '1,60p'
echo
echo "== public declarations / test fixture references =="
rg -n "test_fixtures|fixture|pub (mod|struct|const|enum|fn)|#\[doc\(hidden\)\]" src CHANGELOG.md -S || true
echo
echo "== test_fixtures module files =="
git ls-files | grep 'test_fixtures\|fixtures' || trueRepository: LOUST-PRO/LLMmempipe
Length of output: 13236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test_fixtures contents =="
cat -n src/test_fixtures.rs | sed -n '1,120p'
echo
echo "== test import sites =="
cat -n src/pipeline/scrubber.rs | sed -n '55,90p'
cat -n tests/scrubber_fixtures_meta.rs | sed -n '1,140p'
echo
echo "== cargo workspace / test config context =="
git ls-files | grep -E '(^|/)Cargo\.toml$|(^|/)tests/' | sort | sed -n '1,120p'Repository: LOUST-PRO/LLMmempipe
Length of output: 150
Avoid exposing the test fixture module to the public crate API.
#[doc(hidden)] suppresses rustdoc, but pub mod test_fixtures still allows downstream crates to import the module and its #\[doc(hidden)\] pub const fixtures. Keep it test-only, e.g. behind a #[cfg(test)] or test-only Cargo feature; if it must remain public, update the CHANGELOG note to acknowledge the public API change.
📍 Affects 2 files
src/lib.rs#L11-L14(this comment)CHANGELOG.md#L21-L24
🤖 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 `@src/lib.rs` around lines 11 - 14, Restrict the test_fixtures module
declaration in src/lib.rs to test-only compilation, preferably with
#[cfg(test)], so it is not exposed through the public crate API; no direct
change is required in CHANGELOG.md because the public API change should be
removed.
Comment on lines
122
to
129
| #[test] | ||
| fn redacts_email_address() { | ||
| let cfg = PipelineConfig::with_safe_defaults(); | ||
| let mut msg = make_msg("ping davidmirelesll@outlook.com later"); | ||
| let mut msg = make_msg(&format!("ping {} later", FAKE_EMAIL)); | ||
| let report = scrub(&mut msg, &cfg); | ||
| assert!(report.redacted_kinds.contains(&SecretKind::EmailAddress)); | ||
| assert!(!msg.content.contains("@outlook.com")); | ||
| assert!(!msg.content.contains("@b.com")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert removal of the shared fixture, not a stale suffix.
The input now uses FAKE_EMAIL, but the assertion still hardcodes @b.com. If the fixture changes, this can pass while the actual email remains in the output.
Proposed fix
- assert!(!msg.content.contains("`@b.com`"));
+ assert!(!msg.content.contains(FAKE_EMAIL));📝 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
| #[test] | |
| fn redacts_email_address() { | |
| let cfg = PipelineConfig::with_safe_defaults(); | |
| let mut msg = make_msg("ping davidmirelesll@outlook.com later"); | |
| let mut msg = make_msg(&format!("ping {} later", FAKE_EMAIL)); | |
| let report = scrub(&mut msg, &cfg); | |
| assert!(report.redacted_kinds.contains(&SecretKind::EmailAddress)); | |
| assert!(!msg.content.contains("@outlook.com")); | |
| assert!(!msg.content.contains("@b.com")); | |
| } | |
| #[test] | |
| fn redacts_email_address() { | |
| let cfg = PipelineConfig::with_safe_defaults(); | |
| let mut msg = make_msg(&format!("ping {} later", FAKE_EMAIL)); | |
| let report = scrub(&mut msg, &cfg); | |
| assert!(report.redacted_kinds.contains(&SecretKind::EmailAddress)); | |
| assert!(!msg.content.contains(FAKE_EMAIL)); | |
| } |
🤖 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 `@src/pipeline/scrubber.rs` around lines 122 - 129, Update the
redacts_email_address test to assert that the scrubbed message no longer
contains the shared FAKE_EMAIL fixture, rather than checking the hardcoded
`@b.com` suffix. Keep the existing redaction-kind assertion unchanged.
Comment on lines
+1
to
+15
| //! Meta-test: verifies the scrubber recognizes every fixture in | ||
| //! `test_fixtures` as a redactable pattern. | ||
| //! | ||
| //! This is a regression guard against two failure modes: | ||
| //! | ||
| //! 1. **Fixture added but regex not updated to match.** If a future | ||
| //! contributor adds a new `FAKE_*` constant to `src/test_fixtures.rs` | ||
| //! but forgets to add a corresponding pattern to | ||
| //! `PipelineConfig::with_safe_defaults`, this test catches it. | ||
| //! | ||
| //! 2. **Regex updated but fixture strings no longer match.** If the | ||
| //! scrubber's regex patterns get tightened (e.g., length minimum | ||
| //! increased), the fixture strings may no longer match, and this | ||
| //! test catches it before the individual `redacts_*` unit tests | ||
| //! silently start failing. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the meta-test fixture table authoritative.
The tests iterate manually maintained arrays, so adding a new FAKE_* constant without adding it to both arrays leaves the suite green, contrary to the documentation. The two lists can also drift. Define one table containing the fixture name, SecretKind, and value, then drive both tests from it.
Also applies to: 49-56, 84-91
🤖 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/scrubber_fixtures_meta.rs` around lines 1 - 15, Replace the separately
maintained fixture-name and fixture-value arrays in the meta-test with one
authoritative table containing each fixture’s name, SecretKind, and value.
Update both test paths, including the sections around the existing arrays and
the additional referenced ranges, to iterate over this shared table so adding or
removing a fixture cannot leave the lists out of sync.
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.
Summary
Consolidates 6 fake-key/fake-PII placeholder strings into a single
src/test_fixtures.rsmodule so the scrubber test suite has one source of truth. Also adds a regression meta-test that catches future fixture/regex drift.Closes the smell that triggered KeyScout issue #1 (false-positive security alert on the inline Anthropic placeholder).
What changed
src/test_fixtures.rs: public module with 6FAKE_*constants + module doc explaining why each is obviously not a real secretsrc/lib.rs: declares#[doc(hidden)] pub mod test_fixtures;+ refactorsscrubber_patterns_match_realistic_secretsto use the constantssrc/pipeline/scrubber.rs: refactors 7redacts_*unit tests to import fixturestests/pipeline_e2e.rs: refactorspipeline_with_secrets_redacts_and_records_statsto use fixturestests/scrubber_fixtures_meta.rs: 2 meta-tests verifying every fixture is recognized by the scrubber's regex patterns and survives redactionWhy this resolves issue #1
The original KeyScout flag pointed at
src/pipeline/scrubber.rs:104. That string (sk-ant-api03-abcdefghijklmnopqrstuvwxyz0123456789ABCD) is a constructed test fixture, not a real Anthropic key — real keys have ~93 random base62 chars after the prefix, never contiguous alphabet.The actual quality issue the report caught was that the same fixture shape was duplicated across 3 files. Consolidating to one source reduces future false-positive hits from upstream scanners that pattern-match on the prefix shape.
Test results
cargo fmt --check: cleancargo clippy --all-targets -- -D warnings: cleanScope boundary
This PR does NOT:
This PR does:
7a45dca)Post-merge plan (separate ops, gated)
cargo yank --version 0.6.1(hide v0.6.1 from newcargo addsearches)cargo publishfor v0.6.2git tag v0.6.2+ push tagloust davidmirelesll@outlook.com
Summary by CodeRabbit
What’s New
Bug Fixes
Notes