claude-code-review.yml: add a claude-debug label to skip the cost gate - #64
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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}
| # push/re-run needed. Scoped in the job's `if:` below to only actually | ||
| # proceed when the label added IS claude-debug -- otherwise every | ||
| # unrelated label added to a PR would trigger another paid review. | ||
| types: [opened, synchronize, reopened, ready_for_review, labeled] | ||
|
|
||
| concurrency: | ||
| group: claude-review-${{ github.event.pull_request.number }} |
There was a problem hiding this comment.
Bug: adding labeled here can silently cancel an in-progress review with nothing replacing it.
The workflow-level concurrency block just below (group: claude-review-${{ github.event.pull_request.number }}, cancel-in-progress: true on the next line) is unchanged by this PR and is keyed only on the PR number. on.pull_request_target.types has no way to filter by which label was added — that filtering only happens in the job's if: (line 42). So labeled fires a new workflow run for any label, not just claude-debug.
Concurrency cancellation is resolved at run-creation time, before job-level if: conditions are evaluated. So the sequence: a review is running (triggered by synchronize) → a maintainer adds an unrelated label (e.g. documentation) → GitHub creates a new run in the same concurrency group → the in-progress review is cancelled → the new run's claude-review job is then skipped by the if: (since the label isn't claude-debug) → no review ever posts for that push, and nothing automatically retriggers it.
Before this PR, labeled wasn't a trigger at all, so this failure mode didn't exist. A fix would need to keep non-claude-debug labeled events out of the shared concurrency group (e.g. fold github.event.label.name into the group key for labeled events), rather than relying solely on the job-level if: to no-op them.
cat_tools/.github/workflows/claude-code-review.yml
Lines 18 to 24 in 59581e0
There was a problem hiding this comment.
Fixed in ac1ef09: labeled events for anything other than claude-debug now get their own per-label concurrency group, so they can never collide with (and thus never cancel) the real review's group. A claude-debug labeling event still deliberately shares the real group, since it's meant to supersede an in-progress review.
…review Caught by claude-review itself on this PR (review comment Postgres-Extensions#64 (comment)): concurrency-group cancellation is resolved when a run is admitted, before the job's `if:` is ever evaluated -- the `if:` can only no-op the new run's job, it can't un-cancel whatever the run's mere existence already displaced. Since `labeled` became a trigger type, a group keyed only on PR number meant ANY label (not just claude-debug) admitted a new run here, cancelling a real in-progress review (triggered by `synchronize`) with nothing to replace it. Fix: only give a labeled event its own per-label group when the label is NOT claude-debug, so it can never collide with (and thus never cancel) the real review's group. A labeled+claude-debug event deliberately keeps the plain group -- it's meant to supersede an in-progress real review, not run alongside it.
ac1ef09 to
d2462a7
Compare
…review Caught by claude-review itself on this PR (review comment Postgres-Extensions#64 (comment)): concurrency-group cancellation is resolved when a run is admitted, before the job's `if:` is ever evaluated -- the `if:` can only no-op the new run's job, it can't un-cancel whatever the run's mere existence already displaced. Since `labeled` became a trigger type, a group keyed only on PR number meant ANY label (not just claude-debug) admitted a new run here, cancelling a real in-progress review (triggered by `synchronize`) with nothing to replace it. Fix: only give a labeled event its own per-label group when the label is NOT claude-debug, so it can never collide with (and thus never cancel) the real review's group. A labeled+claude-debug event deliberately keeps the plain group -- it's meant to supersede an in-progress real review, not run alongside it.
| # BUG (found by claude-review itself, PR #64 review comment): concurrency | ||
| # cancellation is resolved when the run is admitted, BEFORE the job's `if:` | ||
| # is ever evaluated -- a job's `if:` can only no-op that job, it can't | ||
| # un-cancel whatever the run's mere existence already displaced. Since | ||
| # `labeled` is now a trigger type (see the `on:` comment above), a group | ||
| # keyed only on PR number meant ANY label -- not just claude-debug -- would | ||
| # admit a new run here, cancelling a real in-progress review with nothing | ||
| # to replace it (the new run's job then no-ops via its `if:`, too late). | ||
| # Fix: only a labeled event whose label is NOT claude-debug gets its own | ||
| # per-label group, so it can never collide with (and thus never cancel) | ||
| # the real review's group. A labeled+claude-debug event deliberately keeps | ||
| # the plain group -- it's meant to supersede an in-progress real review. | ||
| group: claude-review-${{ github.event.pull_request.number }}${{ (github.event.action == 'labeled' && github.event.label.name != 'claude-debug') && format('-{0}', github.event.label.name) || '' }} |
There was a problem hiding this comment.
This comment recounts the bug's full history (the old grouping, the specific labeled-trigger scenario, the exact cancellation consequence) rather than briefly stating the guard fact, per CLAUDE.md's "Bug Fixes" section:
Comment the fix where it isn't self-evident, but keep it concise — no novels. Do NOT recount the bug's history (what a past version got wrong) UNLESS the same mistake could realistically be made again; if it could, briefly state the guard fact that prevents it.
The guard fact ("concurrency cancellation is resolved when the run is admitted, before the job's if: is ever evaluated") is legitimate to keep since a future trigger type could hit the same pitfall — but the surrounding narration of the specific historical scenario and consequence goes beyond "briefly."
cat_tools/.github/workflows/claude-code-review.yml
Lines 23 to 37 in d2462a7
There was a problem hiding this comment.
Fixed in 761bc24: trimmed the comment to the guard fact only, dropping the incident narration.
| run: | | ||
| enabled=$(gh pr view "$PR" --repo "$REPO" --json labels \ | ||
| --jq 'any(.labels[]; .name == "claude-debug")') | ||
| echo "enabled=$enabled" >> "$GITHUB_OUTPUT" | ||
| echo "claude-debug label present: $enabled" |
There was a problem hiding this comment.
This step has no error tolerance for a transient gh pr view failure (API 5xx, rate limit, network blip). Under GitHub Actions' default bash -e, a non-zero exit inside $(...) fails the assignment and thus the step — and since this step is now unconditional and runs first, it will hard-fail the whole claude-review job on every ordinary (non-debug) PR review, not just debug runs.
This is inconsistent with the adjacent "Wait for CI; skip the paid review if any check failed" step, which explicitly tolerates the same class of failure (gh api ... 2>/dev/null || json='', then retries). Consider || enabled=false here so a transient failure degrades to "not in debug mode" instead of failing the run.
cat_tools/.github/workflows/claude-code-review.yml
Lines 84 to 90 in d2462a7
There was a problem hiding this comment.
Fixed in 761bc24: added || enabled=false, matching the fallback style already used in the adjacent cost-gate step.
Debugging why the paid review isn't behaving as expected (e.g. the missing --comment flag that silently swallowed every review before Postgres-Extensions#57) previously meant either waiting 5-20+ min per iteration for the cost gate to clear, or hand-editing the workflow just to see the raw transcript. Adding the "claude-debug" label to a PR now (a) skips the cost gate entirely, and (b) sets show_full_output: true so the full raw Claude Code JSON transcript, including tool results, lands in the job log. The label is checked live via `gh pr view` rather than the static event payload, so adding it and clicking "Re-run jobs" picks it up without a new push. `labeled` is added as a trigger type so applying the label alone kicks off a fresh run; the job's `if:` scopes that to only the claude-debug label itself, so tagging a PR with anything else doesn't trigger another paid review.
…review Caught by claude-review itself on this PR (review comment Postgres-Extensions#64 (comment)): concurrency-group cancellation is resolved when a run is admitted, before the job's `if:` is ever evaluated -- the `if:` can only no-op the new run's job, it can't un-cancel whatever the run's mere existence already displaced. Since `labeled` became a trigger type, a group keyed only on PR number meant ANY label (not just claude-debug) admitted a new run here, cancelling a real in-progress review (triggered by `synchronize`) with nothing to replace it. Fix: only give a labeled event its own per-label group when the label is NOT claude-debug, so it can never collide with (and thus never cancel) the real review's group. A labeled+claude-debug event deliberately keeps the plain group -- it's meant to supersede an in-progress real review, not run alongside it.
This PR just made claude-code-review.yml's concurrency group conditional (only a non-claude-debug labeled event gets its own per-label group), so verify-cancel-on-close-coupling's expected literal string (added by Postgres-Extensions#63, now on master) needs to match the new formula. Confirmed via `gh pr checks 64` that this is exactly what's currently failing -- this PR's own claude-code-review.yml change is compared, under the merge ref, against master's not-yet-updated expectation.
Both caught by claude-review on this PR's own commit: - The concurrency-group comment recounted the bug's full discovery history instead of briefly stating the guard fact, per this repo's Bug Fixes convention. Trimmed to the guard fact and the fix rationale. - The claude-debug label check ran unconditionally with no fallback; under the default `bash -e`, a transient `gh pr view` failure (rate limit, blip) would hard-fail the whole claude-review job on every ordinary review, not just debug runs. Add `|| enabled=false`, matching the fallback style already used in the adjacent cost-gate step.
761bc24 to
de7f925
Compare
|
Claude finished @jnasbyupgrade's task in 2m 33s —— View job Claude Code Review
ReviewI re-reviewed the current state of the diff (HEAD
Other things I checked and found sound:
No new issues found. This looks ready to merge from a workflow-logic and CLAUDE.md-compliance standpoint. |
1 participant
Debugging why the paid review isn't behaving as expected (e.g. the missing
--commentflag that silently swallowed every review before #57, or the missing inline-comment--allowedToolsflag before #62) previously meant either waiting 5-20+ min per iteration for the cost gate to clear, or hand-editing the workflow just to see the raw transcript.Adding the
claude-debuglabel to a PR now (a) skips the cost gate entirely, and (b) setsshow_full_output: trueso the full raw Claude Code JSON transcript, including tool results, lands in the job log. The label is checked live viagh pr viewrather than the static event payload, so adding it and clicking "Re-run jobs" on an existing run picks it up without a new push.labeledis added as a trigger type so applying the label alone kicks off a fresh run with no push/re-run needed; the job'sif:scopes that to only theclaude-debuglabel itself, so tagging a PR with anything else doesn't trigger another paid review.Created the
claude-debuglabel on the repo as part of this.