Quiet PR-level findings: threads over prose, confidence over volume#80
Conversation
PR #76 gave findings with no anchorable diff line a home so a REQUEST_CHANGES review could never read "Actionable comments posted: 0" with nothing to act on. That invariant is right and stays. But the home it chose — prose in the review summary body — is the one DiffSentry channel a reader cannot resolve, reply to, or collapse, and four things funnelled into it: - Every warning-level drift finding was stamped `severity: "major", confidence: "medium"`, and isNitpick never consulted confidence, so all drift counted as actionable and claimed a Major slot at the top. - parse.ts demoted ANY un-anchorable major/critical inline finding there — findings that wanted a line and missed. - submitReview creates a new review per push, and the cross-review dedup keys on fingerprintFor(path, line, title). PR-level findings have line 0 and often no path, leaving the title as the whole key — and drift titles are free prose regenerated per run. One re-wording minted a fresh fingerprint and the finding reprinted in every review body. - The dismissal pass only handled CHANGES_REQUESTED, but drift bumps APPROVE→COMMENT and never blocks — so the exact review class #76 introduced was the one class never cleaned up. Route findings to a channel by what the channel costs the reader: - github: post file-scoped findings as real resolvable threads (subject_type: "file"); post them BEFORE the review so any GitHub rejects fold back into the body and a finding is never lost between channels. Retire our superseded reviews — dismiss CHANGES_REQUESTED, and stub COMMENTED bodies via updateReview since GitHub refuses to dismiss them and submitted reviews can't be deleted. Match on our own body marker, not `user.type === "Bot"`, which reached other bots. - review-body: split prLevel by whether a path survived (thread-able vs body-only); gate the body section on high confidence and collapse the rest; add isVisiblyActionable as the single source of truth for both the count and the REQUEST_CHANGES invariant, so the two can't disagree. - drift: ask for confidence and carry it through instead of hardcoding "medium"; default an omitted value to "medium", not the "high" ReviewComment assumes. severity stays "major" — severity is how bad if real, confidence is whether it's real, and conflating them caused this. Only high-confidence drift may withhold an APPROVE. - parse/state: titleSimilarity (Jaccard over content words, 0.6) catches re-worded PR-level repeats that fingerprinting misses, backed by a capped postedPrLevelKeys; same-path-only, so a real finding is never swallowed by a same-sounding one about different code. On the review that prompted this, "Actionable comments posted: 3" becomes 1 and both Major/Medium drift findings collapse — still present, no longer shouting. Adds tests/unit/pr-level-noise.test.ts (29 tests). 361 unit tests pass, typecheck and lint clean. The subject_type:"file" call and review retirement are covered by mocked-Octokit tests, not a live PR — worth watching on the first real review after this ships. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughRouted PR-level findings to resolvable file threads or confidence-gated body sections, and retired superseded DiffSentry reviews so unanchored drift no longer accumulates as permanent, unresolvable prose. Changes
Sequence Diagram(s)sequenceDiagram
participant Reviewer
participant Drift
participant Parse
participant ReviewBody
participant GitHub
Reviewer->>Drift: detectDescriptionDrift
Drift-->>Reviewer: findings with confidence
Reviewer->>Parse: isRepeatPrLevelFinding
Parse-->>Reviewer: drop re-worded repeats
Reviewer->>ReviewBody: formatReviewBody
Note over ReviewBody: high conf body findings stay prominent
Note over ReviewBody: medium/low collapse uncounted
Reviewer->>GitHub: submitReview
GitHub->>GitHub: post subject_type file threads
GitHub->>GitHub: retire superseded DiffSentry reviews
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested Labels
Risk AssessmentScore: 3/100 — 🟢 Low
Test Coverage Signal🟢 177 prod / 414 test lines added.
Linked Issues
🚥 Pre-merge checks | ✅ 5 | ❌ 0✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in your ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Generate docstrings (beta)
🧹 Simplify (beta)
🪄 Autofix unresolved comments (beta)
Comment |
![diffsentry[bot]](https://avatars.githubusercontent.com/in/3360270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
This PR cleanly routes PR-level findings by cost to the reader: file-scoped ones become resolvable threads, high-confidence body findings stay prominent, and the rest collapse. The confidence-gated isVisiblyActionable source of truth, similarity-based PR-level dedup, and superseded-review retirement all look solid and well-tested. One major gap remains: medium/low-confidence drift can still force APPROVE→COMMENT via reconcileApproval even though the new high-confidence gate was meant to stop that.
🔎 Issues not tied to a specific line (1)
These findings concern the change as a whole (for example, the diff versus the PR description) and can't be attached to a single changed line.
🛠️ Refactor suggestion | 🟡 Minor
Coverage is strong, but the approval-path regression needs a focused test.
The new unit file is excellent on rendering, similarity dedup, drift confidence parsing, and submitReview retirement/thread posting. What it does not lock down is the end-to-end approval interaction: a clean AI approval plus only medium-confidence drift should remain APPROVE after fold + reconcileApproval.
Add that case next to the existing reconcile tests so the main product promise of this PR cannot regress.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In tests/unit/pr-level-noise.test.ts, add an end-to-end-style case that builds a ReviewResult with approval APPROVE and only medium/low-confidence body-level drift findings, then runs the same fold/reconcile rules used in reviewer.ts and asserts the final approval remains APPROVE while the finding is collapsed/uncounted. Also assert a high-confidence drift finding still yields COMMENT.
🧹 Nitpick comments (4)
src/ai/parse.ts (1)
Line 206: Repeat matching ignores empty-path vs path-scoped drift synonyms only by exact path.🛠️ Refactor suggestion | 🟡 Minor
Repeat matching ignores empty-path vs path-scoped drift synonyms only by exact path.
🤔 Medium confidence — verify against intent before acting.
isRepeatPrLevelFindingcorrectly refuses to match across different paths, which is good. One edge case still hurts the main noise source: drift findings are emitted withpath: "", so a later reworded drift finding is only suppressed if the prior key also had an empty path. That is fine today, but if any future path-bearing PR-level finding reuses the same title prose, the empty-path drift repeat will not collapse against it and vice versa.Not a blocker for this PR, but the key format makes path part of identity forever. Consider documenting that callers must keep drift unscoped (
path: "") or normalizing both sides to the same scope before comparison.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/parse.ts around isRepeatPrLevelFinding near line 206, document or normalize path scoping so empty-path PR-level findings only compare against other empty-path keys, and add a short comment/test clarifying that drift must continue to use path: "" for repeat suppression to work. No behavior change required unless you choose to intentionally allow empty-path candidates to match same-title path-scoped priors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/parse.ts around isRepeatPrLevelFinding near line 206, document or normalize path scoping so empty-path PR-level findings only compare against other empty-path keys, and add a short comment/test clarifying that drift must continue to use path: "" for repeat suppression to work. No behavior change required unless you choose to intentionally allow empty-path candidates to match same-title path-scoped priors.src/github.ts (1)
Line 601: APPROVED DiffSentry reviews are left intact forever.🛠️ Refactor suggestion | 🟡 Minor
APPROVED DiffSentry reviews are left intact forever.
🤔 Medium confidence — verify against intent before acting.
retireSupersededReviewsonly dismissesCHANGES_REQUESTEDand stubsCOMMENTED. Reviews that endedAPPROVEDstill matchREVIEW_BODY_MARKERand remain on the timeline with their full body.That is probably acceptable, but after this PR a later push that finds medium-confidence drift can leave both an old green approval and a new commented review visible. If the goal is one current DiffSentry review body, consider stubbing APPROVED reviews the same way as COMMENTED, or explicitly document why approvals are retained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/github.ts around retireSupersededReviews near line 601, decide whether APPROVED DiffSentry reviews should also be rewritten to SUPERSEDED_REVIEW_STUB like COMMENTED reviews. If yes, handle state === "APPROVED" via updateReview; if no, add a brief comment explaining that prior approvals are intentionally left visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/github.ts around retireSupersededReviews near line 601, decide whether APPROVED DiffSentry reviews should also be rewritten to SUPERSEDED_REVIEW_STUB like COMMENTED reviews. If yes, handle state === "APPROVED" via updateReview; if no, add a brief comment explaining that prior approvals are intentionally left visible.src/review-body.ts (1)
Line 434: Actionable count includes file-level findings that later fail to post.🛠️ Refactor suggestion | 🟡 Minor
Actionable count includes file-level findings that later fail to post.
🤔 Medium confidence — verify against intent before acting.
formatReviewBodycounts file-level findings as actionable beforesubmitReviewtries to post them. If GitHub rejects the file thread and folds the finding into the fallback body section, the header can still say "Actionable comments posted: N" while the only visible copy is the non-threaded fallback block.This is rare, but it reintroduces a softer version of the #76 inconsistency on the rejection path. Either recount after
postFileLevelComments, or have the fallback section clearly count those items as posted-through-fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/review-body.ts around formatReviewBody near line 434 and the submitReview fallback path in src/github.ts, reconcile the actionable count with unpostable file-level findings. Either format the body after postFileLevelComments returns, or ensure rejected file-level findings remain represented in a way that keeps the header count honest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/review-body.ts around formatReviewBody near line 434 and the submitReview fallback path in src/github.ts, reconcile the actionable count with unpostable file-level findings. Either format the body after postFileLevelComments returns, or ensure rejected file-level findings remain represented in a way that keeps the header count honest.src/ai/prompt.ts (1)
Line 40: Prompt growth is justified; keep the confidence guidance tight.🧹 Nitpick | 🟢 Trivial
Prompt growth is justified; keep the confidence guidance tight.
🤔 Low confidence — flagging as a hypothesis; may not apply.
The new
prLevelCommentscost/confidence sentence is the right lever and should reduce expensive body findings. It is still prompt growth on every review. Given the measured win described in the PR, this looks worth it, but watch for models over-collapsing real cross-cutting issues into medium confidence after this ships.No code change required unless live reviews start under-reporting genuine PR-level blockers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/prompt.ts around line 40, leave the new prLevelComments confidence guidance in place for now. After the first few live reviews, check whether high-confidence PR-level blockers are being under-emitted and tighten the wording only if that regression appears.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/ai/prompt.ts around line 40, leave the new prLevelComments confidence guidance in place for now. After the first few live reviews, check whether high-confidence PR-level blockers are being under-emitted and tighten the wording only if that regression appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `src/github.ts`:
- Line 650: In src/github.ts around postFileLevelComments (near line 650) and its submitReview call site, stop unconditionally creating a new file-scoped review comment for every file-level finding. Reuse the existing fingerprint / postedPrLevelKeys dedup already computed in reviewer.ts, or check previously posted DiffSentry file comments on the same path, and skip re-posting repeats so successive pushes do not stack duplicate file threads.
- Line 601: In src/github.ts around retireSupersededReviews near line 601, decide whether APPROVED DiffSentry reviews should also be rewritten to SUPERSEDED_REVIEW_STUB like COMMENTED reviews. If yes, handle state === "APPROVED" via updateReview; if no, add a brief comment explaining that prior approvals are intentionally left visible.
In `src/ai/parse.ts`:
- Line 206: In src/ai/parse.ts around isRepeatPrLevelFinding near line 206, document or normalize path scoping so empty-path PR-level findings only compare against other empty-path keys, and add a short comment/test clarifying that drift must continue to use path: "" for repeat suppression to work. No behavior change required unless you choose to intentionally allow empty-path candidates to match same-title path-scoped priors.
In `src/review-body.ts`:
- Line 434: In src/review-body.ts around formatReviewBody near line 434 and the submitReview fallback path in src/github.ts, reconcile the actionable count with unpostable file-level findings. Either format the body after postFileLevelComments returns, or ensure rejected file-level findings remain represented in a way that keeps the header count honest.
In `src/ai/prompt.ts`:
- Line 40: In src/ai/prompt.ts around line 40, leave the new prLevelComments confidence guidance in place for now. After the first few live reviews, check whether high-confidence PR-level blockers are being under-emitted and tighten the wording only if that regression appears.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 8d28e42b-4388-421b-b318-3bd244accd0c
📥 Commits
Reviewing files at 3cfdacf (base SHA unavailable).
📒 Files selected for processing (8)
src/ai/parse.tssrc/ai/prompt.tssrc/drift.tssrc/github.tssrc/review-body.tssrc/reviewer.tssrc/walkthrough-state.tstests/unit/pr-level-noise.test.ts
✅ Files skipped from review due to trivial changes (1)
src/types.ts
📌 Status — last updated
|
| Risk score | 3/100 (Low) █▁ |
| Unresolved threads | 0 |
| Failing checks | 0 |
| Pending checks | 1 |
| Files reviewed | 4 (5 skipped) |
| Updated | 2026-07-14 19:02Z |
Live-updated by DiffSentry on every push. Use @diffsentry ship for a verdict, @diffsentry timeline for full history.
Extract the drift approval gate into applyDriftToApproval (drift.ts) so the rule that hedged drift must not cost an approval is testable in isolation rather than buried inline in the reviewer's main method. No behaviour change — same condition, now pinned by tests. Adds 8 tests: the APPROVE + medium/low-drift path end to end through both verdict rules, and coverage proving isRepeatPrLevelFinding spans file-level findings (not just body-level ones) so re-worded file findings can't stack duplicate threads across pushes. Documents the path-scope contract on isRepeatPrLevelFinding: path is part of a finding's identity, comparison is same-scope only, and drift must keep emitting with path: "" to collapse against its own history. 369 unit tests pass, typecheck and lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
This PR addresses the PR-level noise problem thoughtfully: the confidence gating, threaded file-scoped findings, and superseded-review retirement all move the system in the right direction. I found one correctness issue in the new title-similarity dedupe path: prLevelRepeatKey uses a raw tab-delimited serialization, so titles containing tabs can corrupt parsing and cause missed or incorrect repeat suppression. Aside from that, the tests are strong and the overall design is coherent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: d3e469bc-3cf7-418d-b13b-87c9924c0587
📥 Commits
Reviewing files that changed from 3cfdacf to bd43de4. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (4)
src/ai/parse.tssrc/drift.tssrc/reviewer.tstests/unit/pr-level-noise.test.ts
✅ Files skipped from review due to trivial changes (1)
src/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
src/ai/prompt.tssrc/github.tssrc/review-body.tssrc/walkthrough-state.ts
ℹ️ The primary review model was unavailable; this review was generated by the configured backup provider.
1 participant
The problem
Our own PRs got noisy. The "🔎 Issues not tied to a specific line" section was showing Major / Medium-confidence findings at the top of every review, and they couldn't be resolved, replied to, or collapsed — they just accumulated.
It traces to #76, which fixed something real: a
REQUEST_CHANGESreview could read "Actionable comments posted: 0" with nothing to act on, because findings with no anchorable diff line were being dropped. That invariant is right and is preserved here. The problem is the home it chose — prose in the review summary body, the one DiffSentry channel a reader can't dismiss — and four things funnelled into it:reviewer.tsstampedseverity: "major", confidence: "medium"on every warning-level drift finding, andisNitpicknever consulted confidence — so all drift counted as actionable and claimed a slot at the top.parse.tsdemoted any un-anchorable major/critical inline finding there — findings that wanted a line and missed the line number.submitReviewcreates a new review per push, and cross-review dedup keys onfingerprintFor(path, line, title). PR-level findings haveline: 0and often no path, leaving the title as effectively the whole key — and drift titles are free prose regenerated per run. One re-wording minted a fresh fingerprint and the finding reappeared.CHANGES_REQUESTED, but drift bumpsAPPROVE → COMMENTand never blocks — so the exact review class Fix "requested changes" reviews that name no actionable fix #76 introduced was the one class that stayed on the timeline forever.The change
Route each finding to a channel based on what that channel costs the reader.
github.ts— Post file-scoped findings as real resolvable threads (subject_type: "file"). They post before the review so anything GitHub rejects folds back into the body, and a finding is never lost between channels. Retire our superseded reviews: dismissCHANGES_REQUESTED, and stubCOMMENTEDbodies viaupdateReview, since GitHub refuses to dismiss those and submitted reviews can't be deleted. Match on our own body marker rather thanuser.type === "Bot"— the old filter reached other bots' reviews.review-body.ts— SplitprLevelby whether a path survived (thread-able vs. body-only). Gate the body section on high confidence; collapse the rest.isVisiblyActionablebecomes the single source of truth for both the count and theREQUEST_CHANGESinvariant, so the header number and the verdict can't disagree.drift.ts/reviewer.ts— Ask the model for confidence and carry it through instead of hardcoding"medium". An omitted value defaults to"medium", not the"high"thatReviewCommentassumes — drift is the class least entitled to the benefit of the doubt. Severity stays"major"deliberately: severity is how bad if real, confidence is whether it's real, and conflating them is what caused this. Only high-confidence drift may withhold anAPPROVE.parse.ts/walkthrough-state.ts—titleSimilarity(Jaccard over content words, 0.6 threshold) catches re-worded repeats that fingerprinting misses, backed by a cappedpostedPrLevelKeys. Same-path-only, so a real finding is never swallowed by a same-sounding one about different code.Verification
Rendered the exact review that prompted this (2 Major/Medium drift findings + 1 inline bug) through
formatReviewBody:Adds
tests/unit/pr-level-noise.test.ts(29 tests) covering the confidence gate, the thread/body split, the reconcile invariant under gating, similarity dedup, drift confidence parsing, andsubmitReview's thread posting + review retirement. 361 unit tests pass; typecheck and lint clean (0 errors).Worth knowing
DiffSentry is a GitHub App, so the
subject_type: "file"call and the review-retirement path are covered by mocked-Octokit tests, not a live PR. Those are the two pieces to watch on the first real review after this merges — including on this PR itself.🤖 Generated with Claude Code
Summary
Routed PR-level findings to resolvable file threads or confidence-gated body sections, and retired superseded DiffSentry reviews so unanchored drift no longer accumulates as permanent, unresolvable prose.
Changes
src/ai/parse.tssrc/drift.tssrc/reviewer.tstests/unit/pr-level-noise.test.ts