feat(canary): differs-aware benign classes (version_independent) — #668 increment 1#672
Conversation
…usion (#668 increment 1) The #548 gate disables the entire benign_failure_classes allowlist whenever the candidate changed the reusable (differs=1). For an actively-developed agent (dev-lead: differs=1 almost always) this chronically false-blocks promotion on failures that are inherently environmental — #864 (Dependabot- context startup failures held dev-lead 6 days) and #664 (exit-124 workload timeouts needed a human override). Fix: a benign class may declare "version_independent": true, meaning the failure occurs before/independent of the candidate's own code and can NEVER be candidate-caused. Such classes stay excluded from cum_fail even at differs=1; every unmarked class still disables, preserving the invariant that the allowlist cannot mask a candidate-introduced regression. - scripts/canary-rollout.sh: _benign_patterns is differs-aware (jq filter); _cumulative_health takes differs instead of apply_benign; _frontier_state always applies the (filtered) allowlist. - standards/canary-rings.json: dev-lead dependabot-context-dispatch marked version_independent (fails at secrets evaluation, before any candidate code); fix-review-git-push-permission deliberately NOT marked (a candidate could change push behaviour). ci-failure-analyst's cloned dev-lead classes removed (inert: workflow regex 'Dev-Lead Agent' can never match its runs). _benign_note updated fleet-wide. - tests/canary_rollout.bats: filter unit tests + end-to-end gate verdicts (version_independent match at differs=1 -> PROMOTE with benign=80; non-flagged match at differs=1 -> still BLOCKED+REGRESSION) + registry shape tests. 106/106 pass. Part of #668 (correctness-aware gate design); does NOT change behaviour for any agent without version_independent classes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J4z5uYVjwdyQjh2wFZxkut
📝 WalkthroughWalkthroughChangesCanary gating and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FrontierState
participant ReusableDiffers
participant CumulativeHealth
participant BenignPatterns
participant GhAPI
FrontierState->>ReusableDiffers: Determine reusable difference
FrontierState->>CumulativeHealth: Evaluate health with differs
CumulativeHealth->>BenignPatterns: Request filtered allowlist
BenignPatterns-->>CumulativeHealth: Return eligible benign classes
CumulativeHealth->>GhAPI: Read workflow failure evidence
GhAPI-->>CumulativeHealth: Return runs and failure signatures
CumulativeHealth-->>FrontierState: Return cumulative health status
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 differs-aware benign failure class mechanism to the canary rollout process, allowing failures marked as version_independent to be excluded from the cumulative failure count even when the candidate changed the reusable workflow. The feedback suggests using the optional/try operator ? in jq to prevent potential fatal indexing errors when accessing agent gate properties, and utilizing $BATS_TEST_TMPDIR during temporary directory creation in tests to ensure proper isolation and cleanup.
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
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 (1)
standards/canary-rings.json (1)
66-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMisleading
reasonfor a deliberately-unflagged class.
fix-review-git-push-permissionis intentionally not markedversion_independent: true(a candidate can change push behavior, per the test at Line 1331), yet itsreasonstill asserts it is a "version-independent benign failure". This contradicts the design rationale and could lead a maintainer to add the flag, which would let the class mask a candidate-introduced regression atdiffers=1. Recommend dropping the "version-independent" wording and stating why the flag is withheld.📝 Proposed wording fix
- "reason": "fix-review git push denied by branch protection / missing write scope; version-independent benign failure, not a candidate regression." + "reason": "fix-review git push denied by branch protection / missing write scope; NOT marked version_independent because a candidate change to push behavior could legitimately cause this, so it must stay disabled at differs=1."🤖 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 `@standards/canary-rings.json` at line 66, Update the reason for the fix-review-git-push-permission entry to remove the incorrect “version-independent” characterization and explicitly state that it remains unflagged because a candidate can change push behavior and must not mask regressions at differs=1, as covered by the existing test.
🤖 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 `@standards/canary-rings.json`:
- Line 66: Update the reason for the fix-review-git-push-permission entry to
remove the incorrect “version-independent” characterization and explicitly state
that it remains unflagged because a candidate can change push behavior and must
not mask regressions at differs=1, as covered by the existing test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac350be2-ff13-4d63-9f6b-bf6d8550a8bb
⛔ Files ignored due to path filters (34)
node_modules/.bin/batsis excluded by!**/node_modules/**node_modules/.package-lock.jsonis excluded by!**/node_modules/**node_modules/bats/LICENSE.mdis excluded by!**/node_modules/**node_modules/bats/README.mdis excluded by!**/node_modules/**node_modules/bats/bin/batsis excluded by!**/node_modules/**node_modules/bats/install.shis excluded by!**/node_modules/**node_modules/bats/lib/bats-core/common.bashis excluded by!**/node_modules/**node_modules/bats/lib/bats-core/formatter.bashis excluded by!**/node_modules/**node_modules/bats/lib/bats-core/preprocessing.bashis excluded by!**/node_modules/**node_modules/bats/lib/bats-core/semaphore.bashis excluded by!**/node_modules/**node_modules/bats/lib/bats-core/test_functions.bashis excluded by!**/node_modules/**node_modules/bats/lib/bats-core/tracing.bashis excluded by!**/node_modules/**node_modules/bats/lib/bats-core/validator.bashis excluded by!**/node_modules/**node_modules/bats/lib/bats-core/warnings.bashis excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/batsis excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/bats-exec-fileis excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/bats-exec-suiteis excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/bats-exec-testis excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/bats-format-catis excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/bats-format-junitis excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/bats-format-prettyis excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/bats-format-tapis excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/bats-format-tap13is excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/bats-gather-testsis excluded by!**/node_modules/**node_modules/bats/libexec/bats-core/bats-preprocessis excluded by!**/node_modules/**node_modules/bats/man/Makefileis excluded by!**/node_modules/**node_modules/bats/man/README.mdis excluded by!**/node_modules/**node_modules/bats/man/bats.1is excluded by!**/node_modules/**node_modules/bats/man/bats.1.ronnis excluded by!**/node_modules/**node_modules/bats/man/bats.7is excluded by!**/node_modules/**node_modules/bats/man/bats.7.ronnis excluded by!**/node_modules/**node_modules/bats/package.jsonis excluded by!**/node_modules/**node_modules/bats/uninstall.shis excluded by!**/node_modules/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonscripts/canary-rollout.shstandards/canary-rings.jsontests/canary_rollout.bats
|
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
Dev-Lead — waiting on PR blockers (intent: fix-reviews)PR: #672 |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 193a654f750abfa9894e8c13ed684bf9276a4ee5
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
Differs-aware canary gate change: at differs=1 the benign-failure allowlist narrows to classes marked version_independent (only dev-lead's Dependabot-context class, which provably fails before candidate code runs), preserving the #548/#1025 invariant that the allowlist cannot mask a candidate-introduced regression. The two triage signals dissolve on inspection: the Gemini jq null-index finding is already fixed at head (optional operators present; verified no 'Cannot index null' crash; new null-safety tests added), and the 6000+ line diff is entirely vendored node_modules/bats. All CI gates are green (ShellCheck, CodeQL, gitleaks, SonarCloud, npm audit, AgentShield). Approving at MEDIUM. Downstream impact: (none).
Findings
- INFO: Gemini's flagged fatal jq 'Cannot index null' risk at _benign_patterns is already resolved at head: expression is
.agents[$a]?.gate?.benign_failure_classes? // []with all optional operators. Empirically verified no crash for absent-agent, null-gate, and absent-agents-key inputs (all rc=0). Dedicated null-safety bats tests added. (scripts/canary-rollout.sh:276) - MAJOR: Full node_modules/bats tree (~6000 lines) is committed and node_modules is not in .gitignore, yet CI installs bats via
apt-get install -y bats(lint.yml) — so the vendored dependency is unused dead weight in the repo. Recommend gitignoring node_modules and removing the vendored tree (keep package.json/package-lock.json only if a documented npm-based path exists). (node_modules/bats) - MINOR: tests/canary_rollout.bats (106 tests incl. the 11 new ones) is not listed in lint.yml's bats job, so it does not gate merge in CI — the new logic is only validated locally. Pre-existing gap (file predates this PR), but worth wiring in given this PR expands its coverage of a security-relevant gate. (.github/workflows/lint.yml)
- INFO: Change loosens the canary auto-promotion gate but stays within the documented safety bar: only classes that fail before/independent of candidate code (version_independent:true) are excluded at differs=1; the fix-review push class and #664 timeout class are deliberately NOT flagged. run_secret_scanning MCP tool was not available; gitleaks CI check passed. (standards/canary-rings.json)
Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.
2 participants
What
First increment of #668 (correctness-aware canary gate): resolve the differs=1 chronic-false-block by letting a benign failure class declare
"version_independent": true.Today the #548 gate disables the entire
benign_failure_classesallowlist whenever the candidate changed the reusable (differs=1) — correct for classes a candidate could cause, but it means an actively-developed agent (dev-lead isdiffers=1almost always) gets blocked as REGRESSION by inherently environmental failures:secrets:evaluation, before any candidate code executes; held dev-lead ~6 days.How
_benign_patterns <agent> <differs>: atdiffers=1, emit only classes markedversion_independent: true; atdiffers=0, all classes (unchanged). Every unmarked class still disables atdiffers=1, so the allowlist still cannot mask a candidate-introduced regression._cumulative_healthtakesdiffers(wasapply_benign) and always applies the filtered allowlist;_frontier_statepasses it through. Pure core (lib/canary-rollout.sh) untouched.dependabot-context-dispatch→version_independent: true. Airtight: the failure mechanism (no Actions secrets in a Dependabot context) fires before any step of the candidate runs.fix-review-git-push-permission→ deliberately NOT flagged: a candidate could change push behaviour, so it stays differs=0-only. (The Canary blocker: dev-lead next->ring0 (cum_fail=3, REGRESSION) #664 timeout class is likewise NOT eligible — a timeout can be a real perf regression; that's the SUSPECT-triage follow-up in the Enhancement: canary gate should assess correctness, not just run-reliability #668 design.)workflow: "Dev-Lead Agent"regex can never match a ci-failure-analyst run._benign_noteupdated fleet-wide (viajq -a, em-dashes preserved) documenting the flag and the bar for using it (provable from the failure mechanism, not merely likely).Tests
bats tests/canary_rollout.bats→ 106/106 pass (11 new):PROMOTE(benign=80 excluded); differs=1 + failures matching the unflagged push class → stillBLOCKED+REGRESSION,Behaviour change surface
Zero for any agent without
version_independentclasses (13 of 14 agents). For dev-lead, exactly the #864 failure shape stops counting as REGRESSION at differs=1. Rollout of this engine change is just a merge — the engine runs from main, not via channel tags.Design context: full write-up on #668.
🤖 Generated with Claude Code
https://claude.ai/code/session_01J4z5uYVjwdyQjh2wFZxkut
Summary by CodeRabbit
Bug Fixes
Documentation
Tests