Add additive numeric scoring and configurable confidence intervals with supporting tests

[omkar-foss]

Closes #12 and #74.

This PR adds additive numeric scoring (example here) and configurable configurable intervals (default ones are low=0 to 30, medium=31 to 70, high=71 to 100). Also adds supporting tests (which is most of the diff here).

Additionally:

1. this fixes a panic when git hash passed is less than 12 characters. Although this may or may not possibly be a real occurrence, I think we should validate the slicing and avoid panics.
2. moves output.ConfidenceFromString(minConfFlag) to detection.ConfidenceFromString(minConfFlag) to keep all Confidence-related functionality together.

[omkar-foss]

Just for reference, implementation of numeric scoring here is based on this: #12 (comment)

[andrew]

Thanks for taking this on, and for the thorough tests.

The main thing I'd like to revisit is the consolidation step. ConsolidateFindingScore takes the max score per detector and then averages across detectors, so a commit with just a Co-Authored-By trailer scores 85, but the same commit with an additional tool mention in the message body drops to 52.5. Adding corroborating evidence shouldn't lower the score. #12 was heading toward something additive (SpamAssassin-style) rather than an average — probably worth pinning that down on the issue before reworking the code.

Related: the repo-wide OverallScore pools every finding from every commit into one max-per-detector average, so for a 500-commit range with one AI commit it just reports that one commit's score. I'm not sure a single number across a range is meaningful; the per-commit score is the useful bit.

A couple of structural things:

• confidenceScores and scan.Weights are package-level mutable state set from the CLI. --confidence-scores changes what every detector reports as Confidence and never resets, which leaks across Run() calls (and between tests — TestRunScanScoreFlags leaves it modified). Would prefer these threaded through as arguments rather than globals.
• SetConfidenceScoresFromStrings doesn't check the thresholds are ordered, so low=50,medium=30 makes ScoreToConfidence(40) return low.

Minor:

• strconv.ParseFloat(fmt.Sprintf("%.2f", overall), 64) → math.Round(overall*100)/100
• Replit Agent and Assistant used to be medium vs low confidence; both are now TrailerMatchBaseScore — intentional?
• The IIFE in FormatJSONFindings can be a plain local.
• Stray blank line at committer.go:28, typo overridence in detection.go.

The hash-slicing panic fix and the ConfidenceFromString move are both good and would happily take those as a separate PR if you want them in sooner.

[omkar-foss]

Thanks for your review, my comments below.

The main thing I'd like to revisit is the consolidation step. ConsolidateFindingScore takes the max score per detector and then averages across detectors, so a commit with just a Co-Authored-By trailer scores 85, but the same commit with an additional tool mention in the message body drops to 52.5. Adding corroborating evidence shouldn't lower the score. #12 was heading toward something additive (SpamAssassin-style) rather than an average — probably worth pinning that down on the issue before reworking the code.

Yes the max part is intentional, and this scoring indeed should be additive. But I guess we also need to have better weight defaults to avoid the score drops. In your case, score drops to 52.5 because both detectors get equal weights by default so 85x0.5 (trailer) + 20x0.5 (toolmention) = 52.5. I've used weights to normalize the score so that it always stays between 0 and 100 to automatically adjust for new detectors in future. Could you try with custom weights via cli? disclosure scan --weights=trailer=0.9,toolmention=0.1. Please try it and let me know your feedback :)

Related: the repo-wide OverallScore pools every finding from every commit into one max-per-detector average, so for a 500-commit range with one AI commit it just reports that one commit's score. I'm not sure a single number across a range is meaningful; the per-commit score is the useful bit.

Yes currently overall score is based on weighted average of findings across all commits. Makes sense, I'll update it to show score per commit (it's already in there just not using it yet).

Will also resolve the other 6 points (structural and minor) along with these changes. Thanks

[andrew]

Tried it. With --weights=trailer=0.9,toolmention=0.1, trailer+toolmention comes out to 78.5 (85×0.9 + 20×0.1), which is better than the 52.5 default, but it's still below the 85 that trailer-alone gets. Any weighted mean over the present detectors has that property: a weaker corroborating signal pulls the score toward itself.

I'd rather the consolidated score be max(perDetectorScores). Strongest signal wins, extra findings can't lower it, and it stays in 0–100 without needing weight tuning. The per-detector map you're already returning covers anyone who wants the breakdown.

Can revisit an additive scheme from #12 later if max turns out to be too coarse.

[omkar-foss]

I'd rather the consolidated score be max(perDetectorScores). Strongest signal wins, extra findings can't lower it, and it stays in 0–100 without needing weight tuning. The per-detector map you're already returning covers anyone who wants the breakdown.

Thanks for trying it out. I'll update this to use max, then let's try it out again. Yes agreed, if that too doesn't work well then we can revise to just have simple additive scoring.

[andrew]

Got a couple conflicts that need resolving here

[omkar-foss]

Got a couple conflicts that need resolving here

No problem, will resolve in next push with these changes. Thanks

[omkar-foss]

Can revisit an additive scheme from #12 later if max turns out to be too coarse.

