test(reverse-whois): record the 0.60/0.30 calibration basis and pin #122's Jaccard arms (ENG-5374) - #127
Conversation
… over-corroboration (ENG-5172)
decideConfidence compared the query org against a candidate's resolved
registrant with tokenSimilarity, which divides by the SHORTER token set
(containment). For a single-token query org, any registrant that merely
CONTAINS that token scored 1.0 and corroborated at 0.60 (top of the
needs_review band). e.g. registrant "Acme Enterprises LLC" (normalized
{acme, enterprises}) vs query {acme} -> 1/1 = 1.0.
Add tokenJaccard (|A n B| / |A u B|) and use it in the reverse-whois
verifier so both sides' distinguishing tokens count against the score. The
Acme-Enterprises case now scores 1/2 = 0.5, landing in the unverified band
(needs review) rather than spuriously corroborating. Genuine exact matches
after legal-suffix stripping ("Acme" vs "Acme Corp") still corroborate at
1.0. The simCorroborate (0.60) / simMismatch (0.30) thresholds are
unchanged, now Jaccard-interpreted.
github_org's name-similarity signal deliberately keeps tokenSimilarity
(containment): there a partial name overlap is a weak, 0.25-weighted hint,
not a corroboration gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…token test - Remove the unreachable `if union == 0` guard in the Jaccard core (Gemini): after the empty-side early return both token sets hold >= 1 element, so the union is always >= 1. Extract the token-slice core as jaccardTokenSets. - decideConfidence tokenizes each org once via a new normalizeOrgTokens and feeds the slices straight to jaccardTokenSets, dropping the redundant re-tokenize that tokenJaccard(normalizeOrg(...), ...) performed (Gemini). normalizeOrg is retained as a thin string wrapper for its other callers/tests. - Add a duplicate-token case to TestTokenJaccard so the distinct-set semantics are pinned (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…te (ENG-5374)
tokenSimilarity is the containment coefficient m/min(kq,kc), so its
denominator discards the longer side's unmatched tokens and the value is
invariant under adding disconfirming evidence: query org "Apple" scores
exactly 1.0 against "Apple Tree Landscaping", against "Apple Tree
Landscaping Of Tampa Bay", and against every longer superstring. Feeding
that to a fixed threshold made an unrelated landscaping company
corroborate at the top of the needs_review band.
Gate the corroborate arm of decideConfidence on
corroborationHasResolution: exact equality after normalizeOrg, or a
minimum token count of 2. The exact-equality exemption is load-bearing --
normalizeOrg strips legal-form suffixes, so the legitimate "Praetorian"
vs "Praetorian Inc" reduces to one identical token, and a bare floor
would de-rank exactly the matches this plugin exists to find. The guard
sits after the empty-normalization check and is confined to the
corroborate arm, so a clean single-token mismatch ("Apple" vs "Walmart",
m=0) still de-ranks rather than being rescued.
The shared metric is untouched. Its other consumer, the github-org
plugin, takes similarity as a graded multiplier (0.25*sim) with no
decision boundary, and that plugin's test pins the shorter-denominator
behavior as desired via a GreaterOrEqual floor at 1.0. Changing the
denominator would fail that pin and halve the name contribution for every
single-token org -- a behavior change this ticket never scoped.
Thresholds confirmed, not re-tuned, with the basis recorded. Because sim
is always m/k, the reachable value set at k=1 is exactly {0.0, 1.0}: the
ambiguous arm is unreachable and the cutoffs have zero discriminating
power there for every possible simCorroborate in (0,1]. Re-setting 0.60
would not have changed this defect's verdict at all -- the problem was the
metric's resolution, not the threshold's value. Recorded known
limitation: below k=4, simMismatch is reachable only at m=0.
Calibration is therefore evidenced by enumeration rather than by a
fixture corpus. That 0.60/0.30 are precision-optimal is not establishable
hermetically -- it needs candidates from the real ViewDNS/Whoxy
distribution, live-resolved registrant strings, and ownership labels of
independent provenance, the latter two forbidden by this ticket's
hermetic requirement. Hand-authored fixtures cannot substitute, since
their labels would come from the same judgment that picked the threshold.
Full containment at k>=2 is deliberately left alone: crossing
simCorroborate there already forces at least 2 shared normalized tokens,
and the ambiguous arm is reachable, so the cutoffs retain real
discriminating power.
Also retitles a mislabeled existing case. "corroborated partial (shorter
fully contained)" used "Acme" vs "Acme Corp", but normalizeOrg strips
"corp" so both sides become "acme" -- an exact equality. No pinned case
ever exercised non-exact single-token containment, which is the hole that
let this defect survive.
De-rank-never-drop holds: the fix only ever moves 0.60 to 0.50, both
inside [0.35, 0.65), so every candidate is still emitted and still
flagged for review. One confidenceDecision per call, one AddConfidence
call, and the existing PII-free justification is reused, so no registrant
string enters the output.
Fixes ENG-5374
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Instruction-File Drift DetectionCode changes in this PR may have made documentation stale:
|
| Section | Issue | Evidence |
|---|---|---|
| "Reverse-WHOIS Verify-After-Retrieve" (lines 112-115) | Documented corroboration condition is incomplete | The condition "corroborated (similarity ≥ 0.60)" is no longer accurate. PR adds a new corroborationHasResolution() guard in decideConfidence (line 650 of reverse_whois_verify.go) that is now required alongside the similarity check. At k=1 (single token), corroboration now additionally requires exact string equality. This affects the documented behavior but CLAUDE.md does not mention this guard or the minCorroborateTokens = 2 constant. |
Automated drift check — please review and update if needed.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review
Critical Issues
pkg/plugins/domains/reverse_whois_verify.go:455uses raw token count for the new resolution guard, buttokenSimilaritycounts duplicate tokens on the shorter side against a set of longer-side tokens. A query like"Apple Apple"versus"Apple Tree Landscaping"is non-equal, has only one distinct shared token, but getssim == 1.0andmin(len(tokens)) == 2, sodecideConfidenceat:390still returns corroborated. That leaves the ENG-5374 false-positive class open whenever one side repeats the shared token. Consider basing the guard on distinct shared tokens or deduping tokens for this decision.
Security
No security concerns flagged.
Suggestions
No suggestions.
Reviewed by Codex (gpt-5.5)
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughReverse-WHOIS confidence classification now uses Jaccard similarity over normalized distinct token sets. Symmetric containment handling keeps contained names unverified while sparse, non-contained overlaps can be de-ranked. The change adds token-set scoring and updates normalization and confidence decisions. New tests cover reachable similarity values, classification paths, review-band invariants, justifications, saturation, monotonicity, symmetry, and token-set containment. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pkg/plugins/domains/reverse_whois_calibration_test.go`:
- Around line 374-379: Correct the explanatory comment around
assert.Len(covered, 5) to state that covered is keyed by the table’s declared
returnPoint labels and the assertion only detects an extra label added to the
table without updating the expected count. Do not claim it detects new return
statements in decideConfidence that lack a matching table case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81851412-0761-4076-9308-5bbd997b7853
📒 Files selected for processing (3)
pkg/plugins/domains/reverse_whois_calibration_test.gopkg/plugins/domains/reverse_whois_verify.gopkg/plugins/domains/reverse_whois_verify_test.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4106b96c0c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if nq == nc { | ||
| return true | ||
| } | ||
| return min(len(tokenize(nq)), len(tokenize(nc))) >= minCorroborateTokens |
There was a problem hiding this comment.
For org names that repeat the same token, this guard treats duplicate occurrences as independent evidence and lets the ENG-5374 false-positive class through. For example decideConfidence("Apple Apple", org("Apple Tree"), nil) still corroborates: tokenSimilarity counts both apple occurrences as matches and returns 1.0, while this min(len(tokenize(...))) check sees two tokens and allows the corroborate arm even though there is only one distinct shared token and unmatched remainder. If the resolution floor is meant to require at least two corroborating tokens, it needs to be based on distinct normalized tokens/shared evidence, not raw token slice length.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
CONFIRMED — fixed in 20678b3.
Reproduced before changing anything. The mechanism is exactly as described, and it is worse than the "Apple Apple" example suggests because it is reachable from realistic WHOIS strings: normalizeOrg strips legal-form suffixes and rejoins whatever is left, so a registrant reading "Acme Corp Acme Inc" normalizes to "acme acme" — two raw tokens carrying one distinct token. Against a query org of "Acme Tree Services" that cleared min(3, 2) >= minCorroborateTokens, then scored m/k = 2/2 = 1.0, and corroborated at 0.60 on a single shared token. No duplicated word in the query is needed.
Root cause is the one you name: tokenSimilarity compares a multiset against a set — it walks the shorter side's token slice while testing membership in a map built from the longer side — so a repeat scores a match on every occurrence. A raw-length floor therefore counts one token of evidence as two.
Fix, per your suggestion: corroborationHasResolution now counts distinct normalized tokens on both sides via a new distinctTokenCount helper. Outcomes, all pinned as table cases:
| query | registrant | normalized (raw/distinct) | sim | before | after |
|---|---|---|---|---|---|
Acme Tree Services |
Acme Corp Acme Inc |
3/3 vs 2/1 | 1.00 | 0.60 corroborated | 0.50 unverified |
Apple Apple |
Apple Tree Landscaping |
2/1 vs 3/3 | 1.00 | 0.60 corroborated | 0.50 unverified |
Praetorian Security |
Praetorian Security Group |
2/2 vs 3/3 | 1.00 | 0.60 | 0.60 corroborated (unchanged) |
Praetorian |
Praetorian Inc |
equal after normalization | 1.00 | 0.60 | 0.60 corroborated (exemption) |
Apple |
Walmart |
1/1 vs 1/1 | 0.00 | 0.40 | 0.40 mismatch, not rescued |
Both new cases fail under an -overlay reversion to the raw-length form, and nothing else in the package does — which also confirms the pre-existing suite had no coverage of this class.
One scope note recorded in the code rather than silently: the multiset/set asymmetry inside tokenSimilarity is not fixed, because that is the metric the github-org plugin scores against and its pinned floor assertion forbids changing it (the same constraint that leaves k >= 2 saturation out of scope here). sim still returns 1.0 for those first two rows — the guard, not a cutoff, is what withholds the corroboration. The metric-level defect is being filed separately, since it also lets github-org award its full 0.25 name-similarity credit on a duplicated token.
There was a problem hiding this comment.
Round-2 re-review request. These three reviewers only trigger on pull_request: [opened, ready_for_review], not synchronize, so none of you saw commit 20678b3 — the fix for the P2 raw-token-count finding above. Requesting it explicitly per the mention gate.
What changed in 20678b3 (4 files, 140+/11-):
corroborationHasResolutionnow measures distinct normalized tokens via a newdistinctTokenCounthelper (slices.Sort+slices.Compactover the freshly allocatedtokenizeslice), so"acme acme"counts as k=1 and can no longer clearminCorroborateTokens.- Doc block above the predicate explains why the floor must be distinct-counted, and states explicitly that the metric-level asymmetry inside
tokenSimilarityis not fixed here (now tracked as ENG-5890). - Four new
TestDecideConfidencecases plusTestDistinctTokenCountpin the class in both directions (duplicate on the candidate side, duplicate on the query side, duplicate with zero overlap still de-ranking, and full containment at two genuinely distinct tokens still corroborating). - The overclaiming comment on
assert.Len(t, covered, 5)in the calibration test was corrected; the assertion itself is byte-identical.
Specific things worth attacking:
- Is
slices.Sortontokenize's return genuinely safe — i.e. is that slice freshly allocated on every call with no other holder? The comment claims it is. - Does the distinct-token floor change any pre-existing classification outcome? I believe not (an
-overlayreversion check makes exactly the two new duplicate-token cases fail and nothing else), but a second opinion on reachability is welcome. github_org.goandgithub_org_test.goare byte-identical toorigin/main— deliberate, because that plugin's test pins containment as desired. Confirm the diff really is empty.
There was a problem hiding this comment.
Claude Review
Critical issues
None. The fix is correct and narrowly scoped: gating the corroborate arm on corroborationHasResolution(nq, nc) closes the k=1 saturation defect without touching the shared tokenSimilarity (empty github_org.go diff confirms this). The predicate is placed after the empty-after-normalization guard so the unverifiable/mismatch distinction survives, and it is confined to the corroborate arm so a clean k=1/m=0 mismatch (Apple vs Walmart) still de-ranks to 0.40 rather than being rescued. The nq == nc exemption compares normalizeOrg output, so legitimate suffix-only diffs (Praetorian vs Praetorian Inc) still corroborate. The min builtin is fine on go 1.25. De-rank-never-drop holds (0.60 -> 0.50, both in-band).
Security
No security concerns flagged. No new network calls, no keys, and the change reuses the existing PII-free unverified justification with no interpolation of the resolved registrant string — it strictly reduces false corroboration of unrelated orgs.
Test coverage
Excellent. Enumeration/property tests cover the reachability ladder, argument-swap symmetry, monotonicity, and all five decideConfidence return points; the ENG-5374 case and exact-equality exemption are pinned in the pre-existing table, and the PR reports the guard was reversion-tested via -overlay (4 functions fail without the fix).
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
- Optimize token counting in
corroborationHasResolution: Becausenqandncare already normalized bynormalizeOrg, they are guaranteed to be single-space-delimited strings containing only[a-z0-9]. Callingtokenize()on them again redundantly allocates astrings.Builderand new slices for strings that are already sanitized. You can safely replacelen(tokenize(nq))withstrings.Count(nq, " ") + 1(and the same fornc) to compute the token counts with zero allocations while preserving the exact same behavior.
Reviewed by Gemini (gemini-3.1-pro-preview)
…s (ENG-5172)
Switching decideConfidence to Jaccard fixed the single-token
over-corroboration this ticket was filed for, but pushed the same
containment shape off the other end of the band. Jaccard divides by the
union, so a query org fully contained in a longer registrant sinks on
length asymmetry alone: "Walmart" vs "Walmart Global Enterprises Holdings
LLC" is {walmart} vs {walmart, global, enterprises, holdings} = 1/4 =
0.25, under simMismatch (0.30). The bare threshold read that as a clear
MISMATCH and de-ranked a plausible subsidiary to 0.40 -- below a
candidate whose WHOIS never resolved at all. The PR's own doc comment
asserted such a name "stays unverified", which was false.
Sparse overlap is not the de-rank test; disagreement is. Add
tokenSetContained (distinct tokens, symmetric, false on an empty side)
and gate the de-rank arm on it, so reaching 0.40 now requires that each
side contribute a token the other lacks. A contained pair is
under-specification, not contradiction, and falls through to
confReverseWhoisUnverified (0.50).
Thresholds, justifications, tokenSimilarity, tokenJaccard and
jaccardTokenSets are unchanged, as is github_org.go's deliberate
retention of containment for its weak 0.25-weighted name hint. The
de-rank-never-drop invariant is preserved -- every outcome stays in
[0.35, 0.65).
Reported by Codex (P2) on PR #122. Verified red-before-green: with the
containment gate reverted, exactly the two new cases fail package-wide
(0.40 where 0.50 is required) and all 15 pre-existing TestDecideConfidence
subtests -- both disjoint-mismatch boundary cases included -- still pass,
so the new cases discriminate the fix rather than the file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Round-2 review found tokenJaccard had no production caller — only its own test. It was a one-line wrapper over jaccardTokenSets, which is what decideConfidence actually calls. The wrapper was also the double-tokenizing form, so the next caller reaching for the string-shaped helper would have silently reintroduced a second tokenize pass. jaccardTokenSets is now the single entry point. tokenSimilarity is kept: unlike tokenJaccard it has a live production caller at github_org.go:209, where containment is the deliberate choice for the weak 0.25-weighted name hint. The Jaccard-vs-containment rationale that lived on the wrapper's doc comment is merged into jaccardTokenSets rather than deleted. TestTokenJaccard becomes TestJaccardTokenSets, tokenizing at the call site so it still covers tokenize + Jaccard together; all 9 cases carry over unchanged, including the duplicate-token case added earlier for CodeRabbit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…(ENG-5374) Adjudication of automated review findings on PR #127. CONFIRMED (codex-review, P2): the resolution floor added by the previous commit counted raw token-slice length, so a repeated token passed as two independent ones and the ENG-5374 false-positive class survived. The shared similarity metric compares a multiset against a set -- it walks the shorter side's token slice while testing membership in a map built from the longer side -- so one shared token repeated on the shorter side scores a match on every occurrence, and normalizeOrg does not dedupe. A registrant reading "<Name> Corp <Name> Inc" normalizes to two raw tokens carrying one distinct token, cleared k >= 2, then scored m/k = 2/2 = 1.0 against a query org sharing only that token. corroborationHasResolution now counts DISTINCT normalized tokens via a new distinctTokenCount helper. Reproduced before fixing and pinned after: "Acme Tree Services" vs "Acme Corp Acme Inc" and "Apple Apple" vs "Apple Tree Landscaping" both drop 0.60 -> 0.50, and both fail under an overlay reversion to the raw-length form. Controls pin the narrowing as deliberate: two-distinct-token containment still corroborates, the exact-equality exemption still corroborates, and a clean k=1/m=0 mismatch still de-ranks to 0.40 rather than being rescued. The multiset/set asymmetry in the shared metric is documented as NOT fixed -- it is the metric the github-org plugin scores against and its pinned floor assertion forbids changing it. The guard only prevents that duplicate-counting from producing a corroboration. CONFIRMED (CodeRabbit): a comment overclaimed what Len(covered, 5) detects. It is keyed on labels declared in the table, so a sixth return statement in decideConfidence with no matching case still passes. Comment corrected to state that scope; the assertion is unchanged. CONFIRMED (drift-check): the repo instruction file's documented corroboration condition was incomplete. Updated to record the resolution requirement and why. REFUTED (gemini-review): replacing the token count with strings.Count(s, " ")+1 is not behavior-identical -- it returns 1 for the empty string where tokenize returns 0 -- and it relies on an upstream caller invariant for correctness. It is also moot now that the floor needs distinct tokens. Shared metric untouched: the github-org plugin and its test stay byte-identical to origin/main. Refs ENG-5374 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Automated-review adjudication — round 1Every finding read against the cited code and either reproduced-then-fixed or refuted with evidence. Pushed as
#1 and #2 are answered in their own threads. #1 was the real one: it was reachable from realistic WHOIS strings, not just the contrived #3 CONFIRMED. The bullet said #4 REFUTED, on behavior rather than on taste. Verification after the fix
Two things a human reviewer needs to decide
|
There was a problem hiding this comment.
Gemini Review
{
"session_id": "db12ba5e-aea3-464b-9dbd-964f4c702da3",
"response": "",
"stats": {
"models": {
"gemini-3.1-pro-preview": {
"api": {
"totalRequests": 2,
"totalErrors": 0,
"totalLatencyMs": 4386
},
"tokens": {
"input": 23240,
"prompt": 31385,
"candidates": 26,
"total": 31482,
"cached": 8145,
"thoughts": 71,
"tool": 0
},
"roles": {
"main": {
"totalRequests": 2,
"totalErrors": 0,
"totalLatencyMs": 4386,
"tokens": {
"input": 23240,
"prompt": 31385,
"candidates": 26,
"total": 31482,
"cached": 8145,
"thoughts": 71,
"tool": 0
}
}
}
}
},
"tools": {
"totalCalls": 1,
"totalSuccess": 1,
"totalFail": 0,
"totalDurationMs": 9,
"totalDecisions": {
"accept": 1,
"reject": 0,
"modify": 0,
"auto_accept": 0
},
"byName": {
"read_file": {
"count": 1,
"success": 1,
"fail": 0,
"durationMs": 9,
"decisions": {
"accept": 1,
"reject": 0,
"modify": 0,
"auto_accept": 0
}
}
}
},
"files": {
"totalLinesAdded": 0,
"totalLinesRemoved": 0
}
},
"error": {
"type": "INVALID_STREAM",
"message": "Invalid stream: The model returned an empty response or malformed tool call."
}
}EOF
gemini_errors<<EOF
Warning: 256-color support not detected. Using a terminal with at least 256-color support is recommended for a better visual experience.
Warning: tools.exclude in settings.json is deprecated and will be removed in 1.0. Migrate to Policy Engine: https://geminicli.com/docs/core/policy-engine/
YOLO mode is enabled. All tool calls will be automatically approved.
YOLO mode is enabled. All tool calls will be automatically approved.
Ripgrep is not available. Falling back to GrepTool.
[STARTUP] Phase 'cleanup_ops' was started but never ended. Skipping metrics.
[STARTUP] Cannot measure phase 'cleanup_ops': start mark 'startup:cleanup_ops:start' not found (likely cleared by reset).
Reviewed by Gemini (gemini-3.1-pro-preview)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20678b306e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // distinctTokenCount counts the distinct tokens in s — the resolution-bearing | ||
| // token count for corroborationHasResolution, where a repeated token is one | ||
| // piece of evidence rather than several. Sorting in place is safe because |
There was a problem hiding this comment.
Fresh evidence in this version is that the new guard only checks each side has two distinct tokens, not that two distinct tokens are shared. For example, queryOrg="Acme Acme Holdings" and registrant "Acme Landscaping Tampa" normalize to raw token counts of 3 and distinct counts of 2/3, so this line passes; tokenSimilarity then counts the repeated acme twice (2/3 >= 0.60) and the candidate still corroborates on one independent shared token. This leaves the duplicate-token false-positive class open whenever the duplicated side also carries one unrelated distinct token; the floor needs to be based on the distinct intersection/shared evidence, not just per-side distinct cardinality.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
CONFIRMED — fixed in b4dd124 (branch now at c494241). Good catch; this was a real hole and your example is exactly right.
I verified it by executing the real package functions under go test -overlay rather than reasoning about it, because it turns on whether holdings survives normalization (it does — it is deliberately excluded from orgLegalSuffixes as a disambiguating token):
q="Acme Acme Holdings" c="Acme Landscaping Tampa"
nq="acme acme holdings" nc="acme landscaping tampa"
rawq=3 rawc=3 dq=2 dc=3 distinctShared=1 sim=0.6667 gate=true corroborates=true
So the per-side floor I added in 20678b3 measured the wrong thing. Two distinct tokens on a side says nothing about shared evidence; the property that matters is two distinct tokens in the intersection. The floor is now:
return distinctSharedTokenCount(nq, nc) >= minCorroborateTokensThe shared count is bounded by each side's distinct count, so this strictly subsumes the retired predicate — one floor, not an AND. distinctTokenCount is deleted. The candidate side is deduped before it is walked; skipping that would have reproduced inside the floor the very multiset/set asymmetry the floor exists to neutralize.
After the fix both defect pairs land at 0.50 unverified — de-ranked inside [0.35, 0.65), not dropped — while "Praetorian Security" vs "Praetorian Security Group" (distinctShared=2) still corroborates at 0.60, "Praetorian"/"Praetorian Inc" still corroborates through the exact-equality exemption, and "Apple"/"Walmart" still de-ranks to 0.40 rather than being rescued.
Your finding also refutes the Claude review on this PR, which is worth recording since the two reviews contradict each other and only one can be right. Claude wrote:
To pass the
min(distinct) >= 2gate while corroborating on one real shared token, the shorter side would need a repeat — which drops its distinct count below 2 and the gate refuses it. Robust.
That inference does not hold: a three-raw-token side can carry a repeat and still have two distinct tokens. "acme acme holdings" is precisely that shape, which is why it passed the gate. Claude's "Robust" conclusion on this specific point is REFUTED; the rest of its review (guard placement, PII, band invariant, no new network calls) is accurate and independently verified.
Gemini produced no round-2 review — its run succeeded but the model returned INVALID_STREAM: The model returned an empty response or malformed tool call, so there is nothing to adjudicate from it. Re-requesting below rather than counting silence as approval.
Also corrected the reasoning this finding falsified, in four comment sites plus the repo doc. The claim "crossing simCorroborate at k >= 2 already forces m >= 2 shared normalized tokens" was load-bearing for leaving k >= 2 out of scope, and it is false — m counts matching occurrences, not distinct tokens. The guard now enforces the shared-evidence property directly instead of assuming the metric implies it, and what remains out of scope is stated plainly: full containment on two genuinely distinct shared tokens still corroborates, which is real evidence rather than a duplicate artifact.
Reversion proof (mine, re-run independently — not relayed from the implementing agent): under an overlay restoring the per-side floor, exactly four subtests across three test functions fail, all of them the new round-2 cases, and nothing else in the package does. That is direct evidence the pre-existing suite could not have caught this class.
The metric itself is still unrepaired — github-org scores against the same tokenSimilarity and its pinned GreaterOrEqual floor assertion encodes containment as desired for that caller — now tracked as ENG-5890. github_org.go and github_org_test.go are byte-identical to origin/main.
This branch also merged origin/main (upstream unit-test timeout fixes); domains now runs in 2.6s instead of 116s.
Round-3 re-review please — the fix for a round-2 finding is itself unreviewed, and these workflows do not trigger on synchronize. @codex @gemini @claude
There was a problem hiding this comment.
Codex Review: Didn't find any major issues. Swish!
Reviewed commit: c4942418a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Claude Review
Critical issues
None. The fix is minimal and correct: gating the corroborate arm on corroborationHasResolution(nq, nc) closes the k=1 saturation class (sim = m/min(kq,kc) divides by the shorter side, so any single shared token scored 1.0). Verified statically:
slicesis already imported, sodistinctTokenCountbuilds;unverifiedis pre-constructed so the fall-through preserves de-rank-never-drop; the guard sits after the empty-normalization guard and is confined to the corroborate arm, so a clean m=0 mismatch still de-ranks rather than being rescued.- The distinct-token floor (vs raw slice length) genuinely closes the duplicate loophole:
tokenSimilaritydivides by rawlen(shorter)and matches per-occurrence, so "acme acme" would otherwise pose as k=2. To pass themin(distinct) >= 2gate while corroborating on one real shared token, the shorter side would need a repeat — which drops its distinct count below 2 and the gate refuses it. Robust.
Security
No security concerns flagged. No new network calls; the fix reuses the existing PII-free justifyReverseWhoisUnverified string with no interpolation, so no resolved registrant reaches output. The SSRF-safe dialer / byte-cap machinery is untouched.
Test coverage
Excellent and unusually rigorous — the asymmetric pair builder reproduces the exact defect shape (kq=1, kc>=2, m=1, strings never equal), reversion-testing confirmed four test functions fail without the guard, and the query-side/candidate-side duplicate cases plus benign controls pin the distinct-count floor from both directions.
No critical issues — LGTM pending human review.
… floor (ENG-5374) Round-2 review finding, confirmed by execution. The previous commit made the resolution floor count distinct tokens PER SIDE, which is not the property that matters. A side with three raw tokens can carry a repeat and still hold two distinct tokens, clearing a per-side floor while only one token is genuinely shared: query "Acme Acme Holdings" -> "acme acme holdings" (3 raw, 2 distinct) registrant "Acme Landscaping Tampa" -> "acme landscaping tampa" (3 raw, 3 distinct) min(2, 3) >= 2 passes the floor, yet exactly ONE token is shared. The shared metric then credits the repeated "acme" once per occurrence -- it walks the shorter side's token slice against a membership set -- so 2/3 = 0.667 crosses simCorroborate and the candidate corroborated on a single shared token. "holdings" is deliberately not a legal-form suffix, so the duplicate survives normalization. The floor now measures the distinct token INTERSECTION. That count is bounded by each side's distinct count, so the new predicate strictly subsumes the old one -- one floor, not an AND. The candidate side is deduped before it is walked, since skipping that would reproduce inside the floor the same multiset/set asymmetry the floor exists to neutralize. Also retracts the inference the finding falsified, in four comment sites and the repo doc: "crossing simCorroborate at k >= 2 already forces m >= 2 shared tokens" is false, because m counts matching OCCURRENCES rather than distinct tokens. The guard now enforces the shared-evidence property directly instead of assuming the metric implies it. What remains out of scope is stated plainly: full containment on two genuinely distinct shared tokens still corroborates, which is real evidence rather than a duplicate artifact. The shared metric itself is still not repaired -- the github-org plugin scores against it and its pinned floor assertion encodes containment as desired for that caller. Tracked as ENG-5890. Verified independently, not relayed: gofmt clean, go vet exit 0, build exit 0, full suite green (domains 116.280s). Under an overlay reverting the floor to its per-side form, exactly four subtests across three test functions fail -- all of them the new round-2 cases -- and nothing else in the package does, which is direct evidence the pre-existing suite could not have caught this class. The github-org plugin and its test are byte-identical to the merge base. Refs ENG-5374, ENG-5890
…eng-5374-pius-reverse-whois-calibrate-corroboration-thresholds-fix
There was a problem hiding this comment.
Gemini Review
Critical Issues
None.
Security
No security concerns flagged.
Suggestions
- Optimize
distinctSharedTokenCount: Since bothqueryTokensandcandidateTokensare freshly allocated bytokenize, you could sort and compact both slices in-place. This allows you to find the number of shared tokens using a simple two-pointer intersection walk, completely avoiding theinQuerymap allocation. - Redundant Tokenization: In
decideConfidence,normalizeOrgtokenizes and joins the strings,tokenSimilaritytokenizes them again, anddistinctSharedTokenCounttokenizes them a third time. To save allocations, consider refactoringnormalizeOrgto return a[]stringand updating the downstream scoring helpers to accept token slices directly (though this would also require updating the sharedtokenSimilarityusage ingithub_org.go).
Reviewed by Gemini (gemini-3.1-pro-preview)
There was a problem hiding this comment.
Claude Review
Critical issues
None. The fix is narrow and correct: corroborationHasResolution gates only the corroborate arm, and because sim >= simCorroborate (0.60) can never also satisfy sim < simMismatch (0.30), an unresolved high-similarity pair falls through to the existing unverified decision rather than being mis-classified as a mismatch. distinctSharedTokenCount dedupes both sides (query via set, candidate via slices.Compact), so the multiset/set asymmetry it exists to neutralize cannot leak back in, and the nq==nc exemption runs after the empty-string guard so it cannot fire on all-suffix orgs. Symmetry, band-membership, and de-rank-never-drop invariants all hold.
Security
No security concerns flagged. No new network calls or secrets; the change is a pure scoring refinement. Registrant PII in justifications stays guarded: the fix reuses the existing PII-free unverified justification with no interpolation, and TestDecideConfidenceEveryReturnPointStaysInsideTheReviewBand asserts no justification reproduces the resolved registrant.
Test coverage
Thorough. New reverse_whois_calibration_test.go (enumeration + property tests) plus TestDistinctSharedTokenCount, the ENG-5374 round-2 duplicate-token cases, and the exact-equality exemption pin. One note: that suite Len(covered, 5) check keys off table labels, not production, so a sixth return added to decideConfidence would not trip it (already flagged as a manual step in the comment, which is acceptable).
Note: I could not execute the suite here (build/test commands are permission-gated in this environment), so the above is a static review. The PR reports go test ./... green with a guard-reversion check failing four functions, which is consistent with the diff.
Automated-review adjudication — ENG-5374 (final round)frame check skipped — no non-trivial machinery: the shipped diff swaps one predicate inside an existing guard at the layer that already owned the decision, adding no new layer, abstraction, or subsystem (condition: size). Re-examined after round 2 grew the diff to 4 files / +321−105, and the condition still holds: 222 of those lines are tests, and the round-2 change is the narrowest form of the same fix at the same layer. The ticket's second item — build a calibration harness to derive the thresholds empirically — did get a premise verdict, and it was WRONG-LAYER: any harness buildable under the hermetic-test AC would score against the same synthetic corpus that produced the current constants, making it a circular measurement rather than a calibration. That verdict is discharged by compliance — the harness was not built. What shipped instead is an enumeration + property table that pins what can be established hermetically, and the calibrate-against-production-data work stays in ENG-5166, which already carries that AC verbatim. Nothing undischarged remains. reviewer coverage @ Findings
Finding 5 — the confirmation, and what it falsifiedVerified by executing the real package functions under The floor is now the distinct intersection, not per-side cardinality: return distinctSharedTokenCount(nq, nc) >= minCorroborateTokensThe shared count is bounded by each side's distinct count, so this strictly subsumes the retired predicate — one floor, not an AND. It also falsified the inference that had justified leaving the Reversion proof (run here, not relayed from the implementing agent — an overlay restoring the per-side floor, with exactly one verbatim match asserted before replacing so a silent no-op stub was impossible): exactly four subtests across three test functions fail, all of them the new round-2 cases, and nothing else in the package does. Direct evidence the pre-existing suite could not have caught this class. Finding 8 — measuredThe suggestion is behaviorally identical (169 ordered pairs over a 13-name corpus agree) and ~8% faster in CPU. But its stated benefit does not exist: Identical bytes and identical allocation counts, because there is no map allocation to avoid — escape analysis puts it on the stack: The residual ~24ns/call is real but immaterial: at the Verification
Three rounds used, which is the full budget. Not merging — that stays a human decision, and #122 below needs one first.
Reviewer items collected through: 2026-08-05T21:58:10Z |
#127 now stacks on #122 and must merge after it. ENG-5374's Item 1 — the over-corroboration fix — is delivered by #122's Jaccard metric, not by this branch, so this branch's own production predicate is deleted rather than reconciled. Deleted: corroborationHasResolution, distinctSharedTokenCount, and minCorroborateTokens. Against Jaccard that floor is provably redundant, since 1/(kq+kc-1) >= 0.60 forces kq+kc <= 2, i.e. only kq=kc=1 — both sides the same single token, which is the legitimate match the plugin exists to find. It was also a small regression on top of Jaccard: "Acme Acme" vs "Acme" has distinct token sets that are the same singleton (Jaccard 1.0, correctly corroborated) yet differ as joined strings, so the floor's equality exemption missed and it de-ranked a pair #122 scores correctly. The reason it must not be re-added is recorded in reverse_whois_verify.go. Conflicts resolved in #122's favor; what remains here is additive: reverse_whois_verify.go #122's version, plus a comment-only CALIBRATION BASIS block (zero added non-comment lines) — no production predicate of its own. reverse_whois_calibration_test.go re-derived for Jaccard. TestSimilarityReachabilityByTokenCount -> TestJaccardReachabilityByTokenCount; every cell's expected similarity AND arm hand-written, so a regression must be mirrored in both encodings to pass. All 27 equal-count cells and 9 asymmetric rows matched production on the first run. reverse_whois_verify_test.go keeps this PR's retitle of the mislabeled "corroborated partial (shorter fully contained)" row; #122's competing title discarded, inputs and expectation untouched. CLAUDE.md #122's two lines verbatim plus an additive calibration-basis sub-bullet. Also pinned as deliberate, neither previously covered: kq=2,kc=4,m=2 moves 1.0 corroborated -> 0.5 unverified, and kq=1,kc=5,m=1 moves 1.0 -> 0.2 UNVERIFIED rather than mismatch, rescued from the de-rank arm by #122's tokenSetContained and pinned against its non-contained sibling kq=2,kc=5,m=1 (0.1667, mismatch). Stated narrowly: #122 did not make containment stop corroborating — a contained pair scores exactly kq/kc, so kq=3,kc=4 is still 0.75 — it removed containment's power to corroborate regardless of how much longer the candidate is. github_org.go is byte-identical to #122; github_org_test.go is #122's content plus origin/main's network-error test refactor picked up by this branch's earlier merge. This branch contributes nothing to either. Item 2 of the ticket (build a calibration harness) keeps its WRONG-LAYER premise verdict, unchanged and metric-independent, and stays in ENG-5166. gofmt clean · go vet ./... clean · go build ./cmd/pius clean · go test -count=1 ./... green. Tests hermetic: no live ViewDNS/Whoxy/RDAP/WHOIS, no keys. Refs ENG-5374, ENG-5172 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
santiago-praetorian
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/plugins/domains/github_org.go (1)
324-341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving
jaccardTokenSetsnext to its only consumer.
github_org.godeliberately keepstokenSimilarityas its own metric, as the doc comment states. The only production caller ofjaccardTokenSetsisdecideConfidenceinreverse_whois_verify.go. A shared token-metric file, orreverse_whois_verify.goitself, would keep the dependency direction clearer. The set-building loop is also duplicated intokenSetContained; a smalldistinctTokens([]string) map[string]boolhelper would serve both. Behavior stays identical, so this is optional.🤖 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/plugins/domains/github_org.go` around lines 324 - 341, Move jaccardTokenSets from github_org.go next to its sole production consumer, decideConfidence in reverse_whois_verify.go, or to an appropriate shared token-metric file. Extract the duplicated token set construction into a distinctTokens helper and reuse it from both jaccardTokenSets and tokenSetContained, preserving all existing behavior.
🤖 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/plugins/domains/github_org.go`:
- Around line 324-341: Move jaccardTokenSets from github_org.go next to its sole
production consumer, decideConfidence in reverse_whois_verify.go, or to an
appropriate shared token-metric file. Extract the duplicated token set
construction into a distinctTokens helper and reuse it from both
jaccardTokenSets and tokenSetContained, preserving all existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 465c92ac-aff3-464a-a339-46d68729dbde
📒 Files selected for processing (6)
CLAUDE.mdpkg/plugins/domains/github_org.gopkg/plugins/domains/github_org_test.gopkg/plugins/domains/reverse_whois_calibration_test.gopkg/plugins/domains/reverse_whois_verify.gopkg/plugins/domains/reverse_whois_verify_test.go
| // 0.95 would not have changed a single verdict. That is why neither ENG-5374 nor | ||
| // ENG-5172 was a threshold re-tuning: the defect was the metric's RESOLUTION, so | ||
| // both tickets changed what is measured and left the constants where they were. | ||
| func TestJaccardReachabilityByTokenCount(t *testing.T) { |
There was a problem hiding this comment.
@codex @gemini @claude — please re-review from scratch. Revision 5 changed what this PR is, so the three review rounds already adjudicated on revisions 1–4 do not cover it; please do not rely on them.
What changed: this PR now stacks on #122 (ENG-5172) and merges its branch in. Its own production predicate — corroborationHasResolution / distinctSharedTokenCount / minCorroborateTokens — is deleted, because under #122's Jaccard metric it is provably redundant (1/(kq+kc-1) >= 0.60 forces kq+kc <= 2, i.e. only kq=kc=1) and was a small regression on duplicate-token equal-singleton inputs. reverse_whois_verify.go is now #122's version plus a comment-only CALIBRATION BASIS block — zero added non-comment lines. What remains here is tests and docs.
Reading the diff: until #122 merges, GitHub diffs this branch against main, so #122's changes appear here too (jaccardTokenSets, tokenSetContained, the verifier's swap onto Jaccard). Those are #122's and are not under review here. This PR's own contribution is the diff against 33ae0b9: CLAUDE.md +1 line, reverse_whois_verify.go +52 comment-only lines, a retitled TestDecideConfidence row, and this file.
Where I would most value scrutiny:
- The enumeration's correctness — this file hand-writes an expected similarity and an expected arm for all 27 equal-count cells and 9 asymmetric rows. If any cell is wrong, the table and production are wrong together and the test passes anyway. Please recompute a few independently rather than checking internal consistency.
- The redundancy argument for deleting the floor. If a shape corroborates on a single shared distinct token with
kq+kc > 2under Jaccard, the deletion is wrong and I want to know. - The narrow claim about containment. I deliberately do not say "containment no longer corroborates" — a fully contained pair scores exactly
kq/kc, sokq=3,kc=4,m=3is0.75and still corroborates. The pinned rows sit atkc = 2*kq. Please check the comments state the narrow claim and not the tidy-but-false one. - Whether the
kq=1,kc=5,m=1vskq=2,kc=5,m=1contrast actually isolatestokenSetContainedas the cause.
On CI: ci / Test is red, and it is not this PR. The 8 failures are all in favicon_hash_test.go and reproduce on main's own tip (8de8a07) standalone — main is currently red for the same reason. Cause: favicon_hash.go:181 and :253 guard the CIDR observation with if err != nil on faviconHostCIDR's error, so a successfully parsed IP emits no CIDR finding. Introduced by #124 (commit a1a9b93), not by this branch, which does not touch that file. Reported separately; please review this PR's diff on its merits.
There was a problem hiding this comment.
Gemini Review
{
"session_id": "246967e0-4fdf-4b10-a35f-295ac4bc3019",
"response": "",
"stats": {
"models": {
"gemini-3.1-pro-preview": {
"api": {
"totalRequests": 2,
"totalErrors": 0,
"totalLatencyMs": 6614
},
"tokens": {
"input": 33887,
"prompt": 42047,
"candidates": 26,
"total": 42201,
"cached": 8160,
"thoughts": 128,
"tool": 0
},
"roles": {
"main": {
"totalRequests": 2,
"totalErrors": 0,
"totalLatencyMs": 6614,
"tokens": {
"input": 33887,
"prompt": 42047,
"candidates": 26,
"total": 42201,
"cached": 8160,
"thoughts": 128,
"tool": 0
}
}
}
}
},
"tools": {
"totalCalls": 1,
"totalSuccess": 1,
"totalFail": 0,
"totalDurationMs": 9,
"totalDecisions": {
"accept": 1,
"reject": 0,
"modify": 0,
"auto_accept": 0
},
"byName": {
"read_file": {
"count": 1,
"success": 1,
"fail": 0,
"durationMs": 9,
"decisions": {
"accept": 1,
"reject": 0,
"modify": 0,
"auto_accept": 0
}
}
}
},
"files": {
"totalLinesAdded": 0,
"totalLinesRemoved": 0
}
},
"error": {
"type": "INVALID_STREAM",
"message": "Invalid stream: The model returned an empty response or malformed tool call."
}
}EOF
gemini_errors<<EOF
Warning: 256-color support not detected. Using a terminal with at least 256-color support is recommended for a better visual experience.
Warning: tools.exclude in settings.json is deprecated and will be removed in 1.0. Migrate to Policy Engine: https://geminicli.com/docs/core/policy-engine/
YOLO mode is enabled. All tool calls will be automatically approved.
YOLO mode is enabled. All tool calls will be automatically approved.
Ripgrep is not available. Falling back to GrepTool.
Reviewed by Gemini (gemini-3.1-pro-preview)
There was a problem hiding this comment.
Codex Review
Critical Issues
None.
Security
No security concerns flagged.
Suggestions
No suggestions.
Static review only: go test ./pkg/plugins/domains could not run because the environment is read-only and Go could not create a module/build cache.
Reviewed by Codex (gpt-5.5)
There was a problem hiding this comment.
Claude Review
Critical issues
- None. This PR's own diff (vs. its stacked base #122) is tests + comments only — the Jaccard metric swap,
jaccardTokenSets,tokenSetContained, and thedecideConfidencerewrite all belong to #122, which appears here only because GitHub diffs againstmain. There is no new production predicate of its own to break. Reviewers must not merge before #122 (author flagged this prominently). - Verified the calibration enumeration by hand across both tables (equal-count
k=1..6and the asymmetric shapes): everym/(kq+kc-m)value and arm assignment checks out, the boundary cellk=4,m=3=0.60correctly relies on the inclusive>=, and thecalibrationTokenspool covers the largest shape (kq+kc-m=11). Math is sound.
Security
No security concerns flagged. No new network calls, crypto, auth, or secrets — all added tests are hermetic (no live ViewDNS/Whoxy/RDAP/WHOIS), and the justification-never-logs-PII invariant is itself pinned by a test assertion.
Test coverage
Tests are present and unusually thorough (enumeration + boundary + symmetry + review-band invariants). One self-acknowledged gap worth keeping in view: TestDecideConfidenceEveryReturnPointStaysInsideTheReviewBand's Len(covered, 5) observes the table's own labels, not decideConfidence's actual return statements — a sixth return added in production with no matching case would pass silently. The author documents this as a manual step; fine, but it is a real coverage limit rather than a proof.
No critical issues — LGTM pending human review (and pending #122 landing first).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd5ceb1027
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // kq+kc >= 8, so the mismatch arm was in practice "no shared tokens". | ||
| // Jaccard divides by the union, so that bound drops to kq+kc >= 5 | ||
| // ("Walmart" vs {walmart,global,enterprises,holdings} = 1/4 = 0.25) — and | ||
| // 34 of the 342 shapes below simMismatch are PURE CONTAINMENT. Making the |
There was a problem hiding this comment.
This calibration claim gives an exact 34 of the 342 count without defining the finite universe being counted. With unrestricted org token counts, the below-simMismatch shared-token shapes are unbounded, and pure-containment examples continue indefinitely, e.g. (kq=1,kc>=4,m=1). If this is meant to summarize a bounded enumeration, the bound needs to be stated (or the count removed), otherwise the recorded calibration basis is misleading.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
CONFIRMED — fixed in 19190ef.
You were right, and re-deriving it made the point sharper. The figures are reproducible, but only over a bound the comment never disclosed: exhaustive enumeration over 1 <= kq,kc <= 12, 1 <= m <= min(kq,kc), sim = m/(kq+kc-m) < 0.30 gives exactly 342 shapes, 34 of them pure containment. Nothing motivates 12, and neighbouring universes disagree:
| universe | total below simMismatch |
of which pure containment |
|---|---|---|
kq,kc <= 10, m >= 1 |
194 | 22 |
kq,kc <= 11, m >= 1 |
261 | 28 |
kq,kc <= 12, m >= 1 |
342 | 34 |
kq,kc <= 13, m >= 1 |
437 | 40 |
kq+kc <= 13, m >= 1 |
145 | 34 |
Only that one universe reproduces both numbers, and the last row shows 34 alone doesn't pin it. The bound also silently dropped m = 0, which moves the total to 486 while leaving the containment count unchanged — so the count was an artifact twice over.
Rather than disclose an arbitrary cutoff, I took your first branch and removed the count, replacing it with the exact unbounded statement, since you're right that the family is infinite: under Jaccard a fully contained pair (m = kq) scores exactly kq/kc, so it sits below simMismatch for every kc > 10*kq/3 — (1,4), (1,5), ... , (3,11), ... without end. The claim the block exists to support is unchanged and now rests on a closed form instead of a bounded sample.
Comment-only: zero added non-comment lines (verified against the diff), gofmt clean, go build clean, package tests green.
…ation basis (ENG-5374) Codex review finding (P3) on PR #127: the CALIBRATION BASIS block quoted "34 of the 342 shapes below simMismatch are PURE CONTAINMENT" without defining the universe being counted. CONFIRMED. The pair is reproducible, but only over an arbitrary and undisclosed bound -- exhaustive enumeration over 1 <= kq,kc <= 12 with m >= 1 and sim < 0.30 yields exactly 342 shapes, 34 of them pure containment. No other square universe reproduces both figures, and no kq+kc <= K universe does either. The count was therefore an artifact of a bound the comment never disclosed, and the reviewer is right that a recorded calibration basis must not read that way. The underlying set is infinite, so the repair is to state the exact unbounded fact rather than to name the bound: under Jaccard a fully contained pair (m = kq) scores exactly kq/kc, so it sits below simMismatch for every kc > 10*kq/3 -- (1,4), (1,5), ... , (3,11), ... without end. The qualitative claim the block exists to support is unchanged and now rests on an exact statement: making the metric specificity-monotone is what put pure-containment shapes in reach of the de-rank arm, which is why tokenSetContained (#122) is needed. Comment-only: zero added non-comment lines, verified against the diff. gofmt clean, go build clean, package tests green. Refs ENG-5374, ENG-5172 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adjudication — round 4 (revision 5, the
|
| universe | total below simMismatch |
of which pure containment |
|---|---|---|
kq,kc <= 10, m >= 1 |
194 | 22 |
kq,kc <= 11, m >= 1 |
261 | 28 |
kq,kc <= 12, m >= 1 |
342 ✓ | 34 ✓ |
kq,kc <= 13, m >= 1 |
437 | 40 |
kq+kc <= 13, m >= 1 |
145 | 34 |
Only that one universe reproduces both figures — note the last row, where the containment count matches but the total does not, so 34 alone does not pin it. The count was an artifact of an undisclosed cutoff on kc, exactly the reviewer's read; and it also silently excluded m = 0, which changes the total (486, not 342) while leaving the containment count alone.
Rather than disclose an arbitrary bound, I removed the count and stated the exact unbounded fact, since Codex is right that the family is infinite: under Jaccard a fully contained pair (m = kq) scores exactly kq/kc, so it sits below simMismatch for every kc > 10·kq/3 — (1,4), (1,5), …, (3,11), … without end. The claim the block exists to support is unchanged and now rests on an exact statement instead of a bounded sample.
Fixed in the follow-up commit; comment-only, zero added non-comment lines (verified against the diff), gofmt/go build/package tests green.
2. CodeRabbit, nitpick — relocate jaccardTokenSets, extract distinctTokens (github_org.go:324-341) — ❌ REFUTED (out of scope)
Both cited lines are #122's code, not this PR's. jaccardTokenSets and tokenSetContained are introduced by #122 (ENG-5172); this PR contributes no change to github_org.go — it is byte-identical to #122's version, which I verified rather than asserted.
Acting on this would recreate the exact collision revision 5 exists to resolve: I would be re-editing another engineer's in-flight lines in a PR that is stacked on theirs, and #122 would then conflict on merge. This is the same disposition as round-3 finding 9, for the same reason. The suggestion is reasonable on its merits and belongs on #122, where the author can take or leave it; it is not this PR's to make.
3. Claude — assert.Len(t, covered, 5) is a coverage limit, not a proof — 📌 ACCEPTED as a recorded known limit, no change
Right, and it's already documented as such in the test rather than dressed up as exhaustiveness. The assertion pins that the enumeration reached all five arms; it does not prove the table covers every reachable shape, and nothing in the PR claims it does. What carries the exhaustiveness weight is the closed-form derivation (1/(kq+kc-1) >= 0.60 ⟹ kq+kc <= 2), which is an exact bound rather than a search limit — and, after finding 1 above, the containment family is now stated in closed form too. Recording the observation here so it's on the thread; no code change, because the limitation is real and honestly labelled.
Claude also independently re-derived every m/(kq+kc-m) value and arm assignment across both tables by hand and confirmed them, including that the boundary cell k=4, m=3 = 0.60 correctly depends on the inclusive >=. That independent check is the useful result here.
4. Codex body review — ✅ CLEAN
"Critical Issues: None. Security: No security concerns flagged. Suggestions: No suggestions." Noted with its own caveat: static-review-only, because the environment was read-only.
5. Gemini — ⚠️ NO RESULT (INVALID_STREAM, second occurrence)
The run failed with INVALID_STREAM: "The model returned an empty response or malformed tool call." — it produced no review at all. This is the second time Gemini has failed this way on this PR (round 2's finding 7 was the same). This is not an approval and is not counted as one. Gemini's coverage of revision 5 is simply absent, and I'm recording that as a gap rather than papering over it.
CI
ci / Test is red on this branch for a reason that is not this PR's: main's tip (8de8a07) carries an inverted error check in favicon_hash.go (if err != nil where it must be err == nil, at both :179-181 and :251-253, from commit a1a9b93). GitHub Actions checks out refs/pull/127/merge — this branch merged with main's tip — so main's 8 failures land on every open PR. I reproduced all 8 standalone in a throwaway detached worktree at 8de8a07, and this branch does not contain that commit (merge-base 6f82e49). #132 already fixes it in flight and its ci / Test passes. I'll git merge origin/main (never rebase) and re-verify once #132 lands.
Status
Merge order is unchanged and binding: #122 first, then this PR. Merging is a human decision — I'm stopping at adjudicated + green-CI-pending-#132 and not merging. ENG-5374 stays In Progress.
2 participants
Fixes ENG-5374
Retraction: this PR's original production fix was redundant, and is deleted
The first three revisions of this PR added a shared-evidence floor to the corroborate arm —
corroborationHasResolution/distinctSharedTokenCount/minCorroborateTokens = 2. All three are now deleted. #122 replaces the metric with Jaccard, and against Jaccard that floor is provably redundant:Under Jaccard,
sim = m/(kq+kc-m)for distinct token countskq,kcand shared distinct countm. Asking which shapes can corroborate on a single shared token:That is an exact bound, not a search limit — and
kq=kc=1means both sides normalize to the same single token ("Praetorian"vs"Praetorian Inc"), which is the legitimate match the plugin exists to find. So Jaccard already forces ≥2 shared distinct tokens to corroborate. Under the old containment coefficientm / min(kq,kc)the same question admitted an unbounded family — every(1, kc)shape scored1/1 = 1.0on one shared token — which was exactly the ENG-5374 defect.The floor was not merely redundant but a small regression on top of Jaccard:
"Acme Acme"vs"Acme"has distinct token sets that are the same singleton (Jaccard1.0, correctly corroborated), yet the two differ as joined strings, so the floor's equality exemption missed and it de-ranked a pair #122 scores correctly. It is deliberately not reinstated, and the reason is recorded inreverse_whois_verify.goso a future reader doesn't re-add it.Premise-gate verdict on the ticket's Item 2: WRONG-LAYER — unchanged, and metric-independent
This part of the review stands exactly as before; #122 does not touch it.
The ticket asked for: a hermetic fixture corpus + measurement harness, run to confirm-or-reset
simCorroborate(0.60) andsimMismatch(0.30), on the premise that the mis-scoring is attributable to those values.Why that premise is refuted — corpus-free, by enumeration. At
kq=kc=1the reachable similarity set is exactly{0.0, 1.0}; the ambiguous arm is unreachable. So there the cutoffs have zero discriminating power for every possible value ofsimCorroboratein (0,1] — re-setting 0.60 to 0.7/0.8/0.95 changes no verdict. This held for the containment coefficient and holds identically for Jaccard, which is why neither ticket is a re-tuning. The defect was the metric's resolution, never the threshold's value.Why the fixture corpus was also refuted, independently — label provenance. "0.60 is the right cutoff" is a precision/recall claim at a decision boundary. It needs (i) candidates from the real ViewDNS/Whoxy return distribution, (ii) live-resolved registrant strings, (iii) per-candidate ownership labels of independent provenance. (ii) and (iii) are forbidden by this ticket's own hermetic AC, and fixtures authored here cannot substitute: their labels would be assigned by the same judgment that picked the threshold, so any precision/recall figure would be the prior re-derived wearing the authority of a measurement. That work needs production data and stays in ENG-5166, which already carries the AC verbatim.
Constants stay at 0.60 / 0.30, confirmed rather than silently dropped, with the basis written down in the
CALIBRATION BASISblock inreverse_whois_verify.goand inCLAUDE.md.What this PR contributes on top of #122
1. The enumeration, re-derived for Jaccard (
reverse_whois_calibration_test.go,TestJaccardReachabilityByTokenCount). Corpus-free. Every cell's expected similarity and expected arm is hand-written, and production is checked against the table — so a regression has to be mirrored in both encodings to pass. Two tables: equal-count (kq=kc=k, k=1..6) and asymmetric (kq<kc), the second because the equal-count construction structurally cannot express the defect's shape.2. Two of #122's behavior changes, pinned as deliberate. Neither was covered before:
kq=2, kc=4, m=21.0→ corroborated0.5→ unverifiedkq/kc, so it no longer corroborates regardless of how much longer the candidate iskq=1, kc=5, m=11.0→ corroborated0.2→ unverified, not mismatchtokenSetContainedrescues it from the de-rank armThe second row's contrast is the whole justification for the containment guard, so it is pinned against its non-contained sibling
kq=2, kc=5, m=1(0.1667→ mismatch), checked against production rather than against the table's own expectation. Both sides are first asserted to sit belowsimMismatch, so the contrast cannot silently degenerate into "one of them was simply above the threshold".Worth stating precisely, because the obvious summary is wrong: #122 did not make containment stop corroborating. Under Jaccard a fully-contained pair scores exactly
kq/kc, so it still corroborates whenever the candidate is at most ~1.67× longer (kq=3, kc=4, m=3→0.75). What #122 removed is containment's power to corroborate regardless of how much longer the candidate is — the old coefficient returned1.0for everykc. Both table rows above sit atkc = 2·kq, hence exactly0.5. The test comment states the narrow claim rather than the tidy one.3. A corrected mislabeled test case. A pre-existing
TestDecideConfidencerow was titled"corroborated partial (shorter fully contained)"while its inputs ("Acme"vs"Acme Corp") reduce after suffix-stripping to a single identical token — i.e. Jaccard1/1 = 1.0, an equality, not containment. The old title claimed the table covered non-exact single-token containment, which it never did. That gap is precisely what let the over-corroboration defect pass review. Retitled to"corroborated exact equality after suffix stripping (acme == acme)"; inputs and expectation untouched. Attributed against merge based760ec8— both #122 and this PR retitled that same row, and this PR's title is the accurate one.4. A recorded known limitation, now measurably smaller. Under containment, the similarity value could only fall below
simMismatchwith a shared token present oncekq+kc >= 8, so in practice the mismatch arm meant "no shared tokens at all". Jaccard divides by the union, so that drops tokq+kc >= 5(kq=1, kc=4, m=1→0.25). Note the distinction that makes the guard necessary: crossing the threshold is a statement about the value, not about the arm firing — that particular shape is pure containment, sotokenSetContainedholds it at unverified anyway. The value-crossing region grows; the de-rank region does not follow it blindly. For equal-count shapes, where containment cannot apply, the firstkat which mismatch is genuinely reachable with a shared token moves fromk=4tok=3(0.20), and that figure is asserted.tokenSimilarity's own multiset/set asymmetry is untouched (github_orgstill scores against it and pins a floor assertion) and stays tracked as ENG-5890.Scope discipline
github_org.go/github_org_test.gocarry #122's content, auto-merged with no conflict — this PR contributed no change to either, before or after the merge.github_org.gois byte-identical to #122's version;github_org_test.gois #122's content plusorigin/main's network-error test refactor (TestGitHubOrgPlugin_Run_GracefulOnHTTPError/…OnNetworkError), which this branch picked up in revision 4'sorigin/mainmerge and #122 predates — so that hunk isorigin/main's, not this PR's, and #122 acquires it on its own merge.jaccardTokenSetsandTestJaccardTokenSetsin those files are #122's, and both are attributed to it in the diff.pkg/plugins/domains/shows 5 files listed bygofmt -l; all 5 are byte-identical toorigin/main(pre-existing drift, verified, and CI is green on main), so they are untouched here.Verification
gofmtclean on every file this PR touches ·go vet ./...clean ·go build ./cmd/piusclean ·go test -count=1 ./...green. All tests hermetic — no live ViewDNS/Whoxy/RDAP/WHOIS, no keys, no network. De-rank never drop: every outcome inside[0.35, 0.65), exactly oneconfidenceDecisionperdecideConfidencecall.Revision history
origin/main34 of the 342shape count with a closed form (comment-only)kq,kc <= 12); the family is infinite, so it is now stated askc > 10*kq/3Four rounds of automated review have been adjudicated. Revisions 1–4: 11 findings (3 confirmed and fixed, 5 refuted with evidence, 1 no-result, 2 clean). Revision 5 emptied the production diff, so it was re-reviewed from scratch rather than inheriting those rounds — round 4 ran 5 findings (1 confirmed and fixed, 1 refuted as out of scope, 1 recorded as a known limit, 1 clean, 1 no-result). Verdicts and evidence are in the adjudication comment on this PR.