Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
38 changes: 36 additions & 2 deletions scripts/dev-lead-fix-reviews.sh
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ REVIEWS_MARKER_PREFIX="<!-- dev-lead-fix-reviews pr="
# When this pass started, i.e. a lower bound on when it read the PR's comments.
# Stamped on terminal markers as read_at= so the #2008 stale-edit dedup can tell a
# pass that read an edited body from one that merely FINISHED after the edit.
PASS_STARTED_AT="${PASS_STARTED_AT:-$(date -u +%Y-%m-%dT%H:%M:%SZ)}"
PASS_STARTED_AT="${PASS_STARTED_AT:-$(date -u +%Y-%m-%dT%H:%M:%S.%3NZ)}"
INTEGRITY_MARKER_PREFIX="<!-- dev-lead-conflict-integrity pr="
NONCONVERGE_MARKER_PREFIX="<!-- dev-lead-review-nonconverge pr="
# Consecutive not-applied passes against the same review before escalating to a
Expand Down Expand Up @@ -551,7 +551,7 @@ ${msg}"
comment_part="${comment_part} version=${COMMENT_VERSION}"
fi
fi
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="${REVIEWS_MARKER_PREFIX}${PR_NUMBER}${sha_part} intent=${intent} status=${status}${comment_part}${read_part} -->"
Expand Down Expand Up @@ -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.
Expand All @@ -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 `<!-- dev-lead` prefix is a registered automation
# marker, so the comment gate does not count it as a finding. Best effort: a failed
# post must not stop the pass.
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}(\.[0-9]+)?Z$ ]]; then
read_part=" read_at=${PASS_STARTED_AT}"
fi
local marker="<!-- dev-lead-fix-reviews-start pr=${PR_NUMBER}${sha_part} intent=fix-reviews edit=${STALE_LATEST_EDIT}${read_part} -->"
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.
Expand Down Expand Up @@ -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"
Expand Down
21 changes: 13 additions & 8 deletions scripts/dev-lead-retry.sh
Original file line number Diff line number Diff line change
Expand Up @@ -512,17 +512,21 @@ 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
# (`<!-- dev-lead-fix-reviews pr=<N> … intent=fix-reviews
# status=<any>`) that started at/after the latest such edit counts
# as one attempt. After STALE_DISPOSITION_MAX_PASSES (default 2) it returns 3:
# hold for a human rather than spend a model run on every sweep. A new edit
# starts a new count. A pass counts from when it STARTED (the marker's
# read_at=, a lower bound on when it read the comments), not when its marker
# was posted: a pass that began before the edit never saw the edited body. A
# legacy marker without read_at= falls back to its createdAt. Only a marker
# from a trusted author (OWNER/MEMBER/COLLABORATOR, as dev-lead's own markers
# are) counts, so an outside commenter cannot exhaust the cap.
# legacy marker without read_at= falls back to its createdAt. A pass's start
# marker (`<!-- dev-lead-fix-reviews-start pr=<N> … intent=fix-reviews`, posted
# before the model runs, #2247) counts too, so a pass that dies before its
# terminal marker still uses up an attempt. Markers are counted by distinct
# read_at, so a pass that posts both a start and a terminal marker counts once.
# Only a marker from a trusted author (OWNER/MEMBER/COLLABORATOR, as dev-lead's
# own markers are) counts, so an outside commenter cannot exhaust the cap.
# 1 otherwise, including unreadable input (never dispatch on a guess). Pure
# apart from reading the reviewer registry.
stale_disposition_needs_dispatch() {
Expand All @@ -534,13 +538,14 @@ stale_disposition_needs_dispatch() {
latest_edit=$(jq -r 'map(.lastEditedAt) | max // ""' <<< "$stale" 2>/dev/null) || return 1
[ -n "$latest_edit" ] || return 1
later_runs=$(jq -r --arg e "$latest_edit" \
--arg re "<!-- dev-lead-fix-reviews pr=${pr_number} [^>]*intent=fix-reviews status=" '
--arg re "<!-- dev-lead-fix-reviews pr=${pr_number} [^>]*intent=fix-reviews status=" \
--arg start "<!-- dev-lead-fix-reviews-start pr=${pr_number} [^>]*intent=fix-reviews[ >]" '
[ .[] | objects
| select((.authorAssociation // "") as $a | ["OWNER","MEMBER","COLLABORATOR"] | index($a) != null)
| select((.body // "") | test($re))
| select((.body // "") | test($re) or test($start))
| (.createdAt // "") as $posted
| ([(.body // "") | capture("read_at=(?<r>[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) ] | length' <<< "$nodes" 2>/dev/null) || return 1
| ([(.body // "") | capture("read_at=(?<r>[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
Expand Down
111 changes: 111 additions & 0 deletions tests/dev-lead/unit/test_dev_lead_retry.bats
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,117 @@ _sweep_nodes() {
[ "$status" -eq 3 ]
}

# ── #2247: a pass that dies before any terminal marker still counts ────────────
# A fix-reviews pass that sees a stale disposition posts a start marker before the
# model runs. A pass whose runner is lost after that posts nothing else, so the cap
# has to count start markers too. A pass that posts both counts once (same read_at).

# _start_node <id> <createdAt> <read_at> [pr] [association]: a start-marker node.
_start_node() {
jq -cn --arg id "$1" --arg c "$2" --arg r "$3" --arg pr "${4:-2000}" --arg a "${5:-OWNER}" '
{id:$id, author:{login:"don-petry", __typename:"User"}, authorAssociation:$a,
body:"<!-- dev-lead-fix-reviews-start pr=\($pr) sha=abc intent=fix-reviews edit=2026-10-01T20:35:00Z read_at=\($r) -->",
createdAt:$c, isMinimized:false, minimizedReason:null, lastEditedAt:null}'
}

# _with_nodes <nodes-json> <node-json>...: 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 ]
Expand Down
74 changes: 70 additions & 4 deletions tests/dev-lead/unit/test_fix_reviews.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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 <comments-json|FAIL>: run export_stale_dispositions against a gh stub
# whose comment read returns <comments-json> (or fails), and print the result.
# _stale_env <comments-json|FAIL> [var]: run export_stale_dispositions against a gh
# stub whose comment read returns <comments-json> (or fails), and print <var>
# (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
Expand All @@ -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}\"
"
}

Expand Down Expand Up @@ -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 <STALE_LATEST_EDIT> [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='<!-- dev-lead-fix-reviews pr='
eval \"\$(sed -n '/^post_stale_pass_start()/,/^}/p' '$FIX_REVIEWS_SCRIPT')\"
post_stale_pass_start
"
}

@test "fix-reviews(#2247): a pass with a stale disposition posts a start marker carrying pr, sha, edit and read_at" {
_start_post 2026-10-10T13:03:53Z
[ "$status" -eq 0 ]
grep -qF '<!-- dev-lead-fix-reviews-start pr=54 sha=ddd444eee555 intent=fix-reviews edit=2026-10-10T13:03:53Z read_at=2026-10-10T13:20:00Z -->' "$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 '<!-- dev-lead-fix-reviews-start [^>]*-->' "$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"* ]]
}
8 changes: 8 additions & 0 deletions tests/dev-lead/unit/test_maintainer_comment_gate.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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":"<!-- dev-lead-fix-reviews-start pr=54 sha=ddd444eee555 intent=fix-reviews edit=2026-10-10T13:03:53Z read_at=2026-10-10T13:20:00Z -->","createdAt":"2026-10-10T13:20:01Z","isMinimized":false,"minimizedReason":""}]}'
_run_check "$json"
[ "$status" -eq 0 ]
}
Loading