@andrew I digged a little deeper, and also tried max for overall score, one major problem with using max seems to be that in case the highest score signal is a false positive, then it gets a boost over true positives. And another problem is that the lower score contributions of other detectors get foreshadowed by the highest score detector.

e.g. say trailer=85 and committer=75, if trailer score is a false positive then it foreshadows committer and gives an overall score of 85.

These problems don't occur with additive scoring. So like you suggested, I think it's best if we stick to the original additive scoring like we discussed in #12, I had documented it in this example, and it's based on SpamAssassin-like scoring.

Let me know if this direction works for you, I have the additive changes ready since I was comparing it with weighted average and max on my system. Once you give a go, I'll push it. Thanks

[omkar-foss]

I've rebased for now, tests will pass after changes are pushed.

[andrew]

Yep sounds good, tests still failing btw

[omkar-foss]

Yep sounds good, tests still failing btw

Done, I've pushed the additive scoring changes, tests passing now. Let me know if any changes needed, thanks

[andrew]

Thanks for switching to the additive sum and dropping weights, that's the shape I was after. The per-commit (score: %.1f) in text output and the hash-slice guard both look good.

The sum is now uncapped but still rendered as Overall score: %.1f / 100 in both FormatText and FormatTextFindings. A typical Claude Code commit (committer 95 + trailer 85 + toolmention 20) prints 200.0 / 100. Either clamp the total in CalculateTotalScore or drop the / 100 from the label; unbounded SpamAssassin-style is fine, just don't claim a denominator. The same uncapping bites the EntireIO check: 35 + (n-1)×20 is passed to ScoreToConfidence, which errors above 100 and falls through to ConfidenceNone, so a commit with enough matching trailers reports zero confidence.

Summary.OverallScore still pools every finding from every commit into one max-per-detector sum, so a 500-commit range with one AI commit reports that commit's score as the range score. Now that per-commit scores are printed I'd drop the range-wide number rather than try to give it a meaning.

A couple of things from the last round are still open. confidenceScores is still package-level state mutated by SetConfidenceScoresFromStrings, so it leaks across Run() calls and between tests; I'd rather it was threaded through than global. SetConfidenceScoresFromStrings also still doesn't check the thresholds are ordered, so --confidence-scores=low=50,medium=30 makes Medium unreachable. And Replit Agent vs Assistant are still both TrailerMatchBaseScore where they used to be Medium vs Low; if that's deliberate just say so.

Smaller bits, none blocking on their own: ConfidenceNone = 0 has no type annotation where its siblings do; filterReport sets Score on the rebuilt CommitResult but not PerDetectorScores, so filtered JSON has "score": 85, "per_detector_scores": null; --confidence-scores is wired to scan but not text; the IIFE in FormatJSONFindings, the blank line at committer.go:28, and the overridence typo in detection.go are all still there.

[omkar-foss]

Sure thanks, let's keep it uncapped and yeah I'll remove the overall score at report level, I think it's causing too much confusion. Regarding other minor things, my comments below

A couple of things from the last round are still open. confidenceScores is still package-level state mutated by SetConfidenceScoresFromStrings, so it leaks across Run() calls and between tests; I'd rather it was threaded through than global.

Yes this is pending on me, I'll add this, thanks

SetConfidenceScoresFromStrings also still doesn't check the thresholds are ordered, so --confidence-scores=low=50,medium=30 makes Medium unreachable.

Yeah I understand that we should add a validation for this, I'll add it.

And Replit Agent vs Assistant are still both TrailerMatchBaseScore where they used to be Medium vs Low; if that's deliberate just say so.

Yes, this is deliberate :)

Smaller bits, none blocking on their own: ConfidenceNone = 0 has no type annotation where its siblings do;

I'll add this.

filterReport sets Score on the rebuilt CommitResult but not PerDetectorScores

That's because overall score in the report is calculated directly using the findings, so it doesn't need the per detector scores separately. Anyway I'll remove as it's used for overall score.

--confidence-scores is wired to scan but not text

Currently text doesn't need it since all findings have the same score (toolmention base score) and so allowing confidence levels won't be necessary. I suppose we could add it in future when text (toolmention) confidence scores are more varied.

the IIFE in FormatJSONFindings, the blank line at committer.go:28, and the overridence typo in detection.go are all still there.

Oops! I'll check these out, thanks :P

[omkar-foss]

All things in here resolved in this commit, also added some more missing tests. Let me know if any more changes needed, thanks

[andrew]

Thanks, this covers nearly everything from the last round: / 100 label gone, ScoreToConfidence handles scores above high so the EntireIO overflow is fixed, OverallScore dropped, the package-level global replaced by ConfidenceLevels threaded into each detector, ordering validation added, ConfidenceNone typed, filterReport sets PerDetectorScores, and the IIFE / blank line / typo are all cleaned up. The score constants preserve the old confidence buckets under default thresholds, which is what I was hoping for.

