feat: implement issue #688 — canary #668 increment 6: SUSPECT→PRE_EXISTING auto-downgrade (decide_suspect_downgrade) — closes #668#689
Conversation
…STING auto-downgrade (decide_suspect_downgrade) — closes #668
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 26 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 (3)
📝 WalkthroughWalkthroughAdds opt-in SUSPECT auto-downgrade based on candidate and baseline failure rates, with thin-baseline safeguards. The resulting downgrade state flows through evaluation, promotion, blocker issue generation, and comprehensive Bats coverage. ChangesSUSPECT auto-downgrade
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CanaryRollout
participant DecisionHelper
participant BlockerIssue
CanaryRollout->>CanaryRollout: classify failure as SUSPECT
CanaryRollout->>CanaryRollout: compute candidate and baseline rates
CanaryRollout->>DecisionHelper: decide_suspect_downgrade
DecisionHelper-->>CanaryRollout: DOWNGRADE or HOLD
CanaryRollout->>BlockerIssue: render downgrade status and rate metrics
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 (no-changes)No changes were needed for this PR. |
![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 implements a data-driven auto-downgrade mechanism (SUSPECT to PRE_EXISTING) for canary rollouts, specifically opting in the dev-lead workload-timeout failure class. It introduces new helper functions to evaluate candidate-vs-baseline failure rates, updates the orchestrator's state evaluation and blocker reporting, and adds comprehensive test coverage. The review feedback focuses on improving the robustness of the Bash scripts, including guarding against syntax errors from empty command substitutions or unassigned variables, using jq's optional operator to prevent null-indexing errors, handling trailing newlines and carriage returns during TSV parsing, and declaring local variables properly.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 8798c1ca49445e71fcd3aaed1023dfce9e7ba4da
Review mode: triage-approved (single reviewer)
Summary
Implements #668 increment 6: data-driven SUSPECT→PRE_EXISTING auto-downgrade. Pure core (decide_suspect_downgrade) with tiny-n guard and inclusive base+margin boundary; orchestrator applies it only for opted-in suspect classes after classify_failure returns SUSPECT (failure-class variant only); registry opt-in scoped to dev-lead workload-timeout; sync-issues renders a report-only PRE_EXISTING blocker with the rate table and no needs-human. 12 new bats cover every branch incl. regression guard for un-flagged classes. All substantive CI green. The 5 unresolved gemini bot threads were each verified against the code: false positives or hardening-only; dev-lead no-changes disposition concurs.
Linked issue analysis
Closes #688 (final increment of the #668 series). All five scope items delivered:
- Pure core —
decide_suspect_downgrade <cand> <base> <base_sample> <knobs_json>inscripts/lib/canary-rollout.sh: HOLD on thin baseline (base_sample < min_baseline_sample, checked first), DOWNGRADE whencand <= base + margin(inclusive), else HOLD. Non-numeric/absent knobs sanitized to 0. Side-effect-free;classify_failuresignature untouched. - Orchestrator — downgrade evaluated only when
triage=SUSPECT && mix_shift="-"(correctness-variant SUSPECT excluded, per design). Candidate window (since cut) vs trailing baseline window (SOAK_WINDOW_DAYS/baseline_window_days, before cut), rates viaround_div(matched*1000/executed), reusing_run_json/_run_signature/benign_match. Emits the required::notice::with the rate comparison. - Registry —
auto_downgrade {min_baseline_sample:10, margin_permille:100}on dev-leadworkload-timeoutonly; em-dash escapes preserved. Scope-guard bats asserts exactly one opted-in class org-wide. - sync-issues — auto-downgraded blocker renders the PRE_EXISTING note + candidate-vs-baseline rate table, no
needs-human; bats asserts the label is absent from issue ops. - Tests — unit (DOWNGRADE / worse / tiny-n / boundary / min-boundary / empty-knobs) + orchestrator (downgrade, worse-stays-SUSPECT, thin-baseline, no-opt-in regression guard, --allow-pre-existing advance, sync-issues blocker). Acceptance criteria met: lib pure, shellcheck clean, bats green under
Canary-rollout Tests.
Findings
No blocking findings. The 5 unresolved review threads (gemini-code-assist bot) were independently verified against the shipped code:
- HIGH,
scripts/canary-rollout.sh:326(empty$(jq)→ arithmetic syntax error): already mitigated — the substitution carries|| echo 0, andjq '[…]|length'cannot emit empty output on success. False positive. - HIGH,
scripts/canary-rollout.sh:767(empty$ce/$be→ unary-operator error): not reachable —_suspect_class_countsunconditionally echoes"$matched $executed"(both initialized to 0); an upstream failure aborts at thereadunderset -euo pipefailrather than yielding empty variables. - MEDIUM (
jqnull-indexing on.auto_downgrade): guarded byselect(.auto_downgrade != null)plus// 0defaults, on a repo-controlled registry. Hardening-only. - MEDIUM (TSV last-line/CRLF handling): the orchestrator loop already uses
|| [ -n "$dwf" ]and strips CRs; the inner loop consumes jq-generated output (newline-terminated, no CRs). Hardening-only. - LOW (
local count): references a variable that does not exist in the shipped code.
The dev-lead review-changes pass concluded no-changes for these threads; this review concurs with that disposition.
Minor non-blocking observation: the downgrade loop breaks on the first opted-in class whose rates compare no-worse, while the SUSPECT verdict itself is not attributed per-class — acceptable at current scope (exactly one opted-in class, enforced by the scope-guard test); worth revisiting if more classes opt in.
Secret scanning: the run_secret_scanning MCP tool was not available in this run; the gitleaks CI check passed. No secrets, credentials, or auth-surface changes in the diff.
CI status
All substantive checks SUCCESS at 8798c1c: ShellCheck, Lint, Agent Security Scan, Secret scan (gitleaks), Canary-rollout Tests (lint + bats, incl. the 12 new tests), CodeQL (actions), SonarCloud quality gate (0 new issues, 0 hotspots), npm audit, CodeRabbit status. Ecosystem audits (pip/cargo/go/pnpm) SKIPPED — not applicable. Two QUEUED jobs are this auto-review pipeline's own dispatch. mergeable=MERGEABLE; mergeStateStatus=BLOCKED pending this review.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
don-petry
dismissed stale reviews from coderabbitai[bot] and donpetry-bot
via
255c947
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #689 |
|
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. |
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/canary-rollout.sh (1)
1216-1230: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove
needs-humanwhen an existing blocker is downgraded.A blocker first created as
SUSPECTretainsneeds-human: the update path at Line 1251 only conditionally adds labels and never removes old ones. Its body then says “no human needed,” but routing remains incorrect. Remove the label for updatedPRE_EXISTINGblockers and cover this state transition.Proposed fix
- { [ "$triage" = "REGRESSION" ] || [ "$triage" = "SUSPECT" ]; } && gh issue edit "$num" --repo "$ISSUE_REPO" --add-label needs-human >/dev/null 2>&1 || true + if [ "$triage" = "REGRESSION" ] || [ "$triage" = "SUSPECT" ]; then + gh issue edit "$num" --repo "$ISSUE_REPO" --add-label needs-human >/dev/null 2>&1 || true + else + gh issue edit "$num" --repo "$ISSUE_REPO" --remove-label needs-human >/dev/null 2>&1 || true + fi🤖 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 1216 - 1230, The blocker update flow around _issue_find and the PRE_EXISTING state must remove the existing needs-human label when a blocker is downgraded and the body indicates no human action is required. Update the label-handling logic near the referenced update path to explicitly remove needs-human for this transition, while preserving conditional label additions for other states, and add coverage for the SUSPECT-to-downgraded PRE_EXISTING transition.
🤖 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 328-337: Update the baseline signature handling around
_run_signature and the matched-count loop so failed signature lookups are
distinguished from genuine non-matches instead of being skipped. Track
incomplete baseline class evidence when an executed baseline run cannot be read,
and propagate that state to the rollout decision so it returns HOLD rather than
calculating a potentially understated dg_base_rate or downgrading SUSPECT.
- Around line 291-303: Preserve the matched suspect failure class and run
attribution through triage instead of reducing it to the aggregate cum_suspect
flag. Update the downgrade flow around _suspect_downgrade_patterns and
decide_suspect_downgrade so a candidate is downgraded only when every blocking
failure is covered by an eligible class whose comparison passes; unrelated
failures must keep it blocked. Add a regression test covering a mixed
baseline-like timeout and unrelated failed run.
---
Outside diff comments:
In `@scripts/canary-rollout.sh`:
- Around line 1216-1230: The blocker update flow around _issue_find and the
PRE_EXISTING state must remove the existing needs-human label when a blocker is
downgraded and the body indicates no human action is required. Update the
label-handling logic near the referenced update path to explicitly remove
needs-human for this transition, while preserving conditional label additions
for other states, and add coverage for the SUSPECT-to-downgraded PRE_EXISTING
transition.
🪄 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: 911522a9-a166-4020-8332-1243ca0c48d8
📒 Files selected for processing (4)
scripts/canary-rollout.shscripts/lib/canary-rollout.shstandards/canary-rings.jsontests/canary_rollout.bats
Superseded by automated re-review at
|
Superseded by automated re-review at
|
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
|
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 35794ed845fff1b2cc6b1b4c4e30be59d529f2b8
Review mode: triage-approved (single reviewer)
Summary
Re-review after cycle-2 fix-requested. The fix commit 35794ed resolves all three prior findings with targeted code changes plus dedicated regression tests (+170 bats lines). All 7 review threads (gemini + CodeRabbit) are resolved, the CodeRabbit CHANGES_REQUESTED review was dismissed by the new push, and all CI checks are green at head. No new issues introduced by the fix commit.
Linked issue analysis
Closes #688 (#668 increment 6: SUSPECT→PRE_EXISTING auto-downgrade). Scope was verified as substantively delivered in the initial review at 8798c1c (pure decide_suspect_downgrade core, orchestrator rate comparison, registry opt-in scoped to dev-lead workload-timeout, sync-issues rendering, bats coverage). The fix commit hardens that implementation per review findings without scope creep: only scripts/canary-rollout.sh, scripts/lib/canary-rollout.sh, and tests/canary_rollout.bats changed since the prior review.
Findings
All prior findings resolved; no new issues.
- RESOLVED (was MAJOR) — needs-human not removed on auto-downgrade: the sync-issues UPDATE path (scripts/canary-rollout.sh:1276) now has an explicit else-branch that removes the needs-human label when triage is not REGRESSION/SUSPECT. New bats test exercises the SUSPECT→downgraded-PRE_EXISTING UPDATE transition and asserts label removal.
- RESOLVED (was MAJOR) — failed signature lookup understated rates, biasing toward downgrade: _run_signature now distinguishes a lookup failure (exit 1, cached via a sentinel) from a genuine empty signature; _suspect_class_counts emits an incomplete count; the orchestrator skips DOWNGRADE (keeps SUSPECT/HOLD) when incomplete>0 on either the candidate or baseline window — fail-closed on both windows, stricter than the finding required. New bats test simulates a gh run view failure and asserts the gate stays SUSPECT.
- RESOLVED (was MINOR) — aggregate cum_suspect lost per-failure attribution: _cumulative_health/_failure_suspect now count suspect-matched failures, and downgrade is only attempted when cum_suspect >= cum_fail (every blocking failure attributed to an opted-in suspect class); classify_failure updated accordingly. New mixed-failure bats test (workload-timeout + unrelated failure) asserts NOT downgraded.
Verified shell details of the fix: the || [ -n ... ] && continue guard has correct left-associative precedence, and the errexit-sensitive gh run view failure path is wrapped in || { }. ShellCheck passed in CI.
Secret scanning: run_secret_scanning MCP tool not available in this run; gitleaks CI check passed. No secrets, auth, or credential surfaces in the diff.
CI status
All checks green at 35794ed: ShellCheck, Lint, Agent Security Scan, Secret scan (gitleaks), Canary-rollout Tests (lint + bats incl. the new regression tests), CodeQL (actions), SonarCloud quality gate (0 new issues, 0 hotspots), npm audit, AgentShield, CodeRabbit status SUCCESS. Ecosystem audits (pip/cargo/go/pnpm) SKIPPED — not applicable. mergeable=MERGEABLE; mergeStateStatus=BLOCKED pending this review.
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: 35794ed845fff1b2cc6b1b4c4e30be59d529f2b8
Review mode: triage-approved (single reviewer)
Summary
Increment 6 of #668: data-driven SUSPECT→PRE_EXISTING auto-downgrade. Adds a pure decide_suspect_downgrade core (lib), orchestrator rate-comparison wiring gated per suspect class via a new auto_downgrade registry knob (dev-lead workload-timeout only), needs-human label removal on downgrade in sync-issues, and 15 new bats tests covering boundaries, tiny-n guard, mixed-failure attribution, fail-closed incomplete evidence, and increment-2 regression guards. All CI green, no unresolved threads, implementation matches the issue #688 spec exactly.
Linked issue analysis
Closes #688 (increment 6 of the #668 series). Every scope item is delivered: (1) pure decide_suspect_downgrade in scripts/lib/canary-rollout.sh with the tiny-n guard (HOLD when base_sample < min_baseline_sample) and inclusive no-worse threshold (cand <= base + margin); (2) orchestrator applies the downgrade only AFTER classify_failure returns SUSPECT, only for classes carrying auto_downgrade, with two extra fail-closed guards beyond the spec (all failures must be suspect-attributed, and any failed signature lookup HOLDs); (3) registry opt-in for dev-lead workload-timeout only, with a scope-guard test enforcing exactly one flagged class; (4) sync-issues renders the rate-comparison table in PRE_EXISTING blockers and removes stale needs-human labels on the update path; (5) bats cover every acceptance case including the boundary at exactly base+margin and the class-without-auto_downgrade byte-identical guard.
Findings
No blocking findings.
Verified specifically:
- _run_signature now returns exit 1 on lookup failure (with a $'\x01' cache sentinel). All pre-existing callers remain safe under set -euo pipefail: _failure_benign/_failure_suspect are only invoked in if/&& condition contexts (set -e suppressed; empty sig falls through to the existing fail-closed return 1), _blocker_evidence is called with || true, and the new _suspect_class_counts explicitly captures the exit code to count incomplete evidence.
- classify_failure's changed comparison ([ "$suspect" -gt 0 ]) is safe: the third arg defaults to 0, and prior 0|1 callers behave identically (3-arg signature back-compatible).
- The '[ cmi -gt 0 ] || [ bei -gt 0 ] && continue' guard is correct shell (left-associative; continue runs iff either count is nonzero) and does not trip set -e.
- jq filter interpolation of $before/$since uses only internally-derived ISO timestamps (tag dates from the org-controlled host repo), not untrusted input.
- Secret scan: run_secret_scanning MCP tool unavailable in this session; gitleaks CI check is green and the diff contains only test-stub placeholders.
Non-blocking observation: in _frontier_state the downgrade loop breaks on the first class returning DOWNGRADE and re-triages the whole verdict PRE_EXISTING. If multiple suspect classes ever carry auto_downgrade and a candidate has failures across classes, a no-worse class could clear a hold that a worse class should keep. Currently unreachable — the registry has exactly one flagged class and a scope-guard bats test enforces that — but worth revisiting before flagging additional classes.
CI status
All required checks green at 35794ed: ShellCheck ✓, Lint and bats ✓, CodeQL ✓, SonarCloud quality gate ✓, Secret scan (gitleaks) ✓, Agent Security Scan ✓, AgentShield ✓, npm audit ✓ (language-specific audits skipped as not applicable). CodeRabbit state SUCCESS; its earlier changes-requested review was dismissed after the dev-lead fix-reviews cycle applied the requested changes.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
2 participants
Closes #688
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
New Features
Bug Fixes