feat(action): opt-in resolution of the bot's own outdated review threads (#567) - #28
feat(action): opt-in resolution of the bot's own outdated review threads (#567)#28chethanuk wants to merge 4 commits into
Conversation
…ads (alibaba#567) When OCR comments inline and the author pushes a fix, the conversations have to be resolved by hand. Adds an opt-in `resolve_outdated` input with three modes. Resolution is fully deterministic — no model is asked whether a finding was addressed. The signal is GitHub's own server-computed `isOutdated`, which is recomputed against the current head and is force-push aware. false off. Default. No behaviour change. report read-only. Reports which threads it would resolve and why, mutating nothing. Works under the existing contents: read scope. true performs the mutations. Requires contents: write. A thread is resolved only when all of these hold: GitHub reports it outdated and unresolved; every comment in it was written by this token (one human reply and it is left alone — that is a conversation, not a stale marker); the current run parsed its output and finished posting; and no current-run finding overlaps the thread's original span. That last gate is positional rather than a flag: the resolve step sits after the parse-failure and zero-findings exits, so a run that reported nothing can never close a batch of threads. The overlap set is built from the raw parsed findings rather than from what was posted, so a finding suppressed by incremental dedupe still vetoes resolving its own thread. Two behaviours are measured rather than assumed. resolveReviewThread returns "Resource not accessible by integration" under contents: read plus pull-requests: write, and succeeds under contents: write — even when the bot resolves a thread it created itself. And viewerCanResolve reports false for these tokens even when the mutation then succeeds, so it is never branched on; the mutation is attempted and its failure caught. A FORBIDDEN or rate-limited response logs one warning naming the contents: write requirement, stops resolving for the run, and counts zero. It never fails the run.
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughThe action adds disabled, report-only, and active modes for outdated bot review-thread handling. The post-review script lists and filters stale threads, resolves eligible threads with GraphQL, reports counts, and continues after resolution errors. Documentation and comprehensive tests cover the new behavior. ChangesOutdated thread resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubAction
participant runPostReviewComments
participant resolveOutdatedThreads
participant GitHubGraphQL
GitHubAction->>runPostReviewComments: pass resolveOutdated
runPostReviewComments->>resolveOutdatedThreads: pass current finding spans
resolveOutdatedThreads->>GitHubGraphQL: list review threads
GitHubGraphQL-->>resolveOutdatedThreads: return paginated threads
resolveOutdatedThreads->>GitHubGraphQL: resolve eligible threads
GitHubGraphQL-->>resolveOutdatedThreads: return mutation results
resolveOutdatedThreads-->>runPostReviewComments: return resolution statistics
runPostReviewComments-->>GitHubAction: set resolution outputs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XL
User descriptionDescriptionWhen OCR comments inline and the author pushes a fix, the conversations have to be resolved by hand (alibaba#567). This is the deterministic version of that, not the LLM one. @lizhengfeng101's objection on the issue — that inferring "was this addressed?" from a later diff is hallucination-prone, and that tolerance for false positives is near zero at the last gate before merge — rules out asking a model. So nothing here asks one. The signal is GitHub's own server-computed Adds an opt-in
How conservative is it?@stay-foolish-forever asked this directly on the issue, so: a thread is resolved only when all of
(3) is enforced positionally rather than by a flag: the resolve step sits after the parse-failure and zero-findings exits, so a run where the model reported nothing cannot close a batch of threads. That case — model silence reading as "all clear" — is the one worth being paranoid about, and a flag can be got wrong in a later refactor where control flow cannot. The overlap set in (4) is built from the raw parsed findings rather than from what actually got posted. A finding suppressed by incremental dedupe is still a live finding, so it still vetoes resolving the thread on its own lines. Two things measured rather than assumedBoth came out of GitHub Actions runs against a real PR, not from docs:
On a Limitations
Incremental mode is untouched. No Type of Change
How Has This Been Tested?
The two API behaviours above were verified by live GitHub Actions runs on a throwaway PR under each permission set, not by reading documentation. Checklist
Related Issuescloses alibaba#567 CodeAnt-AI DescriptionAdd opt-in cleanup for outdated bot review threads What Changed
Impact
💡 Usage GuideChecking Your Pull RequestEvery time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later. Talking to CodeAnt AIGot a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask: This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code. ExamplePreserve Org Learnings with CodeAntYou can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input: This helps CodeAnt AI learn and adapt to your team's coding style and standards. ExampleRetrigger reviewAsk CodeAnt AI to review the PR again, by typing: Check Your Repository HealthTo analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
examples/github_actions/README.md (1)
186-188: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the pacing cost of
'true'mode.
resolveOutdatedThreadssleeps between mutations. The delay comes fromOCR_RESOLVE_DELAYand defaults to 1000 ms. A run that hits the 50-thread cap therefore adds about 49 seconds to the job. The section states the cap but not the added run time.OCR_RESOLVE_DELAYis also env-only, so a user cannot set it throughwith:.Add one sentence next to the cap so users can predict the job duration before they enable
'true'.📝 Proposed documentation change
-A thread is left alone whenever **a human has replied** to it, when it is already resolved, when a finding from the current run still covers its lines, or when it has more comments than one API page returns (so a reply the action cannot see is never resolved over). Resolution also runs only after a run that actually produced findings: a run that failed to parse OCR's output, or that reported nothing, resolves nothing — "the model said nothing this time" is not evidence the old findings are gone. At most 50 threads are resolved per run; the rest carry over to the next one. +A thread is left alone whenever **a human has replied** to it, when it is already resolved, when a finding from the current run still covers its lines, or when it has more comments than one API page returns (so a reply the action cannot see is never resolved over). Resolution also runs only after a run that actually produced findings: a run that failed to parse OCR's output, or that reported nothing, resolves nothing — "the model said nothing this time" is not evidence the old findings are gone. At most 50 threads are resolved per run; the rest carry over to the next one. Mutations are paced one second apart, so a run that hits the cap adds roughly 50 seconds to the job. Set the `OCR_RESOLVE_DELAY` environment variable (milliseconds) on the workflow to change the pacing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/github_actions/README.md` around lines 186 - 188, Add a sentence beside the 50-thread resolution cap in the README explaining that `'true'` mode waits between mutations using the env-only `OCR_RESOLVE_DELAY`, which defaults to 1000 ms, so resolving 50 threads adds approximately 49 seconds to the job.scripts/github-actions/post-review-comments.js (1)
1279-1308: 🚀 Performance & Scalability | 🔵 TrivialConsider the added job duration from the default pacing.
OCR_RESOLVE_DELAYdefaults to 1000 ms and the loop sleeps between mutations. A run that reachesMAX_RESOLVE_PER_RUNtherefore holds the runner for about 49 extra seconds after the review has already been published. The delay is also env-only, so a workflow author cannot tune it through the action inputs.Two options worth weighing:
- Expose the delay as an action input so operators can trade pacing against job time.
- Emit the elapsed resolution time in the summary log line, so a fleet dashboard can see when this step dominates the job duration.
The sequential design itself is correct for a write endpoint. This is about cost visibility, not correctness.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/github-actions/post-review-comments.js` around lines 1279 - 1308, Expose the resolve pacing delay through an action input and use that value when initializing delay in the sequential resolution loop around parseNonNegInt and OCR_RESOLVE_DELAY, while retaining the current default for backward compatibility. Also include the elapsed resolution duration in the final summary log so operators can identify runs where thread resolution dominates job time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/github-actions/post-review-comments.js`:
- Around line 1211-1213: Update the thread status predicate around the span
construction and overlapsHistory call to return "unverified" when both
thread.originalLine and thread.originalStartLine are null or otherwise cannot
produce a usable span. Preserve the existing "overlap" result for detected
history overlap and "resolve" result for verified non-overlapping spans, and
update the corresponding post-review-comments test expectation to "unverified".
- Around line 1202-1205: Normalize GraphQL author logins and REST bot logins
before the isBotComment check in the nodes loop, so equivalent values such as
github-actions and github-actions[bot] are recognized as the same bot and
bot-only threads do not return human_reply. Update isBotComment or its callers
while preserving human-authored comment detection, and add coverage for the
mismatched login formats.
---
Nitpick comments:
In `@examples/github_actions/README.md`:
- Around line 186-188: Add a sentence beside the 50-thread resolution cap in the
README explaining that `'true'` mode waits between mutations using the env-only
`OCR_RESOLVE_DELAY`, which defaults to 1000 ms, so resolving 50 threads adds
approximately 49 seconds to the job.
In `@scripts/github-actions/post-review-comments.js`:
- Around line 1279-1308: Expose the resolve pacing delay through an action input
and use that value when initializing delay in the sequential resolution loop
around parseNonNegInt and OCR_RESOLVE_DELAY, while retaining the current default
for backward compatibility. Also include the elapsed resolution duration in the
final summary log so operators can identify runs where thread resolution
dominates job time.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff1074fd-0c1f-4038-95f2-4dc3df7ae952
📒 Files selected for processing (4)
action.ymlexamples/github_actions/README.mdscripts/github-actions/post-review-comments.jsscripts/github-actions/post-review-comments.test.js
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| for (const c of nodes) { | ||
| const login = c && c.author && c.author.login ? c.author.login : ""; | ||
| if (!isBotComment({ user: { login } }, botLogin)) return "human_reply"; | ||
| } |
There was a problem hiding this comment.
Suggestion: The ownership check reuses isBotComment, whose unconditional github-actions[bot] suffix fallback treats comments from any workflow using the shared GITHUB_TOKEN identity as this action's comments. In a repository with another automation posting review threads, this can resolve that workflow's outdated threads when resolve_outdated is enabled. Require an action-specific marker or otherwise distinguish comments created by this action before allowing destructive resolution. [security]
Severity Level: Major ⚠️
- ❌ Unrelated GitHub Actions review threads can be resolved.
- ⚠️ Opt-in cleanup may alter another workflow's review state.
- ⚠️ Existing action-specific markers are not used for ownership.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/github-actions/post-review-comments.js
**Line:** 1205:1205
**Comment:**
*Security: The ownership check reuses `isBotComment`, whose unconditional `github-actions[bot]` suffix fallback treats comments from any workflow using the shared `GITHUB_TOKEN` identity as this action's comments. In a repository with another automation posting review threads, this can resolve that workflow's outdated threads when `resolve_outdated` is enabled. Require an action-specific marker or otherwise distinguish comments created by this action before allowing destructive resolution.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| // viewerCanResolve reports false for tokens that CAN resolve, so the | ||
| // predicate must ignore it entirely (no "cannot_resolve" outcome exists). | ||
| { name: "viewerCanResolve:false is ignored", thread: botThread({ viewerCanResolve: false }), spans: sameLine.slice(0, 0), want: "resolve" }, | ||
| { name: "no original line information", thread: botThread({ originalLine: null, originalStartLine: null }), spans: sameLine, want: "resolve" }, |
There was a problem hiding this comment.
Suggestion: The test explicitly requires a thread with no original line coordinates to be resolved. Without coordinates, the resolver cannot prove that a current finding does not overlap the old thread, so this expectation codifies unsafe behavior and will prevent the implementation from adopting the conservative unverified veto. Expect an unverified result instead. [security]
Severity Level: Major ⚠️
- ❌ A bot-only outdated thread with missing coordinates can be resolved without an overlap check.
- ⚠️ A current finding may be hidden behind an incorrectly closed thread.
- ⚠️ GitHub review threads expose nullable original-line fields.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/github-actions/post-review-comments.test.js
**Line:** 3300:3300
**Comment:**
*Security: The test explicitly requires a thread with no original line coordinates to be resolved. Without coordinates, the resolver cannot prove that a current finding does not overlap the old thread, so this expectation codifies unsafe behavior and will prevent the implementation from adopting the conservative unverified veto. Expect an unverified result instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix…le span
Two review findings, both of which made the feature wrong in opposite
directions.
GraphQL's reviewThreads returns a bot's slug ("github-actions") while
getAuthenticatedLogin and isBotComment use REST's suffixed form
("github-actions[bot]"). Confirmed against a live PR: the same comment
reads `github-actions` with __typename "Bot" through GraphQL and
`github-actions[bot]` through REST. Comparing them raw made every one of
our own threads look like a human reply, so nothing would ever have been
resolved and the feature would have looked merely inert. Normalized at
the call site so isBotComment's REST contract stays intact for
incremental mode, its other caller.
A thread with no originalLine and no originalStartLine produced a span
lineSpan cannot build, so overlapsHistory could only ever answer "no
overlap" and the veto was unreachable — the thread resolved on a check
structurally unable to fail. That veto is the entire mitigation for
GitHub reporting a still-live finding's thread as outdated after a
force-push, so an unbuildable span now returns "unverified" and the
thread stays open, exactly as a partial comment view already did.
The table case that asserted "resolve" for the null-line thread had
encoded the bug as expected behaviour; it now expects "unverified", with
cases added for both bot-login shapes and for a User whose login merely
looks bot-ish.
|
@coderabbitai review |
|
Under the default GITHUB_TOKEN every workflow in a repository posts as github-actions[bot], so authorship cannot distinguish OCR's own review threads from a sibling workflow's; under a GitHub App token isBotComment's github-actions[bot] fallback accepts them outright. Gate the destructive path on the marker OCR already stamps into every inline comment it creates (newCommentId), fetched via an aliased root-comment selection so only the one body that can carry the proof crosses the wire. Reported by CodeAnt on the fork PR.
|
@coderabbitai review |
|
Unknown values already fall back to off, which is the correct default but an invisible one: a workflow that says 'TRUE' looks configured and does nothing. Warn so the typo is visible; empty/unset/'false' stay silent as the documented ways to be off.
1 participant
Description
When OCR comments inline and the author pushes a fix, the conversations have to be resolved by hand (alibaba#567).
This is the deterministic version of that, not the LLM one. @lizhengfeng101's objection on the issue — that inferring "was this addressed?" from a later diff is hallucination-prone, and that tolerance for false positives is near zero at the last gate before merge — rules out asking a model. So nothing here asks one. The signal is GitHub's own server-computed
isOutdated, which GitHub recomputes against the current head and which is force-push and rebase aware.Adds an opt-in
resolve_outdatedinput, default off:falsereportcomments_resolved_previewoutput. Mutates nothing.contents: read(today's)truecomments_resolvedoutputcontents: writeHow conservative is it?
@stay-foolish-forever asked this directly on the issue, so: a thread is resolved only when all of
isOutdatedand not already resolved.newCommentId, used today for retry idempotency) — not by the author. Under the defaultGITHUB_TOKENevery workflow in a repo posts asgithub-actions[bot], so authorship cannot tell OCR's threads from a sibling workflow's, and under a GitHub App tokenisBotComment'sgithub-actions[bot]fallback would accept them outright. Only the root comment's body is fetched, via an aliasedcomments(first: 1)selection.lineSpan/sameCommentSpanhelpers rather than a second implementation.(4) is enforced positionally rather than by a flag: the resolve step sits after the parse-failure and zero-findings exits, so a run where the model reported nothing cannot close a batch of threads. That case — model silence reading as "all clear" — is the one worth being paranoid about, and a flag can be got wrong in a later refactor where control flow cannot.
The overlap set in (5) is built from the raw parsed findings rather than from what actually got posted. A finding suppressed by incremental dedupe is still a live finding, so it still vetoes resolving the thread on its own lines.
Two things measured rather than assumed
Both came out of GitHub Actions runs against a real PR, not from docs:
resolveReviewThreadrequirescontents: write. Undercontents: read+pull-requests: write— exactly what.github/workflows/ocr-review.ymlgrants — it fails withResource not accessible by integration. Undercontents: writeit succeeds. This holds even when the bot resolves a thread it created itself. That is whyreportmode exists and why it is the mode that works today: asking consumers to grant push access to a review bot is a real cost, and they should be able to see what this would do before paying it.viewerCanResolveis a false negative here. It returnedfalseon both runs, including the one where the mutation then succeeded. So it is never branched on — the mutation is attempted and its failure caught. Gating on it would have made the feature a permanent silent no-op that still passed unit tests.On a
FORBIDDENor rate-limited response: onecore.warningnaming thecontents: writerequirement, stop resolving for the rest of the run, count zero. Never fails the run.Limitations
isOutdatedis sticky across a force-push back to a byte-identical anchor: the thread stays outdated withline: nulleven though the flagged code is unchanged and the finding may still be live. Gate (4) is what covers this — if the finding is still real, this run re-reports it and the overlap veto fires..github/workflows/ocr-review.ymltriggers onpull_request_target: types: [opened]only, so it never emits asynchronizeevent and cannot exercise this. Consumers who review on update can.reportmode is deliberately the useful first step: how often the predicate actually fires depends on whether fixes tend to rewrite the flagged lines or fix the cause elsewhere, and that is worth measuring on real PRs before anyone turns on mutations.Incremental mode is untouched. No
minimizeComment, no deletion — nothing here removes history.Type of Change
How Has This Been Tested?
make testpasses locallynpm run test:github-actionsexits 0 and prints both suites' pass lines;make testandmake checkare unaffected (no Go changes). All pre-existingpost-review-comments.test.jscases are unmodified and green, with new cases added alongside covering each predicate gate, both non-default modes, the human-reply veto, the zero-findings and parse-failure exits resolving nothing, and the FORBIDDEN stop-and-continue path.The two API behaviours above were verified by live GitHub Actions runs on a throwaway PR under each permission set, not by reading documentation.
Checklist
go fmt,go vet)Related Issues
closes alibaba#567