One thing left before merge: commits with no findings now report "confidence": "low". ScoreToConfidence maps anything <= low to ConfidenceLow, and scanOneCommit calls it unconditionally at scan/scan.go:94, so a clean commit comes out as {"findings": null, "score": 0, "confidence": "low"}. Running disclosure scan --format=json against this branch's own last three commits shows all three as "confidence": "low" with ai_commits: 0. Simplest fix is probably to short-circuit to ConfidenceNone in scanOneCommit when len(findings) == 0, same as the len(detectors) == 0 branch just above.

While you're in there: ConfidenceNone.String() returns "unknown" but UnmarshalJSON at detection/detection.go:44 only accepts "none", so that value doesn't round-trip through JSON. Worth aligning to "none" in both.

Non-blocking, for awareness:

• The Detector interface now requires GetConfidenceLevels(), which is a breaking change for library users and will conflict with #78 (its branchname.Detector doesn't implement it) — whichever lands second won't compile.
• scanOneCommit reads the levels via detectors[0].GetConfidenceLevels() at scan/scan.go:92. Fine for the CLI since allDetectors hands them all the same map, but a library caller whose first detector has a nil map gets zero thresholds. Longer term I'd rather detectors emit only Score and let scan do the bucketing so the interface doesn't carry config, but that can be a follow-up.
• Summary.PerDetectorScores is still the max-per-detector across the whole range (scan/scan.go:123), and filterReport doesn't rebuild it so filtered output has it as null. Since OverallScore is gone I'd drop the summary-level field too, but not blocking.

[omkar-foss]

Hi @andrew, thanks your review. My comments below.

One thing left before merge: commits with no findings now report "confidence": "low". ScoreToConfidence maps anything <= low to ConfidenceLow, and scanOneCommit calls it unconditionally at scan/scan.go:94, so a clean commit comes out as {"findings": null, "score": 0, "confidence": "low"}. Running disclosure scan --format=json against this branch's own last three commits shows all three as "confidence": "low" with ai_commits: 0. Simplest fix is probably to short-circuit to ConfidenceNone in scanOneCommit when len(findings) == 0, same as the len(detectors) == 0 branch just above.

This is intentional, as I think we'd need to keep confidence low instead of none when the detectors find nothing. It basically signals to the user something like - 'hey we didn't find any markers of AI disclosure in your repo but we're not entirely sure that it's absolutely AI-free'.

While you're in there: ConfidenceNone.String() returns "unknown" but UnmarshalJSON at detection/detection.go:44 only accepts "none", so that value doesn't round-trip through JSON. Worth aligning to "none" in both.

Yes better to keep this consistent, I'm updating String to return "none" for ConfidenceNone, thanks

Non-blocking ones:

The Detector interface now requires GetConfidenceLevels(), which is a breaking change for library users and will conflict with #78 (its branchname.Detector doesn't implement it) — whichever lands second won't compile.

No problem, I'll handle the conflicts here, preferably we should try and merge #78 before this one

scanOneCommit reads the levels via detectors[0].GetConfidenceLevels() at scan/scan.go:92. Fine for the CLI since allDetectors hands them all the same map, but a library caller whose first detector has a nil map gets zero thresholds. Longer term I'd rather detectors emit only Score and let scan do the bucketing so the interface doesn't carry config, but that can be a follow-up.

Yes you're right, currently the same set of conf levels are passed to all detectors, I've kept the levels within the detector struct for future extensibility. May be we could have per-detector conf levels in near future.

Summary.PerDetectorScores is still the max-per-detector across the whole range (scan/scan.go:123), and filterReport doesn't rebuild it so filtered output has it as null. Since OverallScore is gone I'd drop the summary-level field too, but not blocking.

Yes, similar to conf levels, this is a bit of groundwork for future extensibility. In this case the per-detector scores in the summary could be used to show verbose scoring breakdown to the user if they pass a flag like --show-score-breakdown. Right now we only show findings level score in output, I think this detector level scores may be a nice addition especially as the number of detectors grows.

[omkar-foss]

Rebased with main, also resolved conflicts and added scoring for branchname detector in this commit.

[omkar-foss]

One thing left before merge: commits with no findings now report "confidence": "low". ScoreToConfidence maps anything <= low to ConfidenceLow, and scanOneCommit calls it unconditionally at scan/scan.go:94, so a clean commit comes out as {"findings": null, "score": 0, "confidence": "low"}. Running disclosure scan --format=json against this branch's own last three commits shows all three as "confidence": "low" with ai_commits: 0. Simplest fix is probably to short-circuit to ConfidenceNone in scanOneCommit when len(findings) == 0, same as the len(detectors) == 0 branch just above.

This is intentional, as I think we'd need to keep confidence low instead of none when the detectors find nothing. It basically signals to the user something like - 'hey we didn't find any markers of AI disclosure in your repo but we're not entirely sure that it's absolutely AI-free'.

I thought about this a bit more, I think yeah it'll be safer to just keep it confidence none instead of low to avoid confusion. I've updated the PR.

[omkar-foss]

@andrew all your comments are resolved in here, it's ready for review. After this is merged, we could move to #81 as it has some scoring related dependencies on this one. Let me know if that's fine with you, thanks.

[andrew]

LGTM