Conversation
Inline review comments from claude-code-review.yml never close. The fingerprint dedup stops a finding being re-posted on the next push, but the thread it opened stays unresolved forever -- so fixed findings accumulate as stale bot conversations and block any branch protection that requires them resolved. Resolve a thread via GraphQL resolveReviewThread when all of: - it is ours (first comment is the bot's and carries an hdxr: fingerprint) - this run's findings no longer mention that fingerprint - GitHub marks the thread isOutdated -- the flagged line actually moved - no human has replied The isOutdated pairing is the load-bearing part. The reviewer measures ~40% recall, so absence from a run means "fixed" or "missed" with no way to tell them apart; resolving on absence alone would silently launder a real critical into "resolved". Requiring the line to have moved as well means we only close what the author plausibly touched. The step is deliberately fail-soft, unlike everything else in this job: resolution is cosmetic, so a GraphQL error warns and moves on rather than failing a review that already posted. The next push retries for free. Needs no new permissions -- pull-requests: write already covers the mutation, including on fork PRs, where pull_request_target's token is base-repo-scoped and the threads live on the base repo. Note this bumps prompt_hash (trusted-hash.sh covers both changed files), so merging re-reviews every open PR once at ~$3 each.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
github-actions
Bot
added
the
review/tier-1
🟢 Tier 1 — TrivialDocs, images, lock files, a dependency bump, or an automated release. No functional code changes detected. Why this tier:
Review process: Auto-merge once CI passes. No human review required. Stats
|
Greptile SummaryThis PR adds fail-soft automatic resolution of stale Claude review threads after a finding disappears and its anchor becomes outdated.
Confidence Score: 5/5The PR appears safe to merge with no actionable defects identified. The resolver checks bot ownership, outdated state, resolvability, human participation, and current fingerprints before mutating a thread, while failures remain retryable and do not invalidate the completed review.
|
| Filename | Overview |
|---|---|
| .github/scripts/code-review/review-comments.cjs | Adds conservative thread-selection logic and resolution-count rendering without an accepted correctness issue. |
| .github/workflows/claude-code-review.yml | Adds paginated thread discovery, fail-soft GraphQL resolution, and summary output wiring. |
| .github/scripts/code-review/tests/review-comments.test.mjs | Covers each resolution guard, malformed inputs, and both clean and finding-bearing summary branches. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Completed healthy review] --> B[Fetch all review threads]
B --> C{Bot-owned and unresolved?}
C -- No --> K[Keep open]
C -- Yes --> D{Outdated and resolvable?}
D -- No --> K
D -- Yes --> E{Human replied?}
E -- Yes --> K
E -- No --> F{Fingerprint still reported?}
F -- Yes --> K
F -- No --> G[Resolve review thread]
G --> H[Count successful resolutions]
H --> I[Render count in sticky summary]
Reviews (1): Last reviewed commit: "ci: auto-resolve Claude review threads o..." | Re-trigger Greptile
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| const ids = helpers.threadsToResolve({ | ||
| threads, findings, botLogin: process.env.BOT_LOGIN, |
There was a problem hiding this comment.
🟠 major — GraphQL author.login for the Actions bot has no [bot] suffix, so threadsToResolve matches nothing and no thread is ever resolved
Every comment this workflow posts is authored by the GitHub Actions App, and GraphQL returns App authors as a Bot whose login omits the [bot] suffix that REST's user.login carries — so authored(nodes[0]) !== botLogin (review-comments.cjs:129) rejects github-actions vs github-actions[bot] on every thread and the step logs auto-resolved 0/0 forever. The tests can't catch it because they feed the same literal to both sides. Normalize before comparing, as this repo already does at .github/scripts/pr-triage-classify.js:155 and .github/workflows/release-reminder.yml:36: pass both process.env.BOT_LOGIN and its .replace(/\[bot\]$/, '') form and accept a match against either, plus a test whose author is the un-suffixed github-actions.
| }`; | ||
|
|
||
| let resolved = 0; | ||
| for (const id of ids) { |
There was a problem hiding this comment.
🟠 major — A finding the reviewer re-reports after its thread was auto-resolved is silently hidden — dedup suppresses the re-post and the thread stays resolved
Push 2: the reviewer misses finding A (~40% recall) and its outdated thread is resolved. Push 3: A is reported again with the same fingerprint, but seenFingerprints (review-comments.cjs:99) is built from listReviewComments, which returns comments inside resolved threads, so buildInlineComments routes A to skipped — nothing is posted and the summary claims "unchanged from an earlier push (already inline above)" for a thread that is collapsed and resolved. Branch protection requiring resolved threads then passes with a re-confirmed finding outstanding. In the same step, also collect threads whose fingerprint IS in the current findings and whose isResolved is true and call unresolveReviewThread on them; the query at line 634 already fetches isResolved and the bodies, so this is a second list out of the same helper ({ resolve, unresolve }) plus one mutation.
| const nodes = (t.comments && t.comments.nodes) || []; | ||
| const authored = c => (c.author && c.author.login) || ''; | ||
| // The first comment carries the fingerprint; the rest tell us whether a human joined. | ||
| if (!nodes.length || authored(nodes[0]) !== botLogin) continue; |
There was a problem hiding this comment.
🔵 minor — The nodes[0] bot-author check is subsumed by the human-reply check on the next line and no test separates them
nodes.some(c => authored(c) !== botLogin) on line 130 already covers nodes[0], so line 129's author comparison can never be the check that rejects a thread — only its !nodes.length half matters (it guards the nodes[0].body deref on line 131). The ignores threads that are not ours test at review-comments.test.mjs:87 passes with it deleted, because its human-authored thread carries no hdxr: fingerprint and falls out at FINGERPRINT_RE anyway. Reduce line 129 to if (!nodes.length) continue;, or keep it and add a test whose first comment is human-authored and carries helpers.commentBody(FIXED) — the only case that distinguishes the two guards.
PR Review3 finding(s): 🔴 0 critical · 🟠 2 major · 🔵 1 minor 3 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
E2E Test Results✅ All tests passed • 334 passed • 1 skipped • 1110s
Tests ran across 4 shards in parallel. |
Deep Review✅ No critical issues found. The core feature ( 🟡 P2 -- recommended
🔵 P3 nitpicks (4)
Reviewers (6): correctness, reliability, security, testing, maintainability, adversarial. Testing gaps:
|
1 participant
Inline review comments from
claude-code-review.ymlnever close. The fingerprint dedup stops a finding being re-posted on the next push, but the thread it opened stays unresolved forever. Fixed findings pile up as stale bot conversations and block any branch protection requiring resolved threads.Fix
New
Resolve fixed review threadsstep calls GraphQLresolveReviewThreadon a thread when all four hold:hdxr:fingerprintisOutdated— the flagged line actually movedThe
isOutdatedpairing is the load-bearing part. The reviewer measures ~40% recall, so a finding vanishing from a run means "fixed" or "missed" with no way to tell them apart. Resolving on absence alone would silently launder a real critical into "resolved". Requiring the line to have moved too means we only close what the author plausibly touched.The step is fail-soft, unlike everything else in this job — resolution is cosmetic, so a GraphQL error warns and moves on rather than failing a review that already posted. The next push retries for free. It runs after the trusted-tree verification, so the helper it
require()s is already covered.Resolve count is reported in the sticky summary, including on the all-clear branch — the run that resolves the most threads is usually the one that finds nothing left.
Not fixed here
deep-review.yml— posts one sticky issue comment. Issue comments have no thread and no resolve state, so there's nothing to resolve. It already self-heals viaedit-mode: replace.Permissions
None added.
pull-requests: writealready covers the mutation, including on fork PRs —pull_request_target's token is base-repo-scoped and the threads live on the base repo.Cost
trusted-hash.shcovers both changed files, so this bumpsprompt_hashand re-reviews every open PR once at ~$3 each on merge. Designed behaviour, but worth timing.Tests
11 new cases in
review-comments.test.mjs(44 total, green), run by the workflow itself before it spends on a review. Each of the three guards was mutation-tested — removingisOutdated, the human-reply check, or the current-findings check each fails exactly one test.make ci-lintgreen: 0 errors, ratchet ok, openapi in sync.No changeset — CI-only, not user-facing.
🤖 Generated with Claude Code