What happened
While driving d-morrison/ai-config#295 to clean, review / require-review repeatedly failed right after pushes that landed close to an @claude review comment (or shortly after a claude.yml agent run finished and re-dispatched its own review).
Root cause
claude-code-review.yml's claude-review job uses a per-PR concurrency group (claude-review-<PR>, cancel-in-progress: true) shared by both the automatic pull_request-triggered review and claude.yml's comment-triggered workflow_dispatch re-review. When both fire close together, one cancels the other — intentional dedupe (only one review should be visible per push).
The problem: require-review asserts needs.claude-review.result == 'success'. A canceled run is not skipped, so require-review's if: still runs and the assert fails — making require-review show a hard failure for what's actually just a stale, superseded run. The survivor run's review is fine.
What this cost
~4 retry cycles (extra empty-commit pushes) chasing what looked like a CI failure but was just this race, before the cause was identified.
Fix applied so far
Documented the gotcha in two places (no behavior change yet):
claude-code-review.yml's claude-review job concurrency comment blockgha/CLAUDE.md (new section: "A canceled review can red-X require-review — don't chase it as a code bug")
Possible follow-up (not done here)
Consider whether require-review should treat a cancelled claude-review result the same as skipped (gray, non-blocking) rather than failure — since a canceled run by design means a later run is the one that should be trusted. That would remove the false-positive red-X entirely instead of just documenting it. Left as a design decision for a future PR rather than bundled into this docs-only one.
What happened
While driving
d-morrison/ai-config#295to clean,review / require-reviewrepeatedly failed right after pushes that landed close to an@claude reviewcomment (or shortly after a claude.yml agent run finished and re-dispatched its own review).Root cause
claude-code-review.yml'sclaude-reviewjob uses a per-PR concurrency group (claude-review-<PR>,cancel-in-progress: true) shared by both the automaticpull_request-triggered review and claude.yml's comment-triggeredworkflow_dispatchre-review. When both fire close together, one cancels the other — intentional dedupe (only one review should be visible per push).The problem:
require-reviewassertsneeds.claude-review.result == 'success'. A canceled run is notskipped, sorequire-review'sif:still runs and the assert fails — makingrequire-reviewshow a hard failure for what's actually just a stale, superseded run. The survivor run's review is fine.What this cost
~4 retry cycles (extra empty-commit pushes) chasing what looked like a CI failure but was just this race, before the cause was identified.
Fix applied so far
Documented the gotcha in two places (no behavior change yet):
claude-code-review.yml'sclaude-reviewjob concurrency comment blockgha/CLAUDE.md(new section: "A canceled review can red-X require-review — don't chase it as a code bug")Possible follow-up (not done here)
Consider whether
require-reviewshould treat acancelledclaude-review result the same asskipped(gray, non-blocking) rather thanfailure— since a canceled run by design means a later run is the one that should be trusted. That would remove the false-positive red-X entirely instead of just documenting it. Left as a design decision for a future PR rather than bundled into this docs-only one.