ci: add canary-rollout-tests.yml — enforce the engine's bats suite (CI gap from #613/#624)#685
Conversation
…s CI gap from #613/#624 relocation) The canary-rollout engine + tests/canary_rollout.bats were relocated into .github by #613/#624 but no CI runner came with them, so the 149+ test suite has been unenforced ever since — every #668 increment (#672/#676/#678/#684) merged without its tests actually running in CI. This workflow runs shellcheck + the full bats suite on any PR touching the engine, its pure core, the suite, or this workflow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4z5uYVjwdyQjh2wFZxkut
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow that runs ShellCheck and the canary-rollout Bats suite for relevant pull requests and pushes to ChangesCanary rollout CI
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/canary-rollout-tests.yml:
- Around line 52-55: Update the actions/checkout step in the canary rollout
workflow to set persist-credentials to false, while preserving the existing
fetch-depth and checkout 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2fd71484-07f4-4d77-bc4b-e71769b87402
📒 Files selected for processing (1)
.github/workflows/canary-rollout-tests.yml
|
@coderabbitai resolve |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
✅ Action performedComments resolved and changes approved. |
|
Dev-Lead — waiting on PR blockers (intent: fix-reviews)PR: #685 |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 34383519be8fb02915c07f82c897a4f62fc186e3
Review mode: triage-approved (single reviewer)
Summary
Adds a single new CI workflow (.github/workflows/canary-rollout-tests.yml, +74/-0) that closes a real gap: the canary-rollout engine and its bats suite were relocated in #613/#624 without a CI runner, so the suite has been unenforced. The workflow follows the house *-tests.yml pattern (shellcheck -x + bats), is path-filtered to the engine/core/suite/workflow files, uses read-only permissions, per-ref cancellable concurrency, and a SHA-pinned checkout. The workflow ran on this PR itself and passed (Lint and bats: SUCCESS), confirming the suite is green.
Linked issue analysis
No linked issues (closingIssuesReferences is empty). The PR body clearly documents the motivating gap (#613/#624 relocation without CI) and the follow-up plan (#684 rebase, #668 increments 5-6). Self-verifying by design: this PR's own CI run is the proof the suite passes.
Findings
No blocking findings.
- Security posture is clean: permissions restricted to contents: read; actions/checkout pinned to 9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0, verified via GitHub API to match the v7.0.0 tag (per AGENTS.md pin policy); persist-credentials: false set; no pull_request_target; no untrusted ${{ }} interpolation in run blocks; GH_TOKEN is an inert dummy string for stubbed tests, not a credential.
- Prior CodeRabbit changes-requested review (persist-credentials) was addressed at head SHA; the single review thread is resolved and CodeRabbit approved the head commit.
- Secret scan: run_secret_scanning MCP tool unavailable in this environment; gitleaks CI check passed (SUCCESS) and manual inspection of the diff shows no secrets.
- Minor, non-blocking: the path filters reference tests/canary_rollout.bats and the two scripts, which exist per the PR context; if the engine files are ever renamed the filters must move with them.
CI status
All quality gates green: Lint, ShellCheck, Agent Security Scan, Secret scan (gitleaks), CodeQL (Analyze actions), SonarCloud (quality gate passed, 0 new issues/hotspots), Dependency audit, CodeRabbit, and the new Canary-rollout Tests job itself (SUCCESS). Three dev-lead dispatch/ci-relay runs show CANCELLED — these are org agent-orchestration runs superseded by concurrency on rapid successive commits, not code-quality checks. Dependabot/pip/cargo/govulncheck/pnpm audits SKIPPED (not applicable ecosystems). mergeStateStatus BLOCKED reflects the pending org-leads review requirement this review satisfies.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fa2e467f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| paths: | ||
| - 'scripts/canary-rollout.sh' | ||
| - 'scripts/lib/canary-rollout.sh' | ||
| - 'tests/canary_rollout.bats' | ||
| - '.github/workflows/canary-rollout-tests.yml' |
There was a problem hiding this comment.
This paths list leaves out standards/canary-rings.json, even though tests/canary_rollout.bats reads that file as RINGS and contains dedicated assertions for its ring/gate shape. A PR that changes only the registry or gate knobs will therefore skip the workflow entirely, letting broken rollout metadata merge without the bats suite this workflow is meant to enforce; add the registry file to both the pull_request and push path filters.
Useful? React with 👍 / 👎.
Review — fix requested (cycle 1/3)The automated review identified the following issues. Please address each one: Findings to fixAutomated review — NEEDS HUMAN REVIEWRisk: MEDIUM SummaryAdds a single new workflow (.github/workflows/canary-rollout-tests.yml, +74/-0) that closes a real CI gap: the canary-rollout engine and its bats suite were relocated by #613/#624 without a runner. The workflow itself is well-built — action pinned by SHA (actions/checkout v7.0.0), Linked issue analysisNo linked closing issues. The PR body substantively documents the motivation (unenforced test suite since #613/#624) and gates follow-ups (#684 rebase, #668 increments 5–6) on this landing. Context is sound. Findings1. Unresolved review thread with a verified-valid finding (blocking auto-approval). chatgpt-codex-connector (P2) flagged that the path filters omit 2. Secret scan: 3. Prior reviews: CodeRabbit's earlier CHANGES_REQUESTED (persist-credentials) was addressed and it re-approved. A prior pr-review-agent approval exists at this same SHA, but the codex thread (created 21:17Z) post-dates it and remains open. CI statusAll green at head 3438351: Lint and bats (this new workflow, first run) ✓, ShellCheck ✓, CodeQL ✓, Secret scan (gitleaks) ✓, SonarCloud quality gate ✓, AgentShield ✓, CodeRabbit ✓, npm audit ✓. Cancelled dev-lead dispatch/ci-relay entries are concurrency supersessions; the latest runs succeeded. Language-specific audits skipped (no matching ecosystems). Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review. Additional tasks
The review cascade will automatically re-review after new commits are pushed. |
…) — restores the cancelled Canary Rollout job (#835) * fix(canary): cache run-list per (repo,workflow) + treat "no workflows" as empty (#819) The 4h sweep enumerated `gh run list --workflow <name>` once per sample window (candidate / baseline / downgrade / correctness) per agent per tier repo. Two defects turned the mode-B "hardening" (#810/#820) into a production regression that cancelled every Canary Rollout run since 2026-07-20 ~02:44Z: 1. A workflow with no runs on a repo makes `gh run list --workflow` exit non-zero with "could not find any workflows named …" — a PERMANENT condition. #810's wrapper classified it as `network-or-unknown` (transient) and retried it 6× with backoff, so every zero-run workflow across ~20 agent×workflow×repo combos burned its full retry budget → ~20-min sweep → job cancelled (exit 143). 2. When a window fails CLOSED (#820) `_run_json` returns empty; callers then ran `$(( executed + $(jq … <<< "$json" || echo 0) ))`, but jq on EMPTY stdin exits 0 printing nothing, so `|| echo 0` never fires → `executed + ` → bash "syntax error: operand expected" at the sample/health accumulators. Fix (#819, the structural cure): - `_repo_wf_runs_cached <repo> <wf>`: ONE unbounded `gh run list --workflow <wf>` per (repo, workflow), memoized in a FILE cache under `$_RUNS_CACHE_DIR` (exported by main() — callers invoke via command-substitution subshells, so an in-memory array would not survive). The candidate/baseline/downgrade windows now share one fetch, collapsing the secondary-rate-limit burst. - "could not find any workflows named …" → cached `[]`, returned immediately, never retried, never recorded as a #820 fetch outage. Genuine transport errors (403/5xx/auth/network) still retry with #810 backoff and fail CLOSED if sustained. - `_run_json` reduces to: guard → cached raw → LOCAL since-cut filter (inclusive, equivalent to the old server-side `--created ">=$since"`). - Arithmetic guard: `<<< "${json:-[]}"` at the three sample/health accumulators so a fail-closed empty payload counts 0 instead of crashing. `--workflow` is retained (deep per-workflow reach — no truncation on busy repos that a per-repo fetch capped at -L 1000 would suffer); only `--created` is dropped so one cache entry serves every window. Tests: +5 unit tests (per-(repo,wf) memoization; local since cut; unbounded fetch passes --workflow not --created; not-found→[] with no retry and no fail-flag; _cumulative_health survives an empty fail-closed payload without an arithmetic crash). Full suite 249 tests; the pre-existing 48 orchestrator env-artifact failures (no local git/gh fixtures; green in CI via #685) are unchanged. Closes #819. Fixes the #803/#811 canary cancellation regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017BggeTEBJLw6BV8kZrM6jD * fix(canary): register cleanup traps robustly across subshells (Gemini #835 review) - _repo_wf_runs_cached: register the errfile EXIT trap unconditionally (not only when tmpfiles is first declared). Command substitution forks a subshell that does not inherit the parent's traps, so the conditional form left subshell invocations without cleanup. Re-running `trap … EXIT` in the same shell is an idempotent reinstall. - main(): add an EXIT trap to reap the auto-created $_RUNS_CACHE_DIR. It lives in the parent shell, independent of the per-subshell errfile trap, so they never clobber. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017BggeTEBJLw6BV8kZrM6jD * fix(canary): hash the run-list cache key to prevent (repo,workflow) collisions (#835 CodeRabbit) The cache filename derived the key by char-substitution (non-[A-Za-z0-9._-] → "_"), which let two distinct workflows on the same repo collide — e.g. names differing only by a space vs a slash both mapped to one file, cross-contaminating their cached run history and therefore their gate health. Workflow display names carry spaces and em-dashes, so this is a real collision surface. Hash the raw "repo//workflow" key with sha1sum (fallback to the old substitution only if no hasher is present). +1 test proving "A B" and "A/B" get distinct cache entries. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017BggeTEBJLw6BV8kZrM6jD * fix(canary): use sha256 (not sha1) for the cache-key hash — clears SonarCloud S4790 SonarCloud flags sha1sum as a weak hash (S4790, CRITICAL) failing the new-code security gate, even though this is a cache-filename derivation, not a security context. sha256sum is not flagged, is equally present (coreutils), and is collision-free. Behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017BggeTEBJLw6BV8kZrM6jD * fix(canary): add shasum -a 256 fallback for macOS runners (#835 CodeRabbit) sha256sum is coreutils (Linux); macOS runners ship `shasum -a 256` instead. Try sha256sum, then shasum, before the substitution fallback — so the cache key stays a real hash (no collision surface) on any runner. This workflow runs on ubuntu-latest, but the fallback is trivial and portability-correct. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017BggeTEBJLw6BV8kZrM6jD --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Don Petry Bot <donpetry+bot@gmail.com>
2 participants
Why
The canary-rollout engine (
scripts/canary-rollout.sh,scripts/lib/canary-rollout.sh) and its 149+ testtests/canary_rollout.batswere relocated into.githubby #613/#624, but no CI runner came with them. Result: the suite has been unenforced — every #668 increment (#672/#676/#678, and #684 in flight) merged without its bats tests running in CI. Discovered while shepherding #684.What
A
Canary-rollout Testsworkflow mirroring the house*-tests.ymlpattern (seeauto-rebase-tests.yml): on any PR/push touching the engine, pure core, suite, or this workflow, it runsshellcheck -x+bats --print-output-on-failure tests/canary_rollout.bats.The point
This PR's own CI run is the verification — it will show, for the first time in Actions, whether the suite is green. If red, we've surfaced real breakage (or stale test stubs) that the missing runner had been hiding.
Follow-ups gated on this:
Summary by CodeRabbit