diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 922c9e1b6..a6a6e1da8 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -128,7 +128,7 @@ REVIEWS_MARKER_PREFIX="" @@ -951,10 +951,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. @@ -979,6 +986,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. @@ -4201,6 +4234,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 b3a08e523..04ac6c643 100755 --- a/scripts/dev-lead-retry.sh +++ b/scripts/dev-lead-retry.sh @@ -512,7 +512,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): 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 + 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 "combsmith/.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 "combsmith/.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 "combsmith/.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 8b8293d9d..46bbfb16c 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -7669,10 +7669,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 @@ -7686,7 +7687,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}\" " } @@ -7879,3 +7880,68 @@ GHEOF [ "$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 aedb688f4..579de4e9a 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 ] +}