Repository navigation
fix(dev-lead): trusted-author tracker links; no auto-merge after a failed deferral (#2045 follow-up) - #2087
Conversation
…2034) FORCE_REVIEW bypasses every review gate, but it was derived from event_name == repository_dispatch alone. The mention listener also dispatches for machine-added 'review_requested: donpetry-bot' events, so automated re-review requests approved past the maintainer-comment gate and the next event run dismissed the approval, looping until the per-PR automation budget ran out. FORCE_REVIEW now requires client_payload.force_review == true. A dispatch without it runs with every gate armed plus the narrow FORCE_RE_REVIEW idempotency bypass, so it can neither approve over an open finding nor re-run a cascade whose verdict already stands at the head. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019K6UsD7r25pAcRjJL3G6Kx
…ct expressions Test-Change-Justification: file is new in this PR (#2078); review threads asked for yq parsing and full-expression assertions
…iled deferral (#2045 follow-up) Follow-up to #2049 for the two major findings in its final pr-review cycle. - deferred-thread-verify: a tracking issue can back a deferral only through text a trusted author wrote. The issue body counts only when the issue's author_association is OWNER/MEMBER/COLLABORATOR, and only comments by such authors count. Previously anyone able to open an issue with the tracker title, or comment on it, could authorize resolving a merge-blocking bot thread. A link from an untrusted author now reports `untrusted-mention` and the thread stays open. - fix-reviews / review-changes / fix-bot-comment: a failed deferral now sets _REVIEW_INCOMPLETE, so try_enable_auto_merge never runs on a pass that did not finish resolving its review items (the #1567 guard). fix-bot-comment's guard now honors the flag too. - Tests: untrusted issue body, untrusted comment, trusted comment on an untrusted issue, an untrusted tracker leaving the thread open end to end, and no auto-merge after a failed deferral on a commit pass. Existing tracker fixtures now carry author_association. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019K6UsD7r25pAcRjJL3G6Kx
…via #2078; superseded) — keep main + follow-up tree
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 10 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (4)
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 |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
don-petry
added
the
dev-lead:hands-off
Claude
![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 restricts the authorization of deferred review findings to trusted author associations (OWNER, MEMBER, and COLLABORATOR) to prevent unauthorized users from resolving merge-blocking bot threads. It also ensures that if a deferred review cannot be resolved, the auto-merge feature is not enabled. Unit tests have been updated and added to verify these behaviors. I have no feedback to provide as there are no review comments.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
CodeAnt PR Risk: Low Risk
Assessed commit: |
… the EXIT-trap restore _REVIEW_INCOMPLETE stopped try_enable_auto_merge, but the EXIT trap's restore_auto_merge still re-enabled auto-merge that hold_auto_merge had turned off at the start of the pass. The three deferral-failure branches now also set _AM_NEEDS_RESTORE=0, matching the existing #2037 path. Tests: a failed deferral leaves held-off auto-merge off; a successful one still turns it back on (the stub can now start a PR with auto-merge on). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019K6UsD7r25pAcRjJL3G6Kx
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #2087 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but I can't mark it done yet: required check |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
dev-lead is withholding action on this item. It is labeled To re-enable automated pickup: remove the |
|
|
No description provided. |
|
No description provided. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 692f8a2e05aaca49479e161e29daaca5d9fadb92
Review mode: triage-approved (single reviewer)
Summary
Follow-up to #2049 that fixes two fail-open gaps in the #2045 deferred-thread flow. (1) dtv_verify_tracking_issue now accepts a thread link only from the tracker issue body when the issue's author_association is OWNER/MEMBER/COLLABORATOR, or from comments by such authors. A link from an untrusted author returns untrusted-mention and the thread stays open. (2) All three deferral-failure branches (fix-reviews, review-changes, fix-bot-comment) now set _REVIEW_INCOMPLETE=1 and _AM_NEEDS_RESTORE=0, and the fix-bot-comment auto-merge guard now honors _REVIEW_INCOMPLETE. A pass whose deferral resolution failed therefore can neither enable auto-merge nor restore it through the EXIT trap. Both changes only fail closed. I confirm the triage-approved assessment, but rate the PR MEDIUM rather than LOW because it changes authorization and auto-merge logic in the dev-lead harness.
Linked issue analysis
Refs #2045 (closed): deferred review threads had no resolvable outcome. This PR does not close an issue. It fixes the two major findings from #2049's final review cycle, which came in after #2049 had already merged. Both findings are addressed: the trust model is tightened, and #1567's "never auto-merge an incomplete pass" guarantee now covers deferral failures, including the EXIT-trap restore path.
Findings
No blocking findings.
- Trust check (
scripts/lib/deferred-thread-verify.sh):author_associationis upper-cased and matched against a fixed allow-list. A missing value falls back to "" and is treated as untrusted (fail closed). The issue body is checked only when the issue author is trusted. Comments are filtered one by one, so a trusted comment on an untrusted author's issue still verifies, which is intended and tested. The extrauntrusted-mentionprobe affects only the log reason; the function still returns 1. - Auto-merge guard (
scripts/dev-lead-fix-reviews.sh): In fix-reviews and review-changes,try_enable_auto_mergewas already gated on_REVIEW_INCOMPLETE, so setting the flag in the deferral-failure branch closes the commit-pass gap wherefinalize_review_applicationwas skipped. For fix-bot-comment, the one-line guard change adds the same check. Setting_AM_NEEDS_RESTORE=0matches the existing #2037_DISPOSITIONS_UNRESOLVEDhandling. - Non-blocking, operational: if a tracker issue is ever opened by an identity whose
author_associationresolves to NONE or CONTRIBUTOR (e.g. a GitHub App token, or a member whose org membership is private and not visible to the token), deferrals against that tracker will never auto-resolve. This fails safe, and the PR's Monitoring section already calls out watching foruntrusted-mention. - Non-blocking, already discussed: cubic noted that suppressing the restore loses a non-squash auto-merge method across the retry. The author declined with sound reasoning: the behavior matches the existing #2037 path and squash is the repo default. The thread is resolved.
- Tests: 3 new unit tests (untrusted body, untrusted comment, trusted comment on an untrusted author's issue) and 4 new integration cases (untrusted tracker stays open; failed deferral does not restore auto-merge; successful deferral still restores it; failed deferral on a commit pass never enables auto-merge). Existing fixtures were updated to carry
author_association.shellcheck --severity=warningis clean on both changed scripts locally. - Secret scan: the MCP
run_secret_scanningtool is not available in this run, so it was skipped. The gitleaks CI check is green. - All 3 bot review threads are resolved. No human reviewer has asked an unanswered question.
CI status
All required checks pass: bats, unit-tests, unit, shellcheck/ShellCheck, CodeQL, SonarCloud, gitleaks, prompt-coverage, actionlint, AgentShield and the standards checks. The dev-lead / dispatch|ci-relay|resume jobs show CANCELLED; they are dispatch-concurrency noise, consistent with the dev-lead:hands-off label, not test failures. The dependency-audit ecosystem jobs were skipped as not applicable.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.
|
pr-review approved on PARTIAL advisory evidence: 4/5 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596). |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 692f8a2e05aaca49479e161e29daaca5d9fadb92
Review mode: triage-approved (single reviewer)
Summary
Follow-up to #2049 that closes the two major findings from its last review cycle. Both changes only fail closed. (1) dtv_verify_tracking_issue now accepts a tracker-issue link only from the issue body when the issue's author_association is OWNER, MEMBER or COLLABORATOR, or from comments by those authors. A link written only by an untrusted author returns untrusted-mention and the thread stays open. (2) All three deferral-failure branches (fix-reviews, review-changes, fix-bot-comment) now set _REVIEW_INCOMPLETE=1 and _AM_NEEDS_RESTORE=0, so a pass that failed to resolve a deferral can't enable auto-merge or restore it on exit. fix-bot-comment's auto-merge guard now honors _REVIEW_INCOMPLETE. I agree with the triage tier's low-risk call; I rate it MEDIUM because it changes dev-lead harness logic.
Linked issue analysis
Refs #2045 (closed), the deferred-thread resolution feature first shipped in #2049. This PR fixes the post-merge findings on that feature: the tracker trust model and the #1567 auto-merge guard after a failed deferral. It uses Refs rather than Closes, which is correct for a follow-up to an already-closed issue.
Findings
No blocking findings.
- Ordering is correct. Each new
_REVIEW_INCOMPLETE=1/_AM_NEEDS_RESTORE=0assignment runs before that intent'stry_enable_auto_mergegate (fix-reviews ~L3256 → L3305, fix-bot-comment ~L3405 → L3430, review-changes ~L3536 → L3575). Clearing_AM_NEEDS_RESTOREfollows the existing #2037_DISPOSITIONS_UNRESOLVEDpattern. The P2 thread about losing the merge strategy across the dropped restore was answered with a reasoned won't-fix. - The trust filter fails closed. A missing or unknown
author_associationcounts as untrusted. Values are upper-cased before the comparison. The extrauntrusted-mentioncheck is diagnostic only and always returns 1. - Tests cover the new paths. There are new unit tests for an untrusted body, an untrusted comment, and a trusted comment on an untrusted author's issue. Harness tests cover an untrusted-only tracker, no auto-merge restore after a failed deferral, restore still happening after a successful deferral, and no auto-merge enable on a commit pass with a failed deferral. Existing fixtures were updated with
author_association: MEMBER. - Non-blocking note: if a tracker issue were created by an identity whose association reads as
NONEorCONTRIBUTOR(for example a GitHub App bot token, or concealed org membership), its deferrals would now stay open asuntrusted-mentioninstead of resolving. That is safe, and the PR's Monitoring section already calls out watching for it.
All 3 review threads (cubic, codeant) are resolved. There are no unanswered human questions.
CI status
All required checks pass: shellcheck, bats, unit-tests, CodeQL, SonarCloud (Quality Gate passed), gitleaks, AgentShield, actionlint and the stub/standards gates. A few duplicate dev-lead / dispatch|resume|ci-relay runs show CANCELLED or SKIPPED, but each has a matching SUCCESS run, so this is concurrency noise, not a failure.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). 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: 692f8a2e05aaca49479e161e29daaca5d9fadb92
Review mode: triage-approved (single reviewer)
Summary
Follow-up to #2049 that closes the two remaining fail-open gaps in the #2045 deferred-thread flow. Both changes fail closed. (1) dtv_verify_tracking_issue now accepts a thread link only from the tracker issue body when the issue's author_association is OWNER, MEMBER or COLLABORATOR, or from comments by such authors. A link written only by an untrusted author returns untrusted-mention, and the thread stays open. (2) All three deferral-failure branches (fix-reviews, review-changes, fix-bot-comment) now set _REVIEW_INCOMPLETE=1 and _AM_NEEDS_RESTORE=0, and fix-bot-comment's auto-merge guard now checks _REVIEW_INCOMPLETE. A pass whose deferral resolution failed can no longer enable or restore auto-merge (#1567). The triage assessment holds.
Linked issue analysis
Refs #2045 (closed), which added the deferred/out-of-scope outcome for review threads. This PR fixes the two major findings from #2049's final review cycle: an untrusted author could authorize auto-resolving a tracker-linked thread, and auto-merge could be enabled after a deferral failed. Both are addressed directly, with regression tests. No closing references, which is correct for a follow-up.
Findings
No blocking findings.
- Trust filter (
scripts/lib/deferred-thread-verify.sh):$trusted | index($a)is applied to both the issue body (via the issue'sauthor_association) and each comment. A missing or empty association reads as untrusted, so it fails closed.untrusted-mentionis computed only after both trusted checks fail, so it is only a diagnostic. The caller (resolve_deferred_bot_threads) treats it likeno-mention: it logs a notice, leaves the thread open, and does not count it as a failure. The RESTissues/{n}andissues/{n}/commentspayloads already carryauthor_association, so no new API calls are needed. - Auto-merge guard (
scripts/dev-lead-fix-reviews.sh): fix-reviews and review-changes already gatedtry_enable_auto_mergeon_REVIEW_INCOMPLETE. The new assignment closes the commit-pass gap, wherefinalize_review_applicationwas skipped. Clearing_AM_NEEDS_RESTOREstops the EXIT trap from re-enabling auto-merge that was held off at the start. Both points were raised by cubic and CodeAnt, and those threads are resolved. fix-bot-comment's one-line guard now matches the other two branches. - Tests: 3 new verifier unit tests (untrusted issue body, untrusted comment, trusted comment on an untrusted author's issue). 4 new harness tests (untrusted tracker stays open; a failed deferral does not restore auto-merge; a successful deferral still restores it; a failed deferral on a commit pass never enables auto-merge). Existing fixtures were updated with
author_association: MEMBER. - Non-blocking note: the tracker issue and its linking comments must now be written by an OWNER, MEMBER or COLLABORATOR account. If dev-lead ever writes to the tracker as a GitHub App or other non-collaborator identity, deferred threads will stay open. That fails closed, and the PR's Monitoring section already says to watch for
untrusted-mention. - Secret scan: the
run_secret_scanningMCP tool was not available in this run. The gitleaks CI check passed.
CI status
All required checks pass: shellcheck, ShellCheck, bats, unit-tests, prompt-coverage, CodeQL (actions and python), SonarCloud quality gate, gitleaks secret scan, AgentShield, actionlint, and the workflow and stub validators. Some dev-lead / dispatch, resume and ci-relay runs show as cancelled, skipped or queued. Those are dev-lead orchestration runs, held by the dev-lead:hands-off label, not code checks, and other runs of the same jobs succeeded. All review threads are resolved, and there are no unanswered human questions.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.
2 participants
Summary
Follow-up to #2049. It fixes the two major findings from #2049's final pr-review cycle, which were found after the approved head (
7bf83ec1) had merged:dtv_verify_tracking_issueaccepted a thread link from any author.author_associationisOWNER,MEMBERorCOLLABORATOR, and only comments by those authors count.untrusted-mention, and the thread stays open.finalize_review_applicationwas skipped and_REVIEW_INCOMPLETEstayed unset. Sotry_enable_auto_mergestill ran._REVIEW_INCOMPLETE=1.Problem
Refs #2045.
dev-lead: deferred review findings, or to comment on it, could authorize auto-resolving a merge-blocking bot review thread.Risk
Medium. This changes the dev-lead harness, which runs only after merge and reaches consumers through ring promotion.
Both changes only fail closed:
The REST issue and comment payloads already carry
author_association, so no new API calls are needed.Test plan
shellcheck --severity=warning scripts/*.sh scripts/lib/*.sh: clean.bats tests/dev-lead/unit/test_deferred_thread_verify.bats: 39/39 pass. Three tests are new: an untrusted issue body, an untrusted comment, and a trusted comment on an untrusted author's issue.bats tests/dev-lead/unit/test_fix_reviews.bats: 222 tests. Only the three that also fail onmainin my local environment fail here (dev-lead reports status=applied / 'Changes committed and pushed' when the requested review changes were not applied #1567 whitespace evidence, and the two dev-lead fix-reviews reads reviewThreads(first:50) with no pagination — open threads past the first 50 are invisible, so the pass does nothing #2056 pagination tests).a tracker linked only by an untrusted author -> stays open.a failed deferral on a commit pass never enables auto-merge (#1567).bash tests/dev-lead/integration/test_prompt_coverage.sh: pass.Rollback
Revert this PR. There are no non-revertible side effects.
Monitoring
untrusted-mentionnotices. Seeing one means a tracker link was ignored and the thread left open.Interaction contract
🤖 Generated with Claude Code
https://claude.ai/code/session_019K6UsD7r25pAcRjJL3G6Kx
Generated by Claude Code