From f7cc3eb3ef70ae8f53ac1699bf559aede7a40316 Mon Sep 17 00:00:00 2001 From: don-petry <{}+don-petry@users.noreply.github.com> Date: Sat, 10 Oct 2026 23:23:02 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20implement=20issue=20#2156=20?= =?UTF-8?q?=E2=80=94=20dev-lead=20claim=20retraction=20does=20not=20cover?= =?UTF-8?q?=20issue-comment=20dispositions:=20a=20fixed=20reply=20for=20a?= =?UTF-8?q?=20commit=20that=20never=20landed=20stays=20up?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- scripts/dev-lead-fix-reviews.sh | 116 +++++++++++++-- scripts/lib/claim-landing.sh | 103 ++++++++++++++ tests/dev-lead/unit/test_claim_landing.bats | 86 +++++++++++ tests/dev-lead/unit/test_claim_sweep.bats | 149 ++++++++++++++++++++ 4 files changed, 446 insertions(+), 8 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 917e22282..508cf48b9 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -17,6 +17,12 @@ source "$(dirname "$0")/lib/review-change-evidence.sh" source "$(dirname "$0")/lib/open-review-threads.sh" source "$(dirname "$0")/lib/resolution-integrity.sh" source "$(dirname "$0")/lib/addressed-claim-verify.sh" +# PR issue-comment disposition verifier (#1813): the issue-comment sibling of +# addressed-claim-verify.sh. Turns a dev-lead comment-disposition reply into a +# machine-checkable claim the harness verifies before minimizing the original +# comment RESOLVED. Sourced before claim-landing.sh, whose `fixed`-disposition +# sweep (#2156) reuses its parser. +source "$(dirname "$0")/lib/comment-disposition-verify.sh" # Claim landing (#2013): "did the push land?" as a pure verdict, and retraction of # this pass's claim replies whose commit did not land on the remote head. source "$(dirname "$0")/lib/claim-landing.sh" @@ -26,11 +32,6 @@ source "$(dirname "$0")/lib/test-tamper-guard.sh" source "$(dirname "$0")/lib/sync-stub-guard.sh" # Test-regression guard (#2013): a pass may not push with the suite newly red. source "$(dirname "$0")/lib/test-regression-guard.sh" -# PR issue-comment disposition verifier (#1813): the issue-comment sibling of -# addressed-claim-verify.sh. Turns a dev-lead comment-disposition reply into a -# machine-checkable claim the harness verifies before minimizing the original -# comment RESOLVED. -source "$(dirname "$0")/lib/comment-disposition-verify.sh" # Review-thread deferral verifier (#2045): the thread-side sibling of the # `out-of-scope` disposition. A bot thread deferred to an open tracking issue that # links it is resolved by resolve_deferred_bot_threads. @@ -122,6 +123,93 @@ github_knows_commit() { return 2 } +# sweep_fixed_dispositions — the issue-comment half of both +# claim sweeps (#2156). A `disposition=fixed` reply posted as a PR issue comment +# cites a commit just like a review-thread claim, but the sweeps listed only +# `pulls/{pr}/comments`, so a "fixed" reply whose commit never landed stayed up. +# Every such reply OUR account posted in the window (`pass`: since PASS_START_ISO, +# `earlier`: before it) whose cited commit is not on is rewritten with +# cl_retract_disposition_body. As for review-thread claims: +# - pass: a commit the push guard rebased onto a foreign commit is re-pointed +# at its landed successor instead (cl_map_sha, #2032); +# - earlier: a commit missing from a still-shallow checkout, one GitHub knows +# (pushed, then rewritten), or one GitHub cannot be asked about is +# left as is. +# Best-effort: returns 1 on a listing/PATCH failure; the caller folds it in. +sweep_fixed_dispositions() { + local ref="${1:-}" window="${2:-pass}" + local bot_user="${BOT_USER:-donpetry-bot}" comments rows + comments=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null | jq -s 'add // []' 2>/dev/null) || { + echo "::warning::sweep_fixed_dispositions: could not list issue comments on PR #${PR_NUMBER} — its ${window} \`fixed\` disposition replies were NOT verified (#2156)" + return 1 + } + rows=$(cl_select_fixed_dispositions "$comments" "$bot_user" "${PASS_START_ISO:-}" "$window") || { + echo "::warning::sweep_fixed_dispositions: could not parse the issue comments on PR #${PR_NUMBER} — its ${window} \`fixed\` disposition replies were NOT verified (#2156)" + return 1 + } + [ -z "$rows" ] && return 0 + + local id sha reason body new_body rebased known_rc deepened=false + local retracted=0 remapped=0 failed=0 + while IFS=$'\t' read -r id sha; do + [ -z "$id" ] && continue + reason="unverifiable" + if [ -n "$sha" ]; then + if [ "$window" = "earlier" ] && ! git cat-file -e "${sha}^{commit}" 2>/dev/null; then + if [ "$deepened" = false ]; then + git_history_deepen "${BASE_REF:-main}" + deepened=true + fi + if ! git cat-file -e "${sha}^{commit}" 2>/dev/null \ + && [ "$(git rev-parse --is-shallow-repository 2>/dev/null || echo true)" != "false" ]; then + echo "::notice::sweep_fixed_dispositions: commit ${sha} of reply ${id} is not in this shallow checkout — left as is (#2156)" + continue + fi + fi + if [ -n "$ref" ] && git merge-base --is-ancestor "$sha" "$ref" 2>/dev/null; then + continue + fi + reason="not-on-ref" + if [ "$window" = "earlier" ]; then + known_rc=0 + github_knows_commit "$sha" || known_rc=$? + if [ "$known_rc" -eq 0 ]; then + echo "::notice::sweep_fixed_dispositions: commit ${sha} of reply ${id} is known to GitHub but not on the remote head (pushed, then rewritten) — left as is (#2156)" + continue + elif [ "$known_rc" -ne 1 ]; then + echo "::notice::sweep_fixed_dispositions: could not ask GitHub about commit ${sha} of reply ${id} — left as is (#2156)" + continue + fi + fi + fi + + body=$(printf '%s' "$comments" | jq -r --arg id "$id" 'first(.[] | select((.id | tostring) == $id)) | .body // ""' 2>/dev/null || echo "") + rebased="" + if [ "$window" = "pass" ] && [ "$reason" = "not-on-ref" ] && [ -n "$ref" ]; then + rebased=$(cl_map_sha "$sha" "${_PUSH_GUARD_REWRITES:-}") || rebased="" + fi + if [ -n "$rebased" ] && git merge-base --is-ancestor "$rebased" "$ref" 2>/dev/null \ + && new_body=$(cl_rewrite_disposition_sha "$body" "$rebased"); then + if gh api -X PATCH "repos/${REPO}/issues/comments/${id}" -f body="$new_body" >/dev/null 2>&1; then + remapped=$((remapped + 1)) + echo "::notice::\`fixed\` reply ${id} on PR #${PR_NUMBER}: ${sha} was rebased onto a foreign commit and landed as ${rebased} — disposition updated, not retracted (#2156)" + continue + fi + echo "::warning::could not update \`fixed\` reply ${id} on PR #${PR_NUMBER} to the rebased commit ${rebased} — retracting it instead (#2156)" + fi + new_body=$(cl_retract_disposition_body "$body" "$reason") + if gh api -X PATCH "repos/${REPO}/issues/comments/${id}" -f body="$new_body" >/dev/null 2>&1; then + retracted=$((retracted + 1)) + echo "::warning::retracted \`fixed\` disposition reply ${id} on PR #${PR_NUMBER} (${reason}; cited ${sha:-}, remote head ${ref:-}) (#2156)" + else + echo "::error::could not retract \`fixed\` disposition reply ${id} on PR #${PR_NUMBER} (${reason}) — a false 'fixed' reply may remain (#2156)" + failed=1 + fi + done <<< "$rows" + echo "::notice::sweep_fixed_dispositions: retracted ${retracted} ${window} \`fixed\` disposition reply(ies) on PR #${PR_NUMBER} (${remapped} re-pointed at rebased commits) (#2156)" + [ "$failed" -eq 0 ] +} + # sweep_earlier_claims — the start-of-pass sweep over EARLIER passes' claim replies # (#2032). retract_unlanded_claims runs only inside the pass that posted a reply, so # a run cancelled or killed after its model replied (a concurrency cancel, a lost @@ -132,6 +220,8 @@ github_knows_commit() { # claimed files, or was committed before the comment it answers is retracted; # - a retracted reply whose claim now passes is restored (cl_restore_body), so a # wrongly retracted fix recovers without a new commit. +# Earlier `fixed` disposition replies on issue comments are swept too +# (sweep_fixed_dispositions, #2156). # It never retracts on an unknown reference: when the remote head cannot be read, # or a cited commit is missing from a checkout that is still shallow, it skips. # Best-effort: returns 1 on a listing/PATCH failure; the caller does not stop. @@ -155,6 +245,10 @@ sweep_earlier_claims() { return 0 fi + # Earlier `fixed` disposition replies on the PR's issue comments (#2156). + local failed=0 + sweep_fixed_dispositions "$ref" earlier || failed=1 + local bot_user="${BOT_USER:-donpetry-bot}" comments rows comments=$(gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/comments?per_page=100" 2>/dev/null | jq -s 'add // []' 2>/dev/null) || { echo "::warning::sweep_earlier_claims: could not list review comments on PR #${PR_NUMBER} — earlier claim replies were NOT verified (#2032)" @@ -164,10 +258,10 @@ sweep_earlier_claims() { echo "::warning::sweep_earlier_claims: could not parse the review comments on PR #${PR_NUMBER} — earlier claim replies were NOT verified (#2032)" return 1 } - [ -z "$rows" ] && return 0 + [ -z "$rows" ] && return "$failed" local id state claim finding_at sha files facts on_ref touches commit_date own cumulative verdict - local body new_body deepened=false retracted=0 restored=0 failed=0 known_rc + local body new_body deepened=false retracted=0 restored=0 known_rc while IFS=$'\x1f' read -r id state claim finding_at; do [ -z "$id" ] && continue sha="" @@ -3247,6 +3341,8 @@ stamp_pass_replies() { # posted since PASS_START_ISO, verify its commit is one this pass produced AND is # on the REMOTE head (acv_claim_in_pass); otherwise rewrite it with # cl_retract_body, which strips both markers so it can never resolve a thread. +# This pass's `fixed` disposition replies on issue comments are swept against the +# same reference head (sweep_fixed_dispositions, #2156). # # The reference head is the remote head. When there is no upstream (rc 1) on an # `ok` outcome, the local HEAD is what was pushed; in every other case — including @@ -3276,13 +3372,17 @@ retract_unlanded_claims() { fi fi + # This pass's `fixed` disposition replies on the PR's issue comments (#2156). + local failed=0 + sweep_fixed_dispositions "$ref" pass || failed=1 + local comments comments=$(gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/comments?per_page=100" 2>/dev/null | jq -s 'add // []' 2>/dev/null) || { echo "::warning::retract_unlanded_claims: could not list review comments on PR #${PR_NUMBER} — this pass's claim replies were NOT verified (#2013)" return 1 } - local rows retracted=0 failed=0 id sha facts on_ref in_base reason body new_body + local rows retracted=0 id sha facts on_ref in_base reason body new_body rows=$(cl_select_pass_claims "$comments" "$bot_user" "$PASS_START_ISO") || { echo "::warning::retract_unlanded_claims: could not parse the review comments on PR #${PR_NUMBER} — this pass's claim replies were NOT verified (#2013)" return 1 diff --git a/scripts/lib/claim-landing.sh b/scripts/lib/claim-landing.sh index e2d792ea0..997da2c4e 100644 --- a/scripts/lib/claim-landing.sh +++ b/scripts/lib/claim-landing.sh @@ -29,6 +29,12 @@ # from earlier passes (a cancelled run never reached its own sweep, #2032) and # cl_earlier_claim_verdict decides each: an unlanded one is retracted, and a # retracted one whose fix is on the remote head is restored (cl_restore_body). +# - both sweeps also cover OUR PR issue-comment replies marked +# `disposition=fixed` (#2156): cl_select_fixed_dispositions picks them, and one +# whose cited commit is not on the remote head is rewritten with +# cl_retract_disposition_body (or re-pointed with cl_rewrite_disposition_sha +# when the push guard rebased it). Other dispositions cite no commit and are +# never selected. # # PURITY / TESTABILITY (ADR-0004) # Every cl_* function except cl_remote_head is PURE: no network, no git, no gh. @@ -42,6 +48,11 @@ if ! declare -F acv_parse_claim >/dev/null 2>&1; then # shellcheck source=addressed-claim-verify.sh source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/addressed-claim-verify.sh" fi +# cdv_parse_disposition is the single disposition parser (#2156). +if ! declare -F cdv_parse_disposition >/dev/null 2>&1; then + # shellcheck source=comment-disposition-verify.sh + source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/comment-disposition-verify.sh" +fi # The retraction marker. Matches the shared `` agent-marker # discriminator, so a retracted reply is still recognised as ours, but it is NOT the @@ -434,6 +445,98 @@ cl_earlier_claim_verdict() { return 0 } +# cl_select_fixed_dispositions +# The issue-comment counterpart of the claim selectors (#2156). is +# the REST `issues/{pr}/comments` array ({id, user.login, body, created_at}). +# Emits one `\t` row per comment that our account authored and that +# carries a `disposition=fixed` comment-disposition marker, created at or after +# (`pass`) or before it (`earlier`). is the cited commit, or +# empty when the marker does not parse (unverifiable -> the caller retracts). +# invalid / answered / informational / out-of-scope replies cite no commit and +# are never selected. An empty or a non-array payload selects +# nothing. Pure. +cl_select_fixed_dispositions() { + local comments_json="${1:-}" bot_user="${2:-}" since="${3:-}" window="${4:-pass}" + [[ -z "$since" ]] && return 0 + local bot_stripped="${bot_user%\[bot\]}" + + local rows + rows=$(jq -r \ + --arg u "$bot_user" --arg us "$bot_stripped" --arg since "$since" --arg w "$window" \ + --arg prefix "$_CDV_MARKER_PREFIX" ' + if type == "array" then + .[] | objects + | (.user.login // "") as $l + | select($l == $u or $l == $us or $l == ($us + "[bot]")) + | (.created_at // "") as $at + | select(if $w == "earlier" then $at < $since else $at >= $since end) + | (.body // "") as $b + | select($b | contains($prefix)) + | select($b | test("` is appended so the reply is +# still recognised as ours. is restricted to [a-z-] (anything else +# becomes `unknown`). Pure. +cl_retract_disposition_body() { + local body="${1:-}" reason="${2:-unknown}" + [[ "$reason" =~ ^[a-z][a-z-]*$ ]] || reason="unknown" + + local stripped quoted + stripped=$(jq -Rsr ' + gsub(""; "") + | sub("\\s+$"; "") + ' <<<"$body" 2>/dev/null) || stripped="" + quoted=$(printf '%s\n' "$stripped" | sed 's/^/> /') + + printf '%s\n\n%s\n\n%s\n\n%s%s -->\n' \ + "**⚠️ Retracted by the dev-lead harness (\`${reason}\`).** This reply's \`fixed\` disposition cited a commit that is not on the PR branch (the push did not land), so it no longer counts as a fix. The comment stays open; the next dev-lead pass re-answers it against the pushed branch." \ + "${_CL_ORIGINAL_HEADER} (unverified — it does not count as a fix):" \ + "$quoted" \ + "$_CL_RETRACTED_PREFIX" "$reason" +} + +# cl_rewrite_disposition_sha +# with its `fixed` disposition marker's sha replaced by (a full +# 40-hex SHA); every other byte is kept. Used when the push guard rebased the +# cited commit and it landed under a new SHA (#2156, as cl_rewrite_claim_sha). +# Returns 1 (echoing nothing) when the body has no valid `fixed` disposition or +# is not a full SHA. Pure. +cl_rewrite_disposition_sha() { + local body="${1:-}" new_sha="${2:-}" + [[ "$new_sha" =~ ^[0-9a-f]{40}$ ]] || return 1 + local parsed old + parsed=$(cdv_parse_disposition "$body") || return 1 + [[ "$(jq -r '.disposition // ""' <<<"$parsed" 2>/dev/null)" == "fixed" ]] || return 1 + old=$(jq -r '.sha // ""' <<<"$parsed" 2>/dev/null) || return 1 + + local rest marker + rest="${body#*"$_CDV_MARKER_PREFIX"}" + marker="${_CDV_MARKER_PREFIX}${rest%%"$_CDV_MARKER_SUFFIX"*}${_CDV_MARKER_SUFFIX}" + printf '%s\n' "${body/"$marker"/"${marker/"sha=${old}"/"sha=${new_sha}"}"}" +} + # cl_remote_head # The single impure reader: fetch the current branch's upstream and echo the TRUE # remote head SHA (not a stale remote-tracking ref). Returns 1 (echoing nothing) diff --git a/tests/dev-lead/unit/test_claim_landing.bats b/tests/dev-lead/unit/test_claim_landing.bats index 2ac6cc6f6..bc9974ca4 100644 --- a/tests/dev-lead/unit/test_claim_landing.bats +++ b/tests/dev-lead/unit/test_claim_landing.bats @@ -536,3 +536,89 @@ _inject_foreign_commit() { run acv_latest_nochange_disposition "$comments" "2026-10-10T00:00:00Z" [[ "$status" -eq 1 ]] } + +# --------------------------------------------------------------------------- +# #2156: issue-comment `fixed` dispositions +# --------------------------------------------------------------------------- + +_ic() { + # _ic — one REST issue comment. + jq -cn --argjson id "$1" --arg l "$2" --arg at "$3" --arg b "$4" \ + '{id:$id, user:{login:$l}, created_at:$at, body:$b}' +} + +_disp() { + printf 'Done.\n\n' "$1" "${2:+ $2}" +} + +@test "cl_select_fixed_dispositions: picks only our fixed replies in the window, with their sha" { + local comments + comments=$(printf '%s\n' \ + "$(_ic 1 donpetry-bot 2026-10-09T02:00:00Z "$(_disp fixed "sha=$A40")")" \ + "$(_ic 2 donpetry-bot 2026-10-09T04:00:00Z "$(_disp fixed "sha=$B40")")" \ + "$(_ic 3 someone-else 2026-10-09T02:00:00Z "$(_disp fixed "sha=$C40")")" \ + "$(_ic 4 donpetry-bot 2026-10-09T02:00:00Z "$(_disp invalid)")" \ + "$(_ic 5 donpetry-bot 2026-10-09T02:00:00Z "$(_disp answered)")" \ + "$(_ic 6 donpetry-bot 2026-10-09T02:00:00Z "$(_disp informational)")" \ + "$(_ic 7 donpetry-bot 2026-10-09T02:00:00Z "$(_disp out-of-scope "ref=#12")")" \ + "$(_ic 8 'donpetry-bot[bot]' 2026-10-09T02:00:00Z "$(_disp fixed "sha=$C40")")" \ + | jq -sc .) + + run cl_select_fixed_dispositions "$comments" donpetry-bot 2026-10-09T03:00:00Z earlier + [[ "$status" -eq 0 ]] + [[ "$output" == "$(printf '1\t%s\n8\t%s' "$A40" "$C40")" ]] + + run cl_select_fixed_dispositions "$comments" donpetry-bot 2026-10-09T03:00:00Z pass + [[ "$status" -eq 0 ]] + [[ "$output" == "$(printf '2\t%s' "$B40")" ]] +} + +@test "cl_select_fixed_dispositions: a fixed marker with a malformed sha is selected with an empty sha" { + local comments + comments=$(_ic 9 donpetry-bot 2026-10-09T02:00:00Z "$(_disp fixed "sha=9313650")" | jq -sc .) + run cl_select_fixed_dispositions "$comments" donpetry-bot 2026-10-09T03:00:00Z earlier + [[ "$output" == "$(printf '9\t')" ]] +} + +@test "cl_select_fixed_dispositions: no pass boundary or a bad payload selects nothing" { + local comments + comments=$(_ic 1 donpetry-bot 2026-10-09T02:00:00Z "$(_disp fixed "sha=$A40")" | jq -sc .) + run cl_select_fixed_dispositions "$comments" donpetry-bot "" earlier + [[ -z "$output" ]] + run cl_select_fixed_dispositions '{"message":"Not Found"}' donpetry-bot 2026-10-09T03:00:00Z earlier + [[ -z "$output" ]] +} + +@test "cl_retract_disposition_body: strips the marker, adds a notice, quotes the original" { + local body new + body=$(_disp fixed "sha=$A40") + new=$(cl_retract_disposition_body "$body" not-on-ref) + [[ "$new" == *"Retracted by the dev-lead harness"* ]] + [[ "$new" == *"> Done."* ]] + [[ "$new" != *"dev-lead:comment-disposition"* ]] + [[ "$new" == *""* ]] + # Still recognised as ours by the maintainer-gate agent-marker discriminator. + [[ "$new" =~ \ ' + [[ "$output" == *""* ]] +} + +@test "cl_rewrite_disposition_sha: re-points the fixed marker at a new sha, keeping the rest" { + run cl_rewrite_disposition_sha "$(_disp fixed "sha=$A40")" "$B40" + [[ "$status" -eq 0 ]] + [[ "$output" == *"Done."* ]] + [[ "$output" == *""* ]] + [[ "$output" != *"$A40"* ]] + run cl_rewrite_disposition_sha "$(_disp fixed "sha=$A40")" "abc" + [[ "$status" -eq 1 ]] + run cl_rewrite_disposition_sha "$(_disp invalid)" "$B40" + [[ "$status" -eq 1 ]] +} diff --git a/tests/dev-lead/unit/test_claim_sweep.bats b/tests/dev-lead/unit/test_claim_sweep.bats index d5f6a29d6..32b4be3fc 100644 --- a/tests/dev-lead/unit/test_claim_sweep.bats +++ b/tests/dev-lead/unit/test_claim_sweep.bats @@ -31,9 +31,11 @@ setup() { REMOTE="$BATS_TEST_TMPDIR/remote.git" WORK="$BATS_TEST_TMPDIR/work" COMMENTS="$BATS_TEST_TMPDIR/comments.json" + ISSUE_COMMENTS="$BATS_TEST_TMPDIR/issue-comments.json" PATCHES="$BATS_TEST_TMPDIR/patches" mkdir -p "$PATCHES" echo '[]' > "$COMMENTS" + echo '[]' > "$ISSUE_COMMENTS" git init -q --bare "$REMOTE" local seed="$BATS_TEST_TMPDIR/seed" @@ -68,6 +70,8 @@ case "\$*" in for a in "\$@"; do case "\$a" in *pulls/comments/*) id="\${a##*/}" ;; + # Issue-comment PATCHes (#2156) are recorded as issue-. + *issues/comments/*) id="issue-\${a##*/}" ;; body=*) body="\${a#body=}" ;; esac done @@ -75,6 +79,7 @@ case "\$*" in echo '{}' ;; *"pulls/54/comments"*) cat "$COMMENTS" ;; + *"issues/54/comments"*) cat "$ISSUE_COMMENTS" ;; # github_knows_commit: GH_KNOWN_COMMIT was pushed; GH_COMMITS_ERROR fails the lookup. *"/commits/"*) all="\$*"; sha="\${all##*/commits/}"; sha="\${sha%% *}" @@ -90,12 +95,14 @@ GHEOF # shellcheck source=/dev/null source "$LIB_DIR/addressed-claim-verify.sh" + source "$LIB_DIR/comment-disposition-verify.sh" source "$LIB_DIR/claim-landing.sh" source "$LIB_DIR/git-push-guard.sh" source "$LIB_DIR/git-history.sh" eval "$(sed -n '/^github_knows_commit()/,/^}/p' "$FIX_REVIEWS_SCRIPT")" eval "$(sed -n '/^sweep_earlier_claims()/,/^}/p' "$FIX_REVIEWS_SCRIPT")" eval "$(sed -n '/^retract_unlanded_claims()/,/^}/p' "$FIX_REVIEWS_SCRIPT")" + eval "$(sed -n '/^sweep_fixed_dispositions()/,/^}/p' "$FIX_REVIEWS_SCRIPT")" declare -F sweep_earlier_claims >/dev/null declare -F retract_unlanded_claims >/dev/null @@ -345,3 +352,145 @@ _landed_fix() { [ -f "$PATCHES/211" ] grep -q "reason=not-on-ref" "$PATCHES/211" } + +# --------------------------------------------------------------------------- +# #2156: issue-comment `fixed` dispositions are swept like review-thread claims +# --------------------------------------------------------------------------- +# PR #2080: comment 6073169970 carried `disposition=fixed` citing 9313650…, which +# never reached the branch; the review-thread claim was retracted, this was not. + +# _disposition [sha] [ref] — the reply's body. +_disposition() { + printf 'Handled the empty-input case.\n\n' \ + "$1" "$2" "${3:+ sha=$3}" "${4:+ ref=$4}" +} + +# _serve_issue [ ]... — the PR's +# issue comments: our replies (REST shape). +_serve_issue() { + local rows=() + while [ "$#" -ge 3 ]; do + rows+=("$(jq -c -n --argjson id "$1" --arg at "$2" --arg body "$3" \ + '{id: $id, user: {login: "donpetry-bot"}, created_at: $at, body: $body}')") + shift 3 + done + printf '%s\n' "${rows[@]}" | jq -s -c . > "$ISSUE_COMMENTS" +} + +@test "#2156 AC1/AC2: an earlier fixed issue-comment reply citing an unlanded commit is retracted" { + local ghost="931365099bfb2c283aecfb16b0f5f0b8d59436b2" + _serve_issue 6073169970 "2026-10-09T02:42:00Z" "$(_disposition IC_kwDO123 fixed "$ghost")" + + run sweep_earlier_claims + [ "$status" -eq 0 ] + [ -f "$PATCHES/issue-6073169970" ] + grep -q "Retracted" "$PATCHES/issue-6073169970" + grep -q "" "$PATCHES/issue-6073169970" + # The marker is gone, so neither the minimizer nor a reader sees "fixed" any more… + ! grep -q "dev-lead:comment-disposition" "$PATCHES/issue-6073169970" + run cdv_parse_disposition "$(cat "$PATCHES/issue-6073169970")" + [ "$status" -eq 1 ] + # …and the original text is kept as a quote. + grep -q "^> Handled the empty-input case." "$PATCHES/issue-6073169970" +} + +@test "#2156 AC1: an earlier fixed reply whose commit landed is kept" { + local fix + fix=$(_landed_fix "2026-10-05T00:00:00Z") + _serve_issue 301 "2026-10-05T00:01:00Z" "$(_disposition IC_kwDO123 fixed "$fix")" + + run sweep_earlier_claims + [ "$status" -eq 0 ] + [[ "$output" == *"sweep_fixed_dispositions: retracted 0 earlier"* ]] + [ ! -f "$PATCHES/issue-301" ] +} + +@test "#2156 AC1: an earlier fixed reply whose commit GitHub knows (pushed, then rewritten) is kept" { + local ghost="931365099bfb2c283aecfb16b0f5f0b8d59436b2" + export GH_KNOWN_COMMIT="$ghost" + _serve_issue 302 "2026-10-09T02:42:00Z" "$(_disposition IC_kwDO123 fixed "$ghost")" + + run sweep_earlier_claims + [ "$status" -eq 0 ] + [[ "$output" == *"pushed, then rewritten"* ]] + [ ! -f "$PATCHES/issue-302" ] +} + +@test "#2156 AC3: invalid / answered / informational / out-of-scope replies are never retracted" { + _serve_issue \ + 311 "2026-10-09T02:42:00Z" "$(_disposition IC_a invalid)" \ + 312 "2026-10-09T02:42:00Z" "$(_disposition IC_b answered)" \ + 313 "2026-10-09T02:42:00Z" "$(_disposition IC_c informational)" \ + 314 "2026-10-09T02:42:00Z" "$(_disposition IC_d out-of-scope "" "#9999")" + + run sweep_earlier_claims + [ "$status" -eq 0 ] + [ -z "$(ls -A "$PATCHES")" ] + + _serve_issue \ + 315 "2026-10-09T03:01:00Z" "$(_disposition IC_a invalid)" \ + 316 "2026-10-09T03:01:00Z" "$(_disposition IC_b answered)" \ + 317 "2026-10-09T03:01:00Z" "$(_disposition IC_c informational)" \ + 318 "2026-10-09T03:01:00Z" "$(_disposition IC_d out-of-scope "" "#9999")" + run retract_unlanded_claims fix-bot-comment failed + [ "$status" -eq 0 ] + [ -z "$(ls -A "$PATCHES")" ] +} + +@test "#2156 AC1/AC2: the end-of-pass sweep retracts this pass's fixed reply whose commit did not land" { + # This pass committed a fix and the model cited it, but the push never landed. + echo fixed > f; git add f; git commit -q -m "fix(reviews): address review comments" + local cited; cited=$(git rev-parse HEAD) + _serve_issue 320 "2026-10-09T03:01:00Z" "$(_disposition IC_kwDO123 fixed "$cited")" + + run retract_unlanded_claims fix-bot-comment failed + [ -f "$PATCHES/issue-320" ] + grep -q "reason=not-on-ref" "$PATCHES/issue-320" + ! grep -q "dev-lead:comment-disposition" "$PATCHES/issue-320" +} + +@test "#2156 AC1: the end-of-pass sweep keeps this pass's fixed reply whose commit landed" { + echo fixed > f; git add f; git commit -q -m "fix(reviews): address review comments" + local cited; cited=$(git rev-parse HEAD) + git push -q origin main + _serve_issue 321 "2026-10-09T03:01:00Z" "$(_disposition IC_kwDO123 fixed "$cited")" + + run retract_unlanded_claims fix-bot-comment ok + [ "$status" -eq 0 ] + [[ "$output" == *"sweep_fixed_dispositions: retracted 0 pass"* ]] + [ ! -f "$PATCHES/issue-321" ] +} + +@test "#2156 AC1: the end-of-pass sweep leaves earlier passes' fixed replies to the start-of-pass sweep" { + local ghost="931365099bfb2c283aecfb16b0f5f0b8d59436b2" + _serve_issue 322 "2026-10-09T02:42:00Z" "$(_disposition IC_kwDO123 fixed "$ghost")" + + run retract_unlanded_claims fix-bot-comment ok + [ ! -f "$PATCHES/issue-322" ] +} + +@test "#2156: a clean rebase re-points this pass's fixed reply at the landed commit instead of retracting it" { + echo fixed > f; git add f; git commit -q -m "fix(reviews): address review comments" + local cited; cited=$(git rev-parse HEAD) + _serve_issue 323 "2026-10-09T03:01:00Z" "$(_disposition IC_kwDO123 fixed "$cited")" + _foreign_push merged-main.txt "from main" + push_no_clobber + local rebased; rebased=$(git rev-parse HEAD) + [ "$rebased" != "$cited" ] + + run retract_unlanded_claims fix-bot-comment ok + [ "$status" -eq 0 ] + [ -f "$PATCHES/issue-323" ] + ! grep -q "Retracted" "$PATCHES/issue-323" + run cdv_parse_disposition "$(cat "$PATCHES/issue-323")" + [ "$status" -eq 0 ] + [ "$(jq -r .sha <<<"$output")" = "$rebased" ] + [ "$(jq -r .disposition <<<"$output")" = "fixed" ] +} + +@test "#2156: a failed issue-comment listing is reported, not read as clean" { + printf 'not json' > "$ISSUE_COMMENTS" + run retract_unlanded_claims fix-bot-comment ok + [ "$status" -ne 0 ] + [[ "$output" == *"issue comments"* ]] +} From f7b0cf11a25f37d58bc65c7118bb9f00da504453 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Sat, 10 Oct 2026 23:38:05 +0000 Subject: [PATCH 2/2] fix(reviews): base64 fallback and slice-based sha rewrite in claim-landing Co-Authored-By: Claude Sonnet 5.5 --- scripts/lib/claim-landing.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/scripts/lib/claim-landing.sh b/scripts/lib/claim-landing.sh index 997da2c4e..290918caa 100644 --- a/scripts/lib/claim-landing.sh +++ b/scripts/lib/claim-landing.sh @@ -482,7 +482,7 @@ cl_select_fixed_dispositions() { local id body_b64 body parsed sha while IFS=$'\t' read -r id body_b64; do [[ -z "$id" ]] && continue - body=$(base64 --decode <<<"$body_b64" 2>/dev/null || printf '') + body=$(base64 --decode <<<"$body_b64" 2>/dev/null || base64 -d <<<"$body_b64" 2>/dev/null || printf '') sha="" if parsed=$(cdv_parse_disposition "$body"); then [[ "$(jq -r '.disposition // ""' <<<"$parsed" 2>/dev/null)" == "fixed" ]] || continue @@ -531,10 +531,13 @@ cl_rewrite_disposition_sha() { [[ "$(jq -r '.disposition // ""' <<<"$parsed" 2>/dev/null)" == "fixed" ]] || return 1 old=$(jq -r '.sha // ""' <<<"$parsed" 2>/dev/null) || return 1 - local rest marker + local before rest after middle new_middle + before="${body%%"$_CDV_MARKER_PREFIX"*}" rest="${body#*"$_CDV_MARKER_PREFIX"}" - marker="${_CDV_MARKER_PREFIX}${rest%%"$_CDV_MARKER_SUFFIX"*}${_CDV_MARKER_SUFFIX}" - printf '%s\n' "${body/"$marker"/"${marker/"sha=${old}"/"sha=${new_sha}"}"}" + after="${rest#*"$_CDV_MARKER_SUFFIX"}" + middle="${rest%%"$_CDV_MARKER_SUFFIX"*}" + new_middle="${middle/"sha=${old}"/"sha=${new_sha}"}" + printf '%s%s%s%s%s\n' "$before" "$_CDV_MARKER_PREFIX" "$new_middle" "$_CDV_MARKER_SUFFIX" "$after" } # cl_remote_head