feature: Confidence Scoring & Tiered Detection - #115
Conversation
|
@addyCooks Thanks for the PR! Could you please resolve the merge conflicts with |
Every Finding now carries a `confidence` (0.0-1.0) and a `method` naming the rule that produced it, so an exact vendor key pattern (0.99, exact-pattern) is distinguishable from a high-entropy guess (0.6, entropy) or a capitalised-word name (0.5, heuristic). The scores reflect how the detectors already work; this makes that implicit precision explicit and filterable. - `--min-confidence <0-1>` on `scrub`, `inspect` and `watch`, `minConfidence` in `ScrubOptions` and in the config file. The flag overrides the config. - Filtering runs before collision resolution, so a discarded low-confidence finding can never mask a higher-confidence one that overlaps it. - `inspect` prints the score and method of every entity, so a threshold can be chosen before it is applied. - Defaults to 0: nothing is filtered and existing output is unchanged unless the threshold is set. - `confidence`/`method` are optional on the Detector interface, so published rule packs keep working; their findings score DEFAULT_CONFIDENCE (0.5). Closes Nano-Collective#95
f24bd75 to
08eb7a0
Compare
|
Thanks @akramcodez ! |
will-lamerton
left a comment
There was a problem hiding this comment.
Nice work, and thanks for the thorough testing notes. I verified the compatibility claim myself: built both main and this branch, ran the README payload through inspect, and both produce the same hash. Default behaviour really is unchanged. Full suite passes here too (290 tests, tsc --noEmit clean).
Filter-before-collision is the right ordering, and the SecretDetector widest-span / strongest-evidence split is a genuinely subtle catch - good spot.
Four things before merge:
1. The new README hash is wrong. For the exact input in that block (including the trailing .), the real output is:
Hash: 7e5eea933db987e10e10e259ebcfea9d3250d8a68925fd9360f515e3a4bfbba9
I get that on both main and this branch. The old 41beda4a... was already stale and 66fcffd8... is a different wrong value. This one matters more than a normal doc typo because the surrounding prose is teaching people that the hash is deterministic and verifiable - anyone who copy-pastes to check will conclude the tool is broken. The Email_1 -> «Email_1» corrections in the same block are right.
2. Nothing tells the user what the threshold dropped.
$ echo "mail alice@example.com and call 555-123-4567" | prompt-scrub scrub --min-confidence 0.9
mail «Email_1» and call 555-123-4567
Scrubbed: 1 entity (1 Email)
The phone number was detected and then discarded, and the output is indistinguishable from "there was no phone number." For a redaction tool, silent under-redaction is the dangerous direction, and this is a flag aimed at automated workflows where nobody runs inspect first. Can we have the summary say so, e.g. Scrubbed: 1 entity (1 Email); 1 suppressed below --min-confidence 0.9 (1 Phone)? runDetectors already has both sets in hand.
3. parseConfidence accepts trailing garbage. --min-confidence 0.9zzz silently runs at 0.9, because Number.parseFloat stops at the first invalid char. That contradicts the function's own doc comment about not silently scrubbing more or less than the user asked for. Number(value) rejects the whole string and still handles 0, 1, .85, 9e-1. Worth adding '0.9zzz' to the existing rejection test.
4. The changeset overstates the guarantee. "existing output is unchanged unless the threshold is set" is true for scrub but not inspect, whose per-entity lines gained the confidence suffix unconditionally. Showing the score by default is the right call, but the changeset ships to consumers, so it should say the display changed and that --hash remains the scripting-stable surface.
Nits, take or leave:
DEFAULT_CONFIDENCEis named in the rule-pack docs but isn't re-exported fromsrc/index.ts, so a pack author has to hardcode 0.5.ARRAY_KEYSis now a hand-maintained list parallel toCONFIG_KEYS, so a future config key silently gets no validation. Net improvement over what was there, but asatisfiesor a switch overCONFIG_KEYSwould make drift a type error.- The comment in
name.tsis backwards: strict mode skips matches containing an allowlisted word, it doesn't remove anything from the allowlist. handleScrubpassesminConfidenceunconditionally while every neighbouring option uses conditional spread. Harmless, just inconsistent.
…sing Addresses the review on Nano-Collective#115. The README hash in the inspect example was wrong. The block uses `echo`, which appends a newline, so the real output is 7e5eea93… — reproduced from a run rather than taken on trust. The previous value corresponds to the same text without the trailing newline. This one matters more than a normal doc typo because the surrounding prose teaches that the hash is verifiable. A threshold no longer drops findings silently. `runDetectors` returns the dropped findings alongside the kept ones, `ScrubStats` gains an optional `suppressed` field, and every surface reports it: $ echo "mail alice@example.com and call 555-123-4567" \ | prompt-scrub scrub --min-confidence 0.9 mail «Email_1» and call 555-123-4567 Scrubbed: 1 entity (1 Email); 1 suppressed below --min-confidence 0.9 (1 Phone) It is reported even when nothing survived the threshold, which is exactly when the output is byte-identical to a prompt that had nothing sensitive in it. `inspect` lists the dropped entities under their own heading and `watch` logs the same notice, including when that means the clipboard or file is left untouched. A dropped finding whose span some surviving finding still redacts is not counted: it was not left in the clear, and a notice that fires on every overlapping detector would soon be ignored. parseConfidence uses Number() rather than Number.parseFloat(), so `--min-confidence 0.9zzz` is rejected instead of quietly running at 0.9. `0`, `1`, `.85` and `9e-1` still parse. The changeset no longer claims existing output is unchanged outright: it now says the inspect display gained the confidence suffix and the suppression section unconditionally, and that `--hash` is the scripting-stable surface. Nits: DEFAULT_CONFIDENCE is re-exported from the package root, since the rule-pack docs name it; config validation is now a total Record<ConfigKey, …> so a new key without validation is a type error rather than a silently unvalidated field; the NameDetector comment said strict mode edits the allowlist when it actually skips matches containing an allowlisted word; handleScrub passes minConfidence by conditional spread like its neighbours. Also collapsed the runDetectors wrapper into the one function now that nothing calls the findings-only form.
|
Thankyou @will-lamerton, 1. README hash. Reproduced rather than trusted: the block uses 2. Suppression reporting. It fires when nothing survived too ( 3. 4. Changeset. Now states the Nits, all four done. Four calls past what you asked, push back on any:
Semgrep isn't this PR. One real find on the way: 4 test files I touched were genuinely misformatted the CRLF working tree hides it, so I checked the LF blobs straight from git. Fixed. Tests: 38 core, 32 e2e, 10 inspect, 22 config, 7 watch. CI unit tests pass in 38s. Ready for another look. |
Description
Closes #95
Every
Findingnow carries aconfidence(0.0-1.0) and amethodnaming the rule thatproduced it, so an exact vendor key pattern (0.99,
exact-pattern) is distinguishable froma high-entropy guess (0.60,
entropy) or a capitalised-word name (0.50,heuristic). Thescores reflect how the detectors already work this makes that implicit precision explicit
and filterable, so false positives no longer have to break automated workflows.
Type of Change
Testing Notes
Automated tests run? Yes 39 new tests, and the existing suite passes.
NameDetectormodes.stringandMessage[]payloads, withstatsreflecting thefiltered set.
Secretthat outranksEmailswallows the address unfiltered, but is dropped at a threshold so the email isdetected instead.
DEFAULT_CONFIDENCE.SecretDetector: a wider low-confidence overlap does not downgrade a vendor-prefixed key.minConfidence), flag-overrides-config precedence, and
parseConfidencerange rejection.inspectdisplay,inspect --min-confidence,scrub --min-confidence,hash agreement between the two, and rejection of out-of-range flag values.
Manual verification steps:
scrub --min-confidence 0.8, on a payload mixing anOpenAI key, a high-entropy string, an email, a name and a bare URL: only the key (0.99) and
email (0.95) were replaced; the 0.60 entropy string, 0.50 name and 0.70 bare URL were left
alone. Without the flag, everything is replaced as before.
0 missing or out-of-range scores.
is kept at
0.8, dropped at0.85).inspect --hashon this branch producesbyte-identical output to
upstream/mainfor two payloads covering every detector includingthe opt-ins.
written against the original
Findinginterface (noconfidence, nomethod). It loads,appears in
rules list, scores0.50 unspecified, and filters correctly at 0.5 and 0.8.--min-confidenceonwatch --file --once, thatinitwrites"minConfidence": 0,that
config showround-trips a configured value, and that a malformedminConfidenceisreported while scrubbing still falls back to 0.
Note on
pnpm run checkon Windows:test:types,test:lint,test:knipandtest:auditpass.test:formatfails locally on 58 files includingbiome.jsonandpackage.json, which this PR does not touch purely because this checkout has a CRLFworking tree while biome expects LF. The committed blobs are LF (
git cat-file blobshows0 CR bytes); extracting the commit to a clean LF tree and re-running
biome cireports"Checked 59 files. No fixes applied." with zero errors, so CI sees a clean format check.
Similarly, 5 tests fail locally (
cli/rulesx3,core/rule-packs,storage) and a few CLIsuites hang, because this checkout path contains a space. They fail identically on a stashed
clean tree, so they are pre-existing and environment-specific; every other suite is green at
238 passed / 0 failed, and the new tests in the hanging suites were run individually via
ava --match.Checklist
pnpm run check).package.json(maintainers will handle this).