From 01ff2d1d1d3f07bfa04d7948f0de3d7332100f22 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sun, 11 Oct 2026 15:45:43 +0000 Subject: [PATCH 1/3] =?UTF-8?q?feat:=20implement=20issue=20#2247=20?= =?UTF-8?q?=E2=80=94=20Stale-disposition=20retry=20cap=20does=20not=20coun?= =?UTF-8?q?t=20a=20fix-reviews=20pass=20that=20dies=20before=20posting=20a?= =?UTF-8?q?ny=20marker=20(follow-up=20to=20the=20edited-comment=20deadlock?= =?UTF-8?q?=20fix)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- scripts/dev-lead-fix-reviews.sh | 34 +++++++ scripts/dev-lead-retry.sh | 19 ++-- tests/dev-lead/unit/test_dev_lead_retry.bats | 92 +++++++++++++++++++ tests/dev-lead/unit/test_fix_reviews.bats | 74 ++++++++++++++- .../unit/test_maintainer_comment_gate.bats | 8 ++ 5 files changed, 216 insertions(+), 11 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index a6292d6aa..d7aa6d92f 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -890,10 +890,17 @@ fetch_pr_context() { # model read a 12:18Z disposition as current for a comment edited at 13:03Z, # posted nothing, and the gate stayed blocked on every pass. A PR whose comments # cannot be read gets an explicit "could not be determined" note, never "none". +# +# Also sets STALE_LATEST_EDIT, the newest lastEditedAt among the stale comments +# (empty when none is stale or the comments cannot be read), for +# post_stale_pass_start (#2247). export_stale_dispositions() { local comments facts + STALE_LATEST_EDIT="" if comments=$(bcr_fetch_pr_comments "$REPO" "$PR_NUMBER" 2>/dev/null) \ && facts=$(maintainer_gate_stale_disposition_facts "$comments" "${BOT_USER:-donpetry-bot}"); then + STALE_LATEST_EDIT=$(maintainer_gate_stale_dispositions "$comments" "${BOT_USER:-donpetry-bot}" \ + | jq -r 'map(.lastEditedAt) | max // ""' 2>/dev/null) || STALE_LATEST_EDIT="" if [ -n "$facts" ]; then # Linux caps one environment string at 128 KiB (MAX_ARG_STRLEN); an oversized # value would stop envsubst and the agent subprocess from starting. @@ -918,6 +925,32 @@ This is settled; do not re-derive it. For each comment listed, read its CURRENT export STALE_DISPOSITIONS } +# post_stale_pass_start: when this pass sees a stale disposition (STALE_LATEST_EDIT, +# set by export_stale_dispositions), post a start marker before the model runs +# (#2247). The dev-lead-retry.sh sweep caps the fix-reviews passes it dispatches +# per edit by counting their markers (stale_disposition_needs_dispatch). A pass that +# dies before its terminal marker (runner lost, timeout, cancel) would otherwise +# leave nothing to count. read_at= matches the terminal marker's, so a pass that +# posts both counts once. The `" + if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then + echo "[dry-run] would post stale-disposition pass start marker: ${marker}" + return 0 + fi + gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$marker" >/dev/null 2>&1 \ + || echo "::warning::could not post the stale-disposition pass start marker on PR #${PR_NUMBER} — the sweep's pass cap will not count this pass if it dies before its terminal marker (#2247)" + return 0 +} + # resolve_bot_outdated_threads: resolves all outdated review threads from bot reviewers. # This is a cleanup function for the no-changes path: when no code changes are needed, # we still want to mark outdated bot comments as resolved so they don't clutter the PR. @@ -3930,6 +3963,7 @@ case "$INTENT_TYPE" in export -n OPEN_THREADS_JSON # an inherited export would carry the payload into every exec fetch_pr_context export_stale_dispositions + post_stale_pass_start rc=0 build_and_run "fix-reviews" || rc=$? [ "$rc" -eq 2 ] && handle_rate_limit "fix-reviews" diff --git a/scripts/dev-lead-retry.sh b/scripts/dev-lead-retry.sh index 33c912d59..3b8ab6e39 100755 --- a/scripts/dev-lead-retry.sh +++ b/scripts/dev-lead-retry.sh @@ -481,7 +481,7 @@ fetch_pr_comment_nodes() { # marker after the edit is not proof the edit was handled: on # empower-personal-dashboard#65/#66/#67 passes ran after the edit, wrote no # fresh disposition, and that marker stopped every later dispatch. -# Run markers only BOUND the retry. A successful fix-reviews pass for THIS PR +# Run markers only BOUND the retry. A fix-reviews pass for THIS PR # (`", + createdAt:$c, isMinimized:false, minimizedReason:null, lastEditedAt:null}' +} + +# _with_nodes ...: append nodes to a node array. +_with_nodes() { + local nodes="$1" n + shift + for n in "$@"; do nodes=$(jq -c --argjson n "$n" '. + [$n]' <<< "$nodes"); done + printf '%s' "$nodes" +} + +@test "sweep(#2247): two passes that only posted a start marker reach the cap (rc 3)" { + local base nodes + base=$(_sweep_nodes 2026-10-01T20:35:00Z 2026-10-01T19:23:54Z) + nodes=$(_with_nodes "$base" "$(_start_node IC_s1 2026-10-01T20:40:01Z 2026-10-01T20:40:00Z)") + run stale_disposition_needs_dispatch "$nodes" 2000 + [ "$status" -eq 0 ] + nodes=$(_with_nodes "$nodes" "$(_start_node IC_s2 2026-10-01T20:55:01Z 2026-10-01T20:55:00Z)") + run stale_disposition_needs_dispatch "$nodes" 2000 + [ "$status" -eq 3 ] +} + +@test "sweep(#2247): a pass that posts a start and a terminal marker counts once" { + local nodes + # The terminal marker (IC_run) carries the same read_at as the start marker. + nodes=$(_with_nodes "$(_sweep_nodes 2026-10-01T20:35:00Z 2026-10-01T19:23:54Z 2026-10-01T20:45:00Z OWNER 2026-10-01T20:40:00Z)" \ + "$(_start_node IC_s1 2026-10-01T20:40:01Z 2026-10-01T20:40:00Z)") + run stale_disposition_needs_dispatch "$nodes" 2000 + [ "$status" -eq 0 ] + STALE_DISPOSITION_MAX_PASSES=1 run stale_disposition_needs_dispatch "$nodes" 2000 + [ "$status" -eq 3 ] +} + +@test "sweep(#2247): a start marker before the edit, for another PR, or from an untrusted author does not count" { + export STALE_DISPOSITION_MAX_PASSES=1 # one counted pass reaches the cap + local base + base=$(_sweep_nodes 2026-10-01T20:35:00Z 2026-10-01T19:23:54Z) + run stale_disposition_needs_dispatch "$(_with_nodes "$base" "$(_start_node IC_s 2026-10-01T20:36:00Z 2026-10-01T20:30:00Z)")" 2000 + [ "$status" -eq 0 ] + run stale_disposition_needs_dispatch "$(_with_nodes "$base" "$(_start_node IC_s 2026-10-01T20:41:00Z 2026-10-01T20:40:00Z 20)")" 2000 + [ "$status" -eq 0 ] + run stale_disposition_needs_dispatch "$(_with_nodes "$base" "$(_start_node IC_s 2026-10-01T20:41:00Z 2026-10-01T20:40:00Z 2000 NONE)")" 2000 + [ "$status" -eq 0 ] + run stale_disposition_needs_dispatch "$(_with_nodes "$base" "$(_start_node IC_s 2026-10-01T20:41:00Z 2026-10-01T20:40:00Z)")" 2000 + [ "$status" -eq 3 ] +} + +@test "sweep(#2247): two passes start and post no terminal marker — the third sweep holds and posts the capped hold" { + export DRY_RUN="false" POSTS_FILE="$BATS_TEST_TMPDIR/posts" + : > "$POSTS_FILE" + gh() { + case "$*" in + *"--method POST"*) printf '%s\n' "$*" >> "$POSTS_FILE" ;; + *"/pulls/2000"*) echo '{"state":"open","head":{"sha":"abc"},"labels":[]}' ;; + *"/comments"*) jq -c 'map(.body)' <<< "$SWEEP_NODES" ;; + *) echo '[]' ;; + esac + } + pr_resume_suppressed() { return 1; } + post_dispatch_guard() { :; } + fetch_pr_comment_nodes() { printf '%s' "$SWEEP_NODES"; } + dispatch_reviews_retry() { echo "DISPATCH intent=$4" >&2; } + + # Sweeps 1 and 2 dispatch; each dispatched pass posts its start marker, then dies. + export SWEEP_NODES + SWEEP_NODES=$(_sweep_nodes 2026-10-01T20:35:00Z 2026-10-01T19:23:54Z) + run scan_pr_for_rate_limits "petry-projects/.github-private" 2000 + [[ "$output" == *"DISPATCH intent=fix-reviews"* ]] + SWEEP_NODES=$(_with_nodes "$SWEEP_NODES" "$(_start_node IC_s1 2026-10-01T20:40:01Z 2026-10-01T20:40:00Z)") + run scan_pr_for_rate_limits "petry-projects/.github-private" 2000 + [[ "$output" == *"DISPATCH intent=fix-reviews"* ]] + [ ! -s "$POSTS_FILE" ] + SWEEP_NODES=$(_with_nodes "$SWEEP_NODES" "$(_start_node IC_s2 2026-10-01T20:55:01Z 2026-10-01T20:55:00Z)") + + # Sweep 3: no dispatch, and the capped hold comment is posted. + run scan_pr_for_rate_limits "petry-projects/.github-private" 2000 + [ "${lines[-1]}" = "0" ] + [[ "$output" != *"DISPATCH"* ]] + [[ "$output" == *"::warning::"*"#2166"* ]] + grep -q 'dev-lead-stale-disposition-hold pr=2000 sha=abc max_passes=2' "$POSTS_FILE" +} + @test "sweep(#2008): unreadable comment data never dispatches (no guessing)" { run stale_disposition_needs_dispatch "not json" 2000 [ "$status" -eq 1 ] diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index c6f625371..b2ca6eec4 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -7183,10 +7183,11 @@ GITEOF # current and posted nothing. The harness now computes the stale set before the # model runs (the gate's own #2008 rule) and hands it over as a fact. -# _stale_env : run export_stale_dispositions against a gh stub -# whose comment read returns (or fails), and print the result. +# _stale_env [var]: run export_stale_dispositions against a gh +# stub whose comment read returns (or fails), and print +# (default STALE_DISPOSITIONS). _stale_env() { - export STALE_COMMENTS="$1" + export STALE_COMMENTS="$1" STALE_VAR="${2:-STALE_DISPOSITIONS}" cat > "$STUB_BIN_DIR/gh" <<'GHEOF' #!/usr/bin/env bash [ "$STALE_COMMENTS" = "FAIL" ] && exit 1 @@ -7200,7 +7201,7 @@ GHEOF source '$SCRIPT_DIR/scripts/lib/bot-comment-retry.sh' eval \"\$(sed -n '/^export_stale_dispositions()/,/^}/p' '$FIX_REVIEWS_SCRIPT')\" export_stale_dispositions >/dev/null 2>&1 - printf '%s' \"\$STALE_DISPOSITIONS\" + printf '%s' \"\${!STALE_VAR}\" " } @@ -7309,3 +7310,68 @@ _stale_nodes() { [ "$status" -eq 0 ] [ "$output" = "2026-10-12T00:00:00Z" ] } + +# ── #2247: a stale-disposition pass records that it started ──────────────────── +# The sweep caps fix-reviews passes per edit by counting their markers. A pass that +# dies before its terminal marker (runner lost, timeout, cancel) left nothing to +# count, so it posts a start marker before the model runs. + +@test "fix-reviews(#2247): export_stale_dispositions records the latest stale edit time, empty when none is stale" { + _stale_env "$(_stale_nodes 2026-10-10T12:18:39Z)" STALE_LATEST_EDIT + [ "$output" = "2026-10-10T13:03:53Z" ] + _stale_env "$(_stale_nodes 2026-10-10T13:10:00Z)" STALE_LATEST_EDIT + [ -z "$output" ] + _stale_env FAIL STALE_LATEST_EDIT + [ -z "$output" ] +} + +# _start_post [DEV_LEAD_DRY_RUN]: run post_stale_pass_start and +# print what the gh stub was asked to post. +_start_post() { + export POSTED="$BATS_TEST_TMPDIR/posted" + : > "$POSTED" + cat > "$STUB_BIN_DIR/gh" <<'GHEOF' +#!/usr/bin/env bash +printf '%s\n' "$@" >> "$POSTED" +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + run bash -c " + export PATH='$STUB_BIN_DIR':\"\$PATH\" REPO=o/r PR_NUMBER=54 HEAD_SHA=ddd444eee555 + export STALE_LATEST_EDIT='$1' DEV_LEAD_DRY_RUN='${2:-false}' PASS_STARTED_AT=2026-10-10T13:20:00Z + REVIEWS_MARKER_PREFIX='' "$POSTED" +} + +@test "fix-reviews(#2247): no start marker when nothing is stale, or in dry-run" { + _start_post "" + [ "$status" -eq 0 ] + [ ! -s "$POSTED" ] + _start_post 2026-10-10T13:03:53Z true + [ "$status" -eq 0 ] + [ ! -s "$POSTED" ] +} + +@test "fix-reviews(#2247): the start marker the pass posts is the one the sweep counts" { + _start_post 2026-10-10T13:03:53Z + local marker nodes + marker=$(grep -o '' "$POSTED") + nodes=$(jq -c --arg m "$marker" '. + [{id:"IC_start", author:{login:"don-petry", __typename:"User"}, + authorAssociation:"OWNER", body:$m, createdAt:"2026-10-10T13:20:01Z", lastEditedAt:null, + isMinimized:false, minimizedReason:null}]' <<< "$(_stale_nodes 2026-10-10T12:18:39Z)") + run bash -c "source '$SCRIPT_DIR/scripts/dev-lead-retry.sh'; STALE_DISPOSITION_MAX_PASSES=1 stale_disposition_needs_dispatch \"\$1\" 54" _ "$nodes" + [ "$status" -eq 3 ] +} + +@test "fix-reviews(#2247): the start marker is posted after the stale set is computed and before the model runs" { + local block + block="$(sed -n '/^ fix-reviews)$/,/build_and_run "fix-reviews"/p' "$FIX_REVIEWS_SCRIPT")" + [[ "$block" == *"export_stale_dispositions"*"post_stale_pass_start"*"build_and_run"* ]] +} diff --git a/tests/dev-lead/unit/test_maintainer_comment_gate.bats b/tests/dev-lead/unit/test_maintainer_comment_gate.bats index 8cd914526..7a5641c1e 100644 --- a/tests/dev-lead/unit/test_maintainer_comment_gate.bats +++ b/tests/dev-lead/unit/test_maintainer_comment_gate.bats @@ -966,3 +966,11 @@ _stale_nodes() { _run_check "$(jq -c '{reviews:[], comments:.}' <<< "$after")" [ "$status" -eq 0 ] } + +# #2247: a fix-reviews pass posts a start marker before the model runs, so the +# sweep's pass cap counts a pass that dies early. It is automation, not a finding. +@test "#2247: the fix-reviews start marker is a registered automation marker → 0" { + local json='{"comments":[{"author":{"login":"don-petry"},"authorAssociation":"OWNER","body":"","createdAt":"2026-10-10T13:20:01Z","isMinimized":false,"minimizedReason":""}]}' + _run_check "$json" + [ "$status" -eq 0 ] +} From afbbd5b0d1e780251bbb0d8e7147f1336e95f3b5 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:24:32 +0000 Subject: [PATCH 2/3] fix(bot): use millisecond-precision timestamps for pass read_at to ensure unique markers when passes start in the same second Co-Authored-By: Claude Haiku 4.5 --- scripts/dev-lead-fix-reviews.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index d7aa6d92f..53d0190c8 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -86,7 +86,7 @@ REVIEWS_MARKER_PREFIX="" @@ -938,7 +938,7 @@ post_stale_pass_start() { [[ "${STALE_LATEST_EDIT:-}" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?Z$ ]] || return 0 local sha_part="" read_part="" [[ "${HEAD_SHA:-}" =~ ^[0-9a-f]+$ ]] && sha_part=" sha=${HEAD_SHA}" - if [[ "${PASS_STARTED_AT:-}" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$ ]]; then + if [[ "${PASS_STARTED_AT:-}" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?Z$ ]]; then read_part=" read_at=${PASS_STARTED_AT}" fi local marker="" From 3a709963538384ebac3d4cfc0e8390867a493c4f Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:36:25 +0000 Subject: [PATCH 3/3] fix(reviews): accept fractional-second read_at in stale-disposition sweep Co-Authored-By: Claude Sonnet 5.5 --- scripts/dev-lead-retry.sh | 4 ++-- tests/dev-lead/unit/test_dev_lead_retry.bats | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/scripts/dev-lead-retry.sh b/scripts/dev-lead-retry.sh index 3b8ab6e39..142061726 100755 --- a/scripts/dev-lead-retry.sh +++ b/scripts/dev-lead-retry.sh @@ -513,8 +513,8 @@ stale_disposition_needs_dispatch() { | select((.authorAssociation // "") as $a | ["OWNER","MEMBER","COLLABORATOR"] | index($a) != null) | select((.body // "") | test($re) or test($start)) | (.createdAt // "") as $posted - | ([(.body // "") | capture("read_at=(?[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z)") | .r] | first // $posted) as $seen - | select($seen >= $e) | $seen ] | unique | length' <<< "$nodes" 2>/dev/null) || return 1 + | ([(.body // "") | capture("read_at=(?[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\\.[0-9]+)?Z)") | .r] | first // $posted) as $seen + | select(def n: sub("Z$"; "") | split(".") as $p | $p[0] + "." + (($p[1] // "") + "000000000")[0:9]; ($seen | n) >= ($e | n)) | $seen ] | unique | length' <<< "$nodes" 2>/dev/null) || return 1 [[ "$later_runs" =~ ^[0-9]+$ ]] || return 1 [ "$later_runs" -lt "$max_passes" ] || return 3 return 0 diff --git a/tests/dev-lead/unit/test_dev_lead_retry.bats b/tests/dev-lead/unit/test_dev_lead_retry.bats index 0ddadac8a..cfbd3e392 100644 --- a/tests/dev-lead/unit/test_dev_lead_retry.bats +++ b/tests/dev-lead/unit/test_dev_lead_retry.bats @@ -300,6 +300,25 @@ _with_nodes() { [ "$status" -eq 3 ] } +@test "sweep(#2247): millisecond-precision read_at is parsed, deduped across start+terminal, and compared to the edit" { + local nodes + # Start (createdAt ...01Z) and terminal (...45Z) markers share read_at=...:00.123Z: one pass. + nodes=$(_with_nodes "$(_sweep_nodes 2026-10-01T20:35:00Z 2026-10-01T19:23:54Z 2026-10-01T20:45:00Z OWNER 2026-10-01T20:40:00.123Z)" \ + "$(_start_node IC_s1 2026-10-01T20:40:01Z 2026-10-01T20:40:00.123Z)") + run stale_disposition_needs_dispatch "$nodes" 2000 + [ "$status" -eq 0 ] + STALE_DISPOSITION_MAX_PASSES=1 run stale_disposition_needs_dispatch "$nodes" 2000 + [ "$status" -eq 3 ] + # A pass that read at 20:30:00.500Z (before the edit) posted after it: not counted. + nodes=$(_sweep_nodes 2026-10-01T20:35:00Z 2026-10-01T19:23:54Z 2026-10-01T20:36:15Z OWNER 2026-10-01T20:30:00.500Z) + STALE_DISPOSITION_MAX_PASSES=1 run stale_disposition_needs_dispatch "$nodes" 2000 + [ "$status" -eq 0 ] + # read_at 20:35:00.000Z equals the edit second: counted (fraction must not sort it below). + nodes=$(_sweep_nodes 2026-10-01T20:35:00Z 2026-10-01T19:23:54Z 2026-10-01T20:36:15Z OWNER 2026-10-01T20:35:00.000Z) + STALE_DISPOSITION_MAX_PASSES=1 run stale_disposition_needs_dispatch "$nodes" 2000 + [ "$status" -eq 3 ] +} + @test "sweep(#2247): a start marker before the edit, for another PR, or from an untrusted author does not count" { export STALE_DISPOSITION_MAX_PASSES=1 # one counted pass reaches the cap local base