feat: implement issue #927 — auto-rebase: default eligibility to all; remove dead review-ready code; lock merge-method invariant - #931
Conversation
… remove dead review-ready code; lock merge-method invariant
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🤖 CodeAnt AI — Review Status
|
|
Warning Review limit reached
Next review available in: 51 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 Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe auto-rebase workflow now defaults to ChangesAuto-rebase eligibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
codeant-ai
Bot
added
the
size:M
PR Summary by QodoAuto-rebase: default eligibility to all; remove review-ready plumbing; add merge invariant test
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
![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 simplifies the auto-rebase workflow by removing the review-ready eligibility mode and its associated helper functions, making all the default mode that updates every behind PR. It also updates documentation and tests accordingly, and introduces a new regression guard test to ensure the workflow always uses the merge update method. Feedback on the new test points out that asserting a non-zero exit status to verify the absence of a pattern is fragile and could lead to false positives; asserting an exit status of exactly 1 is recommended instead.
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #931 |
|
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. |
Code Review by Qodo
Context used✅ Compliance rules (platform):
87 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
.github/workflows/auto-rebase-reusable.yml (2)
111-112: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFold
base.refinto the initial PR listing instead of a per-PR API call.The initial
gh api "repos/$REPO/pulls?state=open&per_page=100"call already returns each pull request'sbase.ref. Line 120 adds a secondgh apicall per PR solely to re-fetch that value. This adds an unnecessary API call per PR on a hot path (every push tomain), and it re-introduces theset -efragility this file otherwise guards against: if the per-PR fetch transiently fails,BASE_BRANCH=$(...)fails the step under the defaultbash -eshell, aborting the whole run and leaving remaining PRs in$PRSunprocessed — the exact starvation scenario the file's issue#594comment (lines 216-218) says must be avoided.Extract
base.refin the initial listing and drop the per-PR fetch.🛠️ Proposed fix
PRS=$(gh api "repos/$REPO/pulls?state=open&per_page=100" \ - --jq '.[] | select(.user.login != "dependabot[bot]") | select(.head.repo != null) | select(.head.repo.full_name == .base.repo.full_name) | "\(.number) \(.head.ref)"') + --jq '.[] | select(.user.login != "dependabot[bot]") | select(.head.repo != null) | select(.head.repo.full_name == .base.repo.full_name) | "\(.number) \(.head.ref) \(.base.ref)"') if [[ -z "$PRS" ]]; then echo "No open non-Dependabot same-repo PRs" exit 0 fi - while IFS=' ' read -r PR_NUMBER HEAD_REF; do - BASE_BRANCH=$(gh api "repos/$REPO/pulls/$PR_NUMBER" --jq '.base.ref') - + while IFS=' ' read -r PR_NUMBER HEAD_REF BASE_BRANCH; doAlso applies to: 119-121
🤖 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 @.github/workflows/auto-rebase-reusable.yml around lines 111 - 112, Update the initial PR listing assigned to PRS to extract both the PR number, head ref, and base.ref, then pass the base branch through the per-PR loop. Remove the per-PR gh api call and derive BASE_BRANCH from the listing data while preserving processing of remaining PRs when individual operations fail.
114-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the eligibility mode before the "no PRs" early exit.
auto_rebase_pr_eligiblenow depends only on$ELIGIBILITY, not on any per-PR data. As written, the unknown-mode check at lines 126-129 only runs once thewhileloop processes at least one PR. If$PRSis empty, the script exits 0 at line 116 before ever callingauto_rebase_pr_eligible, so an invalideligibilityinput silently produces no error on runs with no open PRs — for example when testing viaworkflow_dispatch. Validate the mode once, before the-z "$PRS"check, so an unknown mode always surfaces as::error::, regardless of whether any PRs are open.🛠️ Proposed fix
if [[ -z "$PRS" ]]; then + auto_rebase_pr_eligible "$ELIGIBILITY" > /dev/null || { + [[ $? -eq 2 ]] && { echo "::error::Unknown eligibility mode '$ELIGIBILITY'"; exit 1; } + } echo "No open non-Dependabot same-repo PRs" exit 0 fiAlso applies to: 122-133
🤖 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 @.github/workflows/auto-rebase-reusable.yml around lines 114 - 117, Validate the eligibility mode before the no-PR early exit in the workflow, using the existing auto_rebase_pr_eligible logic or its mode validation to ensure unknown $ELIGIBILITY values emit ::error::. Keep the empty-$PRS success path unchanged for valid modes, and avoid relying on the PR-processing loop to perform this validation.
🤖 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 @.github/workflows/auto-rebase-reusable.yml:
- Around line 111-112: Update the initial PR listing assigned to PRS to extract
both the PR number, head ref, and base.ref, then pass the base branch through
the per-PR loop. Remove the per-PR gh api call and derive BASE_BRANCH from the
listing data while preserving processing of remaining PRs when individual
operations fail.
- Around line 114-117: Validate the eligibility mode before the no-PR early exit
in the workflow, using the existing auto_rebase_pr_eligible logic or its mode
validation to ensure unknown $ELIGIBILITY values emit ::error::. Keep the
empty-$PRS success path unchanged for valid modes, and avoid relying on the
PR-processing loop to perform this validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 21c08983-9a18-493a-b606-474b302075bb
📒 Files selected for processing (7)
.github/scripts/auto-rebase/README.md.github/scripts/auto-rebase/lib/eligibility.sh.github/workflows/auto-rebase-reusable.ymlstandards/ci-standards.mdstandards/workflows/auto-rebase.ymltest/workflows/auto-rebase/eligibility.batstest/workflows/auto-rebase/merge-method.bats
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
|
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
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: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
Removes the review-ready eligibility gate from the auto-rebase reusable workflow, defaulting to updating every behind PR (eligibility: all), and locks in the merge-method invariant with a new regression test. Fully implements issue #927 with clean backward compatibility: review-ready is a deprecated alias treated as all (with a stderr warning), and ready_label is accepted-but-ignored so existing callers do not break.
Linked issue analysis
Issue #927 (part of #926) required: (1) flip the eligibility default from review-ready to all — done in auto-rebase-reusable.yml; (2) remove dead review-ready mode and ready_label plumbing from lib/eligibility.sh and the reusable — done, including the stuck-conflict notice block that only made sense under gated eligibility; (3) a regression test asserting update-branch always uses update_method=merge, never rebase — added in test/workflows/auto-rebase/merge-method.bats. Docs (README, ci-standards.md, ADR, standards template) updated consistently. Substantively addressed.
Findings
No blocking findings.
- Backward compatibility handled correctly: review-ready is a deprecated alias returning eligible (with stderr deprecation warning) and the ready_label input is retained but ignored, so existing callers dispatch without validation errors. This resolved the qodo breaking-contract review thread.
- auto_rebase_post_comment_best_effort remains defined in lib/comments.sh and is still used by the blocked/conflict notice paths at head SHA — no dead code left behind.
- Tests updated to pin the new contract, including guards that the removed predicates are no longer defined.
- Minor (non-blocking): auto_rebase_pr_eligible is still invoked per-PR inside the loop though it now depends only on the mode; acceptable as it preserves the extension point for future per-PR modes.
- Secret-scanning MCP tool unavailable in this run; gitleaks CI check passed (SUCCESS).
CI status
All checks green at head SHA 8f70e32: ShellCheck, bats (x2), Lint, CodeQL, Analyze (actions), agent-shield, Agent Security Scan, Secret scan (gitleaks), SonarCloud quality gate, npm audit, Graphite AI Reviews — all SUCCESS. Ecosystem audits (pip/pnpm/cargo/govulncheck) skipped as not applicable. Two cancelled dev-lead dispatch/ci-relay entries are superseded orchestration runs; their latest runs are success/skipped.
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: 8f70e3265b22718096539d0974539552d68f80d4
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)
Summary
PR #931 implements issue #927: flips auto-rebase-reusable.yml eligibility default from 'review-ready' to 'all', removes the now-dead approval/ready_label predicate plumbing, and adds a regression test locking update_method=merge. Change is backward-compatible — 'review-ready' and 'ready_label' are still accepted as deprecated no-ops, so none of the 8 pinned downstream consumers (.github, .github-private, ContentTwin, TalkTerm, bmad-bgreat-suite, broodly, google-app-scripts, markets) break; only the default fan-out behavior shifts, an explicitly accepted/documented CI-cost tradeoff. No security-sensitive surface touched; CI green and PR already APPROVED.
Downstream impact
This change is consumed by 8 downstream repo(s) that pin the affected reusable workflow / lib / prompt. Impacted consumers:
Impacted shared surfaces:
- .github/workflows/auto-rebase-reusable.yml
Impacted consumers (8, fetching up to 10):
- petry-projects/.github (pins .github/workflows/auto-rebase-reusable.yml)
.github/workflows/auto-rebase.yml
- petry-projects/.github-private (pins .github/workflows/auto-rebase-reusable.yml)
.github/workflows/auto-rebase.yml
- petry-projects/ContentTwin (pins .github/workflows/auto-rebase-reusable.yml)
.github/workflows/auto-rebase.yml
- petry-projects/TalkTerm (pins .github/workflows/auto-rebase-reusable.yml)
.github/workflows/auto-rebase.yml
- petry-projects/bmad-bgreat-suite (pins .github/workflows/auto-rebase-reusable.yml)
.github/workflows/auto-rebase.yml
- petry-projects/broodly (pins .github/workflows/auto-rebase-reusable.yml)
.github/workflows/auto-rebase.yml
- petry-projects/google-app-scripts (pins .github/workflows/auto-rebase-reusable.yml)
.github/workflows/auto-rebase.yml
- petry-projects/markets (pins .github/workflows/auto-rebase-reusable.yml)
.github/workflows/auto-rebase.yml
Findings
- info: Default eligibility change affects 8 consumers pinning auto-rebase-reusable.yml (petry-projects: .github, .github-private, ContentTwin, TalkTerm, bmad-bgreat-suite, broodly, google-app-scripts, markets). Informational only: change is backward-compatible (review-ready and ready_label still accepted as deprecated no-ops), so no consumer errors; they receive broader branch-update fan-out by default, which the linked issue documents as an accepted CI-cost tradeoff.
- info: merge_method=merge invariant preserved (line ~157) and now guarded by new test/workflows/auto-rebase/merge-method.bats; BASE_BRANCH remains used in the compare call after the stuck-notice block removal, so no dead variable. ShellCheck and bats CI pass.
- info: Gemini Code Assist's low-priority note on merge-method.bats:25 (prefer exit-status 1 over -ne 0) is already satisfied — the final assertion uses [ "$status" -eq 1 ]. SonarCloud quality gate passed; Codex review skipped due to usage limits (no finding). No unresolved actionable advisory findings.
- info: MCP run_secret_scanning tool not exposed in this environment; skipped per instructions (not treated as a block). No secrets, credentials, or tokens present in the diff on manual review; gitleaks CI check passed.
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.
Superseded by automated re-review at 8f70e32.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
Flips the auto-rebase reusable's eligibility default from review-ready to all, removes the now-dead approval/label plumbing (auto_rebase_has_current_approval, auto_rebase_has_ready_label, the ready_label gate, and the issue-#711 stuck-conflict notice that only fired on ineligible PRs), updates docs/standards accordingly, and adds a merge-method regression test locking update_method=merge. Triage's low-risk assessment is confirmed: the change is a removal-heavy simplification (95+/263-) with backwards-compatible handling for existing callers.
Linked issue analysis
Closes #927 (part of #926 AC1/AC3/AC5). All three asks are substantively addressed: (1) eligibility default flipped to all in auto-rebase-reusable.yml; (2) review-ready plumbing removed from lib/eligibility.sh and the workflow; (3) merge-method invariant locked via test/workflows/auto-rebase/merge-method.bats. One deliberate deviation: rather than making review-ready an unknown mode, it is kept as a deprecated alias for all (with a stderr warning), and ready_label is accepted-but-ignored — added in the fix-reviews commit in response to a reviewer flagging that existing reusable callers would otherwise break. This is a safer choice than the literal issue text and is covered by tests.
Findings
No blocking findings.
- Security: no new secret/token usage; the change removes per-PR API calls and removes the READY_LABEL interpolation into comment bodies. Inputs still flow through env vars into quoted shell — no injection surface added. Gitleaks, CodeQL (actions), agent-shield, and the Agent Security Scan are all green. (The run_secret_scanning MCP tool was unavailable in this environment; gitleaks CI covers the secret scan.)
- Correctness: eligibility.sh remains a pure predicate; unknown/empty modes still exit 2 and the workflow fails clearly on them. auto_rebase_post_comment_best_effort is still used by the blocked/conflict paths, so no dead code is left behind. Bats suites updated to pin the new behavior, including tests asserting the removed functions no longer exist.
- Minor (non-blocking): the README/ci-standards mode tables no longer mention the deprecated review-ready alias, so its aliasing behavior is only discoverable in eligibility.sh and the bats test. Fine to leave, since the alias is slated for removal.
- All 3 bot review threads (gemini, qodo x2) are resolved; the qodo breaking-callers concern was addressed by the deprecated alias + no-op ready_label input.
CI status
All latest check runs on 8f70e3265b22718096539d0974539552d68f80d4 are green: ShellCheck, Lint, bats (x2), CodeQL, Analyze (actions), agent-shield, Agent Security Scan, Secret scan (gitleaks), SonarCloud, npm audit, Graphite AI review — success; ecosystem audits (cargo/pip/pnpm/govulncheck) skipped as not applicable. Earlier CANCELLED dev-lead dispatch/ci-relay entries in the rollup are superseded runs. 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.
Superseded by automated re-review at 8f70e32.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
PR #931 implements issue #927 fully: flips the auto-rebase reusable's eligibility default from review-ready to all, removes the dead approval/label predicate plumbing, adds a regression test locking the update_method=merge invariant, and updates all four documentation surfaces consistently. Backward compatibility is handled thoughtfully (review-ready kept as a deprecated alias, ready_label kept as an ignored input so existing callers don't hit Actions validation errors). All CI green, all review threads resolved, human+bot approvals in place.
Linked issue analysis
Closes #927 (part of #926, AC1 + AC3 + reusable-side AC5 cleanup). All three requirements verified in the diff: (1) eligibility input default flipped to 'all' in auto-rebase-reusable.yml; (2) auto_rebase_has_current_approval and auto_rebase_has_ready_label removed from lib/eligibility.sh with bats tests asserting they are no longer defined, and the per-PR review/label API calls removed from the workflow loop; (3) new merge-method.bats asserts update_method=merge is present and the rebase method is never used. Docs (README, ci-standards.md, pull-request-limits ADR, standards workflow stub) all updated to match.
Findings
No blocking findings.
- Backward compat handled well:
review-readyis retained as a deprecated alias (stderr warning, treated asall) andready_labelis retained as an accepted-but-ignored input — existing callers will not break or hit workflow-call validation errors. Note the auto-generated PR description claims removed modes "fail clearly", but the actual (better) behavior is the deprecated alias; only truly unknown modes fail with exit 2. - Dead-code removal is consistent: the issue-#711 stuck-conflict notice only fired on eligibility-skipped PRs; under the new
alldefault no PR is skipped, so removing it is correct. If a restrictive mode is added later, that escalation path may need reintroduction. - Tests are sound: empty/unknown modes exit 2 (workflow surfaces ::error and exits 1); merge-method invariant is pinned by grep-based regression tests.
- Secret scanning MCP tool unavailable in this environment; gitleaks CI check passed (SUCCESS).
CI status
All code checks green at 8f70e32: ShellCheck, Lint, bats (x2), Lint and bats, CodeQL, Analyze (actions), Secret scan (gitleaks), Agent Security Scan, agent-shield, npm audit, SonarCloud (quality gate passed), Graphite AI Reviews. Two cancelled dev-lead dispatch/ci-relay runs at 01:16:37Z were concurrency-superseded by later successful runs (01:28–01:38Z). Ecosystem audits not applicable were skipped.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at 8f70e32.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
Flips auto-rebase default eligibility to 'all', removes the dead review-ready/approval/ready-label plumbing, and adds a bats regression guard locking the update_method=merge invariant. Implements issue #927 faithfully, with a pragmatic deviation: 'review-ready' is kept as a deprecated alias for 'all' (stderr warning) and 'ready_label' remains an accepted-but-ignored input so existing workflow_call callers do not break on input validation. Net -168 lines; tests and all four docs updated consistently.
Linked issue analysis
Closes #927 (part of initiative #926). All three acceptance items are substantively addressed: (1) eligibility default flipped from review-ready to all in auto-rebase-reusable.yml; (2) dead review-ready mode, approval predicate, ready-label predicate, and per-PR review/label API calls removed from lib/eligibility.sh and the reusable — the issue-#711 stuck-conflict notice block was also removed, correctly, since no valid mode can skip a PR anymore, making that path unreachable; (3) new test/workflows/auto-rebase/merge-method.bats asserts update_method=merge is present and the rebase method never appears. The deprecated-alias approach is a reasonable safety-preserving deviation from a literal removal, and is pinned by a test.
Findings
No blocking findings.
- Security: no new permissions, secrets, or injection surfaces; the change removes gh api calls and comment-posting logic. ELIGIBILITY is passed via env and quoted. Secret-scan MCP tool unavailable in this run; the gitleaks CI check passed.
- Behavior note (intentional per #926/#927 decision): existing callers still passing eligibility: review-ready now update every behind PR including drafts — the alias logs a deprecation warning to stderr and is covered by a bats test.
- Minor (non-blocking): the deprecated review-ready alias and ignored ready_label input are documented in the workflow input description but not in the README mode table; acceptable since they are deliberately being phased out.
- Tests coherently updated: dead functions asserted undefined via type -t, unknown/empty modes exit 2, alias exits 0.
CI status
All checks green: Lint/ShellCheck, bats (incl. new merge-method suite), CodeQL, SonarCloud quality gate, gitleaks secret scan, npm audit, AgentShield, CodeRabbit, Graphite AI — all SUCCESS. Two CANCELLED dev-lead dispatch/ci-relay entries are agent-automation runs superseded by later successful runs, not correctness checks. Zero unresolved review threads; reviewDecision is APPROVED (CodeRabbit + prior cascade).
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at 8f70e32.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
Flips the auto-rebase reusable's eligibility default from review-ready to all, deletes the now-dead approval/label predicates and the issue-#711 stuck-conflict notice (whose trigger path — skipped PRs — no longer exists), and adds a merge-method regression guard. Net -168 lines. The triage tier's low-risk assessment is confirmed; classified MEDIUM here only because it changes workflow automation behavior, with no security-relevant surface touched.
Linked issue analysis
Closes #927 and substantively addresses all three asks: (1) eligibility default flipped to 'all' in auto-rebase-reusable.yml; (2) review-ready predicates (auto_rebase_has_current_approval, auto_rebase_has_ready_label) and ready_label plumbing removed from lib/eligibility.sh and the workflow, with docs (README, ci-standards.md, ADR, standards stub) updated to match; (3) new test/workflows/auto-rebase/merge-method.bats pins the update_method=merge invariant. Deliberate soft-landing beyond the issue's letter: 'review-ready' is kept as a deprecated alias for 'all' (stderr warning) and ready_label is accepted-but-ignored, so existing callers don't break on dispatch — a reasonable compat choice that arose from review feedback (resolved qodo thread) and is covered by tests.
Findings
No blocking findings.
- Security: no secrets, no new permissions, no injection surface; the change removes two GitHub API calls per PR. Secret scan: run_secret_scanning MCP tool unavailable in this environment; gitleaks CI check is green and the diff contains no credential-like content.
- Correctness: auto_rebase_post_comment_best_effort remains used elsewhere in the reusable (blocked/conflict notices at head), so no dead code; conflicting PRs still surface a conflict notice via the retained path. Bats tests updated to cover all/unknown/empty/deprecated modes and assert the old predicates are gone.
- Minor (non-blocking): the deprecated review-ready alias is documented in code and tests but omitted from the README modes table; the auto-generated CodeAnt PR description claims review-ready 'now fails clearly', which contradicts the actual alias behavior — the code and tests are authoritative.
CI status
All substantive checks green at 8f70e32: Lint, ShellCheck, bats (x2), CodeQL, AgentShield, Agent Security Scan, gitleaks, SonarCloud, npm audit. Remaining CANCELLED/SKIPPED entries are dev-lead dispatch/ci-relay orchestration runs, not required checks. All 3 review threads resolved; reviewDecision is APPROVED; human-account comments are automated dev-lead status notes with no open questions.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at 8f70e32.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
Confirms the triage assessment. The PR flips the auto-rebase eligibility default to all, removes the dead review-ready gating (approval/label predicates, per-PR review+label API calls, the issue-#711 stuck-conflict notice), and locks the update_method=merge invariant with a new bats regression test. Docs (README, ci-standards, ADR, caller stub) are updated consistently. Net −168 lines; the change reduces API surface and comment-posting code paths rather than adding any.
Linked issue analysis
Closes #927 (part of #926, AC1+AC3+AC5 reusable-side cleanup). All three asks are substantively addressed: (1) default flipped from review-ready to all; (2) review-ready plumbing removed from lib/eligibility.sh and the reusable; (3) merge-method regression test added (test/workflows/auto-rebase/merge-method.bats) asserting update_method=merge and never rebase. One deliberate deviation: review-ready is kept as a deprecated alias for all (stderr warning) and ready_label is accepted-but-ignored, rather than hard-removed — a reasonable backward-compat choice for existing callers; the gate itself is gone as intended.
Findings
No blocking findings.
Informational:
- The deprecated
review-readyalias now silently behaves asall— any caller explicitly passingreview-readyexpecting gating will fan out to every behind PR with only a stderr warning. Intentional per #927 (the gate is removed), but worth knowing. - With the stuck-conflict notice removed, conflicting PRs will now surface via a failed update-branch attempt instead of a one-time comment; acceptable since under
allno PR is skipped-and-silent anymore. - Unknown/empty eligibility modes correctly fail with exit 2 (
::error+ exit 1 in the workflow), covered by tests. - The
run_secret_scanningMCP tool is not available in this environment; relied on the green gitleaks CI check. No secret-like content in the diff.
Risk rationale (MEDIUM, not LOW): non-trivial behavior change to org-wide CI automation (fan-out semantics for every adopting repo), though it removes code and reduces surface. No security anti-patterns, no new secret usage, shell variables properly quoted.
CI status
All substantive checks green at 8f70e3265b22718096539d0974539552d68f80d4: Lint, ShellCheck, bats, CodeQL, SonarCloud (quality gate passed, 0 new issues), gitleaks secret scan, npm audit, AgentShield, Agent Security Scan, CodeRabbit, Graphite AI review. Cancelled/skipped entries (dev-lead / dispatch, dev-lead / ci-relay, ecosystem-specific audits) are agent-orchestration or not-applicable jobs; later runs of the same checks succeeded. No unresolved review threads; latest reviews from coderabbitai and donpetry-bot are APPROVED.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at 8f70e32.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
Flips the auto-rebase reusable's eligibility default from review-ready to all, removes the now-dead approval/ready-label predicates and stuck-conflict notice, keeps review-ready as a deprecated alias (and ready_label as an accepted-but-ignored input) for caller compatibility, updates README/standards/ADR docs, and adds a merge-method.bats regression guard pinning update_method=merge. Confirms the triage tier's low-risk assessment; risk rated MEDIUM only because it is a non-trivial CI workflow logic change.
Linked issue analysis
Closes #927 (part of #926, AC1+AC3+AC5 reusable-side cleanup). All three asks are substantively addressed: (1) eligibility default flipped to 'all' in auto-rebase-reusable.yml; (2) dead review-ready mode and ready_label plumbing removed from lib/eligibility.sh and the reusable — review-ready survives only as a deprecated warn-and-pass alias, a deliberate compatibility choice made during the fix-reviews cycle so existing callers do not break, covered by a unit test; (3) merge-method invariant locked by the new test/workflows/auto-rebase/merge-method.bats asserting update_method=merge is present and rebase is never used.
Findings
No blocking findings.
- Non-blocking: the auto-generated CodeAnt PR description claims the removed review-ready mode now "fails clearly" — the code actually treats it as a deprecated alias for 'all' (warns on stderr, returns 0). The code and tests are consistent; only the generated description is inaccurate.
- Non-blocking: the deprecated review-ready alias is documented in code comments but not in the README mode table. Fine to leave until the alias is removed.
- Security: no new GitHub Actions smells — inputs reach the shell via the env: block (no untrusted ${{ }} interpolation in run:), the change removes two API calls and the comment-posting path, reducing attack surface. Secret-scanning MCP tool unavailable in this run; gitleaks CI check passed.
- All 3 bot review threads (gemini, qodo x2) are resolved; qodo's 'breaking reusable workflow contract' finding was addressed by the deprecated alias + ignored ready_label input.
CI status
All checks green: Lint, ShellCheck, bats (incl. new merge-method.bats), CodeQL, SonarCloud quality gate, gitleaks secret scan, agent-shield, npm audit. The CANCELLED dev-lead dispatch/ci-relay entries are superseded agent-orchestration runs, not test failures. reviewDecision: APPROVED; no pending review requests.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at 8f70e32.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
Confirms the triage assessment: this PR correctly implements issue #927. It flips the auto-rebase reusable's eligibility default from review-ready to all, deletes the dead approval/ready-label plumbing (auto_rebase_has_current_approval, auto_rebase_has_ready_label, the stuck-conflict notice from issue #711 that only applied to skipped PRs), updates docs/standards, and adds a bats regression guard locking the update_method=merge invariant. Net -168 lines; the change removes logic rather than adding it, with no new dependencies or security-sensitive surface.
Linked issue analysis
Issue #927 (part of #926, AC1 + AC3 + AC5 reusable-side cleanup) asks for: (1) default eligibility flipped to all — done in auto-rebase-reusable.yml; (2) removal of the dead review-ready mode and ready_label plumbing — done, with a pragmatic deviation: review-ready is kept as a deprecated alias that warns and behaves as all, and ready_label is accepted-but-ignored so existing callers don't hit Actions validation errors (a backward-compat choice made during the fix-reviews cycle, covered by tests); (3) a regression test locking update_method=merge — added in test/workflows/auto-rebase/merge-method.bats. The issue is substantively addressed.
Findings
No blocking findings.
Non-blocking notes:
- The auto-generated PR description (CodeAnt) is stale: it claims the removed
review-readymode "now fails clearly", but the current head treats it as a deprecated alias ofall(warns to stderr, returns 0). The code and tests are consistent; only the description lags. - README.md and ci-standards.md document only the
allmode and omit the deprecatedreview-readyalias. Acceptable for a deprecated path, but worth a line if the alias lives long. - Removing the issue #711 stuck-conflict notice is coherent: with
allas default no behind PR is silently skipped, so the skip-deadlock the notice addressed can no longer occur. - Secret scan: the
run_secret_scanningMCP tool was not available in this run; the gitleaks CI check passed and the diff adds no secret-like content.
CI status
All checks green at 8f70e32: Lint, ShellCheck, bats (eligibility + merge-method), CodeQL (actions), Secret scan (gitleaks), npm audit, Agent Security Scan, AgentShield, SonarCloud, CodeRabbit, Graphite AI all SUCCESS. The CANCELLED/SKIPPED entries are dev-lead dispatch/ci-relay orchestration jobs from earlier commits, not test checks. No unresolved review threads; CodeRabbit and donpetry-bot both approved.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at 8f70e32.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
Confirms the triage assessment: PR #931 correctly implements issue #927. It flips the auto-rebase reusable's eligibility default from review-ready to all, removes the dead approval/ready-label plumbing (auto_rebase_has_current_approval, auto_rebase_has_ready_label, and the issue-#711 stuck-conflict notice that only applied to skipped PRs), updates the README/standards/ADR docs to match, and adds a bats regression guard locking the update_method=merge invariant. Net -168 lines; the change removes logic and API calls rather than adding surface.
Linked issue analysis
Issue #927 (part of #926: AC1 + AC3 + reusable-side AC5 cleanup) asks for: (1) flip the eligibility default to all — done in auto-rebase-reusable.yml; (2) remove the dead review-ready mode and ready_label plumbing — done, with one deliberate deviation: review-ready is kept as a deprecated alias that warns to stderr and behaves as all, and ready_label is accepted-but-silently-ignored so existing callers do not hit Actions dispatch validation errors. This backward-compat softening was made in response to a qodo-code-review finding, is documented in the input description, and is pinned by bats tests; (3) a regression test asserting update-branch always uses update_method=merge and never rebase — added in test/workflows/auto-rebase/merge-method.bats, including a grep-status-eq-1 assertion hardened after a gemini finding to avoid false positives from grep errors.
Findings
No blocking findings.
- Security: the change reduces attack surface (removes two gh api calls per behind PR and the comment-posting stuck-notice path). ELIGIBILITY is passed via env, not interpolated into the script — no injection risk. No secrets, auth, or permissions changes.
- All 3 review threads (gemini x1, qodo x2) are resolved with verified fixes in the head commit: grep exit-status hardening in merge-method.bats, the deprecated review-ready/ready_label compat shims, and the ADR table correction.
- Minor note (non-blocking): the auto-generated CodeAnt PR description claims the removed review-ready mode now 'fails clearly', but the code intentionally treats it as a deprecated alias for all — the code and tests are the source of truth here.
- Secret scan: the run_secret_scanning MCP tool is unavailable in this environment; the gitleaks CI check passed, and the diff contains no credential-like content.
CI status
All required checks green: Lint, ShellCheck, bats, CodeQL, SonarCloud, AgentShield, Agent Security Scan, Secret scan (gitleaks), npm audit, CodeRabbit, Graphite AI Reviews. A couple of dev-lead dispatch/ci-relay runs show CANCELLED/SKIPPED — these are superseded agent-orchestration dispatches, not test failures. mergeStateStatus is BLOCKED pending branch-protection requirements, which this approval addresses.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Superseded by automated re-review at 8f70e32.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
Implements issue #927 faithfully: flips the auto-rebase eligibility default to 'all', removes the dead review-ready/approval/label plumbing from eligibility.sh and the reusable workflow, and adds a bats regression guard locking the update_method=merge invariant. Backward compatibility is handled thoughtfully (review-ready kept as a warning deprecated alias; ready_label kept as an ignored input so existing callers don't hit workflow validation errors). All CI validation checks green, all review threads resolved.
Linked issue analysis
Closes #927 (part of initiative #926). All three acceptance points are addressed: (1) eligibility default flipped from review-ready to all in auto-rebase-reusable.yml; (2) dead review-ready mode and ready_label plumbing removed from lib/eligibility.sh and the workflow (with deliberate deprecation shims for existing callers — a defensible superset of the issue's ask); (3) new test/workflows/auto-rebase/merge-method.bats asserts update-branch is always called with update_method=merge and never rebase — verified update_method=merge is present at the head SHA. Docs (README, ci-standards.md, ADR, standards caller stub) updated consistently.
Findings
No blocking findings.
- No security concerns. The change removes API calls and shell interpolation (the issue #711 stuck-conflict comment block); no new secrets usage, permissions, or untrusted-input interpolation. auto_rebase_post_comment_best_effort remains in use elsewhere (workflow lines 184, 228), so no dead code is left.
- Minor (informational): the auto-generated PR description says the removed review-ready mode "now fails clearly", but the code treats it as a deprecated alias for all (warns on stderr, returns 0). The code matches the issue intent and is tested; only the description is imprecise.
- Secret scan: the run_secret_scanning MCP tool is not available in this environment; the gitleaks CI check passed and the diff contains no credential-like content.
- Triage assessment confirmed: this is a low-risk cleanup/behavior-flip explicitly decided in #926; classified MEDIUM here only because it is a non-trivial workflow logic change.
CI status
All validation checks green: Lint, ShellCheck, bats (including the new merge-method.bats), CodeQL, SonarCloud, agent-shield, Secret scan (gitleaks), npm audit, Graphite AI review. The CANCELLED/SKIPPED entries (dev-lead dispatch / ci-relay) are agent-orchestration runs, not validation checks; later dispatch runs succeeded.
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: 8f70e3265b22718096539d0974539552d68f80d4
Review mode: triage-approved (single reviewer)
Summary
Confirmation review (triage-approved mode) of PR #931, which implements issue #927: flips the auto-rebase eligibility default from review-ready to all, removes the dead review-ready/approval/ready-label plumbing from lib/eligibility.sh and the reusable workflow, and adds a regression test locking the update_method=merge invariant. The triage assessment holds — the change is a net simplification (removes per-PR API calls and the stuck-conflict comment path; adds no secrets, permissions, or dependencies). I rate it MEDIUM rather than LOW because it changes reusable-workflow logic, but all decision gates pass.
Linked issue analysis
Issue #927 (closed via this PR) had three asks, all substantively addressed:
- Default flipped to
all—auto-rebase-reusable.ymlinput default changed; docs (README, ci-standards.md, ADR, standards/workflows/auto-rebase.yml) updated consistently. - Dead review-ready code removed —
auto_rebase_has_current_approvalandauto_rebase_has_ready_labeldeleted (with bats guards asserting they are no longer defined). Deliberate back-compat deviations:review-readyis kept as a deprecated alias forall(stderr warning), andready_labelis kept as an accepted-but-ignored input so existing callers do not hit GitHub Actions validation errors. Both are documented in the workflow header — a reasonable choice for an org-wide reusable. - Merge-method invariant locked — new
test/workflows/auto-rebase/merge-method.batsassertsupdate_method=mergeis present and the rebase method is never used.
Findings
No blocking findings.
- Secret scan: the
run_secret_scanningMCP tool is not available in this environment; the gitleaks CI check passed. No credentials, tokens, or.envcontent in the diff. - Security: no GitHub Actions security smells — the change removes
gh apicalls and a comment-posting path; no new permissions or secret usage. Unknown eligibility modes fail loudly (exit 2→::error→ job failure), covered by tests including the empty-mode case. - Nit (non-blocking): the eligibility check now depends only on the mode but still runs inside the per-PR loop; hoisting it would be marginally cleaner.
- Nit (non-blocking): the auto-generated PR description claims
review-ready"now fails clearly", which is stale — the final code treats it as a deprecated alias forall. - All 3 inline review threads (gemini, qodo ×2) are resolved; CodeRabbit approved; no unanswered human-reviewer questions (don-petry's review comments were addressed in the fix-reviews cycle that produced the current head SHA).
CI status
All quality gates green on 8f70e3265b22718096539d0974539552d68f80d4: Lint, ShellCheck, bats (×2), CodeQL, Analyze (actions), agent-shield, Agent Security Scan, Secret scan (gitleaks), npm audit, SonarCloud quality gate, CodeRabbit, Graphite AI Reviews. Two cancelled dev-lead orchestration runs from 01:16 UTC were superseded by later runs of the same checks (latest: dispatch=success, ci-relay=skipped); they are agent-dispatch jobs, not code-quality gates.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
2 participants
User description
Closes #927
Implemented by dev-lead agent. Please review.
CodeAnt-AI Description
Auto-update every behind pull request while preserving commits and approvals
What Changed
review-readymode, now fail clearly.Impact
✅ Fewer pull requests left behind after merges✅ Preserved commit history and approvals during branch updates✅ Clearer configuration errors💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
alleligibility mode.Bug Fixes
Tests
Documentation