Treat cancelled check conclusions as non-failures - #341
Conversation
The wait-for-checks step counted cancelled check runs as failures, which prevented PR finalization. This happens when autofix workflows push new commits, cancelling the previous run on the old SHA. Add 'cancelled' alongside 'skipped' and 'neutral' in the allowed conclusions list so cancelled checks don't block the pipeline.
![james-in-a-box[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
No agent-mode design concerns.
This is a CI workflow fix that correctly excludes cancelled checks from the failure count — not related to agent design patterns.
(Unable to approve due to GitHub's self-approval restriction — this PR was authored by the same GitHub App identity.)
— Authored by egg
|
egg agent-mode-design completed. View run logs |
This comment has been minimized.
This comment has been minimized.
|
I checked all feedback channels for this PR:
No actionable review feedback was found to address. The PR is ready for human review. — Authored by egg |
There was a problem hiding this comment.
Review Summary
The fix is correct and necessary. When autofix workflows push new commits, GitHub cancels the previous run on the old SHA, and treating cancelled as a failure incorrectly blocks the pipeline.
Issues Found
1. PR Description Inaccuracy (Minor)
The description states "all three jq filter locations" but the diff only modifies two locations (lines 591 and 621). This is a documentation-only issue—the code changes are correct.
2. Inconsistent Fix Across Workflows (Advisory)
The same pattern exists in reusable-review.yml (lines 139 and 146) without the cancelled fix:
# reusable-review.yml:139
failed=$(echo "$checks" | jq '[.[] | select(.status == "completed" and .conclusion != "success" and .conclusion != "skipped" and .conclusion != "neutral")] | length')This means the reusable review workflow may still treat cancelled checks as failures. Depending on whether that workflow faces the same cancellation scenario, this could need a follow-up PR.
Verification
Confirmed via GitHub Docs that cancelled is a valid check run conclusion value, alongside success, failure, neutral, skipped, timed_out, and action_required.
Verdict
Approve. The fix correctly addresses the described issue. The inconsistency with reusable-review.yml is noted but doesn't block this PR.
— Authored by egg
|
egg review completed. View run logs |
This comment has been minimized.
This comment has been minimized.
Address review feedback noting that reusable-review.yml has the same pattern for counting failed checks but was not updated to exclude cancelled conclusions. This ensures consistency across all workflows. Authored-by: egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns.
Re-review summary: Checked changes since commit a68d79ffbe06b865206687abfe9a268b630fc1ee.
-
Previous feedback addressed: The
cancelledcheck conclusion fix has been applied toreusable-review.yml(lines 139 and 146), addressing the inconsistency I noted in my prior review. -
Additional changes: The
workflow_ownerfeature has been removed entirely (from contract schema, models, workflows, and tests). This is unrelated to agent-mode design — it's a workflow simplification that removes the auto-reviewer-assignment feature.
Neither change raises agent-mode design concerns. The PR remains focused on CI/CD workflow logic without impacting how agents operate or constraining their flexibility.
(Unable to approve due to GitHub's self-approval restriction — this PR was authored by the same GitHub App identity.)
— Authored by egg
There was a problem hiding this comment.
Re-Review Summary
The new commit (ba52783) addresses the advisory feedback from the previous review.
Previous Feedback Status
| Issue | Status |
|---|---|
| PR description said "three" locations but only modified two | Fixed — Description now correctly states "2 locations" in each file |
reusable-review.yml had same pattern without the fix |
Fixed — Both jq filters now include cancelled exclusion |
New Changes Review
The changes to reusable-review.yml are correct and mirror the pattern in sdlc-pipeline.yml:
- Line 139: Failed check count filter now excludes
cancelled - Line 146: Failed check display filter now excludes
cancelled
Both filters remain consistent with each other (they use the same selection criteria), which is important for accurate logging.
Verification
All four jq filter locations across both workflows now correctly exclude cancelled:
sdlc-pipeline.yml:591✓sdlc-pipeline.yml:621✓reusable-review.yml:139✓reusable-review.yml:146✓
Verdict
Approve. The feedback has been properly addressed and the fix is now complete and consistent across all workflows.
— Authored by egg
|
egg agent-mode-design completed. View run logs |
| egg is addressing review feedback... |
|
egg review completed. View run logs |
1 participant
Fix SDLC pipeline and reusable-review failing when check runs are cancelled
When autofix workflows (e.g., Address Review Feedback) push new commits,
the previous workflow run on the old SHA gets cancelled by GitHub. The
wait-for-checks step counted these cancelled runs as failures, preventing
the finalize-pr job from marking the PR as ready for review.
This was observed on PR #336, where the "Address Review Feedback" check
was cancelled on the original SHA after it pushed a new commit, causing
the wait-for-checks job to treat it as a failure.
The fix adds
cancelledto the list of allowed check conclusions(alongside
success,skipped, andneutral) in the jq filterlocations within the wait-for-checks polling logic in both:
sdlc-pipeline.yml(2 locations)reusable-review.yml(2 locations)Issue: none
Test plan:
wait-for-checks step does not count it as a failure
Authored-by: egg