Fix case-sensitive ADR triage confidence parsing - #44
Conversation
| if "confidence:" in result_lower: | ||
| try: | ||
| conf_part = result_text.split("confidence:")[1].split()[0].strip() | ||
| conf_part = result_lower.split("confidence:", 1)[1].split()[0].strip() |
There was a problem hiding this comment.
The fix is correct — I reproduced both sides: pre-fix, CONFIDENCE: 0.99 returned 0.8 (the guard tested result_lower while the split ran on the original-case result_text, so the split found no match and IndexError hit the fallback); post-fix it returns 0.99. Lowercasing can't corrupt a numeric token, and maxsplit=1 doesn't change which occurrence is selected, so it's a no-risk clarification.
One follow-up while you're in here: the expression still assumes the number is the bare next whitespace token after the first confidence:, so several common model outputs keep hitting the same 0.8 default this PR set out to eliminate. Verified by running the patched line:
| Model output | Token extracted | Result |
|---|---|---|
CONFIDENCE: 0.99 (prompt format) |
0.99 |
0.99 — fixed by this PR |
**CONFIDENCE:** 0.95 |
** |
0.8 |
REASONING: low confidence: agent intent unclear … CONFIDENCE: 0.30 |
agent |
0.8 |
CONFIDENCE: 0.75. |
0.75. |
0.8 |
CONFIDENCE: 95% |
95% |
0.8 |
The third row is the awkward one: an incidental confidence: inside the reasoning text wins the split, so the real value further down is never reached. Markdown emphasis is probably the most likely in practice, since models reach for ** around field labels unprompted.
A line-anchored parse closes all of them — scan lines, match a confidence: prefix, strip * / [ / ] / % / trailing punctuation before float().
Worth saying explicitly that this is not a regression from this diff and nothing is broken downstream today: confidence is only logged and echoed into DetectionResult.confidence_score, main_detector.py:441 appends it to a confidence_scores list that is never read, and every extract_metrics caller in plot_paper_figures.py discards the confidence array (_ in that position), with no ROC/AUC consuming it. So a wrong value can't flip a classification, suppress an alert, or distort a published figure — this is cleanup, not a blocker.
One note on verification: I traced the parse by executing the expression directly rather than running the suite (no pytest available in the review environment), so the new test is verified by construction, not by a green run.
|
Thanks for the detailed review. We’ve updated the parser to handle the additional formats you identified, including Markdown, punctuation, incidental We found this issue while experimenting with context and provenance in the detection component. We’ll submit additional improvements from that work as separate PRs soon. |
pengyuzhang
left a comment
There was a problem hiding this comment.
Re-reviewed at ec42a44. The line-anchored parse is the right shape and the case-sensitivity bug is properly gone — all five parametrized cases pass when I run the pattern directly, and CONFIDENCE: 85 falling back to 0.8 is a real improvement over the base, which recorded 85.0.
Three follow-ups below. Two of them are cases where the new parse does worse than the code it replaces, which is why I'd suggest fixing them here rather than after merge.
Checked and cleared: re is imported at module scope, so the new re.compile is safe; the out-of-range branch correctly continues scanning later lines instead of breaking, so CONFIDENCE: 1.1\nCONFIDENCE: 0.5 still yields 0.5; match.group("percent") returns "" rather than None, so the truthiness check is sound; and pytest is a declared dev dep with parametrize as a builtin marker, so --strict-markers won't reject the new tests.
Severity note for prioritization: nothing branches on this value today — it is logged and echoed into DetectionResult.confidence_score, main_detector.py:441 appends it to a list that is never read, and every extract_metrics caller in plot_paper_figures.py discards the confidence array. So none of these can flip a classification or distort a figure; they affect the recorded number this PR exists to make correct.
| ) | ||
| \s*(?P<percent>%?) # Optional percentage notation. | ||
| \s*\**\]? # Optional closing asterisks/bracket. | ||
| \s*[.,;:]?\s*$ # Optional trailing punctuation. |
There was a problem hiding this comment.
The $ anchor makes any trailing commentary fall back to the default — and for lowercase output this regresses against the code being replaced.
Ran the base and PR parsers side by side:
| Input | base | this PR |
|---|---|---|
confidence: 0.9 - agent behavior normal |
0.9 | 0.8 |
CONFIDENCE: 0.9 (high) |
0.8 | 0.8 |
CLASSIFICATION: BENIGN | CONFIDENCE: 0.9 |
0.8 | 0.8 |
The old whitespace split took the token immediately after the label and parsed it fine; the anchor now requires the line to end at the number (plus at most one punctuation character). Models routinely append a short justification after the value even when the prompt asks for a bare field, so this is a common shape — and it silently records the same 0.8 default the PR set out to eliminate.
A trailing (?![\d.]) boundary instead of requiring end-of-line, or an unanchored search as a fallback when no anchored line matches, would close it.
| if match.group("percent"): | ||
| parsed_confidence /= 100 |
There was a problem hiding this comment.
Percent scaling is unconditional, so a fractional value carrying a % is recorded 100x too small — and it passes the range guard.
CONFIDENCE: 0.95% parses to 0.0095, which satisfies 0.0 <= parsed_confidence <= 1.0 and is stored as a near-zero confidence. The base implementation raised ValueError on 0.95% and fell back to 0.8, so this newly produces a confidently wrong number where it previously produced an honest default.
| if match.group("percent"): | |
| parsed_confidence /= 100 | |
| if match.group("percent") and parsed_confidence > 1.0: | |
| parsed_confidence /= 100 |
That keeps CONFIDENCE: 95% → 0.95 working while leaving the ambiguous fractional-percent form to fall through to the default.
| r""" | ||
| ^\s* # Start of a line, allowing indentation. | ||
| (?:[-*>]\s*)? # Optional Markdown list/quote marker. | ||
| \**confidence\s*:\**\s* # Label, optionally wrapped in asterisks. |
There was a problem hiding this comment.
The bold-label form with the colon outside the asterisks still misses.
**CONFIDENCE:** 0.95 → 0.95 ✓
**CONFIDENCE**: 0.95 → 0.8 ✗
*CONFIDENCE*: 0.9 → 0.8 ✗
> **Confidence**: 0.6 → 0.8 ✗
\**confidence\s*:\** allows asterisks before the label and after the colon, but not between the label and the colon — and bolding the label while leaving the colon outside the emphasis is the more idiomatic Markdown of the two. The comment added just above cites Markdown decoration as the motivation for this pattern, so the targeted case is only half covered.
| \**confidence\s*:\**\s* # Label, optionally wrapped in asterisks. | |
| \**confidence\**\s*:\s*\** # Label, optionally wrapped in asterisks. |
Not a regression — the base implementation also returned 0.8 for these.
|
Thanks—good catches. I’ve updated the parser to accept trailing commentary and both common Markdown label styles while preserving the line-anchored match, so incidental For percentage-formatted values, I adopted the suggested magnitude-based interpretation with an explicit boundary for the ambiguous case:
This special case is documented directly in the code and covered by unit tests. I intentionally left Regression tests now cover trailing commentary, Markdown variants, percentage boundaries, malformed numeric suffixes, and the intentionally unsupported pipe-delimited form. The focused suite passes with 30 tests. |
Summary
CONFIDENCE:format requested by the triage promptProblem
The parser detects
confidence:in a lowercased copy of the model response, but then splits the original response using the lowercase field name. When a model follows ADR's requested uppercaseCONFIDENCE:format, that split fails and the parser silently returns the default confidence of0.8.This does not change the triage classification. It fixes the confidence recorded for Tier 1 results.
Test plan