fix(canary): cache run-list per (repo,workflow) + not-found→empty (#819) — restores the cancelled Canary Rollout job - #835
Conversation
…" 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
don-petry
added
the
dev-lead:hands-off
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe canary rollout now caches workflow run listings per repository and workflow, filters time windows locally, preserves cached empty results for missing workflows, and hardens downstream counting for empty payloads. Tests cover cache reuse, key isolation, filtering, missing workflows, and fail-closed behavior. ChangesWorkflow Run Caching
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Rollout as _run_json
participant Cache as _repo_wf_runs_cached
participant GitHub as gh run list
participant Filter as jq
Rollout->>Cache: request repository/workflow runs
Cache->>GitHub: fetch uncached workflow runs
GitHub-->>Cache: runs or missing-workflow result
Cache-->>Rollout: cached runs or []
Rollout->>Filter: apply createdAt >= since
Filter-->>Rollout: filtered JSON array
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Restores reliability of the scheduled Canary Rollout sweep by reducing gh run list call volume per evaluation and preventing permanent “workflow not found / no runs” conditions from triggering retry storms and eventual job cancellation.
Changes:
- Added a file-backed per-
(repo, workflow)memoization layer for run-list fetches and applied thesincecutoff locally. - Treated “could not find any workflows…” / “no workflows…”
gherrors as a valid empty result ([]) with no retry and no fail-closed flagging. - Hardened arithmetic accumulators against empty fail-closed payloads and added focused Bats coverage for the new behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/canary-rollout.sh | Adds per-(repo,workflow) run-list caching, not-found→[] handling, local since filtering, and arithmetic guards to prevent sweep cancellation/crashes. |
| tests/canary_rollout.bats | Adds unit tests covering cache behavior, local since filtering, unbounded fetch args, not-found→[] behavior, and the arithmetic crash regression guard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a file-backed caching mechanism (_repo_wf_runs_cached) for GitHub workflow runs to mitigate secondary rate-limiting issues and handle non-existent workflows gracefully without unnecessary retries. It also shifts date filtering locally using jq and adds robust arithmetic guards to prevent syntax errors during failures. The review feedback highlights two important improvements regarding Bash trap handling: ensuring the EXIT trap for temporary files is registered outside the conditional check so subshells always execute it, and adding an EXIT trap in main() to clean up the auto-created cache directory.
…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
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #835 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
![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 `@scripts/canary-rollout.sh`:
- Around line 434-446: Update _repo_wf_runs_cached cache-key generation to
derive the filename from a cryptographic hash of the raw repo/workflow key
instead of character substitution. Preserve the existing cache directory, file
lookup, and cached-output behavior while ensuring distinct (repo, workflow)
pairs cannot collide.
🪄 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: 4704d04a-7626-485d-ba16-7d183dab2cff
📒 Files selected for processing (2)
scripts/canary-rollout.shtests/canary_rollout.bats
…ollisions (#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
…narCloud 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
don-petry
left a comment
don-petry
left a comment
There was a problem hiding this comment.
All required checks are green on the head commit (Lint-and-bats, ShellCheck, CodeQL, SonarCloud, Secret scan); dev-lead correctly skipped (hands-off). The prior readiness eval hit skip-checks-pending while CI was mid-rerun. Re-triggering the readiness re-evaluation.
|
@donpetry-bot all checks are green now and the blocking review is cleared — please run a fresh review. State on head |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/canary-rollout.sh (2)
538-538: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winApply the empty-payload fallback consistently.
The PR updates multiple
jqinvocations to use${json:-[]}to prevent parse errors when the payload is empty (e.g., lines 537, 712, 716). However, the followingjqexecutions still use<<< "$json". Whilejqparse errors might be swallowed by2>/dev/null, applying${json:-[]}here ensures consistent, error-free input evaluation across all JSON processing.
scripts/canary-rollout.sh#L538-L538: Update<<< "$json"to<<< "${json:-[]}".scripts/canary-rollout.sh#L727-L727: Update<<< "$json"to<<< "${json:-[]}".🤖 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 `@scripts/canary-rollout.sh` at line 538, Update the jq invocations at scripts/canary-rollout.sh lines 538-538 and 727-727 to use the empty-array fallback `${json:-[]}` as the here-string input instead of `$json`, preserving the existing jq filters and error handling.
458-464: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winSubshell
EXITtrap prematurely deletes the parent shell's temporary files.Because
_repo_wf_runs_cachedis executed inside a command-substitution subshell (e.g., at line 521), it inherits thetmpfilesarray from the parent shell. When the subshell exits, the trap at line 464 executes and deletes every file currently in its copy oftmpfiles—which includes all temporary files registered by the parent shell prior to the fork, destroying them while the parent still needs them.To prevent data loss for the parent shell, avoid mutating the globally inherited array inside subshells. Since
errfileis already explicitly cleaned up on all return paths (lines 468, 480, 493), you can safely rely on those explicitrmcalls, or scope the trap exclusively to this file.🛠️ Proposed fix scoping the trap to the local file
- declare -p tmpfiles &>/dev/null || declare -g -a tmpfiles=() - tmpfiles+=("$errfile") - # Register the cleanup in THIS (sub)shell: command substitution forks a subshell that - # does NOT inherit the parent's traps, so registering only on first declaration of - # tmpfiles would leave every subshell invocation without one. Re-running `trap … EXIT` - # in the same shell just reinstalls the same idempotent handler. - trap 'rm -f "${tmpfiles[@]+"${tmpfiles[@]}"}"' EXIT + # Scope the trap to just this file to prevent subshells from deleting the parent's temp files. + # We append to any existing trap so it's safe even if called directly in the parent shell. + local current_trap + current_trap="$(trap -p EXIT | sed -E "s/^trap -- '(.*)' EXIT$/\1/")" + trap "rm -f '$errfile'; $current_trap" EXIT🤖 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 `@scripts/canary-rollout.sh` around lines 458 - 464, Update the cleanup logic in the function containing the tmpfiles registration so command-substitution subshells cannot remove the parent shell’s inherited temporary files. Scope the EXIT trap to clean up only the current errfile, or avoid registering it when the file is already explicitly removed on all return paths; preserve the existing parent-shell tmpfiles cleanup behavior.
🤖 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 `@scripts/canary-rollout.sh`:
- Around line 444-454: Update the keyhash derivation near the cachef assignment
to try shasum -a 256 after sha256sum fails, before using the
character-substitution fallback. Preserve the existing repo/workflow key input
and ensure substitution is used only when neither hashing command produces a
hash.
---
Outside diff comments:
In `@scripts/canary-rollout.sh`:
- Line 538: Update the jq invocations at scripts/canary-rollout.sh lines 538-538
and 727-727 to use the empty-array fallback `${json:-[]}` as the here-string
input instead of `$json`, preserving the existing jq filters and error handling.
- Around line 458-464: Update the cleanup logic in the function containing the
tmpfiles registration so command-substitution subshells cannot remove the parent
shell’s inherited temporary files. Scope the EXIT trap to clean up only the
current errfile, or avoid registering it when the file is already explicitly
removed on all return paths; preserve the existing parent-shell tmpfiles cleanup
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: 25bf5ff8-0927-4bd7-a854-4db57b17ffd0
📒 Files selected for processing (2)
scripts/canary-rollout.shtests/canary_rollout.bats
|
@donpetry-bot CodeRabbit's re-review is complete and green; all checks pass now. Please run the review. |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
…abbit) 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
|
@donpetry-bot all checks green on |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: eeb7c38619c661111e4108116c9cfbcea3977324
Review mode: triage-approved (single reviewer)
Summary
Confirmation review after triage clearance (triage-approved mode). PR #835 fixes the production-down canary-rollout regression (job cancelled on every scheduled run since 2026-07-20): (1) a retry storm — the permanent "could not find any workflows named …" condition was classed as transient and retried 6× with backoff across ~20 repo×workflow combos, blowing the sweep past the job budget; (2) an arithmetic crash when a fail-closed window left json empty (executed + operand error). The fix introduces _repo_wf_runs_cached (one unbounded gh run list per (repo,workflow), memoized in a sha256-keyed file cache under $_RUNS_CACHE_DIR), classifies not-found as a cached [] (never retried, never counted as a #820 outage), applies the since cut locally via jq, and guards all arithmetic with ${json:-[]}. Genuine transport errors still retry with #810 backoff and fail CLOSED, preserving the promotion-safety contract. Six new bats tests cover memoization, cache-key collision resistance (the earlier CodeRabbit finding — now resolved with a dedicated test), local since-filtering, unbounded fetch, not-found short-circuit, and the arithmetic guard. Triage's low-risk assessment is confirmed as sound; I classify it MEDIUM per taxonomy (non-trivial logic in org automation), which permits approval.
Linked issue analysis
Closes #819 (canary-rollout: collapse run-list enumeration to a cached per-repo fetch). Acceptance criteria met: per-evaluation gh run list calls collapse from one per (repo×workflow×window) to one per (repo×workflow) with a test proving two windows share one fetch; _run_json's public contract/return shape is unchanged (callers and #810 backoff untouched); fail-closed behavior preserved (sustained transport failure returns non-zero); existing bats stay green and new tests cover the cache + local filter. Note: the PR fetches per (repo,workflow) rather than the issue's literal per-repo suggestion — a reasonable refinement that achieves the same rate-limit collapse while also curing the not-found retry storm.
Findings
No blocking findings.
- Security: no secrets, credentials, or injection surfaces; all expansions quoted; cache dir via
mktemp -dwith parent-shell EXIT reaping; sha256 used only for filename derivation (correctly documented as non-security). Gitleaks, CodeQL, Agent Security Scan, and SonarCloud all green. Therun_secret_scanningMCP tool was not available in this environment — noted per protocol; the gitleaks CI check covers secret detection. - Correctness: subshell trap re-registration for
errfileis correctly reasoned (command substitution does not inherit parent traps); parent-shell_RUNS_CACHE_DIRtrap and per-subshell errfile traps live in different shells and cannot clobber each other; local jqsincecut is inclusive (>=), matching the old server-side--created ">=$since"semantics;${json:-[]}guards fix the operand-expected crash. - Review history: both CodeRabbit CHANGES_REQUESTED rounds are resolved (final state APPROVED); the collision-surface finding is fixed with a hashed cache key plus a regression test. Gemini's advisory about reaping the auto-created cache dir is already implemented in
main(). Zero unresolved review threads. - Minor (non-blocking): the not-found substring match ("no workflows") is slightly broad, and the
-L 1000unbounded fetch caps history at 1000 runs per (repo,workflow) — both acceptable trade-offs for this fix, behavior-compatible with the prior fetch limit.
CI status
All required checks green on eeb7c38619c661111e4108116c9cfbcea3977324: Lint, Lint-and-bats, ShellCheck, CodeQL, Analyze (actions), SonarCloud (quality gate passed, 0 new issues), Secret scan (gitleaks), Agent Security Scan, npm audit, agent-shield — all SUCCESS. dev-lead ci-relay and ecosystem audits appropriately SKIPPED. Mergeable; branch is BEHIND main (auto-rebase will handle).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
|
@donpetry-bot the PR is CLEAN and approved (both donpetry-bot and CodeRabbit), all required checks green on |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: eba9680bf39035157e1d0631d4b6622fe0d122e1
Review mode: triage-approved (single reviewer)
Summary
Restores the cancelled Canary Rollout job by fixing two regressions from the #820/#821 fail-closed hardening: (1) a not-found workflow ('could not find any workflows named …') was classed as transient and retried 6× with backoff across ~20 agent×workflow×repo combos, blowing the sweep past its budget (exit 143); it is now treated as a permanent empty result, cached as [], never retried, and never recorded as a fetch outage. (2) jq on an empty payload exits 0 printing nothing, so '|| echo 0' never fired and bash arithmetic hit 'operand expected'; callers now use ${json:-[]}. Structurally, run-list fetches collapse to ONE unbounded 'gh run list' per (repo,workflow), memoized in a file cache (subshell-safe, sha256-hashed keys so 'A B' and 'A/B' workflows cannot collide), with the since-window cut applied locally — eliminating the secondary-rate-limit burst. Genuine transport errors still retry with #810 backoff and fail CLOSED. Six new bats tests cover memoization, key-collision safety, local since cut, unbounded fetch, not-found short-circuit, and the arithmetic guard. Since the prior review (eeb7c38), the only change is a merge of main (PR #834) — the PR's own diff is unchanged.
Linked issue analysis
Closes #819 (re-split of #811 / #803: collapse run-list enumeration to a cached fetch). Acceptance criteria met: call count materially reduced (memoization test proves two windows share one gh call); same decisions on good data (_run_json return shape and local since-cut match the old server-side '--created >=since' semantics, with a dedicated test); fails closed on genuine fetch errors (non-zero return preserved, test asserts no arithmetic crash on the fail-closed path); existing bats stay green (Lint and bats ✅) plus 6 new tests for the cache and local filter. The PR caches per (repo,workflow) rather than the issue's suggested per-repo granularity — a reasonable refinement that achieves the same rate-limit collapse while keeping payloads bounded.
Findings
No blocking findings.
- Security: No secrets, credentials, or injection surfaces. Cache filenames are sha256-hashed (injective, no path-traversal surface); mktemp -d cache dir with a guarded 'rm -rf "${_RUNS_CACHE_DIR:-}"' EXIT trap; gh args properly quoted. Secret scan (gitleaks) ✅. The run_secret_scanning MCP tool was not available in this environment — noted, not fabricated.
- Correctness: The parent-shell cache-dir EXIT trap and the per-subshell errfile trap live in different shells and cannot clobber each other, as the code comments prove out. Cached '[]' is non-empty so [ -s ] cache hits work for zero-run workflows. jq-on-garbage falls back to [] only after a successful gh call, so fail-closed is not weakened.
- Minor (non-blocking): If _run_json were ever called from the parent shell (not command substitution), its 'tmpfiles' EXIT trap would replace main()'s cache-dir trap, leaking the temp dir — harmless on ephemeral runners, and current callers all use command substitution.
- Prior review threads: CodeRabbit's cache-key collision concern (char-substitution mapping 'A B' and 'A/B' to the same file) was fixed with sha256 keys plus a regression test; CodeRabbit subsequently approved.
CI status
All checks green on eba9680bf39035157e1d0631d4b6622fe0d122e1: Lint ✅, Lint and bats ✅, ShellCheck ✅, CodeQL ✅, Agent Security Scan ✅, AgentShield ✅, Secret scan (gitleaks) ✅, SonarCloud quality gate ✅, npm audit ✅, CodeRabbit ✅. Skipped checks (pnpm/cargo/pip audit, govulncheck, dependabot, ci-relay) are ecosystem-not-present or hands-off skips. Mergeable: CLEAN.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at eba9680.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: eba9680bf39035157e1d0631d4b6622fe0d122e1
Review mode: triage-approved (single reviewer)
Summary
Fixes the production-down Canary Rollout cancellation (#819) with two targeted changes: (1) a new _repo_wf_runs_cached helper that memoizes one unbounded 'gh run list' per (repo,workflow) in a file-backed cache and classifies gh's permanent 'could not find any workflows named …' error as a cached empty result instead of retrying it 6x (the retry storm that blew sweeps to ~20 min and got the job cancelled); (2) ${json:-[]} arithmetic guards in _tier_sample/_cumulative_health so a fail-closed empty payload can no longer cause an 'operand expected' crash. _run_json becomes a thin wrapper applying the since-cut locally via jq, preserving its public contract and fail-closed behavior. Six new bats tests cover memoization, hash-key collision-freedom, local since filtering, unbounded fetch args, not-found no-retry/no-outage-flag, and the arith guard.
Linked issue analysis
Linked issue #819 (collapse run-list enumeration to a cached fetch, fail closed, keep _run_json's contract, add cache tests) is substantively addressed: per-window enumeration collapses to one read per (repo,workflow), the return shape and fail-closed semantics of _run_json are unchanged, and dedicated bats tests exercise the cache and local filter. All acceptance criteria met.
Findings
No blocking findings. Verified: fail-closed preserved (genuine transport errors still retry with #810 backoff and return non-zero); not-found detection is narrowly scoped and fails in the safe direction if gh ever rewords the message; sha256 cache keys are injective with a sanitized fallback (addresses CodeRabbit's collision concern, with a regression test); parent-shell rm -rf trap and per-subshell errfile traps live in different shells; no injection surface (values quoted to gh, jq via --arg, filenames hashed); -L 1000 newest-first page + local since-cut reproduces the old server-side window. Two cosmetic nits, non-blocking: the retry-exhaustion error message still says '_run_json:' though the code now lives in _repo_wf_runs_cached; a truncated cache write (disk full) would be served and jq-degraded to [] — negligible on hosted runners. Secret scanning MCP tool unavailable in this run; gitleaks CI check is green.
CI status
All checks green on eba9680: Lint, Lint and bats, ShellCheck, CodeQL, Analyze (actions), SonarCloud (quality gate passed), Secret scan (gitleaks), Agent Security Scan, npm audit, AgentShield, CodeRabbit (SUCCESS). Remaining checks skipped by design (ecosystem-specific audits, dev-lead hands-off). CodeRabbit approved after its change requests were addressed; reviewDecision is APPROVED; no unresolved threads or unanswered human questions.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: eba9680bf39035157e1d0631d4b6622fe0d122e1
Review mode: triage-approved (single reviewer)
Summary
Confirmation review (triage-approved mode) of PR #835, which restores the cancelled Canary Rollout job (production-down since 2026-07-20 ~02:44Z). Two fixes: (1) a new _repo_wf_runs_cached helper memoizes one unbounded 'gh run list' per (repo,workflow) in a file-backed cache (sha256-hashed keys, subshell-safe EXIT traps) and classifies gh's permanent 'could not find any workflows named …' error as a cached empty [] — never retried, never flagged as a fetch outage — killing the 6x-retry storm that blew sweeps past the budget (exit 143); (2) arithmetic accumulators now use ${json:-[]} so a fail-closed empty payload counts 0 instead of crashing with 'operand expected'. Genuine transport errors (403/5xx/auth/network) still retry with #810 backoff and fail CLOSED, preserving the #820 safety posture. 114 lines of new bats tests cover memoization, cache-key collision-freedom ('A B' vs 'A/B'), local since-filtering, unbounded fetch, not-found-is-empty, and the arithmetic guard. The triage assessment holds; this exact SHA was also approved by prior single-reviewer passes and by CodeRabbit.
Linked issue analysis
Linked issue #819 (collapse run-list enumeration to a cached fetch, keep _run_json's contract, fail closed, add tests) is substantively addressed: the cache is keyed per (repo,workflow) rather than per-repo — a reasonable refinement the PR body justifies (the burst came from per-window re-fetches; candidate/baseline/downgrade/correctness windows now share one fetch). _run_json's public contract and return shape are unchanged, genuine failures still fail CLOSED, and dedicated bats tests prove the reduced call count.
Findings
No blocking findings. Review threads (Gemini's subshell-trap concern, CodeRabbit's cache-key collision and shasum-fallback asks) are all resolved and addressed in the diff — the EXIT trap is reinstalled per subshell, and cache keys use sha256sum with a shasum -a 256 fallback. The mcp secret-scanning tool was unavailable in this run; the gitleaks CI check is green and the diff introduces no credentials, auth changes, or new dependencies. Minor non-blocking note: on a cache hit the entry is served via cat without a trailing newline — harmless since all callers use command substitution.
CI status
All checks green on eba9680bf39035157e1d0631d4b6622fe0d122e1: Lint, Lint and bats, ShellCheck, CodeQL (actions), SonarCloud quality gate, Secret scan (gitleaks), Agent Security Scan, AgentShield, npm audit. dev-lead ci-relay and ecosystem audits skipped as expected (hands-off label / no matching ecosystem). CodeRabbit approved; no unresolved review threads; no unanswered human questions.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Why (production-down regression)
The Canary Rollout job has been cancelled on every scheduled run since 2026-07-20 ~02:44Z — it was
successat 2026-07-19 21:39Z (right after #807) and regressed the moment #821 (fail-closed sync-issues, #820) merged at 01:44Z. Ring promotion is fully halted (no evaluate/promote/gates). Two defects, both introduced by the mode-B "hardening":gh run list --workflow <name>exit non-zero withcould not find any workflows named …— a permanent condition. [#803 split 1/2] _run_json: surface real error + exponential jittered backoff #810's wrapper classed itnetwork-or-unknown(transient) and retried it 6× with backoff across ~20 agent×workflow×repo combos → ~20-min sweep → job cancelled (exit 143). Observed in run 29765836983._run_jsonreturns empty; callers ran$(( executed + $(jq … <<< "$json" || echo 0) )), but jq on empty stdin exits 0 printing nothing, so|| echo 0never fires →executed +→syntax error: operand expected(lines 491/666 in the run log).What (the #819 structural cure)
_repo_wf_runs_cached <repo> <wf>— one unboundedgh run list --workflow <wf>per(repo, workflow), memoized in a file cache under$_RUNS_CACHE_DIR(exported bymain(); callers invoke via command-substitution subshells, so an in-memory array wouldn't survive). Candidate/baseline/downgrade/correctness windows now share one fetch → the secondary-rate-limit burst collapses.could not find any workflows named …→ cached[], returned immediately, never retried, never recorded as a [#811 re-split 2/2] canary-rollout: fail-closed sync-issues (never drop regression tracking on partial data) #820 fetch outage. Genuine transport errors (403/5xx/auth/network) still retry with [#803 split 1/2] _run_json: surface real error + exponential jittered backoff #810 backoff and fail CLOSED if sustained._run_jsonreduces to guard → cached raw → local since-cut filter (inclusive, equivalent to the old server-side--created ">=$since").<<< "${json:-[]}"at the three sample/health accumulators.--workflowis retained (deep per-workflow reach — a per-repo fetch capped at-L 1000would truncate the window on busy repos); only--createdis dropped so one cache entry serves every window.Tests
+5 unit tests: per-
(repo,wf)memoization (two windows → one gh call); local since cut; unbounded fetch passes--workflownot--created; not-found→[]with no retry and no fail-flag;_cumulative_healthsurvives 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 — 0 new.Closes #819. Fixes the #803/#811 canary cancellation regression.
Summary by CodeRabbit
New Features
workflow runlistings (keyed by repo + workflow) to reduce repeated GitHub queries.Bug Fixes
sincefiltering locally to run objects for consistent time-window results.Tests
sincefiltering, fail-closed empty handling, and health output robustness.