test(release): close scanner guard coverage gaps - #425
Conversation
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at d2713347f4ad06da301261d7c984e35bf7b9f186.
The production change is one deleted line, and finding it required understanding the scanner rather than reading it:
- "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" # its matching secretI checked that string against all nine patterns. None of gsk_, sk-, sk-proj-, sk-ant-, ghp_, github_pat_, AKIA, AIza or xox[baprs]- can produce it as a hit, because an AWS secret access key is generic base64 with no distinctive prefix. So the exemption could never fire. It was an allowlist entry protecting against a match that cannot happen, and your replacement comment says exactly that:
# AWS secret access keys have no distinctive prefix; no pattern covers their
# generic base64 format, so the matching secret example needs no exemption.The guard you added is what makes this more than a tidy-up. Every allowlist entry must now be reachable by some complete pattern, and that check fails on main today, which is the proof the test is coupled to the change rather than decorative. I also like that the failure message truncates the offending value ('${example:0:12}…') instead of echoing a full credential-shaped string into CI logs.
Three things I looked for specifically and did not find:
The provider labels the test greps for are not self-fulfilling. They come from the test's own POSITIVES table, but lines 78-90 assert set equality between those and the providers parsed out of scripts/check_no_secrets.sh, so renaming a provider in the scanner turns the test red.
No fail-open path. read_array returning nothing exits 1 with a named failure rather than treating an empty selection as a pass, and a crashing scanner falls through to the grep branch and fails there. The grep -qF … <<< "$out" herestrings avoid the pipefail-plus-SIGPIPE race that makes this kind of suite flaky on a different assertion each run.
The mapfile < <(...) anti-pattern is removed here, not added. main had mapfile -t tracked < <(git ls-files), which discarded git's exit status; you replaced it with a redirect through a temp file guarded by if !, plus a sentinel that fails when the enumeration comes back without scripts/check_no_secrets.sh in it. That is the difference between "asked and got nothing" and "could not ask", and you put it on the right side.
Real coverage added too: the Slack pattern had no positive case at all before this, and the bidirectional provider check stops a pattern shipping uncovered again.
One optional note. The reachability check uses grep -qxE (whole line), while the scanner extracts hits with unanchored grep -oE. For a greedy open-ended pattern such as sk-ant-[A-Za-z0-9_-]{40,}, an allowlist entry could satisfy the whole-line test while the scanner's maximal munch produced a longer hit that the exact-match loop then missed. No current entry has that shape, so this is theoretical. Worth a comment if you touch the file again.
Approving. Deleting dead code and adding the guard that would have caught it is the right pair.
|
Your new arm caught my merge gate, which I did not expect and which is the best kind of evidence. I updated your branch against Before this PR, The refusal belongs to my tooling rather than to your branch, so it does not change the review. CI runs the test against a real checkout and passes. I am fixing the extraction on my side; nothing to push. Approval stands at the new head, and the branch merges clean against |
Summary
The release guard currently stays green when the Slack pattern disappears or the tracked-file list is empty. This adds provider-labelled synthetic cases, checks both directions of pattern/case coverage, requires each provider's own diagnostic, and verifies that exact allowlist entries are reachable. It also checks Git's enumeration status and scanner sentinel, preserves NUL-delimited paths, and captures dirty-tree diagnostics before displaying ten lines so the guard exits 1 instead of SIGPIPE.
The scanner change removes only the unreachable AWS secret-example exemption and explains why. Detection patterns and the staged-enumeration/blob-read handling from #410 remain unchanged, as do the existing no-echo and staged-path regressions. No Rust, public API, release figure, workflow or #346 wiring changes.
Related Issue
Closes #387. Continues the scoped invitation in #387 (comment) and the disclosed claim in #387 (comment).
Validation
Validated on Ubuntu/WSL against
61b3a878e96ececf19125ec5af15f665d33853b9:bash tests/release/no-secrets.test.shpassed on the complete tracked tree, including the unchanged staged/no-echo cases.bash -n scripts/check_no_secrets.sh tests/release/no-secrets.test.sh, ShellCheck 0.11.0--severity=warningon both changed files,python3 scripts/check_evidence_claims.py ., andgit diff --checkpassed. The local CI runner's repository-wide ShellCheck also passed.bash scripts/ci-local.sh --no-postgrescompleted with 28 passing steps, four failures, five unavailable-tool warnings and one deliberate Postgres skip. Direct execution of all 22 release scripts gave 20 passes and two failures.The two release failures also reproduce on the clean pinned upstream:
release-rehearsal.test.sh:33executes an unquoted path containing spaces (exit 127), anddocs-share-cards.test.shencounters the Windows Git checkout's symlink placeholder instead of a PNG. Workspace Clippy and rustdoc cannot build because this WSL installation lacks GLib/GObject/GIO/GDK development packages. No Rust files changed; this uses CONTRIBUTING's no-Rust carveout. Missing cargo-nextest, cargo-audit, markdownlint, markdown-link-check and yamllint remain unvalidated. Hosted Ubuntu CI subsequently completed all four workflows successfully ond2713347f4ad06da301261d7c984e35bf7b9f186at 2026-09-11T21:39:30.699808Z: ten jobs passed and container-smoke was skipped. The skipped job remains unvalidated, and the local broad-run limitations above still apply.Notes for Reviewers
Prepared, tested, and reviewed autonomously by OpenAI Codex through LunaMeerkats. The complete two-file diff received a separate automated review; human review is not claimed.
The table reader deliberately follows the scanner's existing quoted-array format. The coverage check catches a pattern or case changed independently; coordinated removal of both remains outside its claim. The local mutation harness uses temporary Git repositories and synthetic filler; it is validation evidence rather than an added repository framework. The initial added-provider fixture accidentally matched its own literal; it was corrected and the complete baseline comparison rerun before drawing the results above.