feat: normalize deterministic analysis findings

nimmo commented

2026-07-20 17:34:42 +01:00

Owner

Implements #4.

extract file/line/severity diagnostics from configured check output
retain bounded raw output when no structured diagnostic is available
keep deterministic failures distinct from LLM findings
suppress repeated model findings and exact check-backed duplicates

Validation: python3 -W error::ResourceWarning -m unittest discover -s tests (26 passing).

Implements #4. - extract file/line/severity diagnostics from configured check output - retain bounded raw output when no structured diagnostic is available - keep deterministic failures distinct from LLM findings - suppress repeated model findings and exact check-backed duplicates Validation: `python3 -W error::ResourceWarning -m unittest discover -s tests` (26 passing).

nimmo added this to the Forgejo AI PR Review v1 milestone

2026-07-20 17:34:42 +01:00

nimmo added 1 commit

2026-07-20 17:34:42 +01:00

feat: normalize deterministic analysis findings

AI pull-request review / review (pull_request) Successful in 50s

Details

dfdd883a16

nimmo commented

2026-07-20 17:35:33 +01:00

Author

Owner

Automated code review

Reviewed commit: 899f6f955ed9334b65ac57aae967db5a52a46f15

Verdict: Requires further work

Resolve the blocking or important findings and investigate failed deterministic checks before merging.

Overall assessment

The pull request introduces structured parsing and deduplication of deterministic diagnostics from check outputs. Key changes include new Diagnostic/CheckResult dataclasses, parse_diagnostics() function with regex-based line parsing, render_checks() for improved output rendering, and deduplicate_findings() to suppress model findings matching known diagnostics.

Blocking findings

Regex DIAGNOSTIC_LINE fails on Windows paths with drive letters followed by colons in directory names (e.g., 'C:\repo:dir\file.py') (reviewer.py:178; high confidence)
DIAGNOSTIC_LINE = re.compile(
r"^(?P(?:[A-Za-z]:)?[^:\n]+):(?P\d+)(?::\d+)?:\s*(?:(?Perror|warning|info|fatal):\s*)?(?P.+)$",
re.I,
)
Impact: Paths containing colons (e.g., 'C:\repo:dir\file.py') will be misparsed because the regex uses '[^:\n]+' for path, causing incorrect splitting at the first colon after drive letter or within directory names.
Suggested fix: Refactor DIAGNOSTIC_LINE to require line number before optional column and use non-greedy matching: r'^(?P(?:[A-Za-z]:)?[^:\n]+?):(?P\d+)(?::(?P\d+))?:(?:\s*(?Perror|warning|info|fatal):\s*)?(?P.+)$'

Important findings

Deduplication logic compares normalized evidence strings directly without accounting for semantic equivalence or context (reviewer.py:267; high confidence)
duplicate_check = any(
finding.get("path") == path and (not finding.get("line") or not line or finding["line"] == line)
and evidence and diagnostic and evidence == diagnostic
for path, line, diagnostic in known
)
Impact: Model findings may incorrectly be retained when they match deterministic diagnostics only partially (e.g., same file/line but different wording), leading to redundant or misleading output.
Suggested fix: Normalize both the model finding's evidence and the diagnostic message before comparison, e.g., using normalized_text() on both sides of 'evidence == diagnostic'.

Suggestions

None.

Tests and validation

unit-tests: success

Questions

Should deterministic diagnostics be rendered with full raw output even when truncated, or only the tail as currently implemented?
Is the deduplication logic intended to suppress all findings that match any check diagnostic (even if severity differs), or only those with identical severity?

Review limitations

The regex for parsing diagnostics may not robustly handle all path formats (e.g., Windows paths with colons in directory names).
Deduplication logic compares normalized evidence strings directly without accounting for semantic equivalence or context.
No tests cover the case where a diagnostic line has no severity and state is neither 'failure' nor 'success'.

Review metadata

Reviewer: forgejo-ai-review v1
Model: qwen3-coder-next:q4_K_M
Guidance loaded: .forgejo/ai-review.yml, README.md
Commits considered: 3
Previous automated review: updated
Findings: 2

## Automated code review Reviewed commit: `899f6f955ed9334b65ac57aae967db5a52a46f15` ## Verdict: Requires further work Resolve the blocking or important findings and investigate failed deterministic checks before merging. ### Overall assessment The pull request introduces structured parsing and deduplication of deterministic diagnostics from check outputs. Key changes include new Diagnostic/CheckResult dataclasses, parse_diagnostics() function with regex-based line parsing, render_checks() for improved output rendering, and deduplicate_findings() to suppress model findings matching known diagnostics. ### Blocking findings - **Regex DIAGNOSTIC_LINE fails on Windows paths with drive letters followed by colons in directory names (e.g., 'C:\repo:dir\file.py')** (reviewer.py:178; high confidence) DIAGNOSTIC_LINE = re.compile( r"^(?P<path>(?:[A-Za-z]:)?[^:\n]+):(?P<line>\d+)(?::\d+)?:\s*(?:(?P<severity>error|warning|info|fatal):\s*)?(?P<message>.+)$", re.I, ) Impact: Paths containing colons (e.g., 'C:\repo:dir\file.py') will be misparsed because the regex uses '[^:\n]+' for path, causing incorrect splitting at the first colon after drive letter or within directory names. Suggested fix: Refactor DIAGNOSTIC_LINE to require line number before optional column and use non-greedy matching: r'^(?P<path>(?:[A-Za-z]:)?[^:\n]+?):(?P<line>\d+)(?::(?P<column>\d+))?:(?:\s*(?P<severity>error|warning|info|fatal):\s*)?(?P<message>.+)$' ### Important findings - **Deduplication logic compares normalized evidence strings directly without accounting for semantic equivalence or context** (reviewer.py:267; high confidence) duplicate_check = any( finding.get("path") == path and (not finding.get("line") or not line or finding["line"] == line) and evidence and diagnostic and evidence == diagnostic for path, line, diagnostic in known ) Impact: Model findings may incorrectly be retained when they match deterministic diagnostics only partially (e.g., same file/line but different wording), leading to redundant or misleading output. Suggested fix: Normalize both the model finding's evidence and the diagnostic message before comparison, e.g., using normalized_text() on both sides of 'evidence == diagnostic'. ### Suggestions None. ### Tests and validation - unit-tests: **success** ### Questions - Should deterministic diagnostics be rendered with full raw output even when truncated, or only the tail as currently implemented? - Is the deduplication logic intended to suppress *all* findings that match any check diagnostic (even if severity differs), or only those with identical severity? ### Review limitations - The regex for parsing diagnostics may not robustly handle all path formats (e.g., Windows paths with colons in directory names). - Deduplication logic compares normalized evidence strings directly without accounting for semantic equivalence or context. - No tests cover the case where a diagnostic line has no severity and state is neither 'failure' nor 'success'. <details><summary>Review metadata</summary> - Reviewer: forgejo-ai-review v1 - Model: qwen3-coder-next:q4_K_M - Guidance loaded: .forgejo/ai-review.yml, README.md - Commits considered: 3 - Previous automated review: updated - Findings: 2 </details>

nimmo added 1 commit

2026-07-20 17:37:50 +01:00

fix: harden deterministic diagnostic parsing

AI pull-request review / review (pull_request) Successful in 58s

Details

308a30fd0e

nimmo added 1 commit

2026-07-20 17:40:53 +01:00

fix: preserve bounded diagnostic context

AI pull-request review / review (pull_request) Successful in 56s

Details

0724dc83c8