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