fix(holds/pr-review): honour every hold label; pin the escalation hold; re-evaluate after a self-dismissed approval; pin dev-lead to ring0 (#2089, #2090, #1933)

Merged
DO
7d ago
+847-98

Summary

Three fixes to how dev-lead and pr-review treat human holds and pr-review's own approvals, plus switching this repo's dev-lead to the ring0 channel (approved by the maintainer). Each is a separate commit, and review fixes follow in 61a177f.

1. #2089: dev-lead's retry sweep ignored dev-lead:hands-off and dev-lead:needs-human (978b9f4)

  • pr_resume_suppressed now checks the shared hold set in scripts/lib/hold-gate.sh: needs-human-review, dev-lead:needs-human and dev-lead:hands-off.
    • This covers all three retry sweeps and the event resume bridge.
    • On a held PR they stop before posting any marker or dispatch guard, and before dispatching a run.
    • The check runs before any budget API read, and an unreadable label set suppresses (fail closed).
  • When a different hold label replaces a hold notice, the old notice now says "Superseded — dev-lead is now withholding action under the <new> label". It used to say the hold "was lifted" while the label was still on the PR.

2. #2090: pr-review re-reviewed an escalated PR and cleared its hold (903db15, tests only)

  • Root cause, from the run logs. On #1902, the three post-escalation reviews were pr-auto-review repository_dispatch runs on pr-review/v1-next = f34a765. That version predates #2078, and in it any repository_dispatch set FORCE_REVIEW (the human break-glass). So these runs bypassed the escalation pause, the same-commit check and the cycle cap, and the fix-request path removed the label.
  • Already fixed. #2078 is now on v1-next.
  • What this PR adds: tests/test_review_one_pr_escalation_hold.bats replays the #1902 sequence. Automated dispatches stay paused and never call --remove-label, while a human removing the label, or an @mention, still re-engages. A pin on the #2078 expression fails against the f34a765 workflow.

3. #1933: pr-review never re-approved after its own gate dismissed its approval (bc2a0fa)

  • A new library, scripts/lib/self-dismissed-approval.sh, recognises a "pending re-evaluation" state. All of these must hold:
    • pr-review's NEWEST approval at the head is DISMISSED;
    • its latest review_dismissed event was made by automation with one of the two gate messages (#1813 comment disposition, #1415 maintainer thread);
    • no approval of ours is standing at the head.
  • In that state review-one-pr.sh re-runs the full cascade, with every gate armed, at the same head. It never re-posts an approval by itself.
  • A human's dismissal, the #1596 accepted-defect dismissal, or an unreadable timeline keeps the same-commit no-op.

4. This repo's dev-lead runs dev-lead/v139-ring0 instead of v139-stable (1afed77)

  • dev-lead fixes reach this repo days sooner, and ring0 gets real soak here.
  • SC2 still holds: ring0 is promoted from next only after its dwell, and a bad ring0 is rolled back by moving the tag, with no PR to block. tests/test_sc2_self_review_channel.bats now asserts a promoted tier (ring or stable) and still fails on next, @main or a SHA.
  • The ring0 reusable declares the same inputs as stable (agent_ref, event_name). The caller-stub freeze baseline is regenerated for this deliberate channel change, and the SC2 docs are updated.
  • Until #2089's fix is promoted to ring0, the ring0 copy still posts retry markers on held PRs.

Fixes #2089. Fixes #2090. Fixes #1933.

Problem

  • #2089: held PRs (#2085, #1902, #2087) kept getting retry markers and dispatch guards, and a dev-lead run was started for each. The runs could only skip, and they used up retry attempts.
  • #2090: pr-review's escalation to a human, the 3-cycle cap and the needs-human-review hold could all be undone without a human. This is the "disappearing label".
  • #1933: after a gate self-dismissal, the PR stayed unapproved until a new commit landed or someone used the force_review break-glass (#1907).

Risk

Low to medium.

  • #2089: the change only makes the resume paths suppress in more cases.
  • #2090: tests only.
  • #1933: adds a narrow re-review path. It runs only after every gate has passed on the run, it only re-runs the gated cascade, and it fails closed. The two extra REST reads happen only when the latest marker at the head is an approval.
  • ring0 pin: this repo's dev-lead runs v139.48 instead of v139.46, which is newer promoted code. A bad ring0 is rolled back by moving the tag.

Test plan

  • shellcheck --severity=warning scripts/*.sh scripts/lib/*.sh: clean. bash scripts/dev-lead-lint.sh: clean.
  • #2089: tests in tests/test_pr_automation_budget.bats and tests/dev-lead/unit/test_{bot_comment_retry,bot_thread_retry,dev_lead_retry,hold_notice}.bats. Every behaviour test fails on the pre-change code. The hold-notice wrapper is driven end to end with gh stubbed.
  • #2090: tests/test_review_one_pr_escalation_hold.bats, 6/6 pass.
  • #1933: tests/test_review_one_pr_self_dismissed_reeval.bats, 11/11 pass. The end-to-end re-evaluation case, the newest-approval case and the id-typing case fail on the previous code.
  • ring0 pin: the SC2, channel-pin-consistency, caller-stub-freeze and template-drift suites pass (101 tests).
  • All suites that load review-one-pr.sh or these libraries pass. The only exceptions are tests in test_fix_reviews.bats, test_fix_issue*.bats and test_validate_caller_inputs.bats that fail identically on unchanged main in this environment only.
  • lint.yml now runs the two new review-one-pr test files.

Rollback

Revert this PR, or revert any single commit. To undo only the ring0 switch, revert 1afed77. There are no non-revertible side effects.

Monitoring

  • dev-lead retry log: [suppress] … carries <label> — human hold; not resuming (#2089) on held PRs.
  • pr-review:
    • an escalated PR still labelled needs-human-review logs noop: human-escalation active on automated dispatches;
    • after a gate self-dismissal clears, the next run logs re-review: pr-review's approval at <sha> was dismissed by its own gate … (#1933).
  • Downstream repos pin pr-review/v1-stable (776c958), which predates #2078, so they keep the #2090 behaviour until pr-review is promoted.
  • dev-lead runs in this repo use dev-lead-reusable.yml@dev-lead/v139-ring0.

Interaction contract

  • N/A — no agentic role is added or changed. An existing timer's stop condition gets narrower, an existing role gets one more re-evaluation trigger at the same head, and this repo's dev-lead moves to a newer promoted channel; triggers and contracts are unchanged.

Checklist

  • shellcheck scripts/*.sh passes for any changed shell scripts.
  • Docs updated where behavior or standards changed (inline library docs; docs/release/versioning.md, docs/release/sc2-game-day.md and AGENTS.md for the ring0 pin).

🤖 Generated with Claude Code

https://claude.ai/code/session_019K6UsD7r25pAcRjJL3G6Kx