Repository navigation
feat(claude-review): set a pending status while the review runs - #114
crowecawcaw wants to merge 2 commits into
Conversation
The "Claude review" commit status was only set when a review finished, so while it ran the PR showed only the collect check, which goes green in seconds. Authors couldn't tell a running review from one that never started. PR resolution moves into its own read-only resolve job, so a new start job, which runs no model, can set the status to pending before the review job starts. The review job's token stays read-only. post now also runs when the review job fails before preparing its context, and sets the status to error, so pending doesn't stay on the PR head. Supersedes aws-deadline#95, which used a check run from the model's job. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
|
|
||
| jobs: | ||
| review: | ||
| # Resolve the PR, then set the pending status before the review starts. |
There was a problem hiding this comment.
Should fix: The PR commits compiled bytecode: .github/scripts/__pycache__/claude_pr_review.cpython-314.pyc and test/__pycache__/test_claude_pr_review.cpython-314-pytest-9.0.2.pyc. These are local build artifacts. They go stale as soon as the source changes, and they bloat history. The repo has no .gitignore, so the next local test run will add them again.
Fix: remove both files from the PR and add a .gitignore containing __pycache__/ and *.pyc.
There was a problem hiding this comment.
Fixed in 429bb5d: removed both files and added a .gitignore for __pycache__/ and *.pyc.
There was a problem hiding this comment.
Addressed: Both .pyc files are gone from the PR head, and the new .gitignore excludes pycache/ and *.pyc.
| @@ -527,12 +576,13 @@ jobs: | |||
| submodules: false | |||
|
|
|||
| - name: Post review | |||
There was a problem hiding this comment.
Should fix: If the Post review step fails, the head commit stays at the pending status that start set, and it stays there indefinitely. post() calls gh with check=True in several places (fetch_threads, fetch_summary, the summary PATCH/POST), so a transient GitHub API error or a failed artifact download ends the job before set_status runs. The PR then shows "Review in progress" with nothing running. Before this PR the status was simply absent in that case, which the docs define as "not picked up".
Fix: add a final step that runs on failure() and sets state=error for context=Claude review, the same way Report unfinished review does.
There was a problem hiding this comment.
Fixed in 429bb5d: Report unfinished review now runs last, on !cancelled() && (prepared != 'true' || failure()), so a failed download, checkout, or Post review step replaces pending with error.
There was a problem hiding this comment.
Addressed: 'Report unfinished review' now runs as the last step on !cancelled() && (prepared != 'true' || failure()), so if the download, checkout, or Post review step fails, the pending status becomes error.
|
Claude review · advisory Reviewed Open: 0 blocking · 0 should-fix · 0 nit. ✅ Nothing blocking. Fix or reply to each thread; the next revision's review re-checks open threads and resolves those it agrees are handled. Resolving a thread yourself also closes it. Later revisions review only what changed. |
If a step in post failed before Post review set the status (an artifact download, or a GitHub API error in the script), the head stayed at the pending status that start set. Report unfinished review now runs last and also covers that case. Also drop the __pycache__ files committed by mistake, and ignore them. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
What was the problem/requirement? (What/Why)
The
Claude reviewcommit status is only set when a review finishes. While a review runs, the PR shows just the collect check, which goes green in seconds, and then nothing until results post. Authors can't tell a running review from one that never started.#95 tried to fix this with a check run opened from the agent's job. Since #99 that job holds a read-only token, and the commit status already exists, so this PR replaces #95.
What was the solution? (How)
resolve(new, read-only): the existing PR-resolution step, moved out ofreviewunchanged (including the debounce and the skip cases).start(new, no model, no permissions block): setsClaude reviewtopending("Review in progress") on the head SHA. Best effort: withoutstatuses: writeit warns and the run continues.review: gated once at the job level onresolve, in place of the per-stepif:s. Its token is unchanged and still read-only.post: now also runs whenreviewfails before preparing its context (for example, a checkout fails). In that case it sets the status toerrorrather than leavingpendingon the PR head. Otherwise it behaves as before.A run cancelled by a newer push leaves
pendingon a commit that is no longer the head, so the PR doesn't show it.What is the impact of this change?
statuses: writeget the pending status with no changes. Callers that don't, including this repo's own caller, see no change apart from a warning annotation.How was this change tested?
test/test_claude_pr_review.pypasses (65).statuses: write.pending("Review in progress"), thenfailure("Open: 2 blocking, 0 should-fix, 1 nit"). Both bugs were found.pending, thensuccess, with 2 threads resolved.tooling_refat a ref that doesn't exist, soreviewfails at the tooling checkout.postran onlyReport unfinished review, and the status wentpending, thenerror("Review did not finish; push again or re-run to retry").startandreviewboth queued for about 3 minutes before starting.pendingappeared roughly 2 minutes after the collect check finished.postjob itself (the second commit's case). That path uses the same step andfailure().Was this change documented?
Yes, in the workflow header and job comments.
Is this a breaking change?
No.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.