From 9cbc0c5ac28c36cf31c2f3eef14ce5610bb43d75 Mon Sep 17 00:00:00 2001 From: Shahzad Haider Date: Tue, 25 Aug 2026 16:25:47 +0500 Subject: [PATCH] fix(keywordbench): correct verdict yield ranking and four reporting claims --- cmd/keywordbench/main.go | 8 ++-- cmd/keywordbench/report.go | 14 ++++--- cmd/keywordbench/scan.go | 52 +++++++++++++++++++++----- hack/docs/Adding_Detectors_Internal.md | 4 +- 4 files changed, 60 insertions(+), 18 deletions(-) diff --git a/cmd/keywordbench/main.go b/cmd/keywordbench/main.go index ffab12249..472bdd726 100644 --- a/cmd/keywordbench/main.go +++ b/cmd/keywordbench/main.go @@ -2,9 +2,11 @@ // costs on a corpus, so a new detector can be compared against the existing detector // population before it ships. // -// The accuracy-focused scan reuses the engine's own chunker, decoders and -// Aho-Corasick core, so the "calls" it reports are the exact number of FromData -// invocations a production scan would make on the same bytes. +// The scan reuses the engine's own chunker, decoders and Aho-Corasick core, so the +// "calls" it reports are the unverified regex passes a production scan makes over +// the same bytes. It is not the total FromData count: with verification overlap on +// (the default), the engine re-dispatches detectors that produced results for a +// second, verifying pass, which this deliberately does not model. // // Typical use, against the same corpus used for the regex-accuracy test: // diff --git a/cmd/keywordbench/report.go b/cmd/keywordbench/report.go index 3b5a3a8a6..7abb6def3 100644 --- a/cmd/keywordbench/report.go +++ b/cmd/keywordbench/report.go @@ -112,10 +112,13 @@ func (r *report) rankPercentiles() { } slices.Sort(calls) - // Yield is only defined for detectors the prefilter actually woke. + // Rank yield only among detectors that found something. Including the + // zero-result majority would put any detector with a single finding near the + // top of the scale, which is how "13 findings in 801,942 calls" once read as + // productive. var yields []float64 for _, rw := range r.rows { - if rw.calls > 0 { + if rw.results > 0 { yields = append(yields, rw.yield) } } @@ -123,7 +126,7 @@ func (r *report) rankPercentiles() { for i := range r.rows { r.rows[i].callsPctl = percentileOf(calls, r.rows[i].callsPerMB) - if r.rows[i].calls > 0 { + if r.rows[i].results > 0 { r.rows[i].yieldPctl = percentileOf(yields, r.rows[i].yield) } } @@ -219,7 +222,7 @@ func (r *report) renderVerdict(w io.Writer) { fmtFloat(t.callsPerMB), rankPhrase(t.callsPctl, len(r.rows))) tb.row(" How much corpus it re-scans\t%s\t%s of all prefilter work\n", pct(t.regexLoad), pct(safeDiv(float64(t.regexBytes), float64(r.totalRegex)))) - tb.row(" How often it is woken\t%s of chunks\t\n", pct(t.wakeRate)) + tb.row(" How often it is woken\t%s of decoded variants\t\n", pct(t.wakeRate)) if r.cfg.detect { tb.row(" What it found\t%s secrets in %s calls\t%s\n", comma(t.results), comma(t.calls), yieldPhrase(t)) @@ -284,7 +287,7 @@ func (r *report) renderSamples(w io.Writer) { } tb.flush(w) capped := "" - if len(r.tot.samples) >= maxSamples { + if r.tot.sampleCapped { capped = ", sampling capped" } fmt.Fprintf(w, "\n %s hits across %s distinct identifiers%s.\n", @@ -365,6 +368,7 @@ func (r *report) renderRanked(w io.Writer) { fmt.Fprintf(w, "\n calls/MB is how many times a detector's regex runs per MB of corpus.\n") fmt.Fprintf(w, " re-scans is the share of the corpus its keywords hand back to the regex;\n") fmt.Fprintf(w, " it passes 100%% when the spans around separate hits overlap.\n") + fmt.Fprintf(w, " wakes on is measured over decoded variants, not raw chunks.\n") } func (r *report) writeRow(tb *alignedTable, rank int, rw row) { diff --git a/cmd/keywordbench/scan.go b/cmd/keywordbench/scan.go index 67471e76f..748ac0d7b 100644 --- a/cmd/keywordbench/scan.go +++ b/cmd/keywordbench/scan.go @@ -64,6 +64,9 @@ type totals struct { recallTotal uint64 elapsed time.Duration + // sampleCapped records that a worker stopped collecting identifiers. The cap is + // per worker, so the size of the merged map cannot tell you this. + sampleCapped bool // interrupted marks a partial scan, so the report can say so rather than // presenting numbers that look like a completed run. interrupted bool @@ -123,6 +126,7 @@ func (t *totals) merge(o *totals) { t.altVariantsHit += o.altVariantsHit t.recallKept += o.recallKept t.recallTotal += o.recallTotal + t.sampleCapped = t.sampleCapped || o.sampleCapped } type scanner struct { @@ -435,17 +439,14 @@ func (w *worker) processVariant(data []byte) { continue } for _, r := range res { - raw := r.Raw - if len(raw) == 0 { - raw = r.RawV2 - } - if len(raw) == 0 { + raw := rawOf(r) + if raw == "" { continue } if targetRaw == nil { targetRaw = make(map[string]struct{}) } - targetRaw[string(raw)] = struct{}{} + targetRaw[raw] = struct{}{} } } } @@ -470,9 +471,30 @@ func (w *worker) processAlt(data []byte, targetRaw map[string]struct{}) { } } + if len(targetRaw) == 0 { + return + } + + // Byte containment would only prove the secret was in range of the regex, not + // that the regex still returns it. Re-run the detector on the candidate's own + // spans instead. This only runs on variants where the current keywords already + // found something, so it costs almost nothing. + found := make(map[string]struct{}, len(targetRaw)) + for _, span := range spans { + res, err := w.fromData(w.sc.target, span) + if err != nil { + continue + } + for _, r := range res { + if raw := rawOf(r); raw != "" { + found[raw] = struct{}{} + } + } + } + for raw := range targetRaw { t.recallTotal++ - if slices.ContainsFunc(spans, func(s []byte) bool { return bytes.Contains(s, []byte(raw)) }) { + if _, ok := found[raw]; ok { t.recallKept++ } } @@ -489,12 +511,24 @@ func (w *worker) countKeywords(data []byte) { w.seenKW[k] = struct{}{} ks.variantsHit++ } - if _, ok := w.sc.targetKW[k]; ok && len(w.tot.samples) < maxSamples { - w.tot.samples[tokenAround(lower, int(m.Pos()), len(k))]++ + if _, ok := w.sc.targetKW[k]; ok { + if len(w.tot.samples) < maxSamples { + w.tot.samples[tokenAround(lower, int(m.Pos()), len(k))]++ + } else { + w.tot.sampleCapped = true + } } } } +// rawOf returns the identifier a result is deduped on, preferring Raw. +func rawOf(r detectors.Result) string { + if len(r.Raw) > 0 { + return string(r.Raw) + } + return string(r.RawV2) +} + // maxSamples bounds per-worker sample memory on a multi-hour run. const maxSamples = 50_000 diff --git a/hack/docs/Adding_Detectors_Internal.md b/hack/docs/Adding_Detectors_Internal.md index c8095a0fe..2ad4f36b0 100644 --- a/hack/docs/Adding_Detectors_Internal.md +++ b/hack/docs/Adding_Detectors_Internal.md @@ -223,4 +223,6 @@ make keywordbench CORPUS=contents.jsonl.zstd TARGET=YourDetector make keywordbench CORPUS=contents.jsonl.zstd TARGET=YourDetector ALT=candidate ``` -The report ranks the detector against every other one on the same bytes, shows the identifiers its keywords actually landed inside, and says whether the keyword is earning its cost. `LIMIT_MB=4000` gives a stable ranking in a couple of minutes. Per-detector and per-keyword CSVs land in `keywordbench-out/` for joining against the findings table. +The report ranks the detector against every other one on the same bytes, shows the identifiers its keywords actually landed inside, and says whether the keyword is earning its cost. Per-detector and per-keyword CSVs land in `keywordbench-out/` for joining against the findings table. + +`LIMIT_MB` is much faster than a full pass, but it truncates rather than samples: it reads a prefix of the file, and the corpus is heavy-tailed enough that a few repos dominate any slice. Two disjoint 1.2GB slices move ~12% of detectors across a cost bucket, and individual detectors swing several-fold between adjacent slices. Treat a capped run as a rough signal and the full pass as the number you quote.