From a876d5c40d108858fdafbe8ebdcc14a0c77f4495 Mon Sep 17 00:00:00 2001 From: don-petry <{}+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 02:20:26 +0000 Subject: [PATCH 01/14] =?UTF-8?q?feat:=20implement=20issue=20#2079=20?= =?UTF-8?q?=E2=80=94=20dev-lead:=20resolve=20a=20bot=20review=20thread=20o?= =?UTF-8?q?n=20a=20maintainer=20"no=20change=20needed"=20disposition=20(re?= =?UTF-8?q?build=20of=20#1799=20on=20#2049's=20resolver)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- scripts/lib/addressed-claim-verify.sh | 111 ++++++++ .../unit/test_addressed_claim_verify.bats | 236 ++++++++++++++++++ 2 files changed, 347 insertions(+) diff --git a/scripts/lib/addressed-claim-verify.sh b/scripts/lib/addressed-claim-verify.sh index a0e35fec0..975151651 100644 --- a/scripts/lib/addressed-claim-verify.sh +++ b/scripts/lib/addressed-claim-verify.sh @@ -63,6 +63,29 @@ readonly _ACV_CLAIM_SUFFIX=' -->' # merge" shape) rather than any human chatter, so a neutral "thanks" never blocks. readonly _ACV_DISPOSITION_RE_UPPER='(REQUIRED|MUST BE (FIXED|ADDRESSED|RESOLVED|CHANGED)|CHANGES REQUIRED|BLOCKING|REQUEST(ING|ED)? CHANGES)' +# A maintainer "no change needed" disposition (#1743, #2079) — the mirror image of the +# blocking disposition above. Matched case-insensitively against a marker-less +# maintainer comment. It PERMITS resolution of a false-positive bot thread, the +# missing half of the #1692 REQUIRED gate (which WITHHOLDS resolution). Kept as +# conservative as the blocking regex: it recognises an explicit false-positive / +# wontfix / not-applicable assertion, not any human chatter, so a neutral "thanks" +# never clears a thread. Note "no change required" contains the substring REQUIRED +# and so also matches _ACV_DISPOSITION_RE_UPPER; the caller must resolve that +# collision in favour of the no-change intent. +readonly _ACV_NOCHANGE_RE_UPPER='(NO (CODE )?CHANGES? (NEEDED|REQUIRED|NECESSARY|WARRANTED|IS NEEDED|ARE NEEDED)|FALSE[ -]?POSITIVE|WON.?T ?FIX|WONTFIX|WORKING AS INTENDED|NOT A (REAL )?(BUG|ISSUE|PROBLEM|CONCERN|DEFECT)|NOT APPLICABLE|BY DESIGN|INTENDED BEHAVIOU?R)' + +# A NEGATED (or quoted-then-rejected) no-change phrase is NOT an affirmative no-change +# disposition (#1799). The broad phrase match above treats any occurrence of e.g. +# "FALSE POSITIVE" as authorization, so "this is not a false positive" or "I wouldn't +# call it working as intended" would wrongly clear a bot thread a maintainer actually +# wants fixed. This recognises a negator within a few words before a no-change phrase; +# acv_latest_nochange_disposition checks it FIRST and lets such a comment fall through +# to "not a disposition" (fail closed -> the thread stays open for the maintainer). A +# phrase that itself begins with a negator ("NOT A BUG", "NO CHANGE NEEDED") only +# trips the guard when ANOTHER negator precedes it, so the bare affirmative forms are +# never clobbered. +readonly _ACV_NOCHANGE_NEGATION_RE_UPPER='(CANNOT|CAN.?T|COULD ?NOT|COULDN.?T|WILL NOT|WON.?T|WOULD ?NOT|WOULDN.?T|SHOULD ?NOT|SHOULDN.?T|IS ?NOT|ISN.?T|ARE ?NOT|AREN.?T|WAS ?NOT|WASN.?T|WERE ?NOT|WEREN.?T|DOES ?NOT|DOESN.?T|DO ?NOT|DON.?T|DID ?NOT|DIDN.?T|NEVER|NOT)[,[:space:]]+([A-Z,'"'"']+[[:space:]]+){0,6}(FALSE[ -]?POSITIVE|WON.?T ?FIX|WONTFIX|WORKING AS INTENDED|BY DESIGN|INTENDED BEHAVIOU?R|NO (CODE )?CHANGES? (NEEDED|REQUIRED|NECESSARY|WARRANTED|IS NEEDED|ARE NEEDED)|NOT A (REAL )?(BUG|ISSUE|PROBLEM|CONCERN|DEFECT)|NOT APPLICABLE)' + # Post-marker BOT-comment classification (#1735 AC2/AC4). A review bot that replies # AFTER our addressed-marker either ACKNOWLEDGES (accepts our refutation / records a # custom rule — the codeant-ai "✅ Customized review instruction saved!" shape) or @@ -374,14 +397,102 @@ acv_latest_maintainer_disposition() { fi done <<<"$rows" + # Fail closed: any matching disposition with an unparseable timestamp wins over a + # valid latest one, since its true order against the others is unknown (#2079 AC3). + if [[ "$saw_unparseable" -eq 1 ]]; then + echo "unparseable" + return 2 + fi if [[ -n "$latest" ]]; then echo "$latest" return 0 fi + echo "" + return 1 +} + +# acv_latest_nochange_disposition +# Scan ALL comments in a thread (#1743, #2079) for a standing maintainer "no change +# needed" disposition — the mirror of acv_latest_maintainer_disposition. +# is a JSON array of +# {author:{login,__typename}, authorAssociation, body, createdAt}. +# A comment counts as a no-change disposition when it is: a User (not a Bot), from +# a repo maintainer (authorAssociation OWNER|MEMBER|COLLABORATOR — anything else, +# including missing, fails closed), MARKER-LESS per review_thread_is_agent_authored, +# and asserts an affirmative no-change disposition (_ACV_NOCHANGE_RE_UPPER, with +# negated and question forms rejected). +# Authorship is decided by MARKER, never by login: dev-lead posts as the same +# account the maintainer uses, so skipping that login would discard the +# maintainer's own verdict (the #1799 defect), while the marker is what makes the +# agent unable to dismiss a finding on its own (#1743 AC4). +# Returns: +# 0 + echoes the latest such comment's ISO createdAt (a no-change disposition applies) +# 1 + echoes "" (no no-change disposition found) +# 2 + echoes "unparseable" (a no-change disposition was +# found but a createdAt is +# missing/unparseable -> +# fail closed, leave open) +# Pure — no gh/git/network. +acv_latest_nochange_disposition() { + local comments_json="${1:-}" + + # Single jq pass serialises every comment as one row (login, typename, createdAt, + # association, base64(body)). Fields are joined with the ASCII Unit Separator + # (U+001F), NOT a tab: tab is IFS-whitespace, so `read` would collapse a run of + # tabs and DROP an empty middle field (an empty createdAt — the exact fail-closed + # case — would shift the next field into its slot). The body is base64-encoded so + # it can never contain the separator or a newline. + local rows + rows=$(jq -r ' + if type == "array" then + .[] + | [ (.author.login // ""), (.author.__typename // ""), (.createdAt // ""), (.authorAssociation // ""), ((.body // "") | @base64) ] + | join("\u001f") + else empty end + ' <<<"$comments_json" 2>/dev/null) || return 1 + [[ -z "$rows" ]] && return 1 + + local latest="" saw_unparseable=0 + local login typename created assoc body_b64 body up + while IFS=$'\x1f' read -r login typename created assoc body_b64; do + [[ -z "$login$typename$created$assoc$body_b64" ]] && continue + [[ "$typename" != "User" ]] && continue + # Only a repo maintainer may authorize resolution; a missing/unknown association + # fails closed (CWE-863). + case "$assoc" in OWNER|MEMBER|COLLABORATOR) ;; *) continue ;; esac + body=$(base64 --decode <<<"$body_b64" 2>/dev/null || printf '') + # Marker-less is the discriminator: an agent-authored comment (carrying one of + # our markers) is ours, never a maintainer disposition — even from the same login. + review_thread_is_agent_authored "$body" && continue + up="${body^^}" + # A negated phrase ("this is not a false positive") is not an affirmative + # disposition — checked FIRST so it falls through (fail closed). + [[ "$up" =~ $_ACV_NOCHANGE_NEGATION_RE_UPPER ]] && continue + [[ "$up" =~ $_ACV_NOCHANGE_RE_UPPER ]] || continue + # A question or an expression of doubt/disagreement is not an affirmative verdict + # ("Could this be a false positive?", "I disagree with calling this a false + # positive") -> fail closed. + [[ "$up" == *'?'* ]] && continue + [[ "$up" =~ (DISAGREE|DOUBT|UNCLEAR|NOT[[:space:]]SURE|UNSURE|NOT[[:space:]]CONVINCED) ]] && continue + # A disposition with no parseable timestamp cannot be ordered against a newer + # REQUIRED disposition or bot finding -> fail closed. + if [[ -z "$created" ]] || ! _acv_is_iso8601 "$created"; then + saw_unparseable=1 + continue + fi + if [[ -z "$latest" || "$created" > "$latest" ]]; then + latest="$created" + fi + done <<<"$rows" + if [[ "$saw_unparseable" -eq 1 ]]; then echo "unparseable" return 2 fi + if [[ -n "$latest" ]]; then + echo "$latest" + return 0 + fi echo "" return 1 } diff --git a/tests/dev-lead/unit/test_addressed_claim_verify.bats b/tests/dev-lead/unit/test_addressed_claim_verify.bats index 3a6c7a886..43cea6eba 100644 --- a/tests/dev-lead/unit/test_addressed_claim_verify.bats +++ b/tests/dev-lead/unit/test_addressed_claim_verify.bats @@ -183,6 +183,242 @@ setup() { [[ "$output" == "2026-09-05T00:00:00Z" ]] } +@test "acv_latest_maintainer_disposition: a valid date alongside an unparseable one -> rc2 (fail closed, #2079 AC3)" { + # The unparseable disposition's true order against the valid one is unknown, so it + # must win — returning the valid date would let a fix that predates it resolve. + local comments='[ + {"author":{"login":"m1","__typename":"User"},"body":"REQUIRED","createdAt":"2026-09-01T00:00:00Z"}, + {"author":{"login":"m2","__typename":"User"},"body":"blocking issue here","createdAt":""} + ]' + run acv_latest_maintainer_disposition "$comments" "donpetry-bot" + [[ "$status" -eq 2 ]] + [[ "$output" == "unparseable" ]] +} + +# --------------------------------------------------------------------------- +# acv_latest_nochange_disposition — the "no change needed" counterpart (#1743, #2079) +# The mirror of acv_latest_maintainer_disposition: a marker-less human maintainer +# who asserts a false positive / no-change disposition PERMITS resolution of a +# false-positive bot thread. Authorship is decided by MARKER + authorAssociation, +# never by login: dev-lead posts as the same account the maintainer uses +# (don-petry), so a login skip would discard the maintainer's own verdict (#2079). +# --------------------------------------------------------------------------- + +@test "acv_latest_nochange_disposition: no no-change disposition -> rc1, empty" { + local comments='[{"author":{"login":"donpetry-bot","__typename":"User"},"authorAssociation":"MEMBER","body":"Refuted. ","createdAt":"2026-09-01T10:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + [[ -z "$output" ]] +} + +@test "acv_latest_nochange_disposition: marker-less human 'no change needed' -> rc0 with its date" { + local comments='[ + {"author":{"login":"gemini-code-assist[bot]","__typename":"Bot"},"body":"Missing closing backtick.","createdAt":"2026-09-01T09:00:00Z"}, + {"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"This is a false positive — no change needed.","createdAt":"2026-09-02T12:00:00Z"} + ]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-09-02T12:00:00Z" ]] +} + +@test "acv_latest_nochange_disposition: rejected/questioned phrase #0 is not a disposition -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"I do not think this is a false positive.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: rejected/questioned phrase #1 is not a disposition -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"I do not believe this is a false positive.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: rejected/questioned phrase #2 is not a disposition -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"This is not, in my judgement, a false positive.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: rejected/questioned phrase #3 is not a disposition -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"It'\''s not at all clear this is a false positive.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: rejected/questioned phrase #4 is not a disposition -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"Could this be a false positive?","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: rejected/questioned phrase #5 is not a disposition -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"I disagree with calling this a false positive.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: 'working as intended' is a no-change disposition -> rc0" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"Working as intended.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-09-02T12:00:00Z" ]] +} + +@test "acv_latest_nochange_disposition: 'no change required' (collides with REQUIRED) -> classified as no-change rc0" { + # The blocking regex matches the substring REQUIRED; the no-change intent must win + # so a maintainer waving off a finding is not misread as a blocking demand. + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"No change required here.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-09-02T12:00:00Z" ]] +} + +@test "acv_latest_nochange_disposition: an agent-marker comment is never a no-change disposition" { + # Body says 'no change needed' but carries our marker -> agent-authored -> ignored, + # so the agent cannot manufacture its own resolution authorization (#1743 AC4). + local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"MEMBER","body":"No change needed. ","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + [[ -z "$output" ]] +} + +@test "acv_latest_nochange_disposition: a bot comment is never a no-change disposition" { + local comments='[{"author":{"login":"gemini-code-assist[bot]","__typename":"Bot"},"body":"No change needed on our end.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + [[ -z "$output" ]] +} + +@test "acv_latest_nochange_disposition: disposition with unparseable date -> rc2 (fail closed)" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"false positive","createdAt":""}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 2 ]] + [[ "$output" == "unparseable" ]] +} + +@test "acv_latest_nochange_disposition: multiple dispositions -> latest date wins" { + local comments='[ + {"author":{"login":"m1","__typename":"User"},"authorAssociation":"MEMBER","body":"false positive","createdAt":"2026-09-01T00:00:00Z"}, + {"author":{"login":"m2","__typename":"User"},"authorAssociation":"MEMBER","body":"no change needed","createdAt":"2026-09-05T00:00:00Z"} + ]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-09-05T00:00:00Z" ]] +} + +@test "acv_latest_nochange_disposition: neutral chatter is not a no-change disposition -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"Thanks for looking into this.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + [[ -z "$output" ]] +} + +@test "acv_latest_nochange_disposition: negated 'this is not a false positive' does NOT authorize -> rc1" { + # The broad phrase match would see FALSE POSITIVE; the negation guard must win so a + # maintainer explicitly rejecting the false-positive verdict never clears the thread (#1799). + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"This is not a false positive, please fix it.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + [[ -z "$output" ]] +} + +@test "acv_latest_nochange_disposition: negated 'isn't working as intended' does NOT authorize -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"This isn'"'"'t working as intended.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + [[ -z "$output" ]] +} + +@test "acv_latest_nochange_disposition: 'won'\''t fix' (affirmative) is still a no-change disposition -> rc0" { + # The negator-lookalike WON'T is part of the affirmative phrase itself; it must not be + # clobbered by the negation guard, which only fires when a negator precedes a phrase. + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"Won'"'"'t fix — intentional.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-09-02T12:00:00Z" ]] +} + +@test "acv_latest_nochange_disposition: 'not a bug' (affirmative) is still a no-change disposition -> rc0" { + # NOT A BUG begins with a negator but is an affirmative no-change phrase; it is excluded + # from the negation guard's target set so it is never misread as a negated phrase. + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"Not a bug.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-09-02T12:00:00Z" ]] +} + +@test "acv_latest_nochange_disposition: maintainer posting as the dev-lead account (same login) is accepted -> rc0 (#2079)" { + # dev-lead's BOT_USER is don-petry — the maintainer's own account. A marker-less + # reply from that login is the maintainer speaking and must count (the #1799 bug). + local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"OWNER","body":"False positive — no change needed.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-09-02T12:00:00Z" ]] +} + +@test "acv_latest_nochange_disposition: dev-lead writing 'no change needed' can never dismiss a finding -> rc1 (#2079 AC4)" { + # Same account, same association, same phrase as the accepted case above — only the + # dev-lead marker differs. The agent's own statement must never authorize resolution. + local comments='[ + {"author":{"login":"gemini-code-assist[bot]","__typename":"Bot"},"body":"Possible injection here.","createdAt":"2026-09-01T09:00:00Z"}, + {"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"OWNER","body":"False positive — no change needed.\n","createdAt":"2026-09-02T12:00:00Z"} + ]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + [[ -z "$output" ]] +} + +@test "acv_latest_nochange_disposition: pr-review-agent marker comment is never a no-change disposition -> rc1" { + local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"OWNER","body":"\nNo change needed.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + [[ -z "$output" ]] +} + +@test "acv_latest_nochange_disposition: COLLABORATOR is a maintainer -> rc0" { + local comments='[{"author":{"login":"a-collab","__typename":"User"},"authorAssociation":"COLLABORATOR","body":"Not applicable here.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-09-02T12:00:00Z" ]] +} + +@test "acv_latest_nochange_disposition: non-maintainer association (CONTRIBUTOR/NONE/missing) -> rc1 (fail closed)" { + local assoc + for assoc in '"authorAssociation":"CONTRIBUTOR",' '"authorAssociation":"NONE",' ''; do + local comments='[{"author":{"login":"drive-by","__typename":"User"},'"$assoc"'"body":"False positive, no change needed.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + [[ -z "$output" ]] + done +} + +@test "acv_latest_nochange_disposition: negated 'not applicable' / 'wouldn't call it a false positive' -> rc1" { + local body + for body in "This is not not applicable, please fix." "I wouldn't call it a false positive."; do + local comments + comments=$(jq -cn --arg b "$body" '[{"author":{"login":"m","__typename":"User"},"authorAssociation":"MEMBER","body":$b,"createdAt":"2026-09-02T12:00:00Z"}]') + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + done +} + +@test "acv_latest_nochange_disposition: a valid date alongside an unparseable one -> rc2 (fail closed)" { + local comments='[ + {"author":{"login":"m1","__typename":"User"},"authorAssociation":"MEMBER","body":"false positive","createdAt":"2026-09-01T00:00:00Z"}, + {"author":{"login":"m2","__typename":"User"},"authorAssociation":"MEMBER","body":"no change needed","createdAt":"not-a-date"} + ]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 2 ]] + [[ "$output" == "unparseable" ]] +} + +@test "acv_latest_nochange_disposition: empty / invalid input -> rc1" { + run acv_latest_nochange_disposition '[]' + [[ "$status" -eq 1 ]] + run acv_latest_nochange_disposition 'not json' + [[ "$status" -eq 1 ]] +} + # --------------------------------------------------------------------------- # acv_latest_marker_index — find our addressed-marker reply anywhere in the thread (#1735 AC1) # --------------------------------------------------------------------------- From 9742f93221888ebe6b5c2739f3d6dff4676aad91 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 02:35:18 +0000 Subject: [PATCH 02/14] fix(reviews): harden acv_latest_nochange_disposition (hedges, em-dash negation, base64 fallback, in-process timestamp check) Co-Authored-By: Claude Sonnet 5.5 --- scripts/lib/addressed-claim-verify.sh | 8 ++++---- .../unit/test_addressed_claim_verify.bats | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/scripts/lib/addressed-claim-verify.sh b/scripts/lib/addressed-claim-verify.sh index 975151651..0447098ef 100644 --- a/scripts/lib/addressed-claim-verify.sh +++ b/scripts/lib/addressed-claim-verify.sh @@ -84,7 +84,7 @@ readonly _ACV_NOCHANGE_RE_UPPER='(NO (CODE )?CHANGES? (NEEDED|REQUIRED|NECESSARY # phrase that itself begins with a negator ("NOT A BUG", "NO CHANGE NEEDED") only # trips the guard when ANOTHER negator precedes it, so the bare affirmative forms are # never clobbered. -readonly _ACV_NOCHANGE_NEGATION_RE_UPPER='(CANNOT|CAN.?T|COULD ?NOT|COULDN.?T|WILL NOT|WON.?T|WOULD ?NOT|WOULDN.?T|SHOULD ?NOT|SHOULDN.?T|IS ?NOT|ISN.?T|ARE ?NOT|AREN.?T|WAS ?NOT|WASN.?T|WERE ?NOT|WEREN.?T|DOES ?NOT|DOESN.?T|DO ?NOT|DON.?T|DID ?NOT|DIDN.?T|NEVER|NOT)[,[:space:]]+([A-Z,'"'"']+[[:space:]]+){0,6}(FALSE[ -]?POSITIVE|WON.?T ?FIX|WONTFIX|WORKING AS INTENDED|BY DESIGN|INTENDED BEHAVIOU?R|NO (CODE )?CHANGES? (NEEDED|REQUIRED|NECESSARY|WARRANTED|IS NEEDED|ARE NEEDED)|NOT A (REAL )?(BUG|ISSUE|PROBLEM|CONCERN|DEFECT)|NOT APPLICABLE)' +readonly _ACV_NOCHANGE_NEGATION_RE_UPPER='(CANNOT|CAN.?T|COULD ?NOT|COULDN.?T|WILL NOT|WON.?T|WOULD ?NOT|WOULDN.?T|SHOULD ?NOT|SHOULDN.?T|IS ?NOT|ISN.?T|ARE ?NOT|AREN.?T|WAS ?NOT|WASN.?T|WERE ?NOT|WEREN.?T|DOES ?NOT|DOESN.?T|DO ?NOT|DON.?T|DID ?NOT|DIDN.?T|NEVER|NOT)[,[:space:]]+([^[:space:]]+[[:space:]]+){0,6}(FALSE[ -]?POSITIVE|WON.?T ?FIX|WONTFIX|WORKING AS INTENDED|BY DESIGN|INTENDED BEHAVIOU?R|NO (CODE )?CHANGES? (NEEDED|REQUIRED|NECESSARY|WARRANTED|IS NEEDED|ARE NEEDED)|NOT A (REAL )?(BUG|ISSUE|PROBLEM|CONCERN|DEFECT)|NOT APPLICABLE)' # Post-marker BOT-comment classification (#1735 AC2/AC4). A review bot that replies # AFTER our addressed-marker either ACKNOWLEDGES (accepts our refutation / records a @@ -460,7 +460,7 @@ acv_latest_nochange_disposition() { # Only a repo maintainer may authorize resolution; a missing/unknown association # fails closed (CWE-863). case "$assoc" in OWNER|MEMBER|COLLABORATOR) ;; *) continue ;; esac - 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 '') # Marker-less is the discriminator: an agent-authored comment (carrying one of # our markers) is ours, never a maintainer disposition — even from the same login. review_thread_is_agent_authored "$body" && continue @@ -473,10 +473,10 @@ acv_latest_nochange_disposition() { # ("Could this be a false positive?", "I disagree with calling this a false # positive") -> fail closed. [[ "$up" == *'?'* ]] && continue - [[ "$up" =~ (DISAGREE|DOUBT|UNCLEAR|NOT[[:space:]]SURE|UNSURE|NOT[[:space:]]CONVINCED) ]] && continue + [[ "$up" =~ (MAY|MIGHT|COULD[[:space:]]BE|DISAGREE|DECLINE|REFUSE|DOUBT|UNCLEAR|NOT[[:space:]]SURE|UNSURE|NOT[[:space:]]CONVINCED) ]] && continue # A disposition with no parseable timestamp cannot be ordered against a newer # REQUIRED disposition or bot finding -> fail closed. - if [[ -z "$created" ]] || ! _acv_is_iso8601 "$created"; then + if ! [[ "$created" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?Z$ ]]; then saw_unparseable=1 continue fi diff --git a/tests/dev-lead/unit/test_addressed_claim_verify.bats b/tests/dev-lead/unit/test_addressed_claim_verify.bats index 43cea6eba..398811897 100644 --- a/tests/dev-lead/unit/test_addressed_claim_verify.bats +++ b/tests/dev-lead/unit/test_addressed_claim_verify.bats @@ -759,3 +759,21 @@ setup() { [[ "$(echo "$output" | jq -r .on_head)" == "false" ]] [[ "$(echo "$output" | jq -r .in_base)" == "false" ]] } + +@test "acv_latest_nochange_disposition: em-dash negation 'NOT — in my view — a false positive' -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"This is NOT — in my view — a false positive.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: hedged 'may be a false positive' -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"This may be a false positive.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: 'I decline to call this a false positive' -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"I decline to call this a false positive.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] +} From 207e12dede1560209d98ddbe00df202a5669c82b Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 04:41:43 +0000 Subject: [PATCH 03/14] fix(bot): address code quality findings - Line 87: Updated _ACV_NOCHANGE_NEGATION_RE_UPPER regex to allow punctuation (em dashes, hyphens) between negation terms and no-change phrases - Line 479: Replaced weak ISO8601 shape-only check with semantic _acv_is_iso8601 validation - Integrated acv_latest_nochange_disposition into resolve_addressed_bot_threads with dedicated no-change resolution path before addressed-marker gate Co-Authored-By: Claude Haiku 4.5 --- scripts/dev-lead-fix-reviews.sh | 58 ++++++++++++++++++++++++++- scripts/lib/addressed-claim-verify.sh | 4 +- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index b8b9c3ef3..877078a6a 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -836,7 +836,7 @@ resolve_addressed_bot_threads() { ... on PullRequestReviewThread { isResolved path - comments(first:100){nodes{author{login __typename} body createdAt}} + comments(first:100){nodes{author{login __typename} authorAssociation body createdAt}} } } }' @@ -865,6 +865,62 @@ resolve_addressed_bot_threads() { # forever (the regression #1691 exposed). A marker from any other account still # does not authorize resolution (acv_latest_marker_index checks the author). comments_json=$(printf '%s' "$node_json" | jq -c '.data.node.comments.nodes // []' 2>/dev/null || echo "[]") + + # ── No-change disposition path (#2079): a maintainer may authorize resolution ── + # without an addressed-marker by asserting "no changes needed" or a false-positive + # disposition. This is checked FIRST, before the addressed-marker path (#1735). + local nochange_disposition nochange_rc + nochange_disposition=$(acv_latest_nochange_disposition "$comments_json") && nochange_rc=0 || nochange_rc=$? + if [ "${nochange_rc:-0}" -eq 0 ] && [ -n "$nochange_disposition" ]; then + # A no-change disposition was found. Re-read the thread immediately before + # resolution to ensure it hasn't changed (no new required disposition or bot finding). + local fresh_json fresh_resolved + fresh_json=$(gh api graphql -f query="$node_query" -f id="$id" 2>/dev/null || echo "{}") + fresh_resolved=$(printf '%s' "$fresh_json" | jq -r \ + 'if .data.node.isResolved == null then "unknown" + elif .data.node.isResolved then "true" else "false" end' 2>/dev/null || echo "unknown") + if [ "$fresh_resolved" != "false" ]; then + echo "::notice::skipping thread ${id} — already resolved during no-change check" + continue + fi + local fresh_comments + fresh_comments=$(printf '%s' "$fresh_json" | jq -c '.data.node.comments.nodes // []' 2>/dev/null || echo "[]") + if [ "$fresh_comments" != "$comments_json" ]; then + echo "::notice::skipping thread ${id} — thread changed during no-change disposition check; leaving unresolved" + continue + fi + # Reject if a newer required disposition exists (a REQUIRED overrides NO-CHANGE). + local req_disposition req_rc + req_disposition=$(acv_latest_maintainer_disposition "$fresh_comments" "__no-such-account__") && req_rc=0 || req_rc=$? + if [ "${req_rc:-0}" -ne 1 ]; then + echo "::notice::skipping thread ${id} — a required maintainer disposition blocks the no-change verdict; leaving unresolved (#2079)" + continue + fi + # Reject if a bot finding exists (the no-change verdict doesn't override open bot findings). + # Check the first comment (the originating bot finding) to ensure it exists and is from a bot. + local origin_is_bot + origin_is_bot=$(printf '%s' "$fresh_comments" | jq -r \ + '.[0].author as $a | if (($a.__typename // "") == "Bot") or (($a.login // "") | endswith("[bot]")) + then "yes" else "no" end' 2>/dev/null || echo "no") + # If the origin is still a bot finding, we cannot resolve on a no-change disposition alone. + # The maintainer said "no changes needed" but the bot is still reporting a finding. + if [ "$origin_is_bot" = "yes" ]; then + echo "::notice::skipping thread ${id} — the originating bot finding is still present; a no-change disposition alone cannot resolve; leaving unresolved" + continue + fi + if gh api graphql -f query='mutation($id: ID!) { resolveReviewThread(input: {threadId: $id}) { thread { isResolved } } }' \ + -f id="$id" >/dev/null 2>&1; then + resolved_count=$((resolved_count + 1)) + echo "::notice::resolved bot thread ${id} due to no-change disposition (${nochange_disposition})" + else + echo "::warning::failed to resolve bot thread ${id} despite no-change disposition" + fi + continue + elif [ "${nochange_rc:-0}" -eq 2 ]; then + echo "::notice::skipping thread ${id} — a no-change disposition could not be parsed; leaving unresolved (fail closed) (#2079)" + continue + fi + if ! marker_idx=$(acv_latest_marker_index "$comments_json" "$bot_user"); then echo "::notice::skipping thread ${id} — no addressed-marker reply from our account in the thread; leaving unresolved (#1735)" continue diff --git a/scripts/lib/addressed-claim-verify.sh b/scripts/lib/addressed-claim-verify.sh index 0447098ef..f589acafd 100644 --- a/scripts/lib/addressed-claim-verify.sh +++ b/scripts/lib/addressed-claim-verify.sh @@ -84,7 +84,7 @@ readonly _ACV_NOCHANGE_RE_UPPER='(NO (CODE )?CHANGES? (NEEDED|REQUIRED|NECESSARY # phrase that itself begins with a negator ("NOT A BUG", "NO CHANGE NEEDED") only # trips the guard when ANOTHER negator precedes it, so the bare affirmative forms are # never clobbered. -readonly _ACV_NOCHANGE_NEGATION_RE_UPPER='(CANNOT|CAN.?T|COULD ?NOT|COULDN.?T|WILL NOT|WON.?T|WOULD ?NOT|WOULDN.?T|SHOULD ?NOT|SHOULDN.?T|IS ?NOT|ISN.?T|ARE ?NOT|AREN.?T|WAS ?NOT|WASN.?T|WERE ?NOT|WEREN.?T|DOES ?NOT|DOESN.?T|DO ?NOT|DON.?T|DID ?NOT|DIDN.?T|NEVER|NOT)[,[:space:]]+([^[:space:]]+[[:space:]]+){0,6}(FALSE[ -]?POSITIVE|WON.?T ?FIX|WONTFIX|WORKING AS INTENDED|BY DESIGN|INTENDED BEHAVIOU?R|NO (CODE )?CHANGES? (NEEDED|REQUIRED|NECESSARY|WARRANTED|IS NEEDED|ARE NEEDED)|NOT A (REAL )?(BUG|ISSUE|PROBLEM|CONCERN|DEFECT)|NOT APPLICABLE)' +readonly _ACV_NOCHANGE_NEGATION_RE_UPPER='(CANNOT|CAN.?T|COULD ?NOT|COULDN.?T|WILL NOT|WON.?T|WOULD ?NOT|WOULDN.?T|SHOULD ?NOT|SHOULDN.?T|IS ?NOT|ISN.?T|ARE ?NOT|AREN.?T|WAS ?NOT|WASN.?T|WERE ?NOT|WEREN.?T|DOES ?NOT|DOESN.?T|DO ?NOT|DON.?T|DID ?NOT|DIDN.?T|NEVER|NOT)[,[:space:]—-]+([^[:space:]]+[[:space:]]+){0,6}(FALSE[ -]?POSITIVE|WON.?T ?FIX|WONTFIX|WORKING AS INTENDED|BY DESIGN|INTENDED BEHAVIOU?R|NO (CODE )?CHANGES? (NEEDED|REQUIRED|NECESSARY|WARRANTED|IS NEEDED|ARE NEEDED)|NOT A (REAL )?(BUG|ISSUE|PROBLEM|CONCERN|DEFECT)|NOT APPLICABLE)' # Post-marker BOT-comment classification (#1735 AC2/AC4). A review bot that replies # AFTER our addressed-marker either ACKNOWLEDGES (accepts our refutation / records a @@ -476,7 +476,7 @@ acv_latest_nochange_disposition() { [[ "$up" =~ (MAY|MIGHT|COULD[[:space:]]BE|DISAGREE|DECLINE|REFUSE|DOUBT|UNCLEAR|NOT[[:space:]]SURE|UNSURE|NOT[[:space:]]CONVINCED) ]] && continue # A disposition with no parseable timestamp cannot be ordered against a newer # REQUIRED disposition or bot finding -> fail closed. - if ! [[ "$created" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(\.[0-9]+)?Z$ ]]; then + if ! _acv_is_iso8601 "$created"; then saw_unparseable=1 continue fi From 79712347af81b80c6cf6b3c3763aab18b0f69580 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 07:31:39 +0000 Subject: [PATCH 04/14] fix(shellcheck): remove unused variable req_disposition Fixed SC2034 warning by not capturing unused output from acv_latest_maintainer_disposition. Only the return code is needed. Co-Authored-By: Claude Haiku 4.5 --- scripts/dev-lead-fix-reviews.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 877078a6a..ead71487e 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -890,8 +890,8 @@ resolve_addressed_bot_threads() { continue fi # Reject if a newer required disposition exists (a REQUIRED overrides NO-CHANGE). - local req_disposition req_rc - req_disposition=$(acv_latest_maintainer_disposition "$fresh_comments" "__no-such-account__") && req_rc=0 || req_rc=$? + local req_rc + acv_latest_maintainer_disposition "$fresh_comments" "__no-such-account__" >/dev/null && req_rc=0 || req_rc=$? if [ "${req_rc:-0}" -ne 1 ]; then echo "::notice::skipping thread ${id} — a required maintainer disposition blocks the no-change verdict; leaving unresolved (#2079)" continue From 36aa3db7f0d8942a26d684ced408c7624ae287d2 Mon Sep 17 00:00:00 2001 From: don-petry <{}+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 07:58:04 +0000 Subject: [PATCH 05/14] fix(reviews): no-change path blocks only newer REQUIRED/bot comments Co-Authored-By: Claude Sonnet 5.5 --- scripts/dev-lead-fix-reviews.sh | 26 +++++++++---------- scripts/lib/addressed-claim-verify.sh | 10 ++++++- .../unit/test_addressed_claim_verify.bats | 15 +++++++++++ 3 files changed, 37 insertions(+), 14 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index ead71487e..19d3e0c52 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -890,22 +890,22 @@ resolve_addressed_bot_threads() { continue fi # Reject if a newer required disposition exists (a REQUIRED overrides NO-CHANGE). - local req_rc - acv_latest_maintainer_disposition "$fresh_comments" "__no-such-account__" >/dev/null && req_rc=0 || req_rc=$? - if [ "${req_rc:-0}" -ne 1 ]; then + # Only a REQUIRED disposition at/after the no-change verdict blocks; affirmative + # no-change comments ("no change required") are excluded from the scan. + local req_rc req_ts + req_ts=$(acv_latest_maintainer_disposition "$fresh_comments" "__no-such-account__" "skip-nochange") && req_rc=0 || req_rc=$? + if [ "${req_rc:-0}" -eq 2 ] || { [ "${req_rc:-0}" -eq 0 ] && [[ ! "$req_ts" < "$nochange_disposition" ]]; }; then echo "::notice::skipping thread ${id} — a required maintainer disposition blocks the no-change verdict; leaving unresolved (#2079)" continue fi - # Reject if a bot finding exists (the no-change verdict doesn't override open bot findings). - # Check the first comment (the originating bot finding) to ensure it exists and is from a bot. - local origin_is_bot - origin_is_bot=$(printf '%s' "$fresh_comments" | jq -r \ - '.[0].author as $a | if (($a.__typename // "") == "Bot") or (($a.login // "") | endswith("[bot]")) - then "yes" else "no" end' 2>/dev/null || echo "no") - # If the origin is still a bot finding, we cannot resolve on a no-change disposition alone. - # The maintainer said "no changes needed" but the bot is still reporting a finding. - if [ "$origin_is_bot" = "yes" ]; then - echo "::notice::skipping thread ${id} — the originating bot finding is still present; a no-change disposition alone cannot resolve; leaving unresolved" + # Reject if any bot comment postdates the no-change verdict (a newer bot finding + # overrides it). A bot comment with no createdAt cannot be ordered -> fail closed. + local newer_bot + newer_bot=$(printf '%s' "$fresh_comments" | jq -r --arg t "$nochange_disposition" \ + 'map(select(((.author.__typename // "") == "Bot" or ((.author.login // "") | endswith("[bot]"))) + and (((.createdAt // "") == "") or (.createdAt > $t)))) | length' 2>/dev/null || echo "1") + if [ "$newer_bot" != "0" ]; then + echo "::notice::skipping thread ${id} — a bot comment postdates the no-change disposition; leaving unresolved (#2079)" continue fi if gh api graphql -f query='mutation($id: ID!) { resolveReviewThread(input: {threadId: $id}) { thread { isResolved } } }' \ diff --git a/scripts/lib/addressed-claim-verify.sh b/scripts/lib/addressed-claim-verify.sh index f589acafd..b795a05ba 100644 --- a/scripts/lib/addressed-claim-verify.sh +++ b/scripts/lib/addressed-claim-verify.sh @@ -356,7 +356,7 @@ _acv_intersects() { # fail closed, leave open) # Pure — no gh/git/network. acv_latest_maintainer_disposition() { - local comments_json="${1:-}" bot_user="${2:-}" + local comments_json="${1:-}" bot_user="${2:-}" skip_nochange="${3:-}" local bot_user_stripped="${bot_user%\[bot\]}" # Nothing to scan -> no disposition. An empty/invalid array is treated as "none" @@ -386,6 +386,14 @@ acv_latest_maintainer_disposition() { review_thread_is_agent_authored "$body" && continue # Does the marker-less human comment assert a required disposition? [[ "${body^^}" =~ $_ACV_DISPOSITION_RE_UPPER ]] || continue + # Optional (#2079): "no change required" contains REQUIRED; when the caller is + # evaluating a no-change verdict, an affirmative no-change comment is not itself a + # required disposition. + if [[ "$skip_nochange" == "skip-nochange" ]] \ + && [[ "${body^^}" =~ $_ACV_NOCHANGE_RE_UPPER ]] \ + && ! [[ "${body^^}" =~ $_ACV_NOCHANGE_NEGATION_RE_UPPER ]]; then + continue + fi # A disposition with no parseable timestamp cannot be ordered against the fix -> # fail closed. if [[ -z "$created" ]] || ! _acv_is_iso8601 "$created"; then diff --git a/tests/dev-lead/unit/test_addressed_claim_verify.bats b/tests/dev-lead/unit/test_addressed_claim_verify.bats index 398811897..bd15bc0bb 100644 --- a/tests/dev-lead/unit/test_addressed_claim_verify.bats +++ b/tests/dev-lead/unit/test_addressed_claim_verify.bats @@ -195,6 +195,21 @@ setup() { [[ "$output" == "unparseable" ]] } +@test "acv_latest_maintainer_disposition: skip-nochange ignores an affirmative 'no change required' (#2079)" { + local comments='[{"author":{"login":"m","__typename":"User"},"body":"This is a false positive, no change required.","createdAt":"2026-09-01T00:00:00Z"}]' + run acv_latest_maintainer_disposition "$comments" "donpetry-bot" + [[ "$status" -eq 0 ]] + run acv_latest_maintainer_disposition "$comments" "donpetry-bot" "skip-nochange" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_maintainer_disposition: skip-nochange still counts a real REQUIRED comment" { + local comments='[{"author":{"login":"m","__typename":"User"},"body":"Required before merge.","createdAt":"2026-09-03T00:00:00Z"}]' + run acv_latest_maintainer_disposition "$comments" "donpetry-bot" "skip-nochange" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-09-03T00:00:00Z" ]] +} + # --------------------------------------------------------------------------- # acv_latest_nochange_disposition — the "no change needed" counterpart (#1743, #2079) # The mirror of acv_latest_maintainer_disposition: a marker-less human maintainer From 567df3a89f0ce406a30b12ec39bcab3f14cc7622 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 08:16:42 +0000 Subject: [PATCH 06/14] fix(reviews): skip-nochange neutralises only the no-change phrase's REQUIRED Co-Authored-By: Claude Sonnet 5.5 --- scripts/lib/addressed-claim-verify.sh | 19 +++++++++++++------ .../unit/test_addressed_claim_verify.bats | 7 +++++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/scripts/lib/addressed-claim-verify.sh b/scripts/lib/addressed-claim-verify.sh index b795a05ba..9323eba1b 100644 --- a/scripts/lib/addressed-claim-verify.sh +++ b/scripts/lib/addressed-claim-verify.sh @@ -385,14 +385,21 @@ acv_latest_maintainer_disposition() { # our markers) is ours, never a maintainer finding. review_thread_is_agent_authored "$body" && continue # Does the marker-less human comment assert a required disposition? - [[ "${body^^}" =~ $_ACV_DISPOSITION_RE_UPPER ]] || continue + local disp_up="${body^^}" + [[ "$disp_up" =~ $_ACV_DISPOSITION_RE_UPPER ]] || continue # Optional (#2079): "no change required" contains REQUIRED; when the caller is - # evaluating a no-change verdict, an affirmative no-change comment is not itself a - # required disposition. + # evaluating a no-change verdict, neutralise ONLY the REQUIRED contributed by an + # affirmative no-change phrase. A separate blocking assertion in the same comment + # ("no change required, but this MUST BE FIXED") must still count. if [[ "$skip_nochange" == "skip-nochange" ]] \ - && [[ "${body^^}" =~ $_ACV_NOCHANGE_RE_UPPER ]] \ - && ! [[ "${body^^}" =~ $_ACV_NOCHANGE_NEGATION_RE_UPPER ]]; then - continue + && [[ "$disp_up" =~ $_ACV_NOCHANGE_RE_UPPER ]] \ + && ! [[ "$disp_up" =~ $_ACV_NOCHANGE_NEGATION_RE_UPPER ]]; then + local rest="$disp_up" matched + while [[ "$rest" =~ $_ACV_NOCHANGE_RE_UPPER ]]; do + matched="${BASH_REMATCH[0]}" + rest="${rest//"$matched"/ }" + done + [[ "$rest" =~ $_ACV_DISPOSITION_RE_UPPER ]] || continue fi # A disposition with no parseable timestamp cannot be ordered against the fix -> # fail closed. diff --git a/tests/dev-lead/unit/test_addressed_claim_verify.bats b/tests/dev-lead/unit/test_addressed_claim_verify.bats index bd15bc0bb..918bd2081 100644 --- a/tests/dev-lead/unit/test_addressed_claim_verify.bats +++ b/tests/dev-lead/unit/test_addressed_claim_verify.bats @@ -203,6 +203,13 @@ setup() { [[ "$status" -eq 1 ]] } +@test "acv_latest_maintainer_disposition: skip-nochange still counts a separate 'must be fixed' in a no-change comment" { + local comments='[{"author":{"login":"m","__typename":"User"},"body":"No change required here, but this must be fixed.","createdAt":"2026-09-04T00:00:00Z"}]' + run acv_latest_maintainer_disposition "$comments" "donpetry-bot" "skip-nochange" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-09-04T00:00:00Z" ]] +} + @test "acv_latest_maintainer_disposition: skip-nochange still counts a real REQUIRED comment" { local comments='[{"author":{"login":"m","__typename":"User"},"body":"Required before merge.","createdAt":"2026-09-03T00:00:00Z"}]' run acv_latest_maintainer_disposition "$comments" "donpetry-bot" "skip-nochange" From 289c94819c5a2af0819dea13a064f3b7c5cb9826 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:32:57 +0000 Subject: [PATCH 07/14] fix(#2079 AC1): integrate no-change disposition into resolve_deferred_bot_threads Adds recognition of maintainer no-change dispositions in the deferred thread resolver. A bot thread can now be resolved based on a maintainer's "no change needed" verdict, with the same checks as the addressed path: rejecting if a newer REQUIRED disposition or bot finding postdates the no-change verdict. Co-Authored-By: Claude Haiku 4.5 --- scripts/dev-lead-fix-reviews.sh | 49 +++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 19d3e0c52..8def1d0be 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -1313,6 +1313,55 @@ resolve_deferred_bot_threads() { continue fi + # Check for a no-change disposition before checking for required disposition or deferral (#2079 AC1). + # A maintainer's "no change needed" verdict can resolve the thread without a deferral. + local nochange_disposition nochange_rc + nochange_disposition=$(acv_latest_nochange_disposition "$comments_json") && nochange_rc=0 || nochange_rc=$? + if [ "${nochange_rc:-0}" -eq 0 ] && [ -n "$nochange_disposition" ]; then + # A no-change disposition was found. Re-read the thread immediately before resolution. + local fresh_json fresh_resolved fresh_comments + fresh_json=$(gh api graphql -f query="$node_query" -f id="$id" 2>/dev/null) || fresh_json="" + fresh_resolved=$(printf '%s' "$fresh_json" | jq -r \ + 'if .data.node.isResolved == null then "unknown" + elif .data.node.isResolved then "true" else "false" end' 2>/dev/null || echo "unknown") + if [ "$fresh_resolved" != "false" ]; then + echo "::notice::skipping thread ${id} — already resolved during no-change check" + continue + fi + fresh_comments=$(printf '%s' "$fresh_json" | jq -c '.data.node.comments.nodes // []' 2>/dev/null || echo "[]") + if [ "$fresh_comments" != "$comments_json" ]; then + echo "::notice::skipping thread ${id} — thread changed during no-change disposition check; leaving unresolved" + continue + fi + # Reject if a newer required disposition exists (a REQUIRED overrides NO-CHANGE). + local req_rc req_ts + req_ts=$(acv_latest_maintainer_disposition "$fresh_comments" "__no-such-account__" "skip-nochange") && req_rc=0 || req_rc=$? + if [ "${req_rc:-0}" -eq 2 ] || { [ "${req_rc:-0}" -eq 0 ] && [[ ! "$req_ts" < "$nochange_disposition" ]]; }; then + echo "::notice::skipping thread ${id} — a required maintainer disposition blocks the no-change verdict; leaving unresolved (#2079)" + continue + fi + # Reject if any bot comment postdates the no-change verdict (a newer bot finding overrides it). + local newer_bot + newer_bot=$(printf '%s' "$fresh_comments" | jq -r --arg t "$nochange_disposition" \ + 'map(select(((.author.__typename // "") == "Bot" or ((.author.login // "") | endswith("[bot]"))) + and (((.createdAt // "") == "") or (.createdAt > $t)))) | length' 2>/dev/null || echo "1") + if [ "$newer_bot" != "0" ]; then + echo "::notice::skipping thread ${id} — a bot comment postdates the no-change disposition; leaving unresolved (#2079)" + continue + fi + if gh api graphql -f query='mutation($id: ID!) { resolveReviewThread(input: {threadId: $id}) { thread { isResolved } } }' \ + -f id="$id" >/dev/null 2>&1; then + resolved_count=$((resolved_count + 1)) + echo "::notice::resolved deferred bot thread ${id} due to no-change disposition (${nochange_disposition})" + else + echo "::warning::failed to resolve deferred bot thread ${id} despite no-change disposition" + fi + continue + elif [ "${nochange_rc:-0}" -eq 2 ]; then + echo "::notice::skipping thread ${id} — a no-change disposition could not be parsed; leaving unresolved (fail closed) (#2079)" + continue + fi + # Pass a sentinel instead of ${bot_user}: in production BOT_USER is the same # account the human maintainer uses, so excluding by login would hide the # maintainer's own "required" disposition. The marker (our deferral reply carries From 1a91bf2ebfb4a9383e6d8970fc17c8f6599fba0f Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 16:51:25 +0000 Subject: [PATCH 08/14] fix(reviews): require latest maintainer comment to affirm no-change; count deferred no-change failures; fetch authorAssociation in deferred resolver Co-Authored-By: Claude Sonnet 5.5 --- scripts/dev-lead-fix-reviews.sh | 8 ++++- scripts/lib/addressed-claim-verify.sh | 33 ++++++++++++------- .../unit/test_addressed_claim_verify.bats | 10 ++++++ 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 8def1d0be..8ba649af9 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -1215,7 +1215,7 @@ resolve_deferred_bot_threads() { node(id:$id){ ... on PullRequestReviewThread { isResolved - comments(first:100){pageInfo{hasNextPage} nodes{author{login __typename} body createdAt lastEditedAt fullDatabaseId}} + comments(first:100){pageInfo{hasNextPage} nodes{author{login __typename} authorAssociation body createdAt lastEditedAt fullDatabaseId}} } } }' @@ -1324,6 +1324,11 @@ resolve_deferred_bot_threads() { fresh_resolved=$(printf '%s' "$fresh_json" | jq -r \ 'if .data.node.isResolved == null then "unknown" elif .data.node.isResolved then "true" else "false" end' 2>/dev/null || echo "unknown") + if [ "$fresh_resolved" = "unknown" ]; then + echo "::warning::could not re-read review thread ${id} during no-change check" + failed_count=$((failed_count + 1)) + continue + fi if [ "$fresh_resolved" != "false" ]; then echo "::notice::skipping thread ${id} — already resolved during no-change check" continue @@ -1355,6 +1360,7 @@ resolve_deferred_bot_threads() { echo "::notice::resolved deferred bot thread ${id} due to no-change disposition (${nochange_disposition})" else echo "::warning::failed to resolve deferred bot thread ${id} despite no-change disposition" + failed_count=$((failed_count + 1)) fi continue elif [ "${nochange_rc:-0}" -eq 2 ]; then diff --git a/scripts/lib/addressed-claim-verify.sh b/scripts/lib/addressed-claim-verify.sh index 9323eba1b..1667618b1 100644 --- a/scripts/lib/addressed-claim-verify.sh +++ b/scripts/lib/addressed-claim-verify.sh @@ -467,7 +467,10 @@ acv_latest_nochange_disposition() { ' <<<"$comments_json" 2>/dev/null) || return 1 [[ -z "$rows" ]] && return 1 - local latest="" saw_unparseable=0 + # latest_maintainer tracks the newest eligible maintainer comment of ANY content: an + # older affirmation is superseded by a later maintainer comment that does not itself + # affirm no-change (e.g. a hedge, a question, a negation, or unrelated follow-up). + local latest="" latest_maintainer="" saw_unparseable=0 affirms local login typename created assoc body_b64 body up while IFS=$'\x1f' read -r login typename created assoc body_b64; do [[ -z "$login$typename$created$assoc$body_b64" ]] && continue @@ -480,21 +483,26 @@ acv_latest_nochange_disposition() { # our markers) is ours, never a maintainer disposition — even from the same login. review_thread_is_agent_authored "$body" && continue up="${body^^}" - # A negated phrase ("this is not a false positive") is not an affirmative - # disposition — checked FIRST so it falls through (fail closed). - [[ "$up" =~ $_ACV_NOCHANGE_NEGATION_RE_UPPER ]] && continue - [[ "$up" =~ $_ACV_NOCHANGE_RE_UPPER ]] || continue - # A question or an expression of doubt/disagreement is not an affirmative verdict - # ("Could this be a false positive?", "I disagree with calling this a false - # positive") -> fail closed. - [[ "$up" == *'?'* ]] && continue - [[ "$up" =~ (MAY|MIGHT|COULD[[:space:]]BE|DISAGREE|DECLINE|REFUSE|DOUBT|UNCLEAR|NOT[[:space:]]SURE|UNSURE|NOT[[:space:]]CONVINCED) ]] && continue # A disposition with no parseable timestamp cannot be ordered against a newer - # REQUIRED disposition or bot finding -> fail closed. + # REQUIRED disposition, bot finding or maintainer comment -> fail closed. if ! _acv_is_iso8601 "$created"; then saw_unparseable=1 continue fi + if [[ -z "$latest_maintainer" || "$created" > "$latest_maintainer" ]]; then + latest_maintainer="$created" + fi + affirms=1 + # A negated phrase ("this is not a false positive") is not an affirmative + # disposition -> fail closed. + [[ "$up" =~ $_ACV_NOCHANGE_NEGATION_RE_UPPER ]] && affirms=0 + [[ "$affirms" -eq 1 && "$up" =~ $_ACV_NOCHANGE_RE_UPPER ]] || affirms=0 + # A question or an expression of doubt/disagreement is not an affirmative verdict + # ("Could this be a false positive?", "I disagree with calling this a false + # positive") -> fail closed. + [[ "$up" == *'?'* ]] && affirms=0 + [[ "$up" =~ (MAY|MIGHT|COULD[[:space:]]BE|DISAGREE|DECLINE|REFUSE|DOUBT|UNCLEAR|NOT[[:space:]]SURE|UNSURE|NOT[[:space:]]CONVINCED) ]] && affirms=0 + [[ "$affirms" -eq 1 ]] || continue if [[ -z "$latest" || "$created" > "$latest" ]]; then latest="$created" fi @@ -504,7 +512,8 @@ acv_latest_nochange_disposition() { echo "unparseable" return 2 fi - if [[ -n "$latest" ]]; then + # The newest maintainer comment must itself be the affirmation. + if [[ -n "$latest" && "$latest" == "$latest_maintainer" ]]; then echo "$latest" return 0 fi diff --git a/tests/dev-lead/unit/test_addressed_claim_verify.bats b/tests/dev-lead/unit/test_addressed_claim_verify.bats index 918bd2081..313a966aa 100644 --- a/tests/dev-lead/unit/test_addressed_claim_verify.bats +++ b/tests/dev-lead/unit/test_addressed_claim_verify.bats @@ -328,6 +328,16 @@ setup() { [[ "$output" == "2026-09-05T00:00:00Z" ]] } +@test "acv_latest_nochange_disposition: a later non-affirming maintainer comment supersedes the affirmation -> rc1" { + local comments='[ + {"author":{"login":"m1","__typename":"User"},"authorAssociation":"MEMBER","body":"false positive","createdAt":"2026-09-01T00:00:00Z"}, + {"author":{"login":"m2","__typename":"User"},"authorAssociation":"OWNER","body":"Please look at the retry path again.","createdAt":"2026-09-05T00:00:00Z"} + ]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] + [[ -z "$output" ]] +} + @test "acv_latest_nochange_disposition: neutral chatter is not a no-change disposition -> rc1" { local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"Thanks for looking into this.","createdAt":"2026-09-02T12:00:00Z"}]' run acv_latest_nochange_disposition "$comments" From b006fa8fc3cd497d0331593092f7a47560b7b0c8 Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 17:12:15 +0000 Subject: [PATCH 09/14] fix(reviews): fail closed on same-timestamp ties and truncated comment pages in no-change disposition Co-Authored-By: Claude Sonnet 5.5 --- scripts/dev-lead-fix-reviews.sh | 10 ++++++++-- scripts/lib/addressed-claim-verify.sh | 19 +++++++++++-------- .../unit/test_addressed_claim_verify.bats | 6 ++++++ 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 8ba649af9..7b971a1c6 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -836,7 +836,7 @@ resolve_addressed_bot_threads() { ... on PullRequestReviewThread { isResolved path - comments(first:100){nodes{author{login __typename} authorAssociation body createdAt}} + comments(first:100){pageInfo{hasNextPage} nodes{author{login __typename} authorAssociation body createdAt}} } } }' @@ -870,7 +870,13 @@ resolve_addressed_bot_threads() { # without an addressed-marker by asserting "no changes needed" or a false-positive # disposition. This is checked FIRST, before the addressed-marker path (#1735). local nochange_disposition nochange_rc - nochange_disposition=$(acv_latest_nochange_disposition "$comments_json") && nochange_rc=0 || nochange_rc=$? + # A truncated comment page may hide a later neutral maintainer comment that + # supersedes the affirmation -> fail closed (skip this path) unless fully read. + if [ "$(printf '%s' "$node_json" | jq -r '.data.node.comments.pageInfo.hasNextPage // true' 2>/dev/null || echo true)" = "false" ]; then + nochange_disposition=$(acv_latest_nochange_disposition "$comments_json") && nochange_rc=0 || nochange_rc=$? + else + nochange_disposition="" nochange_rc=1 + fi if [ "${nochange_rc:-0}" -eq 0 ] && [ -n "$nochange_disposition" ]; then # A no-change disposition was found. Re-read the thread immediately before # resolution to ensure it hasn't changed (no new required disposition or bot finding). diff --git a/scripts/lib/addressed-claim-verify.sh b/scripts/lib/addressed-claim-verify.sh index 1667618b1..5bde70e6c 100644 --- a/scripts/lib/addressed-claim-verify.sh +++ b/scripts/lib/addressed-claim-verify.sh @@ -467,10 +467,10 @@ acv_latest_nochange_disposition() { ' <<<"$comments_json" 2>/dev/null) || return 1 [[ -z "$rows" ]] && return 1 - # latest_maintainer tracks the newest eligible maintainer comment of ANY content: an + # latest_nonaffirm tracks the newest eligible maintainer comment that does not affirm: an # older affirmation is superseded by a later maintainer comment that does not itself # affirm no-change (e.g. a hedge, a question, a negation, or unrelated follow-up). - local latest="" latest_maintainer="" saw_unparseable=0 affirms + local latest="" latest_nonaffirm="" saw_unparseable=0 affirms local login typename created assoc body_b64 body up while IFS=$'\x1f' read -r login typename created assoc body_b64; do [[ -z "$login$typename$created$assoc$body_b64" ]] && continue @@ -489,9 +489,6 @@ acv_latest_nochange_disposition() { saw_unparseable=1 continue fi - if [[ -z "$latest_maintainer" || "$created" > "$latest_maintainer" ]]; then - latest_maintainer="$created" - fi affirms=1 # A negated phrase ("this is not a false positive") is not an affirmative # disposition -> fail closed. @@ -502,7 +499,12 @@ acv_latest_nochange_disposition() { # positive") -> fail closed. [[ "$up" == *'?'* ]] && affirms=0 [[ "$up" =~ (MAY|MIGHT|COULD[[:space:]]BE|DISAGREE|DECLINE|REFUSE|DOUBT|UNCLEAR|NOT[[:space:]]SURE|UNSURE|NOT[[:space:]]CONVINCED) ]] && affirms=0 - [[ "$affirms" -eq 1 ]] || continue + if [[ "$affirms" -ne 1 ]]; then + if [[ -z "$latest_nonaffirm" || "$created" > "$latest_nonaffirm" ]]; then + latest_nonaffirm="$created" + fi + continue + fi if [[ -z "$latest" || "$created" > "$latest" ]]; then latest="$created" fi @@ -512,8 +514,9 @@ acv_latest_nochange_disposition() { echo "unparseable" return 2 fi - # The newest maintainer comment must itself be the affirmation. - if [[ -n "$latest" && "$latest" == "$latest_maintainer" ]]; then + # The newest maintainer comment must itself be the affirmation: strictly newer than + # every non-affirming one, so a same-timestamp tie fails closed. + if [[ -n "$latest" && ( -z "$latest_nonaffirm" || "$latest" > "$latest_nonaffirm" ) ]]; then echo "$latest" return 0 fi diff --git a/tests/dev-lead/unit/test_addressed_claim_verify.bats b/tests/dev-lead/unit/test_addressed_claim_verify.bats index 313a966aa..ef25c96dd 100644 --- a/tests/dev-lead/unit/test_addressed_claim_verify.bats +++ b/tests/dev-lead/unit/test_addressed_claim_verify.bats @@ -809,3 +809,9 @@ setup() { run acv_latest_nochange_disposition "$comments" [[ "$status" -eq 1 ]] } + +@test "acv_latest_nochange_disposition: same-timestamp non-affirming comment fails closed -> rc1" { + local comments='[{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"This is a false positive.","createdAt":"2026-09-02T12:00:00Z"},{"author":{"login":"a-maintainer","__typename":"User"},"authorAssociation":"MEMBER","body":"Actually, please look again.","createdAt":"2026-09-02T12:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" + [[ "$status" -eq 1 ]] +} From 47fac647ad605024b27ca5090e4c8504ecc0bbc2 Mon Sep 17 00:00:00 2001 From: don-petry <{}+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 19:41:31 +0000 Subject: [PATCH 10/14] fix(reviews): stamp pass replies so dev-lead skip notes never read as a maintainer no-change verdict (#2079 AC4) Harness-side stamp_pass_replies appends to every unmarked reply the pass posted, before either resolver runs; replies older than _ACV_REPLY_MARKER_EPOCH can never authorize. Also fixes hasNextPage // true (jq treats false as falsy), which kept the no-change path from ever running. Co-Authored-By: Claude Sonnet 5.5 --- scripts/dev-lead-fix-reviews.sh | 55 ++++++++++- scripts/lib/addressed-claim-verify.sh | 17 +++- scripts/lib/claim-landing.sh | 36 +++++++ .../unit/test_addressed_claim_verify.bats | 34 +++++++ tests/dev-lead/unit/test_claim_landing.bats | 33 +++++++ tests/dev-lead/unit/test_fix_reviews.bats | 97 +++++++++++++++++++ 6 files changed, 267 insertions(+), 5 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 7b971a1c6..b02b86786 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -807,6 +807,13 @@ resolve_addressed_bot_threads() { # so they match both the raw BOT_USER and its stripped form. local bot_user="${BOT_USER:-donpetry-bot}" + # Attribute this pass's own replies to dev-lead BEFORE any thread is evaluated, on + # every pass (even one with no candidates), so an unstamped skip note can never + # become a later pass's "maintainer verdict" (#2079 AC4). On failure the no-change + # path is disabled for this pass. + local stamp_ok=1 + stamp_pass_replies || stamp_ok=0 + # The enumeration pass ONLY collects candidate thread ids (unresolved, # bot-originated). It deliberately does NOT capture the last reply's body or # author: that snapshot goes stale the moment a new reply lands, and trusting it @@ -872,8 +879,8 @@ resolve_addressed_bot_threads() { local nochange_disposition nochange_rc # A truncated comment page may hide a later neutral maintainer comment that # supersedes the affirmation -> fail closed (skip this path) unless fully read. - if [ "$(printf '%s' "$node_json" | jq -r '.data.node.comments.pageInfo.hasNextPage // true' 2>/dev/null || echo true)" = "false" ]; then - nochange_disposition=$(acv_latest_nochange_disposition "$comments_json") && nochange_rc=0 || nochange_rc=$? + if [ "$stamp_ok" -eq 1 ] && [ "$(printf '%s' "$node_json" | jq -r 'if .data.node.comments.pageInfo.hasNextPage == false then "false" else "true" end' 2>/dev/null || echo true)" = "false" ]; then + nochange_disposition=$(acv_latest_nochange_disposition "$comments_json" "$_ACV_REPLY_MARKER_EPOCH") && nochange_rc=0 || nochange_rc=$? else nochange_disposition="" nochange_rc=1 fi @@ -1207,6 +1214,10 @@ resolve_deferred_bot_threads() { fi local bot_user="${BOT_USER:-donpetry-bot}" + # Stamp this pass's own replies before evaluating any thread (#2079 AC4); on failure + # the no-change path is disabled for this pass. + local stamp_ok=1 + stamp_pass_replies || stamp_ok=0 local ids ids=$(list_unresolved_bot_thread_ids) || { echo "::error::resolve_deferred_bot_threads: could not enumerate review threads on PR #${PR_NUMBER}" @@ -1322,7 +1333,11 @@ resolve_deferred_bot_threads() { # Check for a no-change disposition before checking for required disposition or deferral (#2079 AC1). # A maintainer's "no change needed" verdict can resolve the thread without a deferral. local nochange_disposition nochange_rc - nochange_disposition=$(acv_latest_nochange_disposition "$comments_json") && nochange_rc=0 || nochange_rc=$? + if [ "$stamp_ok" -eq 1 ]; then + nochange_disposition=$(acv_latest_nochange_disposition "$comments_json" "$_ACV_REPLY_MARKER_EPOCH") && nochange_rc=0 || nochange_rc=$? + else + nochange_disposition="" nochange_rc=1 + fi if [ "${nochange_rc:-0}" -eq 0 ] && [ -n "$nochange_disposition" ]; then # A no-change disposition was found. Re-read the thread immediately before resolution. local fresh_json fresh_resolved fresh_comments @@ -2960,6 +2975,40 @@ verify_push_landed() { return 0 } +# stamp_pass_replies — make every review-thread reply this pass posted attributable to +# dev-lead (#2079 AC4). The model posts skip notes from its own shell as the SAME +# account the maintainer uses, and only addressed claims are told to carry a marker, so +# a marker-less "No change needed" skip note would otherwise read as the maintainer's +# verdict and let the next pass resolve the bot thread with no human involved. Every +# reply OUR account created since PASS_START_ISO that lacks an agent marker gets +# `` appended. Enforced here in shell, not in the prompt. +# Returns non-zero when the pass boundary is unknown or any listing/PATCH failed; the +# resolvers then skip the no-change path (fail closed). Idempotent: stamped replies +# carry a marker and are never selected again. Replies from before this stamping +# existed are excluded by _ACV_REPLY_MARKER_EPOCH in acv_latest_nochange_disposition. +stamp_pass_replies() { + [ -z "${PR_NUMBER:-}" ] && return 1 + [ -z "${PASS_START_ISO:-}" ] && return 1 + local bot_user="${BOT_USER:-donpetry-bot}" comments rows id body_b64 body failed=0 + if ! comments=$(gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/comments?per_page=100" 2>/dev/null | jq -s 'add // []' 2>/dev/null); then + echo "::warning::stamp_pass_replies: could not list review comments on PR #${PR_NUMBER} — no-change dispositions are disabled this pass (#2079)" + return 1 + fi + rows=$(cl_select_unmarked_replies "$comments" "$bot_user" "$PASS_START_ISO") || { + echo "::warning::stamp_pass_replies: could not parse the review comments on PR #${PR_NUMBER} — no-change dispositions are disabled this pass (#2079)" + return 1 + } + while IFS=$'\t' read -r id body_b64; do + [ -z "$id" ] && continue + body=$(base64 --decode <<<"$body_b64" 2>/dev/null) || { failed=1; continue; } + if ! gh api -X PATCH "repos/${REPO}/pulls/comments/${id}" -f body="$(cl_reply_stamp_body "$body")" >/dev/null 2>&1; then + echo "::warning::stamp_pass_replies: could not stamp reply ${id} on PR #${PR_NUMBER} (#2079)" + failed=1 + fi + done <<< "$rows" + [ "$failed" -eq 0 ] +} + # retract_unlanded_claims — the claim-retraction sweep (#2013). # The model posts its addressed/claim replies from its own shell BEFORE the # harness commits and pushes, so a failed push, a guard abort, an engine failure, diff --git a/scripts/lib/addressed-claim-verify.sh b/scripts/lib/addressed-claim-verify.sh index 5bde70e6c..07aae62d2 100644 --- a/scripts/lib/addressed-claim-verify.sh +++ b/scripts/lib/addressed-claim-verify.sh @@ -72,6 +72,13 @@ readonly _ACV_DISPOSITION_RE_UPPER='(REQUIRED|MUST BE (FIXED|ADDRESSED|RESOLVED| # never clears a thread. Note "no change required" contains the substring REQUIRED # and so also matches _ACV_DISPOSITION_RE_UPPER; the caller must resolve that # collision in favour of the no-change intent. +# Replies posted before the harness began stamping every pass reply with +# `` (#2079 AC4) are marker-less and indistinguishable from a +# maintainer's own comment, so none of them may authorize a no-change resolution. +# Callers pass this as the 2nd arg of acv_latest_nochange_disposition. Bump it to the +# merge instant of the stamping change if that lands later than this value. +readonly _ACV_REPLY_MARKER_EPOCH='2026-10-10T00:00:00Z' + readonly _ACV_NOCHANGE_RE_UPPER='(NO (CODE )?CHANGES? (NEEDED|REQUIRED|NECESSARY|WARRANTED|IS NEEDED|ARE NEEDED)|FALSE[ -]?POSITIVE|WON.?T ?FIX|WONTFIX|WORKING AS INTENDED|NOT A (REAL )?(BUG|ISSUE|PROBLEM|CONCERN|DEFECT)|NOT APPLICABLE|BY DESIGN|INTENDED BEHAVIOU?R)' # A NEGATED (or quoted-then-rejected) no-change phrase is NOT an affirmative no-change @@ -426,7 +433,9 @@ acv_latest_maintainer_disposition() { return 1 } -# acv_latest_nochange_disposition +# acv_latest_nochange_disposition [epoch_iso] +# The optional [epoch_iso] (production callers pass _ACV_REPLY_MARKER_EPOCH) makes any +# comment created before it non-authorizing; omitted, no cutoff applies. # Scan ALL comments in a thread (#1743, #2079) for a standing maintainer "no change # needed" disposition — the mirror of acv_latest_maintainer_disposition. # is a JSON array of @@ -449,7 +458,7 @@ acv_latest_maintainer_disposition() { # fail closed, leave open) # Pure — no gh/git/network. acv_latest_nochange_disposition() { - local comments_json="${1:-}" + local comments_json="${1:-}" epoch="${2:-}" # Single jq pass serialises every comment as one row (login, typename, createdAt, # association, base64(body)). Fields are joined with the ASCII Unit Separator @@ -489,7 +498,11 @@ acv_latest_nochange_disposition() { saw_unparseable=1 continue fi + # A marker-less comment older than the stamping epoch may be one of dev-lead's own + # unstamped skip notes: it can never authorize, and counts as non-affirming so it + # also supersedes any earlier affirmation (fail closed, #2079 AC4). affirms=1 + [[ -n "$epoch" && "$created" < "$epoch" ]] && affirms=0 # A negated phrase ("this is not a false positive") is not an affirmative # disposition -> fail closed. [[ "$up" =~ $_ACV_NOCHANGE_NEGATION_RE_UPPER ]] && affirms=0 diff --git a/scripts/lib/claim-landing.sh b/scripts/lib/claim-landing.sh index 6ec6234ff..38ecd923d 100644 --- a/scripts/lib/claim-landing.sh +++ b/scripts/lib/claim-landing.sh @@ -112,6 +112,42 @@ cl_select_pass_claims() { done <<<"$rows" } +# cl_select_unmarked_replies +# is the REST `pulls/{pr}/comments` array. Emits one +# `\t` row per comment that our account (bot_user, its +# [bot]-suffixed or stripped form) created at or after (the pass start) +# and that carries NO agent marker (the shared _MAINTAINER_REVIEW_GATE_AGENT_MARKERS +# set). dev-lead posts its skip notes from the model's shell as the SAME account the +# maintainer uses, so such a reply is indistinguishable from a maintainer comment +# until the harness stamps it (#2079 AC4). An empty or a non-array +# payload selects nothing (rc 0 / rc 1 on unparseable input). Pure. +cl_select_unmarked_replies() { + local comments_json="${1:-}" bot_user="${2:-}" since="${3:-}" + [[ -z "$since" ]] && return 0 + local bot_stripped="${bot_user%\[bot\]}" + jq -r \ + --arg u "$bot_user" --arg us "$bot_stripped" --arg since "$since" \ + --arg markers "$_MAINTAINER_REVIEW_GATE_AGENT_MARKERS" ' + if type == "array" then + .[] | objects + | (.user.login // "") as $l + | select($l == $u or $l == $us or $l == ($us + "[bot]")) + | select((.created_at // "") >= $since) + | select(((.body // "") | test($markers)) | not) + | [(.id | tostring), ((.body // "") | @base64)] + | @tsv + else empty end + ' <<<"$comments_json" 2>/dev/null +} + +# cl_reply_stamp_body +# The stamped form of an unmarked pass reply: the original text followed by the +# `` marker, which review_thread_is_agent_authored +# recognises, so the reply can never again be read as a maintainer verdict. Pure. +cl_reply_stamp_body() { + printf '%s\n\n\n' "${1:-}" +} + # cl_retract_body # The retracted form of a claim reply. Removes the addressed-marker and the claim # comment (so neither the thread gate nor another bot can treat it as a fix), diff --git a/tests/dev-lead/unit/test_addressed_claim_verify.bats b/tests/dev-lead/unit/test_addressed_claim_verify.bats index ef25c96dd..76a23989c 100644 --- a/tests/dev-lead/unit/test_addressed_claim_verify.bats +++ b/tests/dev-lead/unit/test_addressed_claim_verify.bats @@ -815,3 +815,37 @@ setup() { run acv_latest_nochange_disposition "$comments" [[ "$status" -eq 1 ]] } + +# ── #2079 AC4: dev-lead's own marker-less skip notes must never authorize ──────── + +@test "acv_latest_nochange_disposition: marker-less pass skip note 'No change needed in this pass' before the epoch -> rc1" { + local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"MEMBER","body":"No change needed in this pass: the guard already exists.","createdAt":"2026-10-09T07:58:15Z"}]' + run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: marker-less 'Skipped: this is a false positive' before the epoch -> rc1" { + local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"MEMBER","body":"Skipped: this is a false positive, the helper is already called from the resolver.","createdAt":"2026-10-09T02:35:36Z"}]' + run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: a stamped pass reply () never authorizes -> rc1" { + local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"MEMBER","body":"No change needed in this pass: the guard already exists.\n\n","createdAt":"2026-10-11T07:58:15Z"}]' + run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + [[ "$status" -eq 1 ]] +} + +@test "acv_latest_nochange_disposition: marker-less maintainer comment after the epoch still authorizes -> rc0" { + local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"OWNER","body":"False positive, no change needed.","createdAt":"2026-10-11T08:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-10-11T08:00:00Z" ]] +} + +@test "acv_latest_nochange_disposition: a pre-epoch legacy note does not hide a later real affirmation" { + local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"MEMBER","body":"Skipped: this is a false positive.","createdAt":"2026-10-09T02:35:36Z"},{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"OWNER","body":"Confirmed, false positive.","createdAt":"2026-10-11T08:00:00Z"}]' + run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + [[ "$status" -eq 0 ]] + [[ "$output" == "2026-10-11T08:00:00Z" ]] +} diff --git a/tests/dev-lead/unit/test_claim_landing.bats b/tests/dev-lead/unit/test_claim_landing.bats index c36513fb9..5d04c9b07 100644 --- a/tests/dev-lead/unit/test_claim_landing.bats +++ b/tests/dev-lead/unit/test_claim_landing.bats @@ -287,3 +287,36 @@ _inject_foreign_commit() { [[ "$status" -eq 1 ]] [[ "$output" == "not-on-ref" ]] } + +# ── #2079 AC4: stamping the pass's own marker-less replies ────────────────────── + +@test "cl_select_unmarked_replies: selects our marker-less replies since the pass start" { + local c='[ + {"id":1,"user":{"login":"don-petry"},"created_at":"2026-10-11T01:00:00Z","body":"No change needed in this pass: the guard already exists."}, + {"id":2,"user":{"login":"don-petry"},"created_at":"2026-10-11T01:00:00Z","body":"Fixed. "}, + {"id":3,"user":{"login":"don-petry"},"created_at":"2026-10-10T00:00:00Z","body":"Skipped: this is a false positive."}, + {"id":4,"user":{"login":"someone-else"},"created_at":"2026-10-11T01:00:00Z","body":"No change needed."}, + {"id":5,"user":{"login":"don-petry"},"created_at":"2026-10-11T02:00:00Z","body":"Already stamped.\n\n"} + ]' + run cl_select_unmarked_replies "$c" "don-petry" "2026-10-11T00:00:00Z" + [[ "$status" -eq 0 ]] + [[ "$output" == 1$'\t'* ]] + [[ "$(printf '%s\n' "$output" | wc -l)" -eq 1 ]] +} + +@test "cl_select_unmarked_replies: no pass boundary selects nothing" { + local c='[{"id":1,"user":{"login":"don-petry"},"created_at":"2026-10-11T01:00:00Z","body":"No change needed."}]' + run cl_select_unmarked_replies "$c" "don-petry" "" + [[ "$status" -eq 0 ]] + [[ -z "$output" ]] +} + +@test "cl_reply_stamp_body: stamped reply is agent-authored and no longer a no-change maintainer verdict" { + local stamped + stamped=$(cl_reply_stamp_body "No change needed in this pass: the guard already exists.") + review_thread_is_agent_authored "$stamped" + local comments + comments=$(jq -cn --arg b "$stamped" '[{author:{login:"don-petry",__typename:"User"},authorAssociation:"MEMBER",body:$b,createdAt:"2026-10-11T08:00:00Z"}]') + run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + [[ "$status" -eq 1 ]] +} diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index 9b2da70f9..b2761d3fc 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -6767,3 +6767,100 @@ _fbc_reply() { # $1=disposition-tail $2=createdAt ! grep -q "the harness rejects a \`fixed\` sha from an earlier pass" "$SCRIPT_DIR/prompts/dev-lead/fix-reviews.md" ! grep -q "only verifies a \`fixed\` sha from the pass that cites it" "$SCRIPT_DIR/prompts/dev-lead/fix-bot-comment.md" } + +# ── #2079 AC4: a pass's own marker-less "no change" skip note must never resolve ── +# dev-lead posts skip notes as the SAME account the maintainer uses, with no marker. +# The harness stamps every reply the pass created (stamp_pass_replies) before either +# resolver reads the thread, so such a note stays attributable to dev-lead and the +# bot thread it "dispositioned" stays open. The gh stub models GitHub: the PATCH that +# stamps the reply rewrites the body the next GraphQL thread read returns. + +_nochange_resolver_case() { + # $1 = JSON array of REST pulls/54/comments the pass created (what stamping sees) + local rest_json="$1" + local tmpdir="$BATS_TEST_TMPDIR/workdir" + mkdir -p "$tmpdir" + MUTATIONS_FILE="$BATS_TEST_TMPDIR/mutations" + PATCH_FILE="$BATS_TEST_TMPDIR/patches" + local body_file="$BATS_TEST_TMPDIR/maintainer-body" + : > "$MUTATIONS_FILE" + : > "$PATCH_FILE" + printf 'No change needed in this pass: the guard already exists.' > "$body_file" + printf '%s' "$rest_json" > "$BATS_TEST_TMPDIR/rest.json" + rm -f /tmp/dev-lead-session-output.txt + + git -C "$tmpdir" init -q + echo "initial" > "$tmpdir/file.txt" + git -C "$tmpdir" add . + git -C "$tmpdir" -c user.email="t@test" -c user.name="T" commit -q -m "init" + git -C "$tmpdir" update-ref refs/remotes/origin/main "$(git -C "$tmpdir" rev-parse HEAD)" + NC_BASE_SHA="$(git -C "$tmpdir" rev-parse HEAD)" + + cat > "$STUB_BIN_DIR/gh" << GHEOF +#!/usr/bin/env bash +ARGS="\$*" +case "\$ARGS" in + *"resolveReviewThread"*) + echo "\$*" >> "$MUTATIONS_FILE" + echo '{"data":{"resolveReviewThread":{"thread":{"isResolved":true}}}}' + ;; + *"-X PATCH"*) + for a in "\$@"; do case "\$a" in body=*) printf '%s' "\${a#body=}" > "$body_file"; echo "\$a" >> "$PATCH_FILE" ;; esac; done + echo '{}' + ;; + *"pulls/54/comments"*) cat "$BATS_TEST_TMPDIR/rest.json" ;; + *"fullDatabaseId"*) echo '{"data":{"node":{"isResolved":false,"comments":{"pageInfo":{"hasNextPage":false},"nodes":[]}}}}' ;; + *"PullRequestReviewThread"*) + jq -cn --rawfile b "$body_file" '{data:{node:{isResolved:false,path:"fix.txt",comments:{pageInfo:{hasNextPage:false},nodes:[ + {author:{login:"gemini-code-assist",__typename:"Bot"},authorAssociation:"NONE",body:"Missing a guard.",createdAt:"2098-01-01T00:00:00Z"}, + {author:{login:"don-petry",__typename:"User"},authorAssociation:"MEMBER",body:\$b,createdAt:"2099-01-01T00:00:00Z"}]}}}}' + ;; + *"reviewThreads"*) + echo '{"data":{"repository":{"pullRequest":{"reviewThreads":{"pageInfo":{"hasNextPage":false,"endCursor":""},"nodes":[{"id":"PRRT_nochange_bot","isResolved":false,"isOutdated":false,"origin":{"nodes":[{"author":{"login":"gemini-code-assist[bot]","__typename":"Bot"}}]}}]}}}}}' + ;; + *"check-runs"*) echo '{"check_runs":[]}' ;; + *"statuses"*) echo '[]' ;; + *"pulls/"*"reviews"*) echo '[]' ;; + *"pulls/"*) echo '{"head":{"sha":"${NC_BASE_SHA}"},"auto_merge":null}' ;; + *"pr checkout"*|*"pr comment"*|*"pr merge"*) exit 0 ;; + *) echo "{}" ;; +esac +GHEOF + chmod +x "$STUB_BIN_DIR/gh" + + cat > "$STUB_BIN_DIR/claude" << 'STUB' +#!/usr/bin/env bash +echo "Addressed feedback." +printf 'fixed\n' > fix.txt +STUB + chmod +x "$STUB_BIN_DIR/claude" + cat > "$STUB_BIN_DIR/git" << 'GITEOF' +#!/usr/bin/env bash +if [ "$1" = "push" ]; then exit 0; fi +exec /usr/bin/git "$@" +GITEOF + chmod +x "$STUB_BIN_DIR/git" + + run bash -c " + cd '$tmpdir' + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=false + export PR_NUMBER=54 HEAD_SHA=$NC_BASE_SHA REPO='petry-projects/.github-private' + export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' + export ACTOR='gemini-code-assist[bot]' + export BOT_USER='don-petry' + export PATH='$STUB_BIN_DIR:$PATH' + bash '$FIX_REVIEWS_SCRIPT' + " 2>&1 +} + +@test "resolve_addressed_bot_threads (#2079 AC4): a marker-less 'No change needed' reply posted by this pass is stamped and does NOT resolve the thread" { + _nochange_resolver_case '[{"id":77,"user":{"login":"don-petry"},"created_at":"2099-01-01T00:00:00Z","body":"No change needed in this pass: the guard already exists."}]' + grep -q 'dev-lead:reply' "$PATCH_FILE" + ! grep -q "PRRT_nochange_bot" "$MUTATIONS_FILE" +} + +@test "resolve_addressed_bot_threads (#2079 AC4): a marker-less maintainer comment the pass did not post still resolves the thread" { + _nochange_resolver_case '[]' + [ ! -s "$PATCH_FILE" ] + grep -q "PRRT_nochange_bot" "$MUTATIONS_FILE" +} From 7e8627e05f712154aacb1bcb71e86c8e348834bd Mon Sep 17 00:00:00 2001 From: don-petry <{}+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 21:35:34 +0000 Subject: [PATCH 11/14] fix(reviews): attribute pass replies by recorded id and gate no-change on configured epoch (#2079 AC4) A gh shim on the model's PATH records the node id of each addPullRequestReviewThreadReply the pass posts; stamp_pass_replies stamps only those ids and never rewrites a comment it did not record. An unattributable reply or missing recorder disables the no-change path. The hard-coded epoch is replaced by DEV_LEAD_NOCHANGE_EPOCH (unset = disabled). Test-Change-Justification: the changed tests were added by 47fac647 on this PR and assert the login/time-window stamping that maintainer review https://github.com/petry-projects/.github-private/pull/2084#discussion_r4234003886 rejects (#2079 AC6); they are replaced by recorded-reply-id tests Co-Authored-By: Claude Sonnet 5.5 --- scripts/dev-lead-fix-reviews.sh | 82 +++++++++++++------ scripts/lib/addressed-claim-verify.sh | 9 +- scripts/lib/claim-landing.sh | 78 ++++++++++++------ .../unit/test_addressed_claim_verify.bats | 10 +-- tests/dev-lead/unit/test_claim_landing.bats | 54 ++++++++---- tests/dev-lead/unit/test_fix_reviews.bats | 67 +++++++++++---- 6 files changed, 204 insertions(+), 96 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index e13d32365..9bafb5a49 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -128,6 +128,10 @@ if [ "${DEV_LEAD_DRY_RUN:-false}" = "false" ] && [ -n "${PR_NUMBER:-}" ]; then # The pass boundary for the claim-retraction sweep (#2013): only claim replies # created at/after this instant are this pass's to retract. PASS_START_ISO="$(date -u +%Y-%m-%dT%H:%M:%SZ)" + # Reply-id record for stamp_pass_replies (#2079 AC4): created empty here so a pass + # that never runs the engine still has a (empty) record; the engine run installs the + # recording gh shim that fills it. A missing record disables the no-change path. + PASS_REPLY_RECORD="$(mktemp "${TMPDIR:-/tmp}/dev-lead-replies.XXXXXX" 2>/dev/null || true)" setup_git_identity # Backfill the five required description sections into a pre-existing PR whose # body still lacks them (#1805). Idempotent + marker-keyed, so open PRs heal on @@ -176,7 +180,17 @@ build_and_run() { fi local rc=0 - run_writer_with_fallback "$prompt_file" "${INTENT_TYPE:-}" || rc=$? + # Run the model with a recording gh shim first on PATH so every review-thread reply it + # posts is attributable by node id (#2079 AC4). No recorder => nothing is trusted. + local shim_dir="" engine_path="$PATH" + if [ -n "${PASS_REPLY_RECORD:-}" ] && shim_dir=$(cl_install_reply_recorder "$PASS_REPLY_RECORD"); then + engine_path="${shim_dir}:${PATH}" + else + echo "::warning::could not install the reply recorder — no-change dispositions are disabled this pass (#2079)" + [ -n "${PASS_REPLY_RECORD:-}" ] && printf 'UNATTRIBUTED\n' > "$PASS_REPLY_RECORD" + fi + PATH="$engine_path" run_writer_with_fallback "$prompt_file" "${INTENT_TYPE:-}" || rc=$? + [ -n "$shim_dir" ] && rm -rf "$shim_dir" rm -f "${prompt_file:-}" return "$rc" } @@ -812,6 +826,8 @@ resolve_addressed_bot_threads() { # become a later pass's "maintainer verdict" (#2079 AC4). On failure the no-change # path is disabled for this pass. local stamp_ok=1 + local nochange_epoch="" + nochange_epoch=$(nochange_epoch_cutoff) || stamp_ok=0 stamp_pass_replies || stamp_ok=0 # The enumeration pass ONLY collects candidate thread ids (unresolved, @@ -880,7 +896,7 @@ resolve_addressed_bot_threads() { # A truncated comment page may hide a later neutral maintainer comment that # supersedes the affirmation -> fail closed (skip this path) unless fully read. if [ "$stamp_ok" -eq 1 ] && [ "$(printf '%s' "$node_json" | jq -r 'if .data.node.comments.pageInfo.hasNextPage == false then "false" else "true" end' 2>/dev/null || echo true)" = "false" ]; then - nochange_disposition=$(acv_latest_nochange_disposition "$comments_json" "$_ACV_REPLY_MARKER_EPOCH") && nochange_rc=0 || nochange_rc=$? + nochange_disposition=$(acv_latest_nochange_disposition "$comments_json" "$nochange_epoch") && nochange_rc=0 || nochange_rc=$? else nochange_disposition="" nochange_rc=1 fi @@ -1217,6 +1233,8 @@ resolve_deferred_bot_threads() { # Stamp this pass's own replies before evaluating any thread (#2079 AC4); on failure # the no-change path is disabled for this pass. local stamp_ok=1 + local nochange_epoch="" + nochange_epoch=$(nochange_epoch_cutoff) || stamp_ok=0 stamp_pass_replies || stamp_ok=0 local ids ids=$(list_unresolved_bot_thread_ids) || { @@ -1334,7 +1352,7 @@ resolve_deferred_bot_threads() { # A maintainer's "no change needed" verdict can resolve the thread without a deferral. local nochange_disposition nochange_rc if [ "$stamp_ok" -eq 1 ]; then - nochange_disposition=$(acv_latest_nochange_disposition "$comments_json" "$_ACV_REPLY_MARKER_EPOCH") && nochange_rc=0 || nochange_rc=$? + nochange_disposition=$(acv_latest_nochange_disposition "$comments_json" "$nochange_epoch") && nochange_rc=0 || nochange_rc=$? else nochange_disposition="" nochange_rc=1 fi @@ -2990,37 +3008,53 @@ verify_push_landed() { return 0 } +# nochange_epoch_cutoff — the instant from which a marker-less maintainer comment may +# authorize a no-change resolution (#2079 AC4). Read from DEV_LEAD_NOCHANGE_EPOCH, never +# hard-coded: dev-lead replies posted by a release that predates the stamping are +# marker-less and look like the maintainer's own, so the maintainer turns the path on +# (by setting this to an instant after the stamping release reached every consumer's +# channel) only when that is safe. Unset or unparseable => returns 1: the no-change +# path is disabled. +nochange_epoch_cutoff() { + local e="${DEV_LEAD_NOCHANGE_EPOCH:-}" + [[ "$e" =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$ ]] || return 1 + _acv_is_iso8601 "$e" || return 1 + printf '%s\n' "$e" +} + # stamp_pass_replies — make every review-thread reply this pass posted attributable to # dev-lead (#2079 AC4). The model posts skip notes from its own shell as the SAME # account the maintainer uses, and only addressed claims are told to carry a marker, so # a marker-less "No change needed" skip note would otherwise read as the maintainer's -# verdict and let the next pass resolve the bot thread with no human involved. Every -# reply OUR account created since PASS_START_ISO that lacks an agent marker gets -# `` appended. Enforced here in shell, not in the prompt. -# Returns non-zero when the pass boundary is unknown or any listing/PATCH failed; the -# resolvers then skip the no-change path (fail closed). Idempotent: stamped replies -# carry a marker and are never selected again. Replies from before this stamping -# existed are excluded by _ACV_REPLY_MARKER_EPOCH in acv_latest_nochange_disposition. +# verdict. Attribution is by IDENTITY: the recording gh shim (cl_install_reply_recorder) +# captured the node id of each reply the pass posted, and ONLY those ids are rewritten +# (`` appended when no agent marker is present). A comment whose +# id was not recorded — e.g. the maintainer's own, posted mid-pass — is never touched. +# Returns non-zero when no recorder record exists, a reply could not be attributed, or a +# fetch/update failed; the resolvers then skip the no-change path (fail closed). +# Idempotent: a stamped reply carries a marker and is skipped. stamp_pass_replies() { - [ -z "${PR_NUMBER:-}" ] && return 1 - [ -z "${PASS_START_ISO:-}" ] && return 1 - local bot_user="${BOT_USER:-donpetry-bot}" comments rows id body_b64 body failed=0 - if ! comments=$(gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/comments?per_page=100" 2>/dev/null | jq -s 'add // []' 2>/dev/null); then - echo "::warning::stamp_pass_replies: could not list review comments on PR #${PR_NUMBER} — no-change dispositions are disabled this pass (#2079)" + local record="${PASS_REPLY_RECORD:-}" ids id body_json body failed=0 + if ! ids=$(cl_recorded_reply_ids "$record"); then + echo "::warning::stamp_pass_replies: no usable reply record — no-change dispositions are disabled this pass (#2079)" return 1 fi - rows=$(cl_select_unmarked_replies "$comments" "$bot_user" "$PASS_START_ISO") || { - echo "::warning::stamp_pass_replies: could not parse the review comments on PR #${PR_NUMBER} — no-change dispositions are disabled this pass (#2079)" - return 1 - } - while IFS=$'\t' read -r id body_b64; do + while IFS= read -r id; do [ -z "$id" ] && continue - body=$(base64 --decode <<<"$body_b64" 2>/dev/null) || { failed=1; continue; } - if ! gh api -X PATCH "repos/${REPO}/pulls/comments/${id}" -f body="$(cl_reply_stamp_body "$body")" >/dev/null 2>&1; then - echo "::warning::stamp_pass_replies: could not stamp reply ${id} on PR #${PR_NUMBER} (#2079)" + if ! body_json=$(gh api graphql \ + -f query='query($id:ID!){node(id:$id){... on PullRequestReviewComment{body}}}' \ + -f id="$id" 2>/dev/null); then + failed=1; continue + fi + body=$(jq -er '.data.node.body // empty' <<<"$body_json" 2>/dev/null) || { failed=1; continue; } + review_thread_is_agent_authored "$body" && continue + if ! gh api graphql \ + -f query='mutation($id:ID!,$body:String!){updatePullRequestReviewComment(input:{pullRequestReviewCommentId:$id,body:$body}){pullRequestReviewComment{id}}}' \ + -f id="$id" -f body="$(cl_reply_stamp_body "$body")" >/dev/null 2>&1; then + echo "::warning::stamp_pass_replies: could not stamp reply ${id} on PR #${PR_NUMBER:-} (#2079)" failed=1 fi - done <<< "$rows" + done <<< "$ids" [ "$failed" -eq 0 ] } diff --git a/scripts/lib/addressed-claim-verify.sh b/scripts/lib/addressed-claim-verify.sh index 07aae62d2..ce38e8189 100644 --- a/scripts/lib/addressed-claim-verify.sh +++ b/scripts/lib/addressed-claim-verify.sh @@ -72,13 +72,6 @@ readonly _ACV_DISPOSITION_RE_UPPER='(REQUIRED|MUST BE (FIXED|ADDRESSED|RESOLVED| # never clears a thread. Note "no change required" contains the substring REQUIRED # and so also matches _ACV_DISPOSITION_RE_UPPER; the caller must resolve that # collision in favour of the no-change intent. -# Replies posted before the harness began stamping every pass reply with -# `` (#2079 AC4) are marker-less and indistinguishable from a -# maintainer's own comment, so none of them may authorize a no-change resolution. -# Callers pass this as the 2nd arg of acv_latest_nochange_disposition. Bump it to the -# merge instant of the stamping change if that lands later than this value. -readonly _ACV_REPLY_MARKER_EPOCH='2026-10-10T00:00:00Z' - readonly _ACV_NOCHANGE_RE_UPPER='(NO (CODE )?CHANGES? (NEEDED|REQUIRED|NECESSARY|WARRANTED|IS NEEDED|ARE NEEDED)|FALSE[ -]?POSITIVE|WON.?T ?FIX|WONTFIX|WORKING AS INTENDED|NOT A (REAL )?(BUG|ISSUE|PROBLEM|CONCERN|DEFECT)|NOT APPLICABLE|BY DESIGN|INTENDED BEHAVIOU?R)' # A NEGATED (or quoted-then-rejected) no-change phrase is NOT an affirmative no-change @@ -434,7 +427,7 @@ acv_latest_maintainer_disposition() { } # acv_latest_nochange_disposition [epoch_iso] -# The optional [epoch_iso] (production callers pass _ACV_REPLY_MARKER_EPOCH) makes any +# The optional [epoch_iso] (production callers pass the DEV_LEAD_NOCHANGE_EPOCH cutoff) makes any # comment created before it non-authorizing; omitted, no cutoff applies. # Scan ALL comments in a thread (#1743, #2079) for a standing maintainer "no change # needed" disposition — the mirror of acv_latest_maintainer_disposition. diff --git a/scripts/lib/claim-landing.sh b/scripts/lib/claim-landing.sh index 38ecd923d..a0134fda6 100644 --- a/scripts/lib/claim-landing.sh +++ b/scripts/lib/claim-landing.sh @@ -112,32 +112,58 @@ cl_select_pass_claims() { done <<<"$rows" } -# cl_select_unmarked_replies -# is the REST `pulls/{pr}/comments` array. Emits one -# `\t` row per comment that our account (bot_user, its -# [bot]-suffixed or stripped form) created at or after (the pass start) -# and that carries NO agent marker (the shared _MAINTAINER_REVIEW_GATE_AGENT_MARKERS -# set). dev-lead posts its skip notes from the model's shell as the SAME account the -# maintainer uses, so such a reply is indistinguishable from a maintainer comment -# until the harness stamps it (#2079 AC4). An empty or a non-array -# payload selects nothing (rc 0 / rc 1 on unparseable input). Pure. -cl_select_unmarked_replies() { - local comments_json="${1:-}" bot_user="${2:-}" since="${3:-}" - [[ -z "$since" ]] && return 0 - local bot_stripped="${bot_user%\[bot\]}" - jq -r \ - --arg u "$bot_user" --arg us "$bot_stripped" --arg since "$since" \ - --arg markers "$_MAINTAINER_REVIEW_GATE_AGENT_MARKERS" ' - if type == "array" then - .[] | objects - | (.user.login // "") as $l - | select($l == $u or $l == $us or $l == ($us + "[bot]")) - | select((.created_at // "") >= $since) - | select(((.body // "") | test($markers)) | not) - | [(.id | tostring), ((.body // "") | @base64)] - | @tsv - else empty end - ' <<<"$comments_json" 2>/dev/null +# cl_install_reply_recorder [real_gh] +# Attribution by IDENTITY, not by time window (#2079 AC4). dev-lead posts as the SAME +# account the maintainer uses, so "our login since the pass start" would also match a +# comment the human posts mid-pass. Instead a `gh` shim is installed in a fresh +# directory (echoed on stdout; the caller prepends it to the engine's PATH). The shim +# forwards every call to the real gh unchanged and, for an +# addPullRequestReviewThreadReply call, appends the posted reply's node id to +# (or the literal UNATTRIBUTED when the response carries no id, so the +# caller can fail closed). The record file is created empty. Returns 1 on failure. +cl_install_reply_recorder() { + local record="${1:-}" real="${2:-}" + [[ -z "$record" ]] && return 1 + [[ -z "$real" ]] && { real=$(command -v gh 2>/dev/null) || real=""; } + [[ -z "$real" ]] && return 1 + local dir + dir=$(mktemp -d "${TMPDIR:-/tmp}/dev-lead-recorder.XXXXXX") || return 1 + : > "$record" || return 1 + { + printf '#!/usr/bin/env bash\n' + printf 'real=%q\nrec=%q\n' "$real" "$record" + cat <<'SHIM' +case "$*" in + *addPullRequestReviewThreadReply*) + rc=0 + out=$("$real" "$@") || rc=$? + printf '%s\n' "$out" + if [ "$rc" -eq 0 ]; then + id=$(jq -r '.data.addPullRequestReviewThreadReply.comment.id // empty' <<<"$out" 2>/dev/null || true) + printf '%s\n' "${id:-UNATTRIBUTED}" >> "$rec" + fi + exit "$rc" + ;; +esac +exec "$real" "$@" +SHIM + } > "$dir/gh" + chmod +x "$dir/gh" + printf '%s\n' "$dir" +} + +# cl_recorded_reply_ids +# Emits the unique node ids the recorder captured, one per line. Returns 1 when the +# record is missing or holds an UNATTRIBUTED entry (a reply whose id is unknown, so +# nothing can be trusted: the no-change path must be disabled). Pure. +cl_recorded_reply_ids() { + local record="${1:-}" + [[ -n "$record" && -f "$record" ]] || return 1 + if grep -qx 'UNATTRIBUTED' "$record"; then + return 1 + fi + { grep -v '^[[:space:]]*$' "$record" || true; } | sort -u + return 0 } # cl_reply_stamp_body diff --git a/tests/dev-lead/unit/test_addressed_claim_verify.bats b/tests/dev-lead/unit/test_addressed_claim_verify.bats index 76a23989c..d7c5ea14f 100644 --- a/tests/dev-lead/unit/test_addressed_claim_verify.bats +++ b/tests/dev-lead/unit/test_addressed_claim_verify.bats @@ -820,32 +820,32 @@ setup() { @test "acv_latest_nochange_disposition: marker-less pass skip note 'No change needed in this pass' before the epoch -> rc1" { local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"MEMBER","body":"No change needed in this pass: the guard already exists.","createdAt":"2026-10-09T07:58:15Z"}]' - run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + run acv_latest_nochange_disposition "$comments" "2026-10-10T00:00:00Z" [[ "$status" -eq 1 ]] } @test "acv_latest_nochange_disposition: marker-less 'Skipped: this is a false positive' before the epoch -> rc1" { local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"MEMBER","body":"Skipped: this is a false positive, the helper is already called from the resolver.","createdAt":"2026-10-09T02:35:36Z"}]' - run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + run acv_latest_nochange_disposition "$comments" "2026-10-10T00:00:00Z" [[ "$status" -eq 1 ]] } @test "acv_latest_nochange_disposition: a stamped pass reply () never authorizes -> rc1" { local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"MEMBER","body":"No change needed in this pass: the guard already exists.\n\n","createdAt":"2026-10-11T07:58:15Z"}]' - run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + run acv_latest_nochange_disposition "$comments" "2026-10-10T00:00:00Z" [[ "$status" -eq 1 ]] } @test "acv_latest_nochange_disposition: marker-less maintainer comment after the epoch still authorizes -> rc0" { local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"OWNER","body":"False positive, no change needed.","createdAt":"2026-10-11T08:00:00Z"}]' - run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + run acv_latest_nochange_disposition "$comments" "2026-10-10T00:00:00Z" [[ "$status" -eq 0 ]] [[ "$output" == "2026-10-11T08:00:00Z" ]] } @test "acv_latest_nochange_disposition: a pre-epoch legacy note does not hide a later real affirmation" { local comments='[{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"MEMBER","body":"Skipped: this is a false positive.","createdAt":"2026-10-09T02:35:36Z"},{"author":{"login":"don-petry","__typename":"User"},"authorAssociation":"OWNER","body":"Confirmed, false positive.","createdAt":"2026-10-11T08:00:00Z"}]' - run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + run acv_latest_nochange_disposition "$comments" "2026-10-10T00:00:00Z" [[ "$status" -eq 0 ]] [[ "$output" == "2026-10-11T08:00:00Z" ]] } diff --git a/tests/dev-lead/unit/test_claim_landing.bats b/tests/dev-lead/unit/test_claim_landing.bats index 5d04c9b07..446183c4d 100644 --- a/tests/dev-lead/unit/test_claim_landing.bats +++ b/tests/dev-lead/unit/test_claim_landing.bats @@ -290,23 +290,47 @@ _inject_foreign_commit() { # ── #2079 AC4: stamping the pass's own marker-less replies ────────────────────── -@test "cl_select_unmarked_replies: selects our marker-less replies since the pass start" { - local c='[ - {"id":1,"user":{"login":"don-petry"},"created_at":"2026-10-11T01:00:00Z","body":"No change needed in this pass: the guard already exists."}, - {"id":2,"user":{"login":"don-petry"},"created_at":"2026-10-11T01:00:00Z","body":"Fixed. "}, - {"id":3,"user":{"login":"don-petry"},"created_at":"2026-10-10T00:00:00Z","body":"Skipped: this is a false positive."}, - {"id":4,"user":{"login":"someone-else"},"created_at":"2026-10-11T01:00:00Z","body":"No change needed."}, - {"id":5,"user":{"login":"don-petry"},"created_at":"2026-10-11T02:00:00Z","body":"Already stamped.\n\n"} - ]' - run cl_select_unmarked_replies "$c" "don-petry" "2026-10-11T00:00:00Z" +@test "cl_install_reply_recorder: records the node id of an addPullRequestReviewThreadReply and passes output through" { + local real="$BATS_TEST_TMPDIR/real-gh" rec="$BATS_TEST_TMPDIR/rec" + printf '#!/usr/bin/env bash\necho "{\\"data\\":{\\"addPullRequestReviewThreadReply\\":{\\"comment\\":{\\"id\\":\\"PRRC_abc\\"}}}}"\n' > "$real" + chmod +x "$real" + local dir + dir=$(cl_install_reply_recorder "$rec" "$real") + run "$dir/gh" api graphql -f query='mutation { addPullRequestReviewThreadReply(input:{}) { comment { id } } }' + [[ "$status" -eq 0 ]] + [[ "$output" == *PRRC_abc* ]] + run cl_recorded_reply_ids "$rec" [[ "$status" -eq 0 ]] - [[ "$output" == 1$'\t'* ]] - [[ "$(printf '%s\n' "$output" | wc -l)" -eq 1 ]] + [[ "$output" == "PRRC_abc" ]] +} + +@test "cl_install_reply_recorder: other gh calls are forwarded and not recorded" { + local real="$BATS_TEST_TMPDIR/real-gh" rec="$BATS_TEST_TMPDIR/rec" + printf '#!/usr/bin/env bash\necho "args: $*"\n' > "$real" + chmod +x "$real" + local dir + dir=$(cl_install_reply_recorder "$rec" "$real") + run "$dir/gh" pr view 5 + [[ "$output" == "args: pr view 5" ]] + [[ ! -s "$rec" ]] } -@test "cl_select_unmarked_replies: no pass boundary selects nothing" { - local c='[{"id":1,"user":{"login":"don-petry"},"created_at":"2026-10-11T01:00:00Z","body":"No change needed."}]' - run cl_select_unmarked_replies "$c" "don-petry" "" +@test "cl_install_reply_recorder: a reply response with no id is UNATTRIBUTED and fails closed" { + local real="$BATS_TEST_TMPDIR/real-gh" rec="$BATS_TEST_TMPDIR/rec" + printf '#!/usr/bin/env bash\necho "{}"\n' > "$real" + chmod +x "$real" + local dir + dir=$(cl_install_reply_recorder "$rec" "$real") + run "$dir/gh" api graphql -f query='mutation { addPullRequestReviewThreadReply(input:{}) { comment { id } } }' + run cl_recorded_reply_ids "$rec" + [[ "$status" -eq 1 ]] +} + +@test "cl_recorded_reply_ids: a missing record fails closed; an empty record is ok" { + run cl_recorded_reply_ids "$BATS_TEST_TMPDIR/nope" + [[ "$status" -eq 1 ]] + : > "$BATS_TEST_TMPDIR/empty" + run cl_recorded_reply_ids "$BATS_TEST_TMPDIR/empty" [[ "$status" -eq 0 ]] [[ -z "$output" ]] } @@ -317,6 +341,6 @@ _inject_foreign_commit() { review_thread_is_agent_authored "$stamped" local comments comments=$(jq -cn --arg b "$stamped" '[{author:{login:"don-petry",__typename:"User"},authorAssociation:"MEMBER",body:$b,createdAt:"2026-10-11T08:00:00Z"}]') - run acv_latest_nochange_disposition "$comments" "$_ACV_REPLY_MARKER_EPOCH" + run acv_latest_nochange_disposition "$comments" "2026-10-10T00:00:00Z" [[ "$status" -eq 1 ]] } diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index 07dcade7e..0cf0117c1 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -6815,23 +6815,28 @@ _fbc_reply() { # $1=disposition-tail $2=createdAt # ── #2079 AC4: a pass's own marker-less "no change" skip note must never resolve ── # dev-lead posts skip notes as the SAME account the maintainer uses, with no marker. -# The harness stamps every reply the pass created (stamp_pass_replies) before either -# resolver reads the thread, so such a note stays attributable to dev-lead and the -# bot thread it "dispositioned" stays open. The gh stub models GitHub: the PATCH that -# stamps the reply rewrites the body the next GraphQL thread read returns. +# Attribution is by IDENTITY: a gh shim on the model's PATH records the node id of each +# addPullRequestReviewThreadReply the pass posts, and the harness (stamp_pass_replies) +# appends `` to ONLY those ids before either resolver reads the +# thread. The gh stub models GitHub: the stamping mutation rewrites the body the next +# thread read returns. The no-change path is gated on DEV_LEAD_NOCHANGE_EPOCH. _nochange_resolver_case() { - # $1 = JSON array of REST pulls/54/comments the pass created (what stamping sees) - local rest_json="$1" + # $1 = pass mode: pass-reply | maintainer | unattributed + # $2 = epoch: set | unset + local mode="$1" epoch="${2:-set}" local tmpdir="$BATS_TEST_TMPDIR/workdir" mkdir -p "$tmpdir" MUTATIONS_FILE="$BATS_TEST_TMPDIR/mutations" PATCH_FILE="$BATS_TEST_TMPDIR/patches" - local body_file="$BATS_TEST_TMPDIR/maintainer-body" + BODY_FILE="$BATS_TEST_TMPDIR/maintainer-body" : > "$MUTATIONS_FILE" : > "$PATCH_FILE" - printf 'No change needed in this pass: the guard already exists.' > "$body_file" - printf '%s' "$rest_json" > "$BATS_TEST_TMPDIR/rest.json" + if [ "$mode" = "maintainer" ]; then + printf 'No change needed, false positive.' > "$BODY_FILE" + else + : > "$BODY_FILE" + fi rm -f /tmp/dev-lead-session-output.txt git -C "$tmpdir" init -q @@ -6849,14 +6854,20 @@ case "\$ARGS" in echo "\$*" >> "$MUTATIONS_FILE" echo '{"data":{"resolveReviewThread":{"thread":{"isResolved":true}}}}' ;; - *"-X PATCH"*) - for a in "\$@"; do case "\$a" in body=*) printf '%s' "\${a#body=}" > "$body_file"; echo "\$a" >> "$PATCH_FILE" ;; esac; done - echo '{}' + *"updatePullRequestReviewComment"*) + for a in "\$@"; do case "\$a" in body=*) printf '%s' "\${a#body=}" > "$BODY_FILE"; echo "\$a" >> "$PATCH_FILE" ;; esac; done + echo '{"data":{"updatePullRequestReviewComment":{"pullRequestReviewComment":{"id":"PRRC_77"}}}}' + ;; + *"addPullRequestReviewThreadReply"*) + for a in "\$@"; do case "\$a" in body=*) printf '%s' "\${a#body=}" > "$BODY_FILE" ;; esac; done + if [ "$mode" = "unattributed" ]; then echo '{}'; else echo '{"data":{"addPullRequestReviewThreadReply":{"comment":{"id":"PRRC_77"}}}}'; fi + ;; + *"PullRequestReviewComment{body}"*) + jq -cn --rawfile b "$BODY_FILE" '{data:{node:{body:\$b}}}' ;; - *"pulls/54/comments"*) cat "$BATS_TEST_TMPDIR/rest.json" ;; *"fullDatabaseId"*) echo '{"data":{"node":{"isResolved":false,"comments":{"pageInfo":{"hasNextPage":false},"nodes":[]}}}}' ;; *"PullRequestReviewThread"*) - jq -cn --rawfile b "$body_file" '{data:{node:{isResolved:false,path:"fix.txt",comments:{pageInfo:{hasNextPage:false},nodes:[ + jq -cn --rawfile b "$BODY_FILE" '{data:{node:{isResolved:false,path:"fix.txt",comments:{pageInfo:{hasNextPage:false},nodes:[ {author:{login:"gemini-code-assist",__typename:"Bot"},authorAssociation:"NONE",body:"Missing a guard.",createdAt:"2098-01-01T00:00:00Z"}, {author:{login:"don-petry",__typename:"User"},authorAssociation:"MEMBER",body:\$b,createdAt:"2099-01-01T00:00:00Z"}]}}}}' ;; @@ -6873,10 +6884,15 @@ esac GHEOF chmod +x "$STUB_BIN_DIR/gh" + # The model stub posts its skip note through gh (the recording shim is first on PATH). cat > "$STUB_BIN_DIR/claude" << 'STUB' #!/usr/bin/env bash echo "Addressed feedback." printf 'fixed\n' > fix.txt +if [ "$NC_MODE" != "maintainer" ]; then + gh api graphql -f query='mutation { addPullRequestReviewThreadReply(input:{}) { comment { id } } }' \ + -f tid=PRRT_nochange_bot -f body='No change needed in this pass: the guard already exists.' >/dev/null +fi STUB chmod +x "$STUB_BIN_DIR/claude" cat > "$STUB_BIN_DIR/git" << 'GITEOF' @@ -6886,26 +6902,41 @@ exec /usr/bin/git "$@" GITEOF chmod +x "$STUB_BIN_DIR/git" + local epoch_export="" + [ "$epoch" = "set" ] && epoch_export="export DEV_LEAD_NOCHANGE_EPOCH=2026-10-10T00:00:00Z" run bash -c " cd '$tmpdir' - export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=false + export INTENT_TYPE=fix-reviews DEV_LEAD_DRY_RUN=false NC_MODE='$mode' export PR_NUMBER=54 HEAD_SHA=$NC_BASE_SHA REPO='petry-projects/.github-private' export REVIEW_ENGINE=claude BASE_REF=main PROMPTS_DIR='$SCRIPT_DIR/prompts/dev-lead' export ACTOR='gemini-code-assist[bot]' export BOT_USER='don-petry' + $epoch_export export PATH='$STUB_BIN_DIR:$PATH' bash '$FIX_REVIEWS_SCRIPT' " 2>&1 } @test "resolve_addressed_bot_threads (#2079 AC4): a marker-less 'No change needed' reply posted by this pass is stamped and does NOT resolve the thread" { - _nochange_resolver_case '[{"id":77,"user":{"login":"don-petry"},"created_at":"2099-01-01T00:00:00Z","body":"No change needed in this pass: the guard already exists."}]' + _nochange_resolver_case pass-reply set grep -q 'dev-lead:reply' "$PATCH_FILE" + grep -q 'dev-lead:reply' "$BODY_FILE" ! grep -q "PRRT_nochange_bot" "$MUTATIONS_FILE" } -@test "resolve_addressed_bot_threads (#2079 AC4): a marker-less maintainer comment the pass did not post still resolves the thread" { - _nochange_resolver_case '[]' +@test "resolve_addressed_bot_threads (#2079 AC4): a maintainer comment the pass did not post stays byte-identical and still resolves the thread" { + _nochange_resolver_case maintainer set [ ! -s "$PATCH_FILE" ] + [ "$(cat "$BODY_FILE")" = "No change needed, false positive." ] grep -q "PRRT_nochange_bot" "$MUTATIONS_FILE" } + +@test "resolve_addressed_bot_threads (#2079 AC4): a pass reply whose id cannot be attributed disables the no-change path" { + _nochange_resolver_case unattributed set + ! grep -q "PRRT_nochange_bot" "$MUTATIONS_FILE" +} + +@test "resolve_addressed_bot_threads (#2079 AC4): an unset DEV_LEAD_NOCHANGE_EPOCH resolves nothing on a no-change verdict" { + _nochange_resolver_case maintainer unset + ! grep -q "PRRT_nochange_bot" "$MUTATIONS_FILE" +} From 008be41191be2c76319f712c7fbc6c7011551ead Mon Sep 17 00:00:00 2001 From: don-petry <36422719+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 22:09:58 +0000 Subject: [PATCH 12/14] fix(reviews): unset DEV_LEAD_NOCHANGE_EPOCH explicitly in the unset-epoch resolver test Test-Change-Justification: the changed line was added earlier on this PR (#2084) and inherited DEV_LEAD_NOCHANGE_EPOCH from the runner in the unset case, per cubic review https://github.com/petry-projects/.github-private/pull/2084 (thread PRRT_kwDOR9SdIs6q9VZL) Co-Authored-By: Claude Sonnet 5.5 --- tests/dev-lead/unit/test_fix_reviews.bats | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/dev-lead/unit/test_fix_reviews.bats b/tests/dev-lead/unit/test_fix_reviews.bats index 0cf0117c1..ffd9b9092 100644 --- a/tests/dev-lead/unit/test_fix_reviews.bats +++ b/tests/dev-lead/unit/test_fix_reviews.bats @@ -6902,7 +6902,7 @@ exec /usr/bin/git "$@" GITEOF chmod +x "$STUB_BIN_DIR/git" - local epoch_export="" + local epoch_export="unset DEV_LEAD_NOCHANGE_EPOCH" [ "$epoch" = "set" ] && epoch_export="export DEV_LEAD_NOCHANGE_EPOCH=2026-10-10T00:00:00Z" run bash -c " cd '$tmpdir' From 9b41edc051e29aaa546032cb9ee1242fbc4d5f78 Mon Sep 17 00:00:00 2001 From: don-petry <{}+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 22:38:46 +0000 Subject: [PATCH 13/14] fix(reviews): require hasNextPage==false on no-change fresh re-read Co-Authored-By: Claude Sonnet 5.5 --- scripts/dev-lead-fix-reviews.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 9bafb5a49..c9a17919b 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -913,7 +913,9 @@ resolve_addressed_bot_threads() { continue fi local fresh_comments - fresh_comments=$(printf '%s' "$fresh_json" | jq -c '.data.node.comments.nodes // []' 2>/dev/null || echo "[]") + fresh_comments=$(printf '%s' "$fresh_json" | jq -c \ + 'if .data.node.comments.pageInfo.hasNextPage == false then (.data.node.comments.nodes // []) else "changed" end' \ + 2>/dev/null || echo '"changed"') if [ "$fresh_comments" != "$comments_json" ]; then echo "::notice::skipping thread ${id} — thread changed during no-change disposition check; leaving unresolved" continue @@ -1372,7 +1374,9 @@ resolve_deferred_bot_threads() { echo "::notice::skipping thread ${id} — already resolved during no-change check" continue fi - fresh_comments=$(printf '%s' "$fresh_json" | jq -c '.data.node.comments.nodes // []' 2>/dev/null || echo "[]") + fresh_comments=$(printf '%s' "$fresh_json" | jq -c \ + 'if .data.node.comments.pageInfo.hasNextPage == false then (.data.node.comments.nodes // []) else "changed" end' \ + 2>/dev/null || echo '"changed"') if [ "$fresh_comments" != "$comments_json" ]; then echo "::notice::skipping thread ${id} — thread changed during no-change disposition check; leaving unresolved" continue From 6adb065a1a9d310e81547a924e68bbb15a32fe99 Mon Sep 17 00:00:00 2001 From: don-petry <{}+don-petry@users.noreply.github.com> Date: Fri, 9 Oct 2026 23:07:24 +0000 Subject: [PATCH 14/14] fix(reviews): persist unstamped replies across passes and record all reply-creating gh calls Co-Authored-By: Claude Sonnet 5.5 --- scripts/dev-lead-fix-reviews.sh | 28 +++++++++++--- scripts/lib/claim-landing.sh | 65 +++++++++++++++++++++++++++------ 2 files changed, 76 insertions(+), 17 deletions(-) diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index c9a17919b..a57fa2b26 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -3039,26 +3039,44 @@ nochange_epoch_cutoff() { # Idempotent: a stamped reply carries a marker and is skipped. stamp_pass_replies() { local record="${PASS_REPLY_RECORD:-}" ids id body_json body failed=0 - if ! ids=$(cl_recorded_reply_ids "$record"); then + local pending_file="${PASS_REPLY_PENDING_FILE:-${RUNNER_TEMP:-/tmp}/dev-lead-unstamped-replies-${PR_NUMBER:-0}}" + local recorded="" pending="" new_pending="" + if recorded=$(cl_recorded_reply_ids "$record"); then + : + else echo "::warning::stamp_pass_replies: no usable reply record — no-change dispositions are disabled this pass (#2079)" - return 1 + failed=1 + recorded="" fi + # Replies that failed to be stamped in an earlier pass stay pending until stamped, + # so a marker-less dev-lead reply is never forgotten (#2079). + [ -f "$pending_file" ] && pending=$(cat "$pending_file" 2>/dev/null || true) + ids=$(printf '%s\n%s\n' "$recorded" "$pending" | awk 'NF && !seen[$0]++') while IFS= read -r id; do [ -z "$id" ] && continue if ! body_json=$(gh api graphql \ -f query='query($id:ID!){node(id:$id){... on PullRequestReviewComment{body}}}' \ -f id="$id" 2>/dev/null); then - failed=1; continue + failed=1; new_pending+="${id}"$'\n'; continue fi - body=$(jq -er '.data.node.body // empty' <<<"$body_json" 2>/dev/null) || { failed=1; continue; } + body=$(jq -er '.data.node.body // empty' <<<"$body_json" 2>/dev/null) || { failed=1; new_pending+="${id}"$'\n'; continue; } review_thread_is_agent_authored "$body" && continue if ! gh api graphql \ -f query='mutation($id:ID!,$body:String!){updatePullRequestReviewComment(input:{pullRequestReviewCommentId:$id,body:$body}){pullRequestReviewComment{id}}}' \ -f id="$id" -f body="$(cl_reply_stamp_body "$body")" >/dev/null 2>&1; then - echo "::warning::stamp_pass_replies: could not stamp reply ${id} on PR #${PR_NUMBER:-} (#2079)" + echo "::warning::stamp_pass_replies: could not stamp reply ${id} on PR #${PR_NUMBER:-} — will retry on later passes (#2079)" failed=1 + new_pending+="${id}"$'\n' fi done <<< "$ids" + # Persist whatever is still unstamped so later passes retry it and keep no-change + # dispositions disabled until it is resolved. + if [ -n "$new_pending" ]; then + printf '%s' "$new_pending" > "$pending_file" 2>/dev/null || \ + echo "::warning::stamp_pass_replies: could not persist unstamped reply ids to ${pending_file} (#2079)" + else + rm -f "$pending_file" 2>/dev/null || true + fi [ "$failed" -eq 0 ] } diff --git a/scripts/lib/claim-landing.sh b/scripts/lib/claim-landing.sh index a0134fda6..57ac4bd66 100644 --- a/scripts/lib/claim-landing.sh +++ b/scripts/lib/claim-landing.sh @@ -118,7 +118,8 @@ cl_select_pass_claims() { # comment the human posts mid-pass. Instead a `gh` shim is installed in a fresh # directory (echoed on stdout; the caller prepends it to the engine's PATH). The shim # forwards every call to the real gh unchanged and, for an -# addPullRequestReviewThreadReply call, appends the posted reply's node id to +# reply-creating call (the GraphQL reply mutation, any */replies* REST path, +# in_reply_to, or a query/payload read from --input / @file), appends the posted reply's node id to # (or the literal UNATTRIBUTED when the response carries no id, so the # caller can fail closed). The record file is created empty. Returns 1 on failure. cl_install_reply_recorder() { @@ -133,18 +134,58 @@ cl_install_reply_recorder() { printf '#!/usr/bin/env bash\n' printf 'real=%q\nrec=%q\n' "$real" "$record" cat <<'SHIM' -case "$*" in - *addPullRequestReviewThreadReply*) - rc=0 - out=$("$real" "$@") || rc=$? - printf '%s\n' "$out" - if [ "$rc" -eq 0 ]; then - id=$(jq -r '.data.addPullRequestReviewThreadReply.comment.id // empty' <<<"$out" 2>/dev/null || true) - printf '%s\n' "${id:-UNATTRIBUTED}" >> "$rec" +flag=0 +tmpin="" +check() { + case "$1" in + *addPullRequestReviewThreadReply*|*/replies*|*in_reply_to*) flag=1 ;; + esac +} +check "$*" +prev="" +for a in "$@"; do + f="" + [ "$prev" = "--input" ] && f=$a + case "$a" in + --input=*) f=${a#--input=} ;; + *=@*) f=${a#*=@} ;; + @*) f=${a#@} ;; + esac + if [ -n "$f" ]; then + if [ "$f" = "-" ]; then + if [ -z "$tmpin" ]; then + tmpin=$(mktemp) + cat > "$tmpin" + fi + check "$(cat "$tmpin")" + elif [ -r "$f" ]; then + check "$(cat "$f")" + else + flag=1 fi - exit "$rc" - ;; -esac + fi + prev=$a +done +run_real() { + if [ -n "$tmpin" ]; then "$real" "$@" < "$tmpin"; else "$real" "$@"; fi +} +if [ "$flag" -eq 1 ]; then + rc=0 + out=$(run_real "$@") || rc=$? + printf '%s\n' "$out" + if [ "$rc" -eq 0 ]; then + id=$(jq -r '(.data.addPullRequestReviewThreadReply.comment.id // .node_id // empty) | tostring' <<<"$out" 2>/dev/null || true) + printf '%s\n' "${id:-UNATTRIBUTED}" >> "$rec" + fi + [ -n "$tmpin" ] && rm -f "$tmpin" + exit "$rc" +fi +if [ -n "$tmpin" ]; then + rc=0 + run_real "$@" || rc=$? + rm -f "$tmpin" + exit "$rc" +fi exec "$real" "$@" SHIM } > "$dir/gh"