Repository navigation
feat: implement issue #949 — P2 (#860 follow-up): keep agent branches rebased/squashed instead of merge-stacked - #1053
Conversation
… rebased/squashed instead of merge-stacked
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'merge-stacked' (rebase/squash candidate) detector to the runaway PR scanner, flagging PRs with a disproportionately high commit count relative to the number of changed files. It includes the core detection logic, reporting updates, and comprehensive test coverage. Feedback is provided regarding an optimization in pr_runaway_scan.sh to avoid spawning a subshell and forking sed for every scanned PR by combining the reason strings using pure Bash.
There was a problem hiding this comment.
Pull request overview
Adds a “merge-stacked” smell detector to the existing runaway PR detection tooling to flag PRs that look like they were repeatedly merge-updated (e.g., very high commit count but small net diff), encouraging rebase/squash behavior as part of the #860 remediation follow-ups.
Changes:
- Introduces
pr_rebase_smellin the runaway detection library to detect “merge-stacked” PRs using commit and changed-file thresholds (env-overridable). - Updates the daily scan script to fetch
changed_filesfrom the PR API and include the new smell reason in reported candidate triggers. - Extends the Bats unit test suite to cover the new smell detector behavior and reporting text.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_pr_runaway_detect.bats | Adds unit tests for the new merge-stacked rebase/squash smell detector and verifies report text mentions the criterion. |
| scripts/pr_runaway_scan.sh | Fetches changed_files and combines pr_rebase_smell output with existing runaway reasons in the daily scan report. |
| scripts/lib/pr-runaway-detect.sh | Implements pr_rebase_smell and updates report generation to document the merge-stacked detection criterion. |
|
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. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 999903a3d1acbfce4f9b1b1d90efd8aea76da0b2
Review mode: triage-approved (single reviewer)
Summary
Adds a merge-stacked (rebase/squash candidate) smell detector to the runaway-PR scanner per issue #949: flags PRs whose commit count vastly exceeds their changed-file count (>50 commits with <20 files, env-overridable), wires it into the daily scan report, and adds 9 bats tests. Detection-only — no PR is mutated, no workflow or permission changes, and the auto-rebase/stable pin and #655 guards are untouched. Triage assessment confirmed.
Linked issue analysis
Closes #949 (P2, #860 follow-up). The issue offered two scoping options; this PR implements the explicitly-sanctioned smell-detector path with the issue's exact suggested thresholds (>50 commits, <20 files). Acceptance criteria met: smell detector in place with documented rationale in code comments; no regression to the #655 stale-base guards or the auto-rebase/stable pin (neither file touched); bats coverage added for all new logic including boundary and degradation cases.
Findings
- No security issues. No secrets in the diff (gitleaks CI check passed; run_secret_scanning MCP tool not available in this session — noted, non-blocking). No injection surface: all inputs pass through the existing _runaway_int/_runaway_threshold sanitizers before use.
- Correctness verified: strict > / < comparisons match the documented semantics and are boundary-tested; the 0-file guard prevents false positives on API glitches; pr_rebase_smell always returns 0 so set -e callers are safe.
- Non-blocking: one unresolved gemini-code-assist thread suggests replacing the printf|sed reason-combining with pure bash to avoid a per-PR fork. Functionally equivalent; overhead is negligible for a daily scan. Worth considering in a follow-up, not a merge blocker.
CI status
All 4 required checks green: SonarCloud, CodeQL, agent-shield / AgentShield, dependency-audit / Detect ecosystems. All substantive optional checks green too (shellcheck, bats, unit-tests, Agent Security Scan, gitleaks, holdout-guard, test-deletion guard). Two CANCELLED entries (dev-lead / dispatch, dev-lead / ci-relay) are non-required agent-orchestration duplicates from a superseded run; the parallel Dev-Lead run completed successfully.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Dev-Lead — rate-limited (intent: review-changes)PR: #1053 |
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 — fix-bot-comment (no-changes)Agent reasoning |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
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: f7ac210ffd0b8319510a7859d2ade64ebdf2c81e
Review mode: triage-approved (single reviewer)
Summary
Adds a merge-stacked (rebase/squash candidate) smell detector to the runaway-PR scanner per issue #949: pr_rebase_smell flags PRs whose commit count vastly exceeds their changed-file count (>50 commits AND 0<files<20, both env-tunable), wires it into the daily scan and report, and adds 9 bats tests. Since the prior (dismissed) MEDIUM approval at 999903a, the only substantive change is a review-changes commit that resolved gemini's per-PR sed-fork feedback with pure-bash reason concatenation; the rest is merges of main.
Linked issue analysis
Issue #949 (P2, #860 follow-up) offered two paths: full rebase automation or a smell detector. This PR implements the smell-detector path with the issue's suggested thresholds (>50 commits, <20 files). Acceptance criteria met: coverage for the new logic (9 bats tests incl. the exact #860 signature 378/18), and no regression risk to the #655 guards or the auto-rebase/stable pin — .github/workflows/auto-rebase.yml is untouched.
Findings
No blocking findings.
- pr_rebase_smell uses defensive integer parsing (_runaway_int/_runaway_threshold), strict > / < boundary semantics, and a deliberate 0-file guard against API glitches; tests assert all of these plus env-override behavior.
- The reason-combination in pr_runaway_scan.sh is pure bash (no subshell/sed fork), resolving the sole review thread (gemini medium-priority; addressed by don-petry, thread resolved).
- No auth/secret/credential/workflow-security surface touched. run_secret_scanning MCP tool unavailable this session; gitleaks CI check is green.
- Minor irony, not a blocker: the branch itself carries several 'Merge branch main' commits — exactly the pattern the detector targets — but that is the repo's current update mechanism.
CI status
All required checks green: shellcheck, ShellCheck, bats, unit-tests, Lint, CodeQL (actions+python), Secret scan (gitleaks), SonarCloud, Agent Security Scan, agent-shield, holdout-guard, template-drift, prompt-coverage, validate-fixtures, and all stub/permission guards. Skipped checks are ecosystem-conditional audits.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Closes #949
Implemented by dev-lead agent. Please review.