Repository navigation
fix(ci): resolve threads autofix finds already addressed - #999
Conversation
On #972 most open threads were answered "declined: already addressed" and left open, because only fixed threads are resolved. Give that case its own status, addressed, which is resolved without needing a commit. Declined (Claude disagrees) and unclear threads still stay open for a maintainer.
|
Caution Review failedThe pull request is closed. Finishing Touches
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewVerdict: The change is small and mostly sound. Three findings are posted as inline comments on Bugs
Security
Performance
Quality
AGENTS.md
Testing note
Change sizeCompared with
|
- Keep addressed to code that already does what the comment asks, and file a comment that misreads the code under declined, so the status that resolves has a clear boundary. - Have an addressed reply cite the path:line it relies on, so a maintainer can check a resolved thread at a glance and reopen it.
| }' --silent || echo "::warning::Couldn't reply to $id." | ||
| # The loop runs in a pipe's subshell, so the threads to resolve go to a file. | ||
| if [ "$status" = fixed ]; then echo "$id" >> "$RUNNER_TEMP/to-resolve"; fi | ||
| case "$status" in fixed|addressed) echo "$id" >> "$RUNNER_TEMP/to-resolve" ;; esac |
There was a problem hiding this comment.
Bug (medium confidence): an addressed verdict resolves a thread on the model's word alone.
The prompt asks Claude to cite path:line for addressed, but nothing here checks that the citation exists. A wrong addressed closes a valid review thread, and a maintainer may not notice. Before this change a wrong verdict left the thread open. A guard that keeps an uncited addressed open:
| case "$status" in fixed|addressed) echo "$id" >> "$RUNNER_TEMP/to-resolve" ;; esac | |
| if [ "$status" = addressed ] && ! grep -Eq '\.[A-Za-z0-9]+:[0-9]+' <<< "$reply"; then status=declined; fi | |
| case "$status" in fixed|addressed) echo "$id" >> "$RUNNER_TEMP/to-resolve" ;; esac |
The reply has already been posted under the addressed label at this point, so a downgraded thread shows addressed but stays open. Moving the downgrade above the reply (before line 461) would keep the label accurate. The check only tests the shape of the citation, not that the line exists, so it is a floor rather than a verification.
| A maintainer's `claude-autofix` label has Claude bring a pull request up to date: it merges the base | ||
| branch and resolves any conflicts, then fixes the open review comments it agrees with, replies to | ||
| each and resolves the ones it fixed or found already fixed, and pushes the commits to the branch (a | ||
| fork's too, when edits by maintainers are allowed). The label comes off when it's done; add it again |
There was a problem hiding this comment.
Nit: this sentence says which threads get resolved but not which stay open. The workflow and the PR description both say declined and unclear threads stay open for a maintainer, and a reader would look for that here. Consider adding "; declined and unclear ones stay open for you" after "found already fixed".
Code reviewBugs
Security
Performance
Quality
AGENTS.md
Change sizeCompared with the base branch: the whole PR against main (merge base 6abc4b9 to PR head 875738e).
|
Follow-up to #997. A new autofix run on #972 (40440498850) worked as written but still resolved nothing. It staged one new thread, and Claude declined it. Only
fixedthreads get resolved.Most of the threads left open on #972 were answered "declined: Already addressed: …". That means the code already does what the comment asks, which should count as resolved. Claude had no other status to put it under.
The change:
addressedis now a status of its own, for code that already does what the comment asks, whether an earlier commit fixed it or the comment misread the code. These threads are resolved without needing a commit.declined(Claude disagrees) andunclear(needs a decision) still stay open for a maintainer. The contributing docs now say which threads get resolved.Not yet exercised: the resolve path with
AUTOMERGE_TOKENfrom #997 hasn't run, because the last run had no fixed thread. The next run with a fixed or addressed thread will show whether it works.