feat: implement issue #675 — canary #668 increment 2: SUSPECT triage — suspect_failure_classes + classify_failure third verdict#676
Conversation
…— suspect_failure_classes + classify_failure third verdict
|
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: 44 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 (2)
📝 WalkthroughWalkthroughAdds optional suspect failure classes and a ChangesSUSPECT Canary Triage
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CanaryEvaluation
participant HealthCalculation
participant FailureTriage
participant Promotion
participant BlockerIssues
CanaryEvaluation->>HealthCalculation: calculate suspect match
HealthCalculation->>FailureTriage: pass suspect flag
FailureTriage-->>CanaryEvaluation: return SUSPECT
Promotion->>Promotion: block unless --override
BlockerIssues->>BlockerIssues: render guidance and needs-human label
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. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
![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 new "SUSPECT" triage category for canary rollouts, allowing certain failure classes (like workload timeouts) to block rollouts while providing specific, actionable guidance to speed up human verification. The changes span the rollout orchestrator script, classification logic, configuration schemas, and test suites. The review feedback suggests enhancing the robustness of the TSV parsing in the newly added _failure_suspect function by handling missing trailing newlines and stripping carriage returns.
![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
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)
870-880: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate path omits
needs-humanforSUSPECT, contradicting the create path and the stated contract.The create path (Line 870) applies
needs-humanforREGRESSIONorSUSPECT, but the update path (Line 880) still checks onlyREGRESSION. When aSUSPECTblocker issue already exists and is refreshed, it never receives theneeds-humanlabel, so escalation routing is lost on subsequent runs. This diverges from the comment at Lines 841-843 and the PR objective to retainneeds-humanforSUSPECT. The current bats coverage exercises the create/evaluate/blocker-body paths but not this update path, so it isn't caught.🐛 Proposed fix
- [ "$triage" = "REGRESSION" ] && gh issue edit "$num" --repo "$ISSUE_REPO" --add-label needs-human >/dev/null 2>&1 || true + { [ "$triage" = "REGRESSION" ] || [ "$triage" = "SUSPECT" ]; } && gh issue edit "$num" --repo "$ISSUE_REPO" --add-label needs-human >/dev/null 2>&1 || true🤖 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 870 - 880, Update the existing blocker issue labeling in the update path near the gh issue edit calls so the needs-human label is applied when triage is either REGRESSION or SUSPECT, matching the create path. Preserve the current behavior for other triage values and keep the dev-lead labeling unchanged.
🤖 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 343-378: The _cumulative_health function currently sets suspect to
1 when any counted failure matches a suspect pattern, causing mixed failures to
be labeled SUSPECT. Track whether counted failures exist and whether every
counted failure matches _failure_suspect, then emit suspect=1 only when all
counted failures are suspect-class matches; preserve benign exclusions and
blocking counts.
---
Outside diff comments:
In `@scripts/canary-rollout.sh`:
- Around line 870-880: Update the existing blocker issue labeling in the update
path near the gh issue edit calls so the needs-human label is applied when
triage is either REGRESSION or SUSPECT, matching the create path. Preserve the
current behavior for other triage values and keep the dev-lead labeling
unchanged.
🪄 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: 157b6b3e-d47b-4f35-8696-1a92e3a19017
📒 Files selected for processing (4)
scripts/canary-rollout.shscripts/lib/canary-rollout.shstandards/canary-rings.jsontests/canary_rollout.bats
|
@coderabbitai resolve |
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
✅ Action performedComments resolved and changes approved. |
Dev-Lead — review-changes (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: f0e5f1b7ad6ff5846fa75a9ce2623384dd42326d
Review mode: triage-approved (single reviewer)
Summary
Implements the SUSPECT third triage verdict (#668 increment 2) exactly per issue #675: a new optional suspect_failure_classes registry key, a backward-compatible classify_failure extension, orchestrator threading, promote-gate blocking, and guidance rendering in blocker issues — with a comprehensive bats decision-matrix and orchestrator test suite. Default-absent config preserves byte-identical behavior for agents that don't opt in.
Linked issue analysis
Closes #675 (canary #668 increment 2). All six spec items are substantively addressed: (1) registry schema with id/workflow/step/reason + guidance seeded for dev-lead workload-timeout (exit-124 signature); (2) pure-core classify_failure gains SUSPECT with correct precedence (environmental > differs=0 > suspect > regression), side-effect-free and unit-tested; (3) orchestrator threads the suspect flag through _cumulative_health/_frontier_state, SUSPECT blocks like REGRESSION; (4) blocker body renders the discriminating question and keeps needs-human labeling on both create and update paths; (5) dev-lead seed class present; (6) bats tests cover the decision matrix, orchestrator SUSPECT/REGRESSION discrimination, promote refusal without --override, --allow-pre-existing non-bypass, schema shape, and sync-issues label routing. The prerequisite #672 patterns are followed (mirrors _benign_patterns/_failure_benign).
Findings
No blocking findings.
- Correctness: single call sites of _cumulative_health (now 4-field output) and classify_failure both updated consistently; classify_failure's new arg defaults to 0 so existing 2-arg callers are unaffected. _failure_suspect fails closed on empty signatures. The empty-patterns fast path is preserved only when BOTH benign and suspect lists are empty — correct, since suspect matching needs per-run signatures.
- Security: no secrets or injection vectors; jq uses --arg binding throughout; gh invocations use fixed arguments. gitleaks, CodeQL, AgentShield, and Agent Security Scan all green. MCP secret-scanning tool unavailable in this run (noted; gitleaks CI covers it).
- Prior bot review threads (gemini-code-assist read-loop nit, CodeRabbit stability nit) are both resolved; CodeRabbit's CHANGES_REQUESTED review was addressed by the dev-lead fix-reviews commit and dismissed.
CI status
All checks green at f0e5f1b: ShellCheck, Lint, CodeQL, SonarCloud (quality gate passed), CodeRabbit, AgentShield, Agent Security Scan, gitleaks secret scan, npm audit. Ecosystem-specific audits skipped (not applicable).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
…I gap from #613/#624) (#685) * ci: add canary-rollout-tests.yml — run the engine's bats suite (closes 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 * chore: dev-lead update (review-changes) [skip ci-relay] * chore: dev-lead update (review-changes) [skip ci-relay] --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
2 participants
Closes #675
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
New Features
Bug Fixes