Fix "requested changes" reviews that name no actionable fix#76
Merged
Conversation
A REQUEST_CHANGES review could be submitted with "Actionable comments posted: 0" and only prose describing the problem — most often a diff-vs-PR description discrepancy, which has no diff line to anchor an inline comment to. The review schema forced every finding onto a diff line, so the model raised these in the summary + approval only, they were dropped by the line-anchoring gate (or never emitted), and nothing reconciled the verdict with the surviving comment set. Give non-line findings a first-class home and enforce the invariant that a block must be backed by a concrete finding: - types: add `prLevel` flag on ReviewComment (no diff-line anchor; excluded from inline posting by the existing line > 0 filter). - prompt: add a `prLevelComments` channel + rule that REQUEST_CHANGES must be backed by a finding, never summary-only. - parse: parse prLevelComments (no anchoring); demote un-anchorable major/critical inline findings to PR-level instead of dropping them. - review-body: render an "Issues not tied to a specific line" section and count PR-level findings toward "Actionable comments posted"; add the pure reconcileApproval helper. - reviewer: downgrade REQUEST_CHANGES -> COMMENT when no actionable finding survives; fold warning-level description drift into PR-level findings (bumps APPROVE -> COMMENT, never forces a block). - drift: reclassify the "description too short" sentinel as info so it isn't folded into an actionable finding. Adds tests/unit/pr-level-findings.test.ts. All 304 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughAdded first-class PR-level findings so non-line-specific issues could survive parsing and review rendering. Enforced that REQUEST_CHANGES was downgraded when no actionable finding remained to support a blocking verdict. Changes
Sequence Diagram(s)sequenceDiagram
participant Model
participant Parser as parseReviewResponse
participant Review as ReviewResult
Model->>Parser: inline comment on invalid or distant line
Parser->>Parser: validate anchor
alt major or critical and un-anchorable
Parser->>Review: add PR-level finding with line 0
else minor or trivial and un-anchorable
Parser->>Review: drop finding
else nearby valid anchor exists
Parser->>Review: remap to nearest diff line
end
sequenceDiagram
participant Drift as Drift Detection
participant Reviewer
participant Builder as buildReviewComment
participant Review as ReviewResult
Drift->>Reviewer: warning-level description drift
Reviewer->>Builder: normalize as PR-level finding
Builder-->>Reviewer: formatted comment and fingerprint
Reviewer->>Review: append actionable PR-level comment
Reviewer->>Reviewer: bump APPROVE to COMMENT if needed
sequenceDiagram
participant Reviewer
participant Reconcile as reconcileApproval
participant Final as Final Review
Reviewer->>Reconcile: approval plus merged comments
Reconcile->>Reconcile: check for actionable findings
alt REQUEST_CHANGES with no actionable findings
Reconcile-->>Final: COMMENT
else actionable findings exist
Reconcile-->>Final: keep original approval
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested Labels
Risk AssessmentScore: 20/100 — 🟡 Moderate
Test Coverage Signal🔴 154 line(s) of production code added with no test changes.
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 0
This PR addresses the empty-blocking-review failure mode cleanly by introducing a PR-level finding path, preserving otherwise-lost major findings, and reconciling approval state against the surviving actionable comments. The overall design is solid and the new tests cover the main regression well. I found one maintainability issue where PR-level drift findings are constructed manually instead of reusing the new shared formatter, which risks the two code paths diverging over time, plus one prompt-cost/documentation gap worth tightening.
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 7799868c-e620-4781-9ded-d0b25f2f9f2e
📥 Commits
Reviewing files at 59e0ed8 (base SHA unavailable).
📒 Files selected for processing (8)
src/ai/parse.tssrc/ai/prompt.tssrc/drift.tssrc/review-body.tssrc/reviewer.tssrc/types.tstests/unit/finding-verification.test.tstests/unit/pr-level-findings.test.ts
📌 Status — last updated
|
| Risk score | 20/100 (Moderate) ▁▃█ |
| Unresolved threads | 1 |
| Failing checks | 0 |
| Pending checks | 1 |
| Files reviewed | 2 (6 skipped) |
| Updated | 2026-07-08 19:03Z |
Live-updated by DiffSentry on every push. Use @diffsentry ship for a verdict, @diffsentry timeline for full history.
Owner
Author
|
@diffsentry full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
The PR addresses the core inconsistency well: non-line-specific findings now have a first-class path through parsing, rendering, and approval reconciliation, and the added tests cover the main regression. I found one significant contract mismatch in the new PR-level schema handling and one maintainability issue where PR-level comment construction is duplicated instead of reusing the shared parser helper.
🧹 Nitpick comments (1)
src/reviewer.ts (1)
Line 951: PR-level drift findings bypass the shared comment-construction path.🛠️ Refactor suggestion | 🟡 Minor
PR-level drift findings bypass the shared comment-construction path.
🤔 Medium confidence — verify against intent before acting.
reviewer.tsnow hand-builds PR-levelReviewCommentobjects usingrenderInlineCommentBody()andfingerprintFor(), even thoughsrc/ai/parse.tsjust introducedbuildReviewComment()specifically to normalize formatting and fingerprint generation across inline, demoted, and PR-level findings. This duplication creates drift risk:
- Future changes to comment-body formatting, confidence defaults, or fingerprint seeding can easily update one path and miss the other.
- The comment shape for AI-originated PR-level findings and drift-originated PR-level findings is now coupled only by convention.
This is fixable by extracting the shared builder into a reusable helper module and having both parser and reviewer call the same function for PR-level comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/reviewer.ts around line 951, remove the hand-built PR-level `ReviewComment` construction for `driftWarnings` and reuse the same normalization logic used by `parseReviewResponse`. Extract `buildReviewComment` (and any minimal dependencies it needs) from `src/ai/parse.ts` into a shared helper module, then call it from both places so PR-level formatting and fingerprinting stay consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/reviewer.ts around line 951, remove the hand-built PR-level `ReviewComment` construction for `driftWarnings` and reuse the same normalization logic used by `parseReviewResponse`. Extract `buildReviewComment` (and any minimal dependencies it needs) from `src/ai/parse.ts` into a shared helper module, then call it from both places so PR-level formatting and fingerprinting stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `src/ai/parse.ts`:
- Line 495: In src/ai/parse.ts around line 495, make `prLevelComments` parsing match the schema in `src/ai/prompt.ts`; stop reading `c.path` for PR-level entries and always build them with `path: ""` and `line: 0`. If you want PR-level comments to optionally retain file context, then instead update the prompt schema, docs, and tests together to explicitly allow `path`.
In `src/reviewer.ts`:
- Line 951: In src/reviewer.ts around line 951, remove the hand-built PR-level `ReviewComment` construction for `driftWarnings` and reuse the same normalization logic used by `parseReviewResponse`. Extract `buildReviewComment` (and any minimal dependencies it needs) from `src/ai/parse.ts` into a shared helper module, then call it from both places so PR-level formatting and fingerprinting stay consistent.
🪄 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: 4457e54b-f11f-4440-a9b5-e9e7f4da6960
📥 Commits
Reviewing files at 59e0ed8 (base SHA unavailable).
📒 Files selected for processing (8)
src/ai/parse.tssrc/ai/prompt.tssrc/drift.tssrc/review-body.tssrc/reviewer.tssrc/types.tstests/unit/finding-verification.test.tstests/unit/pr-level-findings.test.ts
- parse.ts: prLevelComments now always build with path: "" per the ai/prompt.ts schema (which gives these entries no path/line), keeping their title-based fingerprint stable instead of reading c.path. - parse.ts: export buildReviewComment + RawComment. - reviewer.ts: drift-warning PR-level findings now go through the shared buildReviewComment instead of a hand-built ReviewComment, so body formatting and fingerprinting stay consistent with model-emitted findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
This PR addresses the core UX bug effectively by introducing a first-class PR-level finding path and reconciling blocking verdicts against the final actionable finding set. The overall design is sound, but there is one significant regression risk: PR-level findings are accepted from the model without validating required agent metadata, which weakens the review contract compared to inline comments and can reintroduce non-actionable output in a new channel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `src/ai/parse.ts`:
- Line 494: In src/ai/parse.ts around line 494, tighten validation for the `parsed.prLevelComments` loop so PR-level entries require `title`, `body`, and `aiAgentPrompt` before calling `buildReviewComment`; drop malformed entries instead of preserving them as actionable findings. Keep the validation criteria aligned with the schema in `ai/prompt.ts` so PR-level comments cannot back `REQUEST_CHANGES` unless they are fully actionable.
🪄 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: dc35823f-2517-406b-b223-20dd3468f64f
📥 Commits
Reviewing files that changed from 59e0ed8 to 1bc3bcf. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (2)
src/ai/parse.tssrc/reviewer.ts
🚧 Files skipped from review as they are similar to previous changes (6)
src/ai/prompt.tssrc/drift.tssrc/review-body.tssrc/types.tstests/unit/finding-verification.test.tstests/unit/pr-level-findings.test.ts
Owner
Author
|
@diffsentry release-notes |
📣 Release Notes✨ What's new
🛠 Improvements
🐛 Fixes
Marketing-speak version of this PR. Re-run with |
mk7luke
added a commit
that referenced
this pull request
Jul 14, 2026
) * Quiet PR-level findings: threads over prose, confidence over volume 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DiffSentry occasionally leaves a REQUEST_CHANGES review that reads "Actionable comments posted: 0" with only prose describing a problem — no inline comment pinning down what to fix (see the reported screenshots). In practice these are mostly diff-vs-PR-description discrepancies, which have no single changed line to anchor an inline comment to.
Root cause
The review schema required every finding to anchor to a diff line (
prompt.ts), so when the model detected a description/diff discrepancy it raised it insummary+approval: REQUEST_CHANGESwith no inline comment. That comment was then dropped by the line-anchoring gate inparse.ts(or never emitted), and nothing reconciled the verdict against the surviving comment set. Result:{ REQUEST_CHANGES, comments: [], summary }→ a blocking review pointing at a phantom issue. The substance never left the model — which is why@diffsentry explaincan reconstruct it on request.Fix
Give non-line findings a first-class home and enforce the invariant that a block must be backed by a concrete finding.
types.tsprLevelflag onReviewComment(no diff-line anchor; excluded from inline posting by the existingline > 0filter).ai/prompt.tsprLevelCommentsschema channel + rule: a REQUEST_CHANGES verdict must be backed by a finding, never summary-only.ai/parse.tsprLevelComments; demotes un-anchorable major/critical inline findings to PR-level instead of dropping them (minor/trivial still dropped).review-body.tsreconcileApprovalhelper.reviewer.tsREQUEST_CHANGES → COMMENTwhen no actionable finding survives; folds warning-level description drift into PR-level findings (bumps APPROVE→COMMENT, never forces a block).drift.tswarning → infoso it isn't folded into an actionable finding.A blocking review with nothing to act on is now structurally impossible. The parse-side demotion, render section, and invariant all hold regardless of whether the model adopts the new
prLevelCommentschannel.Testing
tests/unit/pr-level-findings.test.tscovers the PR-level channel, severity-based demotion, rendering + count, and thereconcileApprovalinvariant.tscclean; lint 0 errors.🤖 Generated with Claude Code
Summary
Added first-class PR-level findings so non-line-specific issues could survive parsing and review rendering. Enforced that REQUEST_CHANGES was downgraded when no actionable finding remained to support a blocking verdict.
Changes
src/ai/parse.tssrc/reviewer.ts