feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) - #1312
Conversation
…T author — races the human driver (dropped a commit)
|
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: 52 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)
📝 WalkthroughWalkthroughThe PR adds dev-lead authorship checks to intent routing, skips non-dev-lead-authored events, preserves human on-mention handling, and introduces guarded pushes that prevent overwriting unseen remote commits. ChangesDev-lead authorship gating
No-clobber push protection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant IntentScript as dev-lead-intent.sh
participant AuthorshipCheck as is_dev_lead_authored
participant Intent
GitHub->>IntentScript: webhook event
IntentScript->>AuthorshipCheck: inspect PR branch and author
AuthorshipCheck-->>IntentScript: dev-lead-authored or false
IntentScript->>Intent: emit review/fix intent
IntentScript->>Intent: emit skip(not-dev-lead-authored)
sequenceDiagram
participant ReviewFix as commit_and_push
participant PushGuard as push_no_clobber
participant Remote as Git remote
ReviewFix->>PushGuard: push branch
PushGuard->>Remote: normal git push
Remote-->>PushGuard: success or non-fast-forward rejection
PushGuard->>Remote: force-with-lease when local history diverged safely
Remote-->>ReviewFix: final push result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #1312 |
|
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. |
![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 PR-authorship gate and a no-clobber push mechanism (push_no_clobber) to prevent the dev-lead bot from acting on human-authored PRs or overwriting concurrent remote commits. Feedback on these changes includes improving jq queries in dev-lead-intent.sh to safely handle missing or null parent objects, using POSIX-compliant head syntax with || true to prevent SIGPIPE failures under pipefail, and refactoring BATS tests to use $BATS_TEST_TMPDIR instead of manual mktemp and teardown cleanups.
There was a problem hiding this comment.
Pull request overview
This PR fixes the dev-lead automation defect from #1311 where dev-lead could seize human-authored PRs (triggered by trusted reviews/comments), push changes, and drive them to merge—creating concurrent-writer races and potentially dropping human commits. It adds an explicit PR-authorship gate in dev-lead-intent.sh and a no-clobber push helper to prevent overwriting unseen remote commits.
Changes:
- Add an authorship gate so fix/push/merge intents only run on PRs authored by dev-lead (branch
dev-lead/issue-*or author login ==BOT_USER), otherwiseskip not-dev-lead-authored. - Add
push_no_clobberto ensure dev-lead never overwrites remote commits it hasn’t fetched (force only via--force-with-leaseand only on rewritten history). - Add/adjust unit + e2e coverage and fixtures to lock in the new routing and push safety behavior.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/dev-lead-intent.sh | Adds is_dev_lead_authored and gates PR-driving intents to dev-lead-authored PRs; emits skip not-dev-lead-authored otherwise. |
| scripts/lib/git-push-guard.sh | Introduces push_no_clobber to avoid clobbering unseen commits; retries with --force-with-lease only on rewritten-history divergence. |
| scripts/dev-lead-fix-reviews.sh | Switches push step to push_no_clobber to prevent concurrent-writer overwrite. |
| tests/dev-lead/unit/test_intent_stub.bats | Updates expected routing for human-opened PRs to skip with not-dev-lead-authored. |
| tests/dev-lead/unit/test_intent_authorship.bats | Adds focused unit coverage for the authorship gate across review/comment event types (including fail-closed indeterminate payload). |
| tests/dev-lead/unit/test_push_no_clobber.bats | Adds unit tests validating fast-forward push, lease-based rewrite push, and refusal to overwrite when remote advanced unseen. |
| tests/dev-lead/e2e/scenarios/05-skip-anti-loop.sh | Updates e2e expectation: human sync on human-authored branch is now skipped as not-dev-lead-authored. |
| tests/dev-lead/fixtures/events/pr_opened_dev_lead.json | Adds fixture for dev-lead-authored PR open routing. |
| tests/dev-lead/fixtures/events/pr_review_bot_human_authored.json | Adds fixture for bot review on human-authored PR → skip. |
| tests/dev-lead/fixtures/events/pr_review_comment_bot_human_authored.json | Adds fixture for bot review comment on human-authored PR → skip. |
| tests/dev-lead/fixtures/events/issue_comment_bot_human_authored.json | Adds fixture for bot issue comment on human-authored PR → skip. |
| tests/dev-lead/fixtures/events/pr_review_bot_dev_lead_authored.json | Adds fixture for bot review on dev-lead-authored PR → fix-reviews. |
| tests/dev-lead/fixtures/events/pr_review_indeterminate_author.json | Adds fixture ensuring authorship indeterminate → fail-closed skip. |
| tests/dev-lead/fixtures/events/pr_review_human_owner.json | Adjusts fixture head ref to dev-lead branch naming to align with authorship gate expectations. |
| tests/dev-lead/fixtures/events/pr_review_gemini_changes.json | Adjusts fixture head ref formatting to dev-lead branch naming. |
| tests/dev-lead/fixtures/events/pr_review_copilot_commented.json | Adjusts fixture head ref formatting to dev-lead branch naming. |
| tests/dev-lead/fixtures/events/pr_review_copilot_approved.json | Adjusts fixture head ref formatting to dev-lead branch naming. |
| tests/dev-lead/fixtures/events/pr_review_comment_copilot.json | Adjusts fixture head ref formatting to dev-lead branch naming. |
| tests/dev-lead/fixtures/events/pr_review_comment_codex.json | Adjusts fixture head ref formatting to dev-lead branch naming. |
| tests/dev-lead/fixtures/events/pr_review_codex_commented.json | Adjusts fixture head ref formatting to dev-lead branch naming. |
| tests/dev-lead/fixtures/events/pr_review_coderabbit_approved.json | Adjusts fixture head ref formatting to dev-lead branch naming. |
| tests/dev-lead/fixtures/events/issue_comment_sonarqube.json | Updates fixture to include issue author login (dev-lead bot) for authorship gating. |
| tests/dev-lead/fixtures/events/issue_comment_coderabbit.json | Updates fixture to include issue author login (dev-lead bot) for authorship gating. |
![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/dev-lead-fix-reviews.sh`:
- Around line 1020-1023: Remove the generic echo error message from the
push_no_clobber failure handler, leaving push_no_clobber responsible for all
formatted error annotations; preserve the existing exit 1 behavior when the
command fails.
In `@scripts/lib/git-push-guard.sh`:
- Around line 1-2: Add `set -euo pipefail` immediately after the bash shebang in
git-push-guard.sh, preserving the existing script header and applying the
required strict shell options.
🪄 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: d7328d38-45d1-44fc-b065-c855550bf475
📒 Files selected for processing (23)
scripts/dev-lead-fix-reviews.shscripts/dev-lead-intent.shscripts/lib/git-push-guard.shtests/dev-lead/e2e/scenarios/05-skip-anti-loop.shtests/dev-lead/fixtures/events/issue_comment_bot_human_authored.jsontests/dev-lead/fixtures/events/issue_comment_coderabbit.jsontests/dev-lead/fixtures/events/issue_comment_sonarqube.jsontests/dev-lead/fixtures/events/pr_opened_dev_lead.jsontests/dev-lead/fixtures/events/pr_review_bot_dev_lead_authored.jsontests/dev-lead/fixtures/events/pr_review_bot_human_authored.jsontests/dev-lead/fixtures/events/pr_review_coderabbit_approved.jsontests/dev-lead/fixtures/events/pr_review_codex_commented.jsontests/dev-lead/fixtures/events/pr_review_comment_bot_human_authored.jsontests/dev-lead/fixtures/events/pr_review_comment_codex.jsontests/dev-lead/fixtures/events/pr_review_comment_copilot.jsontests/dev-lead/fixtures/events/pr_review_copilot_approved.jsontests/dev-lead/fixtures/events/pr_review_copilot_commented.jsontests/dev-lead/fixtures/events/pr_review_gemini_changes.jsontests/dev-lead/fixtures/events/pr_review_human_owner.jsontests/dev-lead/fixtures/events/pr_review_indeterminate_author.jsontests/dev-lead/unit/test_intent_authorship.batstests/dev-lead/unit/test_intent_stub.batstests/dev-lead/unit/test_push_no_clobber.bats
|
@coderabbitai resolve |
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
✅ Action performedComments resolved and changes approved. |
Auto-dismissed (#617): coderabbitai[bot] CHANGES_REQUESTED on a superseded commit. The bot re-reviews the new head automatically — a valid concern will return as a fresh review.
|
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 1107d85f50dbe3a84c107385a3f842a587c14842
Review mode: triage-approved (single reviewer)
Summary
Implements the #1311 fix: dev-lead's fix/push/merge intents are now gated on PR authorship (head branch dev-lead/issue-* or author == BOT_USER), emitting skip not-dev-lead-authored on human-authored PRs and failing closed when authorship is indeterminate. Adds push_no_clobber (scripts/lib/git-push-guard.sh) as defense-in-depth: plain push first, --force-with-lease only for rewritten history, hard refusal when the remote advanced past the last-fetched ref — never a bare --force. 23 files changed (+684/−51), the bulk being bats tests and event fixtures.
Linked issue analysis
Closes #1311 (dev-lead seized and merged a human's in-flight PR, dropping a commit — the #1303 incident). All four acceptance criteria are substantively addressed:
- Authorship gate on every fix/push/merge intent path (
pull_requestopened/synchronize,pull_request_review,pull_request_review_comment,issue_comment) — verified in the diff and covered bytest_intent_authorship.bats. emit_skip "not-dev-lead-authored"with bats coverage for the human-authored-PR case, plus the e2e scenario 05 updated to assert the human sync is left alone.push_no_clobberaborts on a moved remote head via--force-with-lease(lease = last-fetched remote-tracking ref; deliberately no re-fetch before the retry), with real-git-remote tests proving the unseen commit survives, and a meta-test forbidding bare--force.- Fails closed: indeterminate authorship (no head ref AND no author) returns skip — covered by
pr_review_indeterminate_author.json.
The explicit-human@dev-leadon-mention path is correctly exempt (direct authorization), matching the issue's "advisory-only participation on human PRs is fine" requirement.
Findings
No blocking findings.
- Correctness verified:
EVENT_PATHandBOT_USERare defined (lines 139–140 of the head version) beforeis_dev_lead_authoredis invoked; shellcheck is clean on both changed scripts. - Non-blocking note: the branch-name half of the gate (
dev-lead/issue-*) could in principle be satisfied by a collaborator naming a branch that way, but fork PRs are already skipped and same-repo branch creation requires write access — acceptable. - Non-blocking note:
push_no_clobber's lease can be refreshed if an interveninggit fetchruns between checkout and push; the primary authorship gate makes this residual window advisory-only. Detection of non-fast-forward relies on grep of git's English stderr, which is stable on C-locale CI runners. - Prior bot review threads (gemini ×6, coderabbit ×2) are all resolved; CodeRabbit's final review is an approval. The omission of
set -euo pipefailin the sourced lib matches the dominant scripts/lib convention. - Secret scanning:
run_secret_scanningMCP tool unavailable in this run — relied on the green gitleaks check; no credential-like content in the diff.
CI status
All required checks green: shellcheck, ShellCheck, bats, unit, unit-tests, validate-fixtures, prompt-coverage, CodeQL (actions+python), agent-shield, Agent Security Scan, Secret scan (gitleaks), SonarCloud, actionlint, gh-aw-compile, holdout-guard, caller-stub/permissions suite, CodeRabbit — SUCCESS. The CANCELLED entries (dev-lead dispatch/ci-relay, one review/review, one Dismiss) are superseded duplicate runs, each with a later SUCCESS where required. mergeStateStatus BLOCKED reflects only the pending review requirement this review satisfies.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
…T author — races the human driver (dropped a commit) (#1312) * feat: implement issue #1311 — dev-lead fixes-and-merges PRs it did NOT author — races the human driver (dropped a commit) * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
Closes #1311
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit
New Features
@dev-leadmentions remain available regardless of pull request authorship.Tests