Repository navigation
fix(pr-review): honour DRY_RUN in the advisory-gate marker posts - #2096
Conversation
maybe_post_rate_limited_marker ran `gh pr comment` without checking DRY_RUN, so a DRY_RUN=true pr-review run still stamped a `<!-- pr-review-agent rate-limited v1 sha=… -->` comment on the PR when an advisory bot was rate-limited. maybe_post_partial_evidence_marker had the same gap on the gate's non-deferred path. Both helpers now skip the post under DRY_RUN=true and log what they would have posted. The dedup check still runs first, so the log line only appears when a real run would have written. Tests drive the rate-limited detect-then-post sequence and the non-deferred partial-evidence path with a recording gh stub and assert no gh call is made. Found while reviewing PR #1902, whose diagnose mode guards only itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughBoth marker helpers now log the intended post and return without a GitHub write when ChangesMarker dry-run behavior
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable issue is identified with the dry-run marker changes; they are mergeable after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 PR Risk: Low Risk
Assessed commit: |
![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 DRY_RUN mode to the advisory review gate and partial evidence marker scripts, allowing them to log would-be GitHub posts instead of executing writes. Unit tests have been added to verify this behavior. The review feedback suggests improving the robustness of these tests by adding error handling to the mktemp -d commands used for creating temporary directories.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #2096 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but I can't mark it done yet: required checks |
Resolved — the `dev-lead:hands-off` hold was lifted; dev-lead has picked this item up. Click to expand the prior hold notice.dev-lead is withholding action on this item. It is labeled To re-enable automated pickup: remove the |
|
No description provided. |
|
No description provided. |
|
This is a CodeRabbit OSS review-limit notice indicating the free review quota is temporarily exhausted. The notice reports no code findings—there are no actionable issues to address in the diff. All other reviewers (cubic, gemini) found no issues, and all CI checks pass. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
No description provided. |
|
No description provided. |
|
Acknowledged: Codex review usage-limit notice. No code finding; nothing to change. |
|
Acknowledged: Qodo trial-ended/billing notice. No code finding; nothing to change. |
|
Acknowledged: CodeAnt reports Low Risk with no findings (DRY_RUN helpers return before GitHub writes; tests cover both paths). Nothing to change. |
|
Acknowledged: SonarCloud Quality Gate passed. No findings; nothing to change. |
Dev-Lead — fix-reviews (no-changes)Agent reasoning |
This comment has been minimized.
This comment has been minimized.
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: c8cf95759adacd75a7effc94f3511918e711983a
Review mode: triage-approved (single reviewer)
Summary
Adds a DRY_RUN=true early return to maybe_post_rate_limited_marker (scripts/lib/advisory-review-gate.sh) and maybe_post_partial_evidence_marker (scripts/lib/partial-evidence-marker.sh). Under a dry run they now log the comment they would post instead of calling gh pr comment. The guard sits after the dedup check, so the would-post line is logged only when a real run would actually write. Runs with DRY_RUN unset or false behave exactly as before. Two bats tests cover the change with a recording gh stub. Each test asserts that gh was never called and that the would-post line was logged.
Linked issue analysis
No linked issue. The PR says the gap was found while reviewing #1902 and that no separate issue was filed. The description explains the problem, the fix, the risk, rollback and monitoring, so it stands on its own. The fix closes the gap it describes: these two gh pr comment calls were the only writes in the two helpers, and both now respect DRY_RUN.
Findings
No blocking findings.
- Correctness: The check
"${DRY_RUN:-false}" == "true"matches the conventionreview-one-pr.shalready uses (e.g. lines 337, 932). Both helpers return 0 on the dry-run path, as they do on the dedup-skip path, so callers see no new error.log_infois defined in advisory-review-gate.sh, and partial-evidence-marker.sh is sourced from it, so the call works in both helpers. - Behaviour impact: In a dry run no rate-limited marker is posted, so the sweep's rate-limited retry will not fire for that PR. That is the intended dry-run behaviour. Live runs are unaffected.
- Tests: The rate-limited test runs the same detect-then-post sequence as
review-one-pr.sh. The partial-evidence test covers the non-deferred path (PARTIAL_EVIDENCE_STATE_FILEempty). Both check that the call log is empty, not just that it lackspr comment. - Review threads: gemini-code-assist left two low-priority suggestions to add error handling to
mktemp -din the tests. The author answered both (bats runs tests under errexit, and the form matches neighbouring tests), and both threads are resolved. I agree; nothing to change. - Secret scan: The MCP
run_secret_scanningtool is not available in this run. gitleaks CI passed. - Triage check: I agree with triage. This is a small, defensive guard plus tests, with no secrets, auth, workflow or gate-decision changes.
CI status
All required checks pass: shellcheck, ShellCheck, bats, unit, unit-tests, CodeQL, Analyze (actions/python), SonarCloud (Quality Gate passed), gitleaks, actionlint, AgentShield, Agent Security Scan and the repo's validation checks. Skipped or cancelled entries are dependency-audit ecosystems that don't apply here and superseded dev-lead dispatch/relay/resume runs. CodeRabbit approved, and cubic and CodeAnt reported no issues.
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: 3/4 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596). |
Dismissing approval due to a PR issue comment lacking a verified disposition (#1813)
|
No description provided. |
|
Acknowledged: CodeRabbit review complete with no actionable findings. All pre-merge checks passed, merge risk is minimal, and all required CI checks pass. The DRY_RUN marker helpers are working as intended. |
2 participants
Summary
DRY_RUN=truepr-review runs no longer write to GitHub from the advisory-gate marker helpers.maybe_post_rate_limited_markerandmaybe_post_partial_evidence_markernow skip theirgh pr commentcall underDRY_RUN=trueand log what they would have posted.Problem
scripts/review-one-pr.shcallsmaybe_post_rate_limited_markerwhen advisory bots are still reviewing and one of them is rate-limited. That helper rangh pr commentwithout checkingDRY_RUN, so aDRY_RUN=truepr-review run still posted a<!-- pr-review-agent rate-limited v1 sha=… -->comment on the PR.Found while reviewing PR #1902 (feat(pr-review): "why isn't this PR approved?" diagnostic built on the real gate chain). That PR protects only its own diagnose mode (
PR_REVIEW_DIAGNOSE=true); ordinaryDRY_RUN=trueruns stayed exposed. No separate issue was filed.What changed:
maybe_post_rate_limited_marker(scripts/lib/advisory-review-gate.sh) skips the post whenDRY_RUN=trueand logsDRY_RUN: would post rate-limited marker on ….maybe_post_partial_evidence_marker(scripts/lib/partial-evidence-marker.sh) had the same gap and gets the same guard.review-one-pr.shwas not exposed through it — it defers that post topost-pr-review.sh, which exits early on a dry run — but the gate's non-deferred path (noPARTIAL_EVIDENCE_STATE_FILE) was.Those two
gh pr commentcalls are the only GitHub writes in the two files; every otherghcall there is a read.Risk
Low. The change adds an early return that is taken only when
DRY_RUN=true; runs withDRY_RUNunset orfalseexecute the same code as before. No workflow, prompt or gate decision changes.Test plan
tests/dev-lead/unit/test_advisory_review_gate.bats: runs the same detect-then-post sequence asreview-one-pr.shwith a recordingghstub andDRY_RUN=true; asserts noghcall at all and that the would-post line is logged.tests/dev-lead/unit/test_partial_evidence_marker.bats: the same assertion for the non-deferred partial-evidence path.shellcheck --severity=warning -x scripts/*.sh scripts/lib/*.sh(the CI invocation): clean.mainwithout this change and look macOS-specific (one is ased -icall BSD sed rejects): "quiescence fallback fires when head time unavailable and submissions are old" and "a failed check-run fetch degrades to reviews/comments only, never an API error (issue Advisory gate cannot count Graphite's check-run clean pass: every clean PR waits out the 20-minute timeout #2005)".tests/test_advisory_offline_mode.bats,test_qa_lead_advisory_gate.bats,test_delayed_rate_limit_retry.bats,test_reviewer_sources.bats,test_review_one_pr_nonrequired_gate.bats,test_pr_review_miss_rate.bats: 96 of 96 pass.Rollback
Revert this PR. No non-revertible side effects: nothing is migrated, and no marker format changes.
Monitoring
A
DRY_RUN=truepr-review run on a PR with a rate-limited advisory bot should log[advisory-gate] DRY_RUN: would post rate-limited marker on …and leave no newpr-review-agent rate-limited v1comment on the PR. Regression signal: such a comment appearing after a dry run. Real runs are unaffected, so the sweep's rate-limited retry keeps working as before.Interaction contract
Checklist
shellcheck scripts/*.shpasses for any changed shell scripts.DRY_RUNbehavior; no other doc describes these helpers' write behavior.)🤖 Generated with Claude Code