fix(whois): Jaccard org similarity to stop containment over-corroborating (ENG-5172) - #143
fix(whois): Jaccard org similarity to stop containment over-corroborating (ENG-5172)#143josephwhenry wants to merge 1 commit into
Conversation
…ting (ENG-5172)
`whois.Corroborate` scored org names with `strutil.TokenSimilarity`, which
divides by the SHORTER token set (containment). Any resolved registrant that
merely CONTAINED a single-token pivot org scored 1.0 and reported "match":
Acme vs Acme Enterprises LLC sim=1.0000 -> match
Okta vs Okta Security Inc. sim=1.0000 -> match
Walmart vs Walmart Global Enterprises Holdings sim=1.0000 -> match
Switch the metric to Jaccard (shared distinct tokens / union) so both sides'
distinguishing tokens count against the score. Those three become 0.5, 0.5 and
0.25 -> "unverifiable", while genuine equality after legal-suffix stripping
("Acme" vs "Acme Corp" -> {acme} vs {acme}) still scores 1.0 and matches.
Jaccard alone would over-correct in the other direction: it divides by the
union, so a contained pivot sinks on length asymmetry and `Walmart` at 0.25
falls BELOW the 0.30 floor, reporting a plausible subsidiary as a
contradiction -- a worse verdict than an unresolved lookup. Gate the mismatch
arm on `strutil.TokenSetContained`: containment in either direction is
under-specification, not disagreement, so it falls through to unverifiable.
Reporting "mismatch" now requires each side to contribute a token the other
lacks. Thresholds are unchanged at 0.60/0.30, now Jaccard-interpreted.
`github_org` deliberately keeps containment: it uses the value as a WEIGHT
(0.25 * similarity) on a weak hint rather than as a threshold gate, so a
display name containing the target org name is the signal it wants to reward
at full weight. The reason is recorded at the call site.
Also replaces CLAUDE.md's "Reverse-WHOIS Verify-After-Retrieve" section, which
documented `reverse_whois_verify.go`, `verifyCandidates`, `decideConfidence`,
`registrantResolver` and `whoisclient.go` -- all deleted by the WHOIS
consolidation. The referral-hardening bullet is retained with its current
symbol names (`tcp43RawDial`, `netutil.SSRFSafeControl`, `maxResponseBytes`).
Supersedes #122 and #127, whose diffs targeted the deleted verifier.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughThe change adds Jaccard token-set similarity and symmetric containment detection. WHOIS organization normalization now removes legal suffixes and compares distinct token sets. Corroboration returns unverifiable for empty or contained sets, and reports mismatches only for low-similarity, non-contained sets. Tests cover scoring, containment, normalization, and corroboration outcomes. Documentation records the updated workflow and WHOIS referral limits. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/whois/normalize_test.go (1)
94-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the suffix-only test description and add the reversed case.
"Acme Corp"normalizes to{acme}. It does not reduce to zero tokens. State that the pivot reduces to zero tokens. Add a case where the resolved organization is"Co., Ltd."to preserve symmetric coverage.Proposed update
- // Both sides reduce to nothing after legal-suffix stripping, so similarity + // The pivot reduces to nothing after legal-suffix stripping, so similarity // is undefined — unverifiable, never a mismatch. {"all-suffix orgs are unverifiable", "Co., Ltd.", "Acme Corp", "unverifiable"}, + {"all-suffix resolved org is unverifiable", "Acme Corp", "Co., Ltd.", "unverifiable"},🤖 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 `@pkg/whois/normalize_test.go` around lines 94 - 96, Update the test case description in the similarity table to state that the pivot reduces to zero tokens, not both organizations. Add the reversed case with resolved organization “Co., Ltd.” and the corresponding pivot organization so suffix-only normalization remains covered symmetrically.
🤖 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.
Nitpick comments:
In `@pkg/whois/normalize_test.go`:
- Around line 94-96: Update the test case description in the similarity table to
state that the pivot reduces to zero tokens, not both organizations. Add the
reversed case with resolved organization “Co., Ltd.” and the corresponding pivot
organization so suffix-only normalization remains covered symmetrically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08992688-67d8-4655-a1c5-224299a599be
📒 Files selected for processing (6)
CLAUDE.mdpkg/lib/strutil/strutil.gopkg/lib/strutil/strutil_test.gopkg/plugins/domains/github_org.gopkg/whois/normalize.gopkg/whois/normalize_test.go
There was a problem hiding this comment.
Codex Review
Critical Issues
None.
Security
No security concerns flagged.
Suggestions
No suggestions.
Verification: attempted go test ./pkg/lib/strutil ./pkg/whois ./pkg/plugins/domains, but it failed before running because the read-only filesystem could not create /home/runner/go.
Reviewed by Codex (gpt-5.5)
There was a problem hiding this comment.
Claude Review
Critical issues
None. The fix is correct and well-scoped. Corroborate guards zero-token sides before the Jaccard division (safe), the mismatch arm is gated by TokenSetContained in both directions, and the empty-pivot ("") / privacy (unverifiable) short-circuits are preserved above the changed hunk. normalizeOrg -> normalizeOrgTokens drops the only strings.Join use but strings stays heavily used elsewhere in the file, so no compile break. Keeping github_org on containment (a weight, not a gate) is the right call and is now documented at the call site.
Security
No security concerns flagged. Pure scoring-metric change: no new network calls, auth, or input-trust surface; the SSRF / byte-cap / hop-cap hardening described in CLAUDE.md is pre-existing.
Test coverage
Present and thorough. New strutil_test.go pins the Jaccard-vs-containment contrast, direction symmetry, and duplicate collapse; TestCorroborate / TestOrgSimilarity add both over-corroboration shapes, both containment directions, the single-token exact match, a reachable non-contained mismatch, and the all-suffix case.
No critical issues — LGTM pending human review.
There was a problem hiding this comment.
Gemini Review
Critical Issues
None.
Security
No security concerns flagged.
Suggestions
TokenSimilarityscore inflation: Inpkg/lib/strutil/strutil.go,TokenSimilaritycurrently iterates over the rawshorterslice rather than its distinct token set. If a string contains repeated tokens (e.g.,"acme acme") and the other string contains just one instance of it ("acme widgets"),matchesis incremented for every duplicate. This allows the metric to artificially inflate to1.0. Consider adopting thedistinctmap approach forshorterto prevent this edge case.- Unused exported function:
OrgSimilarityinpkg/whois/normalize.gois no longer used internally, asCorroboratewas refactored to inline the tokenization step and callstrutil.JaccardTokenSetsdirectly (avoiding duplicate work). If this function is not part of a public API consumed by other repositories, it and its associated tests can be safely removed as dead code.
Reviewed by Gemini (gemini-3.1-pro-preview)
|
Closing — reopened from the correctly-named branch. This PR was raised from |
Fixes ENG-5172. Supersedes #122 and #127 — see Why a new PR below.
The bug, on
maintodaywhois.Corroboratescores org names withstrutil.TokenSimilarity, which divides by the shorter token set (containment). Any resolved registrant that merely contains a single-token pivot org scores1.0and reports"match":This is ENG-5172 unchanged. The WHOIS consolidation deleted
reverse_whois_verify.go, butdecideConfidencewas reincarnated aswhois.Corroboratewith the same 0.60/0.30 thresholds and the same containment metric, so the defect survived the refactor intact.Blast radius today is small:
Corroborationis a metadata string on the finding (whoisFindingData.Corroboration, carried in Guard atp_whois.go) and does not currently feed confidence scoring. That makes now the cheap moment to correct the metric, before it becomes load-bearing.Change
1. Jaccard instead of containment.
OrgSimilaritynow scoresstrutil.JaccardTokenSets— shared distinct tokens ÷ union — so both sides' distinguishing tokens count against the score. Same output on the same inputs:Genuine equality after legal-suffix stripping (
AcmevsAcme Corp→{acme}vs{acme}) still scores1.0and still matches.2. A containment guard on the mismatch arm — the other half of the fix. Jaccard alone over-corrects in the opposite direction. Because it divides by the union, a contained pivot sinks on length asymmetry alone:
Walmartabove lands at0.25, below the0.30floor, so a bare threshold would report a plausible subsidiary as a contradiction — a strictly worse verdict than an unresolved lookup.strutil.TokenSetContainedgates the arm: containment in either direction (the pivot may be the longer name) is under-specification, not disagreement, and falls through to"unverifiable". Reporting"mismatch"now requires each side to contribute a token the other lacks.Note in the output above that
Walmartsits below the mismatch floor and still correctly reportsunverifiable— that is the guard working. Do not take Jaccard without it.Thresholds are unchanged at 0.60 / 0.30, now Jaccard-interpreted. Mismatch stays strictly-less-than. An org that reduces to zero tokens after suffix stripping (
"Co., Ltd.") is"unverifiable"— similarity is undefined, not zero.3.
github_orgdeliberately keeps containment. It callsstrutil.TokenSimilarityand uses the result as a weight (0.25 * similarity) on a weak hint, not as a threshold gate — a display name containing the target org name is exactly the signal it wants to reward at full weight, and Jaccard would penalize it for length asymmetry. The two callers score differently on purpose, and the reason is now recorded at the call site instead of being an accident of where the old helper happened to live. Switchinggithub_orgontoOrgSimilarityis defensible on its merits (suffix stripping is better for org names) but it is a scoring change to a shipped plugin with its own test updates, so it does not belong in a bug fix.4. CLAUDE.md. Replaces the
Reverse-WHOIS Verify-After-Retrievesection, which documentedreverse_whois_verify.go,verifyCandidates,decideConfidence,registrantResolverandwhoisclient.go— all deleted by the consolidation. It was stale onmainindependent of this fix. The referral-hardening bullet describes surviving code and is retained with its current symbol names (tcp43RawDial,netutil.SSRFSafeControl,maxResponseBytes,maxReferrals).Placement
The primitives go in
pkg/lib/strutilbesideTokenSimilarity; the org-aware wrapper stays inpkg/whois. Both take token slices soCorroboratetokenizes once and feeds the same tokens to the metric and the guard.normalizeOrgbecomesnormalizeOrgTokensfor that reason.Scope is contained: only
CorroboratecalledOrgSimilarity, onlywhois.gocallsCorroborate, and there are no callers in guard, aurelian, or praetorian-cli.Why a new PR instead of resolving #122 / #127
git merge-treefor #122 againstmain:The correct resolution for the modify/delete pairs is
git rmboth — which leaves #122 holding one uncalled function plus edits to prose about deleted code. That is a reimplementation, not a conflict resolution. #127 compounds it: it is stacked on #122's branch, and by its own description carries zero production lines (its deliverable is ~52 lines of comment in the deleted file plus a calibration test). The one argument for keeping them split — #122 the fix, #127 the calibration record — collapses when the fix is ~5 lines of production code.#122's
jaccardTokenSetsandtokenSetContainedare carried forward here essentially verbatim, along with their test tables and thenormalizeOrgTokensrefactor. #127's reachability enumeration is deliberately not ported: it defends thresholds that are not currently load-bearing, at ~1300 lines. It can be revisited if/whenCorroborationstarts driving scores.#129 has no analogue here. It patches
summarizeVerifyPassoverwhoisIncompleteness— the entire incompleteness-tracking subsystem was deleted, and nothing inpkg/whoisreplaces it. It should be closed outright.Tests
pkg/lib/strutil/strutil_test.go(new) —TestJaccardTokenSetspins the metric including the containment case;TestJaccardTokenSets_PenalizesContainmentUnlikeTokenSimilarityasserts the contrast between the two metrics directly, so the reason both exist survives as an assertion rather than a comment;TestTokenSetContainedcovers direction symmetry, equal sets, disjoint sets, private-tokens-both-sides, empty sides, and duplicate collapse.TestCorroborategains 8 cases: both over-corroboration shapes, both containment directions for the over-correction, the single-token exact match that must keep working, a sparse non-contained pair that must still mismatch (so the arm is proven reachable through the guard), and the all-suffix org.TestOrgSimilaritygains the containment values that previously returned1.0, plus argument symmetry.TestOrgSimilarityassertions and all eight pre-existingTestCorroboratecases pass unchanged — they were all equality-after-stripping cases, which is precisely why the table never caught this.Verification
go build ./cmd/piusok ·go vet ./...clean ·go test -count=1 ./...green, 0 failures ·gofmt -loutput is byte-identical tomain's (7 files of pre-existing drift; this branch adds none, andgofmt -dongithub_org.goshows only a pre-existing trailing blank line). All tests hermetic — no network, no keys.🤖 Generated with Claude Code