fix(pr-review): honour DRY_RUN in the advisory-gate marker posts

Merged
DO
7d ago
+64-0

Summary

DRY_RUN=true pr-review runs no longer write to GitHub from the advisory-gate marker helpers. maybe_post_rate_limited_marker and maybe_post_partial_evidence_marker now skip their gh pr comment call under DRY_RUN=true and log what they would have posted.

Problem

scripts/review-one-pr.sh calls maybe_post_rate_limited_marker when advisory bots are still reviewing and one of them is rate-limited. That helper ran gh pr comment without checking DRY_RUN, so a DRY_RUN=true pr-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); ordinary DRY_RUN=true runs stayed exposed. No separate issue was filed.

What changed:

  • maybe_post_rate_limited_marker (scripts/lib/advisory-review-gate.sh) skips the post when DRY_RUN=true and logs DRY_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.sh was not exposed through it — it defers that post to post-pr-review.sh, which exits early on a dry run — but the gate's non-deferred path (no PARTIAL_EVIDENCE_STATE_FILE) was.
  • In both helpers the guard sits after the dedup check, so the "would post" line appears only when a real run would have written.

Those two gh pr comment calls are the only GitHub writes in the two files; every other gh call there is a read.

Risk

Low. The change adds an early return that is taken only when DRY_RUN=true; runs with DRY_RUN unset or false execute the same code as before. No workflow, prompt or gate decision changes.

Test plan

  • Added to tests/dev-lead/unit/test_advisory_review_gate.bats: runs the same detect-then-post sequence as review-one-pr.sh with a recording gh stub and DRY_RUN=true; asserts no gh call at all and that the would-post line is logged.
  • Added to tests/dev-lead/unit/test_partial_evidence_marker.bats: the same assertion for the non-deferred partial-evidence path.
  • Both new tests fail with the guards removed and pass with them.
  • shellcheck --severity=warning -x scripts/*.sh scripts/lib/*.sh (the CI invocation): clean.
  • The two unit suites above, run locally on macOS: 103 of 105 pass. The two failures also fail on main without this change and look macOS-specific (one is a sed -i call 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 #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=true pr-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 new pr-review-agent rate-limited v1 comment 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

  • N/A — this PR does not add or change an agentic role.

Checklist

  • shellcheck scripts/*.sh passes for any changed shell scripts.
  • Docs updated where behavior or standards changed. (The header comments of both helpers now state the DRY_RUN behavior; no other doc describes these helpers' write behavior.)

🤖 Generated with Claude Code