ci: tolerate push-to-main cancelled deps in selected roll-up jobs - #5924
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughSummary by CodeRabbit
WalkthroughFive GitHub Actions rollup workflows now share dependency-result evaluation, including qualifying cancellation handling on pushes to ChangesCI rollup gates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RollupWorkflow
participant job_result_ok
participant GitHubActions
participant OriginMain
RollupWorkflow->>job_result_ok: Pass dependency result and policy
job_result_ok->>GitHubActions: Read event, ref, and SHA
job_result_ok->>OriginMain: Fetch latest main SHA for superseded cancellation
job_result_ok-->>RollupWorkflow: Return accepted or rejected status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
github-actions
Bot
added
size: S
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ✅ Approved | 0 | 0 | 0 | 7a11989eb550 |
Head: 7a11989eb5501bec7a1fdc1baef81c8bf7a8d024
Next: No reviewer action needed.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete blocking issues found in the Reborn E2E workflow change. The new cancellation tolerance is scoped to push runs on refs/heads/main and leaves PR, merge_group, workflow_call, and workflow_dispatch dependency failures unchanged.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/failed/superseded state while reviewers run.
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.19% — 290382 / 340864 lines Per-crate breakdown (63 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
|
🚅 Deployed to the ironclaw-pr-5924 environment in ironclaw-ci-preview
|
github-actions
Bot
added
size: M
|
@claude review |
|
Found 2 issues:
What passed: No security vulnerabilities, no logic bugs, no performance issues. Production reliability improved. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts several GitHub Actions “roll-up” gate jobs to avoid cascading failures when dependency jobs are cancelled due to concurrency, treating cancelled as non-blocking only for push events to refs/heads/main, while keeping strict behavior for PRs / merge queue contexts.
Changes:
- Added a small
job_result_okbash helper to roll-up jobs to acceptsuccess(and in some workflows alsoskipped) as passing results. - Allowed
cancelleddependency results to pass only on push-to-main, preventing roll-up failures caused by concurrency cancellations. - Updated multiple workflow roll-ups (Reborn tests/E2E, coverage, platform/compat, code style) to use the helper consistently.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/reborn-tests.yml | Roll-up gate now treats dependency cancelled as OK on push-to-main via job_result_ok. |
| .github/workflows/reborn-e2e.yml | Reborn E2E roll-up gate now tolerates cancelled dependencies on push-to-main. |
| .github/workflows/platform-and-compat.yml | Platform/Compat roll-up gate now tolerates cancelled dependencies on push-to-main (and still accepts skipped). |
| .github/workflows/coverage.yml | Coverage roll-up gate now tolerates cancelled dependencies on push-to-main. |
| .github/workflows/code_style.yml | Code style roll-up gate now tolerates cancelled dependencies on push-to-main (and selectively accepts skipped). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 2f0539511bd7 |
Head: 2f0539511bd7053077a2beae76cfc94c4763ce65
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found one CI regression in the Code Coverage roll-up cancellation handling.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Do not pass coverage when jobs are cancelled without concurrency
Location: .github/workflows/coverage.yml:317-320
coverage.yml has no concurrency / cancel-in-progress policy, so a cancelled coverage or e2e-coverage dependency is not the superseded-main-run case handled in the other workflows. Because this workflow only runs on push to main, this branch makes any cancelled coverage job report the Coverage roll-up as green even though Codecov upload and/or E2E coverage did not complete. Keep cancellations failing here, or add a narrower guard that only applies to an intentional superseded run.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/failed/superseded state while reviewers run.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/reborn-e2e.yml:
- Around line 291-302: Update job_result_ok to accept a job name parameter and
include it in the cancellation message, matching the existing implementations
for code_style, coverage, and platform-and-compat. Update every job_result_ok
call site to pass the corresponding job name so tolerated cancellations identify
the specific job.
In @.github/workflows/reborn-tests.yml:
- Around line 797-807: Update the job_result_ok function to accept a job name
parameter and, when allowing a cancelled job on a push to main, emit a log
message identifying that job and the tolerated cancellation before returning
success. Update every job_result_ok call site to pass the corresponding job
name, matching the existing implementations’ behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94104432-7cd4-48bf-ae1c-d9be46ae94ac
📒 Files selected for processing (5)
.github/workflows/code_style.yml.github/workflows/coverage.yml.github/workflows/platform-and-compat.yml.github/workflows/reborn-e2e.yml.github/workflows/reborn-tests.yml
think-in-universe
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/coverage.yml (2)
313-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDRY:
job_result_okduplicated across 5+ workflow files.The simple variant (reborn-e2e.yml, reborn-tests.yml) is copy-pasted identically, and the coverage.yml variant diverges with git-fetch logic. A policy change requires updating every copy. Consider extracting to a reusable composite action or a shared script in
scripts/ci/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/coverage.yml around lines 313 - 334, Extract the duplicated job_result_ok logic from coverage.yml and the other workflow files into a shared scripts/ci helper or reusable composite action, preserving the coverage-specific superseded push-to-main handling. Update each workflow to invoke the shared implementation, and ensure the helper accepts the job name/result and retains the same success, cancellation, and failure behavior.
313-344: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd a checkout before the
git fetchincoverage-gate.
This job has no repo checkout, so it lacks.git/origin; the superseded-run branch always falls through and cancelled dependencies still fail the gate. Addactions/checkouthere, or drop the fetch-based fast-path if you don’t want this job to materialize the repo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/coverage.yml around lines 313 - 344, Add an actions/checkout step to the coverage-gate job before the shell step containing job_result_ok, ensuring the repository and origin remote are available for the git fetch superseded-run logic; alternatively remove that fetch-based branch if checkout is intentionally avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/coverage.yml:
- Around line 313-334: Extract the duplicated job_result_ok logic from
coverage.yml and the other workflow files into a shared scripts/ci helper or
reusable composite action, preserving the coverage-specific superseded
push-to-main handling. Update each workflow to invoke the shared implementation,
and ensure the helper accepts the job name/result and retains the same success,
cancellation, and failure behavior.
- Around line 313-344: Add an actions/checkout step to the coverage-gate job
before the shell step containing job_result_ok, ensuring the repository and
origin remote are available for the git fetch superseded-run logic;
alternatively remove that fetch-based branch if checkout is intentionally
avoided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c28bfb96-bcdd-42e6-a1f0-8b3781bfc30b
📒 Files selected for processing (3)
.github/workflows/coverage.yml.github/workflows/reborn-e2e.yml.github/workflows/reborn-tests.yml
|
@claude review |
Code Review for PR #5924Found 6 issues: CRITICAL
HIGH
Additional Notes
The critical logic inversion at line 38 must be fixed before merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/coverage.yml:
- Around line 316-320: Update the `job_result_ok` checks for both `coverage` and
`e2e-coverage` in the workflow to use `superseded_only` instead of `allow`, so
only superseded cancelled runs are treated as non-blocking while other
cancellations fail the workflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4da2b87-4a9b-4660-808f-1ccec11ae41a
📒 Files selected for processing (2)
.github/scripts/ci-job-result-ok.sh.github/workflows/coverage.yml
💤 Files with no reviewable changes (1)
- .github/scripts/ci-job-result-ok.sh
|
@claude review |
|
Found 4 issues in PR #5924:
Positive notes: The overall DRY consolidation is well-executed, security practices are sound, and POSIX compliance is maintained. |
Summary
cancelleddependency job only onpushtorefs/heads/main, while keeping strict failure behavior for PR and merge-group contexts.Tests (Reborn),Platform & Compat,Code Coverage,Code Style, andReborn E2E.Why
CI uses concurrency cancellation (
cancel-in-progress: true) for most of these checks, and superseded runs can still bubble up as a failed parent check when dependent jobs are cancelled.Change
main, with explicit job-name logging when tolerated.merge_group/pull_requestand for non-cancelled failure states.