Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/dev-lead-hold-notice.sh
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ collapse_stale_notices() {
if [ -n "$keep_label" ] && [ "$cur_label" = "$keep_label" ]; then
continue
fi
new_body=$(hold_notice_supersede_body "$old_body")
new_body=$(hold_notice_supersede_body "$old_body" "$keep_label")
if [ "$DRY_RUN" = "true" ]; then
echo "[dry-run] would collapse stale hold notice $cid (label=$cur_label) on $REPO#$SUBJECT_NUMBER"
continue
Expand Down
16 changes: 13 additions & 3 deletions scripts/lib/hold-notice.sh
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,20 @@ body_has_active_hold_notice() {
esac
}

# hold_notice_supersede_body <comment-body>
# hold_notice_supersede_body <comment-body> [replacing-label]
# Rewrite a live notice IN PLACE to collapse it (AC4): drop the active marker,
# stamp the superseded marker, and wrap the remaining prose in a collapsed
# <details> block — reusing the supersede convention in post-pr-review.sh
# (mark_prior_agent_items_obsolete). Idempotent: an already-collapsed body is
# returned unchanged, so re-running never nests wrappers.
#
# With no [replacing-label] the hold was lifted (dev-lead picked the item up).
# With one, a notice for a DIFFERENT hold label is replacing this one — the old
# label may well still be on the item, so the summary must not claim it was
# lifted (#2089: a `dev-lead:hands-off` notice read "lifted" on a PR that still
# carried the label, because `needs-human-review` sorts first in the hold set).
hold_notice_supersede_body() {
local body="${1:-}"
local body="${1:-}" replacing="${2:-}"
case "$body" in
*"<!-- dev-lead-hold-notice superseded"*) printf '%s' "$body"; return 0 ;;
esac
Expand All @@ -124,7 +130,11 @@ hold_notice_supersede_body() {
# longer reads as still-held and no longer suppresses a future notice.
stripped="${body//"$active"/}"
printf '%s\n' "$(hold_notice_superseded_marker "$label")"
printf '<details><summary><em>Resolved — the `%s` hold was lifted; dev-lead has picked this item up. Click to expand the prior hold notice.</em></summary>\n\n' "$label"
if [ -n "$replacing" ] && [ "$replacing" != "$label" ]; then
printf '<details><summary><em>Superseded — dev-lead is now withholding action under the `%s` label (see the newer notice). Click to expand the prior `%s` hold notice.</em></summary>\n\n' "$replacing" "$label"
Comment thread
don-petry marked this conversation as resolved.
Outdated
else
printf '<details><summary><em>Resolved — the `%s` hold was lifted; dev-lead has picked this item up. Click to expand the prior hold notice.</em></summary>\n\n' "$label"
fi
printf '%s\n\n' "$stripped"
printf '</details>'
}
32 changes: 30 additions & 2 deletions scripts/lib/pr-automation-budget.sh
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,25 @@ pr_has_escalation_label() {
<<<"$labels_json" >/dev/null 2>&1
}

# The fleet's human-hold label set (needs-human-review, dev-lead:needs-human,
# dev-lead:hands-off) lives in hold-gate.sh (#1595). Sourced here so the resume
# gate below honours every hold, not just NEEDS_HUMAN_REVIEW_LABEL (#2089).
# shellcheck source=scripts/lib/hold-gate.sh
source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/hold-gate.sh"

# pr_hold_gate_label <labels_json>
# Print the first hold-gate label (hold_gate_labels) present in the JSON array
# of label NAMES and exit 0; print nothing and exit 1 when none is present.
# Malformed/empty/missing input degrades to "not held" (exit 1), like
# pr_has_escalation_label.
pr_hold_gate_label() {
local labels_json="${1:-[]}" names
names=$(jq -r 'if type == "array" then .[] | strings else empty end' \
<<<"$labels_json" 2>/dev/null) || return 1
Comment thread
don-petry marked this conversation as resolved.
[ -n "$names" ] || return 1
hold_gate_first_match "$names"
}

# pr_hold_kind <items_json> <head_sha>
# Classify WHY a PR carrying NEEDS_HUMAN_REVIEW_LABEL is held, so a re-trigger
# timer can log the exact hold it honors (#1550 AC#5) and exempt the ONE hold
Expand Down Expand Up @@ -156,8 +175,8 @@ _pr_items_have_marker() {

# pr_resume_suppressed <pr> <repo> [labels_json] [events_json]
# Exit 0 (SUPPRESS — do NOT resume / re-dispatch) when the PR is human-gated
# (carries NEEDS_HUMAN_REVIEW_LABEL) OR its per-PR automation budget is
# exhausted; exit 1 (proceed) otherwise.
# (carries NEEDS_HUMAN_REVIEW_LABEL or any other hold-gate label, #2089) OR its
# per-PR automation budget is exhausted; exit 1 (proceed) otherwise.
#
# This is the SINGLE stop-condition consulted before any automated resume of a
# blocked/rate-limited dev-lead state — by the event-first resume bridge
Expand All @@ -182,6 +201,15 @@ pr_resume_suppressed() {
echo " [suppress] PR #${pr} in ${repo} carries ${NEEDS_HUMAN_REVIEW_LABEL} — human-gated; not resuming (#946)" >&2
return 0
fi
# Every other human hold (dev-lead:hands-off, dev-lead:needs-human) stops the
# resume/retry paths too. Before #2089 only the escalation label did, so a held
# PR still got retry markers, dispatch guards and a dispatched run that then
# skipped at intent time — clutter plus spent retry attempts.
local hold
if hold=$(pr_hold_gate_label "$labels_json"); then
echo " [suppress] PR #${pr} in ${repo} carries ${hold} — human hold; not resuming (#2089)" >&2
return 0
fi
Comment thread
don-petry marked this conversation as resolved.
Outdated

if [ -z "$events_json" ]; then
if ! events_json=$(gather_pr_automation_events "$pr" "$repo"); then
Expand Down
18 changes: 18 additions & 0 deletions tests/dev-lead/unit/test_bot_comment_retry.bats
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,24 @@ _graphql_page() {
[ "$marker_line" -lt "$dispatch_line" ]
}

@test "sweep(#2089): a dev-lead:hands-off PR gets no retry marker and no dispatch" {
export PR_JSON='{"state":"open","head":{"sha":"abc","ref":"dev-lead/issue-2008-x","repo":{"full_name":"petry-projects/.github-private"}},"user":{"login":"don-petry"},"labels":[{"name":"dev-lead:hands-off"}]}'
_setup_sweep
# Use the REAL resume gate (the default harness stubs it to "proceed").
# shellcheck source=/dev/null
source "$(dirname "$RETRY_SCRIPT")/lib/pr-automation-budget.sh"
export TRUSTED_BOTS="$TRUSTED"
export GRAPHQL_RESPONSE
GRAPHQL_RESPONSE="$(_graphql_page "$(_bot IC_cr coderabbitai 'Walkthrough')")"

run scan_pr_for_undispositioned_bot_comments "petry-projects/.github-private" 2085
[ "$status" -eq 0 ]
[ "${lines[-1]}" = "0" ]
! grep -q '/dispatches' "$GH_LOG"
Comment thread
don-petry marked this conversation as resolved.
Outdated
! grep -q 'dev-lead-bot-comment-retry' "$GH_LOG"
! grep -q 'dev-lead-dispatch-guard' "$GH_LOG"
Comment thread
don-petry marked this conversation as resolved.
Outdated
}

@test "sweep: a dispositioned comment dispatches nothing" {
_setup_sweep
export TRUSTED_BOTS="$TRUSTED"
Expand Down
17 changes: 17 additions & 0 deletions tests/dev-lead/unit/test_bot_thread_retry.bats
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,23 @@ _comments_page() {
[ "$marker_line" -lt "$dispatch_line" ]
}

@test "sweep(#2089): a dev-lead:hands-off PR gets no retry marker and no dispatch" {
export PR_JSON='{"state":"open","head":{"sha":"abc","ref":"dev-lead/issue-1900-x","repo":{"full_name":"petry-projects/.github-private"}},"user":{"login":"don-petry"},"labels":[{"name":"dev-lead:hands-off"}]}'
export THREADS_RESPONSE
THREADS_RESPONSE="$(_threads_page "$(_thread PRRT_cubic cubic-dev-ai 2026-10-01T11:35:00Z)")"
_setup_sweep
# Use the REAL resume gate (the default harness stubs it to "proceed").
# shellcheck source=/dev/null
source "$(dirname "$RETRY_SCRIPT")/lib/pr-automation-budget.sh"

run scan_pr_for_unreplied_bot_threads "petry-projects/.github-private" 1902
[ "$status" -eq 0 ]
[ "${lines[-1]}" = "0" ]
! grep -q '/dispatches' "$GH_LOG"
! grep -q 'dev-lead-bot-thread-retry' "$GH_LOG"
! grep -q 'dev-lead-dispatch-guard' "$GH_LOG"
}

@test "sweep: a pending retry (from the cron or pr-review's hook) dedups the next scan" {
export THREADS_RESPONSE COMMENTS_RESPONSE
THREADS_RESPONSE="$(_threads_page "$(_thread PRRT_cubic cubic-dev-ai)")"
Expand Down
23 changes: 23 additions & 0 deletions tests/dev-lead/unit/test_dev_lead_retry.bats
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,29 @@ _scan_with_markers() {
run scan_pr_for_rate_limits "petry-projects/.github-private" 2000
}

@test "retry(#2089): a dev-lead:hands-off PR with a reset hold neither guards nor dispatches" {
gh() {
case "$*" in
*"/pulls/2000"*) echo '{"state":"open","head":{"sha":"abc"},"labels":[{"name":"dev-lead:hands-off"}]}' ;;
*"/comments"*) echo '["<!-- dev-lead-fix-reviews pr=2000 sha=abc intent=fix-reviews status=rate-limited reason=rate-limited reset=2026-06-18T23:00:00Z -->"]' ;;
*) echo '[]' ;;
esac
}
# The REAL resume gate (from lib/pr-automation-budget.sh) — not the stub the
# other scans use — so the hold-label suppression itself is under test.
# shellcheck source=/dev/null
source "$(dirname "$RETRY_SCRIPT")/lib/pr-automation-budget.sh"
post_dispatch_guard() { echo "GUARD" >&2; }
fetch_pr_comment_nodes() { echo '[{"id":"IC_cr"}]'; }
stale_disposition_needs_dispatch() { return 0; }
dispatch_reviews_retry() { echo "DISPATCH intent=$4" >&2; }
run scan_pr_for_rate_limits "petry-projects/.github-private" 2000
[ "${lines[-1]}" = "0" ]
[[ "$output" != *"DISPATCH"* ]]
[[ "$output" != *"GUARD"* ]]
[[ "$output" == *"carries dev-lead:hands-off"* ]]
}

@test "retry: history-unavailable marker AFTER an older terminal failed marker still dispatches" {
_scan_with_markers '["<!-- dev-lead-fix-reviews pr=2000 sha=abc intent=fix-reviews status=failed -->","<!-- dev-lead-fix-reviews pr=2000 sha=abc intent=fix-reviews status=history-unavailable -->"]'
[[ "$output" == *"DISPATCH intent=fix-reviews"* ]]
Expand Down
20 changes: 20 additions & 0 deletions tests/dev-lead/unit/test_hold_notice.bats
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,26 @@ setup() {
[ "$once" = "$twice" ]
}

@test "supersede_body(#2089): replaced by another hold label never claims the hold was lifted" {
original="$(hold_notice_body "dev-lead:hands-off")"
collapsed="$(hold_notice_supersede_body "$original" "needs-human-review")"
[[ "$collapsed" == *"<!-- dev-lead-hold-notice superseded label=dev-lead:hands-off -->"* ]]
[[ "$collapsed" == *"now withholding action under the \`needs-human-review\` label"* ]]
[[ "$collapsed" != *"was lifted"* ]]
[[ "$collapsed" != *"picked this item up"* ]]
}

@test "supersede_body(#2089): no replacing label (pickup) keeps the lifted wording" {
original="$(hold_notice_body "dev-lead:hands-off")"
collapsed="$(hold_notice_supersede_body "$original")"
[[ "$collapsed" == *"hold was lifted; dev-lead has picked this item up"* ]]
}

@test "hold-notice wrapper(#2089): notice mode passes the new label to the supersede rewrite" {
grep -q 'hold_notice_supersede_body "$old_body" "$keep_label"' \
Comment thread
don-petry marked this conversation as resolved.
Outdated
"$SCRIPT_DIR/scripts/dev-lead-hold-notice.sh"
}
Comment thread
don-petry marked this conversation as resolved.
Outdated

@test "body_has_active_hold_notice: true for a live notice, false once collapsed" {
original="$(hold_notice_body "needs-human-review")"
run body_has_active_hold_notice "$original"
Expand Down
35 changes: 35 additions & 0 deletions tests/test_pr_automation_budget.bats
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,41 @@ mk_bot_events() { # mk_bot_events <count>
[ "$status" -eq 0 ]
}

@test "pr_resume_suppressed(#2089): dev-lead:hands-off → suppress without reading budget events" {
# events_json is deliberately unreadable: a hold must short-circuit before the
# budget read, so the result cannot depend on (or wait for) the events API.
run pr_resume_suppressed 2085 petry-projects/demo '["bug","dev-lead:hands-off"]' 'not json'
Comment thread
don-petry marked this conversation as resolved.
Outdated
[ "$status" -eq 0 ]
[[ "$output" == *"carries dev-lead:hands-off"* ]]
}

@test "pr_resume_suppressed(#2089): dev-lead:needs-human → suppress" {
run pr_resume_suppressed 2085 petry-projects/demo '["dev-lead:needs-human"]' "$(mk_bot_events 1)"
[ "$status" -eq 0 ]
[[ "$output" == *"carries dev-lead:needs-human"* ]]
}

@test "pr_resume_suppressed(#2089): needs-human-review keeps its #946 message" {
run pr_resume_suppressed 2085 petry-projects/demo '["dev-lead:hands-off","needs-human-review"]' '[]'
[ "$status" -eq 0 ]
[[ "$output" == *"carries needs-human-review — human-gated; not resuming (#946)"* ]]
}

@test "pr_resume_suppressed(#2089): a label merely containing a hold name does not suppress" {
run pr_resume_suppressed 2085 petry-projects/demo '["dev-lead:hands-off-later","x-dev-lead:needs-human"]' "$(mk_bot_events 1)"
[ "$status" -ne 0 ]
}

@test "pr_hold_gate_label(#2089): prints the held label; malformed input is not held" {
run pr_hold_gate_label '["bug","dev-lead:hands-off"]'
[ "$status" -eq 0 ]
[ "$output" = "dev-lead:hands-off" ]
run pr_hold_gate_label 'not json'
[ "$status" -ne 0 ]
run pr_hold_gate_label '[]'
[ "$status" -ne 0 ]
}

@test "pr_resume_suppressed: FORCE_REVIEW must not bypass the budget" {
FORCE_REVIEW=true run pr_resume_suppressed 860 petry-projects/demo '["bug"]' "$(mk_bot_events 10)"
[ "$status" -eq 0 ]
Expand Down
Loading
Loading