fix: share canary-ring pin model between audit and deploy sweep (#482)#531
Conversation
#482's root cause is that the per-reusable canonical pin was encoded independently in three places (template, compliance audit, deploy sweep) and diverged. The audit became ring-aware in #529/#870, but the deploy sweep's `is_already_compliant` was still a literal substring match on the template's @<name>/stable pin — so a fleet sweep reported every ring1/next repo as "drifted" and would have reverted the intentional ring pins back to stable (criterion 2 unmet; the dry-run was dirty for TalkTerm/bmad/.github-private). Extract the ring model into `scripts/lib/ring-pins.sh` (single source of truth) and source it from both consumers: - ring_tier_for_repo / ring_canonical_ref / ring_accepted_refs / ring_legacy_csv + the RING_REUSABLES set (now incl. dev-lead, which is ring-released too). - compliance-audit.sh: drop the duplicate ring_tier_for_repo and compute the RING canonical/legacy via the lib — behaviour-identical, 11/11 stub bats pass. - deploy-standard-workflows.sh: is_already_compliant now accepts a repo's tier channel (and the transitional grace) for ring reusables, so the sweep never reverts a ring/next pin; non-ring templates keep the exact-match rule. Now "drift" (deploy) and "non-compliant" (audit) are computed from the same code and cannot diverge again. Full-fleet dry-run drops from TalkTerm+bmad+.github-private noise to only the two genuine off-channel (SHA) pins (ContentTwin ×6, TalkTerm dev-lead) — repinned separately. Tests: new ring-pins.bats (5) + two ring-aware dry-run cases. Refs #482
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
More reviews will be available in 41 minutes and 51 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ 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 |
|
@donpetry-bot please review |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
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 introduces a shared canary-ring pin model in scripts/lib/ring-pins.sh to unify the ring-tier mapping and accepted reference definitions between the compliance audit (compliance-audit.sh) and the deployment sweep (deploy-standard-workflows.sh). This prevents drift between the two scripts and ensures that ring-aware repositories are not reverted to stable templates. Unit and integration tests have been added to verify this behavior. The reviewer feedback suggests making global arrays readonly for defensive programming and explicitly returning exit codes in several Bash functions to avoid relying on implicit exit statuses.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
|
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
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. |
9 similar comments
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
11 similar comments
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot 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: 102bdb1d06a49a9c7f7cbdb9aa5ac62b9fa8d0fc
Review mode: triage-approved (single reviewer)
Summary
Extracts the canary-ring pin model into a shared scripts/lib/ring-pins.sh so the compliance audit ("non-compliant") and the deploy sweep ("drift") compute pins from one source and can no longer diverge — the three-way inconsistency tracked by #482. compliance-audit.sh drops its duplicate ring_tier_for_repo and uses the lib; deploy-standard-workflows.sh's is_already_compliant becomes ring-aware so it never reverts an intentional ring/next pin to the template's stable channel, while non-ring templates keep exact-match. +208/-24 across 5 files, with new unit and dry-run bats coverage.
Linked issue analysis
PR body states it closes the core of #482 (template ↔ audit ↔ deploy pin inconsistency). The change directly addresses the root cause: a single shared module (ring_tier_for_repo, ring_is_ring_reusable, ring_canonical_ref, ring_accepted_refs, ring_legacy_csv) now backs both consumers, so audit and deploy can't disagree. The off-channel/SHA-pin behavior is preserved and explicitly tested. closingIssuesReferences is empty in metadata (issue lives in a different repo / linked by prose), but the linkage is clearly documented and substantively addressed.
Findings
No blocking issues.
- Refactor is behavior-preserving for the audit (PR reports 11/11 existing stub bats pass) and adds the intended ring-awareness to the deploy sweep.
- is_already_compliant correctly gates the ring path on both ring_is_ring_reusable AND the template pinning a channel tag (^base/(stable|next|ring[0-9]+)$), falling back to exact-match otherwise — frozen @vX/SHA template pins are unaffected.
- The legacy grace in ring_accepted_refs (accepting all ring channels + v1/v2) is intentionally broad and documented as transitional, to be tightened in the #870 follow-up.
- Advisory (gemini-code-assist) asked for explicit 'return 0' on ring_canonical_ref/ring_accepted_refs/ring_legacy_csv; already applied in the 'address review comments' commit — current diff shows all three with explicit returns. Resolved.
- coderabbitai review dismissed; CodeRabbit, SonarCloud (0 new issues, quality gate passed), CodeQL all green.
- GitHub Secret Protection MCP (run_secret_scanning) not available in this environment; relied on the green gitleaks CI check instead — no secrets in the diff (shell logic + bats only).
CI status
All substantive checks green: Lint, Lint and bats, ShellCheck, CodeQL, SonarCloud (quality gate passed), CodeRabbit, Agent Security Scan, AgentShield, Secret scan (gitleaks). The CANCELLED/SKIPPED entries are dev-lead dispatch/ci-relay and ecosystem-audit (npm/pnpm/pip/cargo/go) steps that don't apply to this shell-only change. reviewDecision is APPROVED.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
13 similar comments
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot I'm on it — starting a fresh review now. Results will appear in a few minutes. |
|
@donpetry-bot 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: LOW
Reviewed commit: 102bdb1d06a49a9c7f7cbdb9aa5ac62b9fa8d0fc
Review mode: triage-approved (single reviewer)
Summary
Refactors the canary-ring pin model into a new shared lib scripts/lib/ring-pins.sh, sourced by both compliance-audit.sh and deploy-standard-workflows.sh so the audit's "non-compliant" and the deploy sweep's "drift" are computed from one source of truth (closes core of #482). The deploy sweep's is_already_compliant now accepts a ring repo's tier channel (+ transitional grace) instead of a literal substring match on the template's @/stable pin, so a fleet sweep no longer falsely flags every ring1/next repo and no longer reverts intentional ring pins to stable. Behaviour-preserving for non-ring templates (exact match retained).
Linked issue analysis
Refs #482 (the three-way template/audit/deploy pin inconsistency). The PR substantively addresses the deploy side: ring_accepted_refs/ring_canonical_ref/ring_is_ring_reusable are the single source of truth, and is_already_compliant consults them so the sweep accepts a repo's tier channel. closingIssuesReferences is empty (PR uses "Refs #482", not "Closes"), consistent with the body noting two off-channel SHA pins are repinned in separate PRs before #482 fully closes.
Findings
No blocking findings.
- is_already_compliant: ring path is correctly gated by ring_is_ring_reusable AND a channel-tag regex, falling through to exact-match for non-ring/frozen pins. The interpolated regex ^${base}/(stable|next|ring[0-9]+)$ is safe — base is constrained to [a-z0-9-] by the capture.
- Advisory (gemini-code-assist, MEDIUM): asked for explicit
return 0on ring_canonical_ref/ring_accepted_refs/ring_legacy_csv — already present in the reviewed head; nothing to do. - CodeRabbit/Codex were rate-limited and produced no substantive findings.
- Secret-scanning MCP tool not exposed in this environment; the gitleaks CI check passed, so coverage is intact.
- Minor (non-blocking): the grep -qF accepted-ref match is a substring check, but ring tiers are only ring0/ring1 so no realistic false-positive; tests cover the compliant ring1 pin and the still-flagged SHA pin.
CI status
All required checks green: Lint and bats, ShellCheck, CodeQL, Analyze (actions), SonarCloud (Quality Gate passed, 0 new issues), Secret scan (gitleaks), agent-shield/AgentShield, Agent Security Scan, CodeRabbit. Ecosystem-specific audits (npm/pnpm/pip/cargo/govulncheck) SKIPPED as not applicable. reviewDecision=APPROVED. head SHA matches PR_HEAD_SHA; not a draft.
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: 102bdb1d06a49a9c7f7cbdb9aa5ac62b9fa8d0fc
Review mode: triage-approved (single reviewer)
Summary
Refactor that closes the core of #482 (three-way pin inconsistency). Extracts the canary-ring pin model into a new shared lib (scripts/lib/ring-pins.sh) sourced by both compliance-audit.sh and deploy-standard-workflows.sh, so the audit's "non-compliant" and the deploy sweep's "drift" are computed from the same code and can't diverge. The audit refactor is behaviour-identical; the deploy sweep's is_already_compliant becomes ring-aware so it no longer reverts intentional ring/next pins back to the template's stable channel.
Linked issue analysis
PR has no closingIssuesReferences metadata, but the body documents intent against #482 ("closes the core"). The change directly addresses the described drift bug: ring_legacy_csv reproduces the previous 4-channel + v1/v2 legacy grace regardless of tier, keeping the audit identical, while the deploy sweep now accepts a repo's tier channel. Verification narrative (full-fleet --dry-run before vs after) and the two new dry-run bats cases substantiate the fix. #482 is expected to close once the separately-tracked off-channel repins (TalkTerm, ContentTwin) merge.
Findings
No blocking findings.
- Correctness: compliance-audit.sh refactor is behaviour-identical — ring_legacy_csv() always emits next,ring0,ring1,stable,v1,v2, matching the prior hardcoded string for any tier. deploy is_already_compliant() ring-aware branch is sound: base is extracted via BASH_REMATCH '([a-z0-9-]+)-reusable.yml' so its interpolation into the channel regex is injection-safe; the sole caller in deploy_repo() is updated to pass $repo; non-ring/frozen-@vx templates correctly keep the exact-match rule.
- Advisory (non-blocking, already resolved): gemini-code-assist's three medium nits asking for explicit 'return 0' on ring_canonical_ref/ring_accepted_refs/ring_legacy_csv are already satisfied in this HEAD (applied by dev-lead fix-reviews before SHA 102bdb1). Codex/CodeRabbit advisory comments were usage/rate-limit notices, not findings.
- Tests: 5 new unit tests (ring-pins.bats) plus 2 new deploy dry-run cases (ring1 pin compliant; off-channel SHA still flagged). Existing 11 stub bats remain green per author + CI.
- Secret scan: run_secret_scanning MCP tool not available in this run; relying on the green gitleaks CI check. No secrets/credentials touched by the diff.
CI status
All required checks green: Lint, Lint and bats, ShellCheck, Analyze (actions)/CodeQL, SonarCloud (Quality Gate passed, 0 new issues), Secret scan (gitleaks), Agent Security Scan, AgentShield. Language-specific audits (npm/pnpm/pip/cargo/govulncheck) SKIPPED as not applicable. CANCELLED 'dev-lead / dispatch' and 'dev-lead / ci-relay' entries are superseded relay-orchestration runs, not real failures. reviewDecision=APPROVED.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
2 participants
Summary
Closes the core of #482 — the three-way pin inconsistency (template ↔ compliance audit ↔ deploy sweep). The audit became ring-aware in #529/#870, but
deploy-standard-workflows.sh'sis_already_compliantwas still a literal substring match on the template's@<name>/stablepin, so a fleet sweep flagged every ring1/next repo as drifted and would have reverted their intentional ring pins back tostable.What changed
scripts/lib/ring-pins.sh— single source of truth for the canary-ring model:ring_tier_for_repo,ring_canonical_ref,ring_accepted_refs,ring_legacy_csv, and theRING_REUSABLESset (now incl.dev-lead, which is ring-released too).compliance-audit.sh— drops its duplicatering_tier_for_repo; the RING canonical/legacy is computed via the lib. Behaviour-identical (11/11 existing stub bats pass).deploy-standard-workflows.sh—is_already_compliantaccepts a repo's tier channel (+ transitional grace) for ring reusables, so the sweep never reverts a ring/next pin. Non-ring templates (add-to-project, feature-ideation@v1) keep the exact-match rule."Drift" and "non-compliant" are now computed from the same code and can't diverge again.
Verification
Full-fleet
--dry-runbefore → noisy (TalkTerm, bmad,.github-privateall falsely drifted). After → only the two genuine off-channel SHA pins remain:Those are repinned to their channel tags in separate PRs; once merged the fleet dry-run is fully clean (criterion 3) and #482 closes.
Tests
test/scripts/lib/ring-pins.bats— 5 new unit tests for the lib.test/scripts/deploy-standard-workflows/dry-run.bats— 2 new cases (ring1 tier-channel pin = compliant; SHA pin = still flagged).shellcheck --severity=warningclean.Refs #482
🤖 Generated with Claude Code