diff --git a/evals/README.md b/evals/README.md new file mode 100644 index 0000000..b6c6c86 --- /dev/null +++ b/evals/README.md @@ -0,0 +1,185 @@ +# Empirical review findings + +This directory records what actually happened when this repo (`secretscan-loop`, a Rust CLI +that scans a directory for hardcoded secrets, triages candidates through an LLM persona +pipeline, and gates CI/pre-push on the verdict) was reviewed and then actually run. Two rounds +were static code review (reading `src/`, `README.md`, and `docs/design-spec.md` against each +other, no LLM API calls); the third round was real execution — actual `claude -p --model haiku` +calls against the repo's own scanner, with real cost. All numbers below are from things that +were actually checked (`grep`, `git log`, reading generated `report.md`/`state.json`, or a +disposable Rust test), not estimated or asserted from memory. + +## TL;DR — issues found and real cost + +| Round | What | Issues filed | Issues closed | Real LLM cost | +|---|---|---|---|---| +| 1 — static review | Read `scanners.rs`, `lens.rs`, `main.rs` against `README.md`'s safety claims | #2, #3, #4 | #2, #3 fixed same round; #4 left open (stated low confidence) | $0 | +| 2 — deepened static review | Re-examined masking/fingerprint/coverage-count logic specifically | #5, #6, #7 | #5, #6 fixed (same commit also closes #4); #7 fixed | $0 | +| 3 — real CLI execution | `claude -p --model haiku`: scan x2, describe x1, against a small AWS-key/GitHub-token test fixture | #8 | #8 fixed | **$0.7846** | +| **Total** | | **7 issues (#2–#8)** | **7/7 closed** | **$0.7846** | + +Cost breakdown for round 3 (the only round that made real API calls): + +| Call | Purpose | Cost | +|---|---|---| +| scan #1 (`manual-test`) | Full pipeline scan of a 4-file test tree (AWS key + secret pair + GitHub token) | $0.3525 | +| describe (`manual-test`) | `describe.md` generation from the same scan | $0.0308 | +| scan #2 (`exit-check`) | Independent rerun, used to check the exit-code contract | $0.4013 | +| **Total** | | **$0.7846** | + +## What this bought + +- **A real safety-guarantee violation, not a style nit.** #2: for the + `generic_high_entropy_assignment` rule, `mask()` was fed the *whole regex match* + (`m.as_str()`, keyword + operator + quotes + secret) instead of the actual secret capture + group. Since `mask()` keeps a few characters at the tail of whatever it's given, the tail of + the whole match — the end of the real secret plus the closing quote — leaked into + `masked_preview` and `context_line` in plaintext. Those two fields flow straight into the LLM + review prompt and into `report.md`. This directly contradicted the README's stated core + guarantee ("Nothing in this tool ever prints, logs, or serializes a raw secret value") for + the one rule most likely to fire on arbitrary `KEY = "..."`-shaped code. Fixed in `73d2231` by + giving each `Rule` a `secret_group` field and using `captures_iter` + that group instead of + the whole match. +- **A low-confidence issue confirmed, not dismissed, in the next round.** #4 was explicitly + filed as "[Low confidence, needs verification]" — the concern (dedup `fingerprint()` includes + `rule_id`, so builtin `aws_access_key_id` vs. gitleaks `aws-access-token` vs. trufflehog `AWS` + could all describe the same secret without deduping) could not be tested because gitleaks and + trufflehog binaries weren't available in the review environment. Round 2's #6 reproduced the + *identical* root-cause mechanism entirely within the builtin scanner — no external tools + needed — with a deterministic repro: a one-line `config.env` fixture matched both + `aws_access_key_id` and `generic_high_entropy_assignment`, producing two un-deduped + `Candidate`s for the same physical secret, each separately penalized in `quantify::score`. + Commit `4bf3f3f` fixed both in one change (drop `rule_id` from the fingerprint) and its footer + reads `Fixes #5, Fixes #6, Closes #4` — a concrete instance of "flagged with low confidence" + turning into "verified true" one round later, not a hedge that just sat there. +- **#5 — the masking formula itself under-delivered on its own spec.** `mask()`'s + `clamp(n/5, 3, 6)` forced 3 head + 3 tail characters to show in plaintext even just above the + `n<=8` full-mask cutoff — up to 67% of a 9-character secret, against `docs/design-spec.md` + §3's documented contract of "first 4 and last 4 characters" only. Confirmed by iterating + `mask()` over lengths 9–20 in a disposable test and printing the actual reveal ratio, not by + inspection alone. Fixed by lowering the clamp floor from 3 to 1 (same commit as #6, `4bf3f3f`). +- **#7 — reported scan coverage was fabricated by an order of magnitude in realistic repos.** + `files_scanned` counted every file under the target directory, including everything inside + `.git`, `node_modules`, `target`, `dist`, `build`, `.venv`/`venv`, `__pycache__` — directories + `builtin_scan` itself always skips. Measured, not assumed: a test directory with 20 files under + `node_modules/` plus 1 real source file reported `files_scanned=21` while exactly 1 file's + content was actually regex-scanned. In a populated real repo this inflation is orders of + magnitude larger, and it feeds a number a PASS verdict's credibility depends on. Fixed in + `19de6aa` by reusing the same `should_skip` filter `builtin_scan` already applies. +- **#3 — a correctness bug in the discourse-carryover path, not a scanning bug.** `Finding.id` + was generated as `"{lens_id}-{i+1}"` with no round number, unlike `discourse.rs`'s own + round-scoped surface-finding ids (`"surface-r{round}-{i+1}"`). Across a multi-round `--prior` + chain, two separate rounds of the same lens could produce colliding ids, and the + `STILL_OPEN` carry-forward path in `main.rs` re-inserts a prior-round finding into the + *current* round's `resolved` map keyed by that id — silently overwriting an unrelated + current-round finding's already-computed discourse resolution. Fixed in `f8ff490` by threading + the round number into the id (`"{lens_id}-r{round}-{i+1}"`), matching the convention + `discourse.rs` already used. +- **Actually running the CLI found a bug static reading missed: stale documentation of a + just-fixed security-relevant formula.** #8 was found only by executing `claude -p` end to end + against a real test fixture (AWS key + secret pair in `src/config.py`, a GitHub token in + `src/deploy.sh`) and manually cross-checking the real `masked_preview` values in `report.md` + against README.md's Safety design section. The code and its inline comment were correctly + updated by #5's fix; the README's prose was not, and still described the disproven + `clamp(n/5, 3, 6)` formula. The divergence is invisible for secrets ≥15 characters (the two + clamp floors give the same result there) and only shows up in the 9–14 character range #5 was + specifically about — exactly the kind of gap that reading the diff, rather than running the + tool and reading its actual output, would miss. Fixed in `d6b8e0e`. +- **Execution surfaced real, verifiable positive evidence too, not just one more bug.** Both real + scan runs (`runs/manual-test/`, `runs/exit-check/`) reported `files_scanned=4` (correct for the + 4-file fixture tree, consistent with #7's fix — no inflation observed). Hand-checked every + `masked_preview` value against the post-#5 formula: `AKIA****...****MNOP (len=20)` → + `keep = clamp(20/5, 1, 6) = 4` ✓; `wJalrX****...****PLEKEY (len=40)` and + `ghp_AB****...****456789 (len=40)` → `keep = clamp(40/5, 1, 6) = 6` ✓ — both correct. Grepped + both `report.md` files and both `state.json` files for any unmasked occurrence of the actual + scanned secret strings: none found. One apparent hit needs a caveat, recorded honestly rather + than dropped: `exit-check/report.md` contains the literal string `AKIAIOSFODNN7EXAMPLE` — but + that's the LLM reviewer's own free-text reference to AWS's *publicly published* documentation + placeholder key ("Verify if this matches AWS example key (AKIAIOSFODNN7EXAMPLE)"), not the + tool re-emitting the actual scanned fixture value, which was a different string ending in + `...MNOP`. Both real runs produced `Verdict: BLOCK`, which by the documented and implemented + contract (`README.md:403`, `src/main.rs:49`, `BLOCK => 3`) maps to exit code 3 — consistent + with what round 3 set out to check, though the numeric exit code itself was captured during + the live run, not re-executed for this write-up. +- **All 7 issues filed across all three rounds were closed**, each by a commit that fixed the + underlying code (or, for #8, the stale doc) rather than by discussion — every fix commit's + message includes the concrete root cause, the fix, and a `Fixes #N` footer. + +## Round 1 — static code review + +Read `src/scanners.rs`, `src/lens.rs`, `src/main.rs`, `README.md`, and `docs/design-spec.md` +against each other, no code execution. Filed: + +- **#2 — Secret suffix leaks into `masked_preview`/`context_line`** for + `generic_high_entropy_assignment`. Security-priority: this breaks the tool's own core + guarantee. Fixed in `73d2231`. +- **#3 — Finding ids collide across rounds**, `STILL_OPEN` carry-forward can overwrite an + unrelated finding's resolution. Fixed in `f8ff490`. +- **#4 — [Low confidence, needs verification] dedup fingerprint's `rule_id` may block + cross-source dedup.** Explicitly filed as unverified — gitleaks/trufflehog binaries weren't + available to test the cross-source claim directly, so this was reasoning from reading the + code, stated as such rather than presented as confirmed. Left open pending round 2. + +## Round 2 — deepened static review + +Went back specifically at the masking/fingerprint/coverage-count logic that round 1's reading +had flagged as worth a second, more skeptical pass. Filed: + +- **#5 — `mask()` reveals up to 67% of a secret in plaintext for lengths 9–19**, violating + `design-spec.md` §3. Confirmed with an actual reveal-ratio test across lengths 9–20, not by + eyeballing the clamp math. +- **#6 — `fingerprint()`'s `rule_id` lets the same secret produce duplicate un-deduped + candidates.** This is what turned #4 from a stated-low-confidence hypothesis into a confirmed + bug: instead of needing gitleaks/trufflehog, #6 found the exact same failure mode reproducible + purely within the builtin scanner (one secret matched by two builtin rules on one line). Fixed + together with #5 in `4bf3f3f`, whose commit footer explicitly closes #4 alongside fixing #5 + and #6. +- **#7 — `files_scanned` counts files inside skipped directories** (`.git`, `node_modules`, + `target`, `dist`, `build`, `.venv`/`venv`, `__pycache__`), wildly inflating reported coverage. + Measured with a 21-file/1-actually-scanned test directory. Fixed in `19de6aa`. + +## Round 3 — real CLI execution + +This round stopped reading code and actually ran the shipped binary end to end: +`claude -p --model haiku` against a small, self-contained test fixture (`src/config.py` with an +AWS access key + secret key pair, `src/deploy.sh` with a GitHub token — 4 files total). Two +scans and one describe call were made; outputs are preserved at `runs/manual-test/` (scan + +describe) and `runs/exit-check/` (independent rescan used to check the exit-code contract). + +- Both runs correctly classified all three candidates, reached `Verdict: BLOCK`, `Score: 0/100`, + and correctly rejected the two candidates that matched well-known public documentation + placeholder values (AWS's published `wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY` example secret, + a GitHub token with an implausibly sequential numeric suffix) via discourse `CHALLENGE`/reject, + rather than blindly flagging every regex match as confirmed. +- `files_scanned=4` in both runs — correct, and consistent with #7's fix already being in place + by this round. +- **#8 — README masking formula is stale after #5's fix**, found by cross-checking real + `masked_preview` output against README.md's Safety design section during this run, not by + reading the diff. The code itself was already safe (verified: the actual masked values in both + `report.md`s match the post-#5 `clamp(n/5, 1, 6)` formula exactly); only the documentation + still described the pre-fix `clamp(n/5, 3, 6)` formula #5 had disproven. Fixed in `d6b8e0e`. +- Grepped both real run directories (`runs/manual-test/`, `runs/exit-check/`) for the actual + scanned secret strings in unmasked form: not found in either `report.md` or either + `state.json`. See the caveat above about `AKIAIOSFODNN7EXAMPLE` — a real string match, but the + LLM reviewer citing a public AWS documentation placeholder by name in its own reasoning text, + not the tool disclosing the fixture's actual (different) secret value. +- Exit-code contract: both runs' `Verdict: BLOCK` corresponds to exit code `3` per the documented + and implemented contract (`README.md:403`: "`PASS=0`, `WARN=2`, `BLOCK=3`, run/config error=4"; + `src/main.rs:49`: `"BLOCK" => 3`) — the report output and the code agree with each other. + +## Caveats + +- Round 1 and round 2 were static review — no LLM API calls, $0 real cost, and no execution of + the scanner against real input. Everything found there is reasoning from reading the code, not + observed runtime behavior. #4 is the explicit example of that limitation being stated honestly + instead of glossed over, and of it then actually mattering (round 2 found the same bug through + execution-adjacent reasoning that didn't need the missing binaries). +- Round 3 is two scans and one describe call against one small, synthetic 4-file fixture. This + confirms the specific things checked (files_scanned count, masked_preview values, exit code for + a BLOCK verdict, one doc/code drift) on that one fixture — it is not a benchmark and doesn't + establish detection recall/precision or non-determinism across repeated runs the way, e.g., + Code-Review-Loop's own `evals/README.md` does for that project's discourse pipeline. +- All fixes here were verified by either a disposable Rust test (#5, #6, #7 — since removed after + confirming) or direct inspection of real run output (#2, #8); none of the 7 fixes were re-run + through a second live `claude -p` call after fixing to reconfirm end-to-end (that would need + additional real API spend beyond the $0.7846 already spent, and wasn't done).