Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 28 additions & 7 deletions .github/workflows/claude-autofix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,13 @@ jobs:
jq -s '
def trusted: (.author.__typename == "Bot")
or (.authorAssociation | IN("OWNER", "MEMBER", "COLLABORATOR"));
# A thread whose last comment is an autofix reply was answered in an earlier round;
# it comes back once a reviewer replies to it.
[ .[]
| select(.isResolved | not)
| select(.comments.nodes[0] | trusted)
| select(.comments.nodes[-1]
| (.author.__typename == "Bot" and (.body | startswith("**Claude autofix ("))) | not)
Comment on lines +166 to +167

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug (medium confidence): the skip checks the thread's last comment from anyone, but the comment above says the thread "comes back once a reviewer replies". A passer-by's reply after an autofix reply therefore re-stages the thread. The staged comments array keeps only trusted authors, so Claude sees just its own earlier "fixed" reply and answers again ("already addressed"), which is the double answer this PR is meant to stop. Checking the last trusted comment fixes that. The jq is untested here: last on an empty array gives null, but comments.nodes[0] is already required to be trusted, so the array is never empty at this point.

Suggested change
| select(.comments.nodes[-1]
| (.author.__typename == "Bot" and (.body | startswith("**Claude autofix ("))) | not)
| select(.comments.nodes | map(select(trusted)) | last
| (.author.__typename == "Bot" and (.body | startswith("**Claude autofix ("))) | not)

Related, low confidence: comments(first: 50) on line 149 has no pagination. In a thread with more than 50 comments, nodes[-1] is not the real last comment and the skip misses it. Fetching the comments with pagination, or reading totalCount, would close that gap.

| { id, path, line: (.line // .originalLine), outdated: .isOutdated,
comments: [ .comments.nodes[] | select(trusted)
| { author: (.author.login // "ghost"), body } ] } ]
Expand Down Expand Up @@ -431,14 +435,15 @@ jobs:

# Replies go out only for threads that were staged, whatever ids the report holds. A thread
# Claude calls fixed is resolved only when a fix commit was pushed.
- name: Reply to and resolve the threads
- name: Reply to the threads
if: needs.fix.outputs.threads != '0'
env:
GH_TOKEN: ${{ github.token }}
DIR: ${{ runner.temp }}/autofix
LANDED: ${{ needs.fix.outputs.fixed == 'true' && steps.push.outputs.pushed == 'true' }}
run: |
set -euo pipefail
: > "$RUNNER_TEMP/to-resolve"
jq -c --slurpfile t "$DIR/threads.json" \
'($t[0] | map(.id)) as $ids | .[] | select(.id as $i | $ids | index($i))
| select(.status | IN("fixed", "declined", "unclear"))' \
Expand All @@ -458,15 +463,31 @@ jobs:
comment { id }
}
}' --silent || echo "::warning::Couldn't reply to $id."
if [ "$status" = fixed ]; then
gh api graphql -f thread="$id" -f query='
mutation($thread: ID!) {
resolveReviewThread(input: {threadId: $thread}) { thread { id } }
}' --silent || echo "::warning::Couldn't resolve $id."
fi
# 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
done

# Its own step so only these calls see the maintainer's token, which resolving takes:
# GITHUB_TOKEN can reply to a thread but not resolve it ("Resource not accessible by

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (medium confidence): the step split narrows which steps get the maintainer token in env, but it does not keep the token away from the rest of the job. The checkout uses token: AUTOMERGE_TOKEN with credentials persisted by default, so the token already sits in .git/config for every later step, including Reply and Summarise. The push step needs that persisted credential, so this is not a bug in the PR. The comment "so only these calls see the maintainer's token" is still stronger than the reality, and "only in this step's env" would be accurate.

# integration"). A fixed thread left open is staged again next round and answered twice, so
# a failed resolve fails the step.
- name: Resolve the fixed threads
if: needs.fix.outputs.threads != '0'
env:
GH_TOKEN: ${{ secrets.AUTOMERGE_TOKEN }}
run: |
failed=0
while read -r id; do
gh api graphql -f thread="$id" -f query='
mutation($thread: ID!) {
resolveReviewThread(input: {threadId: $thread}) { thread { id } }
}' --silent || { echo "::error::Couldn't resolve $id."; failed=1; }
done < "$RUNNER_TEMP/to-resolve"
exit "$failed"

# Posted once the push went through, even when a reply or a resolve failed after it.
- name: Summarise
Comment thread
giswqs marked this conversation as resolved.
if: ${{ !cancelled() && steps.push.outcome == 'success' }}
env:
GH_TOKEN: ${{ github.token }}
PR: ${{ github.event.pull_request.html_url }}
Expand Down
Loading