feat: implement issue #677 — canary #668 increment 3: opt-in human confirmation at ring1→stable (require_confirmation, dev-lead)#678
Conversation
…nfirmation at ring1→stable (require_confirmation, dev-lead)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe canary rollout adds an ChangesCanary confirmation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant CanaryRollout
participant GitHubIssues
participant Human
Scheduler->>CanaryRollout: evaluate rollout
CanaryRollout->>CanaryRollout: set AWAITING_CONFIRMATION
CanaryRollout->>GitHubIssues: open canary-confirm issue
Human->>CanaryRollout: promote agent --confirm
CanaryRollout->>GitHubIssues: advance ring transition
Possibly related PRs
Suggested labels: 🚥 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 |
![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 Layer 3 of the canary rollout human confirmation feature, introducing an opt-in "AWAITING_CONFIRMATION" state at ring boundaries (configured for the "dev-lead" agent's "ring1->stable" transition). This state halts automated promotion even when reliability checks pass, requiring a manual "--confirm" flag to proceed. It also includes automated creation, updating, and closing of GitHub confirmation issues to facilitate human review. Feedback on the changes suggests handling the edge case where there is no prior stable release to prevent broken comparison URLs in the generated confirmation issue.
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 639-641: Update the confirmation notice in the
AWAITING_CONFIRMATION branch to use the existing $transition variable for the
go/no-go boundary instead of the hardcoded ring1->stable text. Keep the
surrounding confirmation handling and reliability message unchanged.
- Around line 1005-1015: Update the stale confirm-issue close branch in the
rollout status logic to assign the dashboard blocker variable `blk` to the
closed issue identifier and state, matching the existing blocker-close behavior.
Ensure agents leaving AWAITING_CONFIRMATION show the closed canary-confirm issue
in the fleet-status job summary, including dry-run handling as appropriate.
🪄 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: fbe77a13-9295-4a2d-b939-33cc7540f3a6
📒 Files selected for processing (4)
.github/workflows/canary-rollout.ymlscripts/canary-rollout.shstandards/canary-rings.jsontests/canary_rollout.bats
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Dev-Lead — waiting on PR blockers (intent: fix-reviews)PR: #678 |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 983a19c1ae264866730bbd22ea95372341b47e89
Review mode: triage-approved (single reviewer)
Summary
Implements #668 increment 3 (Layer 3): opt-in human confirmation at a require_confirmation ring boundary. When reliability is otherwise PROMOTE and the frontier transition sets require_confirmation, the state overlays to AWAITING_CONFIRMATION; the scheduled sweep never auto-advances it, and only a deliberate 'promote --confirm' dispatch clears it. --confirm cannot bypass a BLOCKED gate (verified by test). sync-issues upserts an evidence-carrying canary-confirm issue (marker-keyed, needs-human) and auto-closes it when the hold clears. Config enables it for dev-lead ring1->stable only; default-absent keeps all other agents byte-identical. 8 new bats tests cover the state overlay, hold-without-confirm, confirm-advance, no-bypass-of-BLOCKED, issue body rendering (incl. empty-prior fallback), and issue open/close lifecycle.
Linked issue analysis
Closes #677 (open; body empty — spec carried in the title: 'canary #668 increment 3: opt-in human confirmation at ring1→stable (require_confirmation, dev-lead)'). The PR substantively delivers exactly that: the require_confirmation knob in standards/canary-rings.json (dev-lead ring1->stable), the AWAITING_CONFIRMATION overlay + --confirm flag in scripts/canary-rollout.sh, the confirm workflow input in canary-rollout.yml, and the canary-confirm issue lifecycle.
Findings
No blocking findings.
- Security: the new 'confirm' workflow input is a boolean passed via env and string-compared ('[ "$CONFIRM" = "true" ]') — no injection surface; mirrors the existing OVERRIDE pattern. The feature is safety-additive (adds a human gate; --confirm provably cannot advance a BLOCKED state — cmd_promote only honours it when state=AWAITING_CONFIRMATION, which is only ever set from a reliability-clean PROMOTE verdict, and a dedicated test asserts the no-bypass property).
- Prior review findings all resolved at this SHA: gemini's empty-prior diff-link concern (fallback '(no prior stable release)' + test), CodeRabbit's hardcoded-transition notice (now uses $transition) and stale-blk dashboard column (now set to '#N (confirm closed)'). All 3 review threads resolved; CodeRabbit dismissed its changes-requested review and approved.
- Verified helper dependencies (_gate_knob, _issue_find, _gh_issue_create, _suspect_guidance) exist at the head SHA and the reused _sync_stub fixture sets SYNC_RINGS.
- Secret scan: run_secret_scanning MCP tool unavailable in this run; gitleaks CI check is green.
- Note: modifies .github/workflows/canary-rollout.yml, which is org automation but NOT one of the protected thin-caller stubs — change is within allowed scope.
CI status
All code-validation checks green: ShellCheck, Lint, CodeQL, Analyze (actions), Secret scan (gitleaks), Agent Security Scan, agent-shield, SonarCloud (quality gate passed, 0 new issues), CodeRabbit, npm audit; ecosystem audits skipped (not applicable). The only non-green entries are 'dev-lead / dispatch' and 'dev-lead / ci-relay' — the dev-lead agent's own pull_request_review-triggered relay jobs, concurrency-cancelled after 0 steps by successive review events; they do not validate the PR's code.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
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 (3)
scripts/canary-rollout.sh (3)
983-1003: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReset confirmation evidence when the candidate changes.
The documented contract says
canary-confirmissues auto-close on candidate changes, but this path keys the issue only by agent and updates it whenever the state remainsAWAITING_CONFIRMATION. A replacement candidate can therefore inherit the previous issue’s approval context. Compare the current candidate SHA with the issue evidence, close the stale issue, and create a fresh confirmation issue when they differ.🤖 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 983 - 1003, Update the AWAITING_CONFIRMATION handling around cnum and _confirm_body to compare the current candidate SHA with the candidate recorded in the existing confirmation issue evidence. When they differ, close the stale issue, clear cnum and related issue state, and create a fresh canary-confirm issue; otherwise retain the existing update behavior.
984-986: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when confirmation evidence cannot be rendered.
|| trueconverts any_confirm_bodyfailure into an empty body, after which the script still creates or updates aneeds-humanissue. That can leave reviewers with no candidate diff or run evidence while the synchronization appears successful. Preserve the failure and skip the issue mutation, or provide an explicit fallback that clearly indicates the evidence is unavailable.Proposed fix
- cbody="$(_confirm_body "$agent" "$transition" "$cand" "$prior" "$host" "$_s" "$_t" || true)" + if ! cbody="$(_confirm_body "$agent" "$transition" "$cand" "$prior" "$host" "$_s" "$_t")"; then + echo "::warning::could not render confirmation issue for $agent" + continue + 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 984 - 986, Update the confirmation flow around _confirm_body so rendering failures are not converted into an empty cbody. Preserve the command failure, then skip the needs-human issue creation or update when evidence cannot be produced, or set an explicit unavailable-evidence fallback that clearly informs reviewers; do not perform the issue mutation as though synchronization succeeded.
622-630: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent
--overridefrom bypassingAWAITING_CONFIRMATION.Line [624] enables advancement for every state before the confirmation-specific check, so
promote <agent> --overridecan advance an awaiting frontier without--confirm. The new gate should require--confirmexclusively forAWAITING_CONFIRMATION.Proposed fix
local advance=false [ "$state" = "PROMOTE" ] && advance=true - [ "$override" = true ] && advance=true + if [ "$state" != "AWAITING_CONFIRMATION" ] && [ "$override" = true ]; then + advance=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 622 - 630, The advance decision logic must not let --override promote an AWAITING_CONFIRMATION frontier. Update the checks initializing advance so override applies only to states other than AWAITING_CONFIRMATION, while retaining the dedicated --confirm requirement in the AWAITING_CONFIRMATION branch and existing PROMOTE/BLOCKED 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.
Outside diff comments:
In `@scripts/canary-rollout.sh`:
- Around line 983-1003: Update the AWAITING_CONFIRMATION handling around cnum
and _confirm_body to compare the current candidate SHA with the candidate
recorded in the existing confirmation issue evidence. When they differ, close
the stale issue, clear cnum and related issue state, and create a fresh
canary-confirm issue; otherwise retain the existing update behavior.
- Around line 984-986: Update the confirmation flow around _confirm_body so
rendering failures are not converted into an empty cbody. Preserve the command
failure, then skip the needs-human issue creation or update when evidence cannot
be produced, or set an explicit unavailable-evidence fallback that clearly
informs reviewers; do not perform the issue mutation as though synchronization
succeeded.
- Around line 622-630: The advance decision logic must not let --override
promote an AWAITING_CONFIRMATION frontier. Update the checks initializing
advance so override applies only to states other than AWAITING_CONFIRMATION,
while retaining the dedicated --confirm requirement in the AWAITING_CONFIRMATION
branch and existing PROMOTE/BLOCKED behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 71dfc202-344a-433f-b546-c5e62019e24b
📒 Files selected for processing (1)
scripts/canary-rollout.sh
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
…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 #677
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
dev-leadtransition from ring 1 to stable.