fix(reviewplan): stop re-posting a finding already raised on the PR - #579
fix(reviewplan): stop re-posting a finding already raised on the PR#579piekstra wants to merge 2 commits into
Conversation
A finding fixed by a later commit is still present in the cumulative base..head diff, so it anchors again on the next review. Posting is idempotent on run id and action id, which a later run never matches. Nothing anywhere compared a new finding to the threads already on the PR, so the same finding could be posted again on every run, most visibly against source text that no longer exists at head. A reader who sees that learns to skim review comments rather than read them. The planner now takes the threads this identity already opened and keeps a finding out of a second inline thread when one of them says the same thing about the same file. Matching is by file and text, never by line: a fix shifts the lines around it, and the repeats worth suppressing are the ones that moved. Everything the planner adds to a posted comment is stripped back off first, along with any HTML comments, because markers carry a run id that differs on every run and would otherwise make every comparison fail. Equality is exact once formatting is folded, since a near-match is a different claim often enough that suppressing it would lose real findings. Only threads this identity authored to report a finding are considered. A human quoting the same code is not the review repeating itself. Demotion, not deletion: the finding stays in the review body, so a reviewer that still believes it remains on record, and an unresolved thread is still there to be read. Resolved and open threads are treated alike; both mean it has been said.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 8b15d594002a
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 1 |
| architecture:solid-reviewer-agnostic | 3 |
| structure:harness-engineering | 0 |
structure:repo-health (1 finding)
Minor - internal/reviewplan/reviewplan.go:668
stripHTMLComments strips every
<!-- ... -->span, but the marker it exists to remove is specifically<!-- codereview:... -->; threadcontext.SanitizeBody, which already sanitizes the same comment bodies before this package ever sees them, only matches thatcodereview:prefix and leaves other HTML comments intact. A finding whose body legitimately quotes diff text containing an unrelated HTML/XML/JSX comment (a plausible thing for a review tool to quote) gets that quoted text silently dropped from the dedupe key here but not from the posted body, so the comparison can diverge from what was actually posted - two findings that differ only in a quoted comment's content would collapse to the same key and one would be wrongly suppressed, and an unterminated quoted comment (e.g. a code block illustrating a bug in an unclosed comment) truncates the rest of the body from the comparison entirely. Reuse threadcontext.SanitizeBody (or match its<!-- codereview:prefix) here instead of a second, broader hand-rolled stripper, so the two packages agree on what counts as a marker.
architecture:solid-reviewer-agnostic (3 findings)
Major - internal/reviewplan/reviewplan.go:661
U-S1 / U-G1:
normalizeFindingTextre-implements knowledge that belongs tocommentBody(reviewplan.go:869-875), and the two are already out of sync.commentBodyrenders a file-level fallback asfileLevelFallbackPrefix + FilePath + "\n\n" + body + "\n\n" + inlineFooter; normalization strips the prefix literal and the footer but leaves the interpolated file path in the text, so the stored body normalizes to"main.go finding body ..."while the candidate normalizes to"finding body ..."and never matches.GitHub reports
NativeFileLevelComments: false(internal/gitprovider/github/client.go:179), so every file-anchored finding on GitHub takes the fallback path - the whole class this feature exists for, since a file-level finding re-anchors to the first hunk on each run. Reproduced with a probe test:ProviderCaps{NativeFileLevelComments: false}plus a file-anchored finding, feedingBuild's own posted body back as anExistingThread, still plans 1 inline comment (want 0). The suite does not cover it becausebaseRequest()setsNativeFileLevelComments: trueand an inline anchor.Fix: derive the key for the candidate from the same renderer the host saw, i.e.
findingKey(anchored.FilePath, commentBody(anchored))indemoteFindingsAlreadyRaised, so both sides pass through one decoration path and future changes tocommentBodycannot silently break matching. Add a fallback-anchoring case todedupe_raised_test.goto lock it in.
Major - internal/pipeline/pipeline.go:2358
U-L1: the filter does not implement its own documented contract. The doc comment says these are "the ones this identity opened to report a finding", but
Status.CRAuthoredFindingis set bythreadcontext.statusForCommentswhen any comment by the posting identity carries a finding or a thread-reply marker (internal/threadcontext/threadcontext.go:230). A thread a human opened and this identity merely replied to therefore qualifies, andthread.Comments[0].Bodythen carries the human's text into the dedupe key.Reproduced against the real normalizer (human opens
main.gowith bodyfinding body, bot replies with athread_replymarker):CRAuthoredFinding=true, carried{Path:main.go Body:finding body Resolved:false}. That is exactly the case the PR intent rules out ("A human quoting the same code is not the review repeating itself, and must not suppress a finding") - a human who quotes or restates a finding can now silence it, and the threads that are carried are keyed on the wrong text.Fix: gate on the opening comment rather than thread-level status, e.g.
if len(thread.Comments) == 0 { continue } first := thread.Comments[0] if !first.AuthoredByPostingIdentity || !first.HasFindingMarker { continue }Both fields already exist on
threadcontext.Comment, so this needs no new provider data.internal/pipeline/existing_threads_test.goconstructsStatus{CRAuthoredFinding: ...}by hand and so cannot catch this; a case built throughthreadcontext.Normalizewould.
Minor - internal/reviewplan/reviewplan.go:111
U-G1 / U-O1:
ExistingThread.Resolvedis new exported surface with no consumer.demoteFindingsAlreadyRaiseddeliberately treats resolved and open threads alike,findingKeynever reads it, and the pipeline only populates it; the only assertions on it are in the new tests. A field on a public request type that nothing branches on invites callers to think it changes behavior and will drift.Either drop it (the doc comment on
demoteFindingsAlreadyRaisedalready records why resolution is irrelevant) or, if it is meant to become a discriminator later, add it when a consumer exists.
Reviewer Coverage
go:implementation-tests— complete (broad); skipped: none; constraints: nonestructure:repo-health— complete (broad); skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (broad); inspected 2 assigned files (4 inspected across reviewers):internal/pipeline/pipeline.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: Findings 1 and 2 were reproduced with throwaway probe tests in a scratch copy of the repo (go test), not by inspection alone. Scope limited to the two assigned files; the two new test files were read for context only.structure:harness-engineering— complete (broad); inspected 2 assigned files (4 inspected across reviewers):internal/pipeline/pipeline.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: none
Inspected files (4)
internal/pipeline/existing_threads_test.gointernal/pipeline/pipeline.gointernal/reviewplan/dedupe_raised_test.gointernal/reviewplan/reviewplan.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 5m 52s | ~$6.82 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.294
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, structure:repo-health, architecture:solid-reviewer-agnostic, structure:harness-engineering |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 5m 52s wall · 16m 42s compute |
| Cost | ~$6.82 (est.) |
| Tokens | 172 in / 50.3k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5- In: 6
- Out: 3.2k
- Cache read: 117.8k
- Cache create: 97.7k
- Cost: ~$0.45 (est.)
- Duration: 37s
go:implementation-tests— claude-sonnet-5- In: 42
- Out: 8.9k
- Cache read: 1.7M
- Cache create: 149.3k
- Cost: ~$1.21 (est.)
- Duration: 3m 58s
structure:repo-health— claude-sonnet-5- In: 50
- Out: 15.5k
- Cache read: 2.2M
- Cache create: 156.9k
- Cost: ~$1.48 (est.)
- Duration: 3m 34s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 50
- Out: 15.8k
- Cache read: 2.1M
- Cache create: 150.2k
- Cost: ~$2.40 (est.)
- Duration: 4m 40s
structure:harness-engineering— claude-sonnet-5- In: 20
- Out: 5.5k
- Cache read: 695.6k
- Cache create: 135.5k
- Cost: ~$0.80 (est.)
- Duration: 3m 28s
orchestrator-rollup— claude-sonnet-5- In: 4
- Out: 1.3k
- Cache read: 40.6k
- Cache create: 121.3k
- Cost: ~$0.49 (est.)
- Duration: 22s
Three corrections, two of which meant the suppression could not fire on the case it exists for. The file-level fallback header interpolates the file path after its prefix. Stripping only the literal prefix left the path in the text, so a stored file-level comment normalized to "main.go finding body" while the finding it came from normalized to "finding body" and never matched. On a host without native file-level comments that is every file-anchored finding, and such a finding re-anchors to the first hunk on every run, so it is the class most in need of this. Stripping every HTML comment took text out of the comparison that stays in the posted body. A finding may quote an HTML, XML, or JSX comment out of the diff, and two findings differing only inside such a quote collapsed to one key. Only this tool's own markers are stripped now. The authorship filter read Status.CRAuthoredFinding, which is also set when this identity merely replied to a thread someone else opened. The opening comment's own author and marker are tested instead, so a human's thread cannot carry their text in as a finding of ours and suppress a real one. ExistingThread.Resolved is dropped. Resolution is deliberately not part of the decision, so a field nothing branches on only invited a caller to think it was.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: d762a66842a8
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 0 |
| structure:harness-engineering | 0 |
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: nonestructure:repo-health— complete (constrained); skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (constrained); inspected 2 assigned files (4 inspected across reviewers):internal/pipeline/pipeline.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: Scope limited to the two assigned files; the two new test files were read as context only. Verified by reproduction in a scratch copy: go build ./..., go vet and gofmt clean, internal/reviewplan and internal/threadcontext suites pass, internal/pipeline TestExistingFindingThreads* pass. The full internal/pipeline suite was not run to completion (exceeds the command timeout in this envir... Wrote an end-to-end probe (Build -> posted body with a real action marker prepended -> threadcontext.Normalize -> existingFindingThreads -> Build) covering inline, file-level fallback, a body quoting a non-marker HTML comment, and a body containing marker text; all four suppress the repeat.structure:harness-engineering— complete (constrained); inspected 2 assigned files (4 inspected across reviewers):internal/pipeline/pipeline.go,internal/reviewplan/reviewplan.go; skipped: none; constraints: none
Inspected files (4)
internal/pipeline/existing_threads_test.gointernal/pipeline/pipeline.gointernal/reviewplan/dedupe_raised_test.gointernal/reviewplan/reviewplan.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 5m 34s | ~$6.23 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.294
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, structure:repo-health, architecture:solid-reviewer-agnostic, structure:harness-engineering |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 5m 34s wall · 10m 27s compute |
| Cost | ~$6.23 (est.) |
| Tokens | 92 in / 20.6k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 32
- Out: 4.7k
- Cache read: 1.7M
- Cache create: 211.5k
- Cost: ~$1.39 (est.)
- Duration: 3m 32s
structure:repo-health— claude-sonnet-5- In: 10
- Out: 1.9k
- Cache read: 392.5k
- Cache create: 220.4k
- Cost: ~$0.97 (est.)
- Duration: 49s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 24
- Out: 10.7k
- Cache read: 1.3M
- Cache create: 228.9k
- Cost: ~$2.36 (est.)
- Duration: 4m 54s
structure:harness-engineering— claude-sonnet-5- In: 20
- Out: 3.0k
- Cache read: 848.5k
- Cache create: 176.0k
- Cost: ~$0.96 (est.)
- Duration: 56s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 349
- Cache read: 134.2k
- Cache create: 136.2k
- Cost: ~$0.56 (est.)
- Duration: 14s
2 participants
What happens today
A finding that a later commit fixes is still present in the cumulative
base..headdiff, so it anchors again on the next review.reconcileInlinematches only on run id, action id, and head/base SHA, none of which a later run reproduces. Nothing anywhere compares a new finding to the threads already on the PR.The result is the same finding posted as a brand-new thread on every review, byte-identical, against source text that no longer exists at head. Replying and resolving does not stop it. A reader who sees that learns to skim review comments rather than read them, which costs more than the duplicate itself.
What this changes
reviewplan.RequestgainsExistingThreads, andBuildkeeps a finding out of a second inline thread when this identity already opened one saying the same thing about the same file. The pipeline populates it from the threads it already fetches, so there is no extra request.Deliberate choices, each of which could reasonably have gone the other way:
Tests
internal/reviewplan/dedupe_raised_test.gobuilds a plan, takes the body it produced, and feeds it back as an existing thread, which is what a later run actually reads from the host:internal/pipeline/existing_threads_test.gocovers the filter: only this identity's finding threads are carried, the opening comment is used rather than a reply, resolution is carried through rather than filtered on, and a thread with no comments is skipped.Verification
go build ./...,make lint(0 issues), and the full suite pass, exceptTestPiRPCReviewerExtensionLoadsInInstalledPi, which fails identically on unmodifiedmainin this environment (Error: Unknown options: --no-builtin-tools, --no-approve) and is unrelated to this change.