From 1f18ee57293f2ba36e0bcfdbc391d3ed01b17617 Mon Sep 17 00:00:00 2001 From: topcoder1 Date: Fri, 14 Aug 2026 17:01:06 -0700 Subject: [PATCH 1/5] fix(findings): stop counting human resolution replies as findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wxa_vpn#1392's false-positive half, measured on wxa_vpn#1523: the report's top three hits were the PR's own "Fixed in 38774043" inline replies, posted 7-12s after the fix commit they cited. A reply that RESOLVES a finding necessarily post-dates the commit it cites — the author pushes, then replies citing the push — so every resolution round tripped the newer-than-last-commit signal and the report re-flagged resolved threads forever. Across that PR: 5/5 false positives carried in_reply_to_id; 0/14 genuine bot findings did. The narrowing drops inline comments that are BOTH a reply AND human: select(.in_reply_to_id == null or ((.user.login // "") | endswith("[bot]"))) Untouched on purpose: human TOP-LEVEL inline comments still count (a human's own comment on the diff is a deliberate finding — author-agnosticism survives), and BOT replies still count (a reviewer bot answering a thread with "still broken after the fix" is a live finding; keeping them cost zero false positives in the measured corpus). Accepted residual: a human typing a NEW finding into an existing thread is skipped — the operator wrote it, so the operator knows it. This matters beyond the operator CLI because the automerge quiet gate runs this detector and quotes its report verbatim into auto-merge-withheld comments — a false flag there declines arms on clean PRs, and a gate that blocks passing PRs gets switched off. Vendored-twin discipline: the same edit ships to dotclaude's templates/ci-workflows/scripts/bb-unaddressed-findings.sh in a paired PR; byte-identity below the header block verified both ways. Tests: selftest/test_findings_reply_narrowing.sh (6 checks), CI-wired via _SHELL_SELFTESTS. Proven to bite by mutation: removing the narrowing fails R1+R4 (the #1523 shape, rc=1 where 0 expected); dropping the bot escape fails R2; over-correcting to bots-only inline fails R3+R5. Restored, all green; shellcheck clean on both files. Stacked on fix/automerge-quiet-anchor-review-trigger (#161) because both PRs touch _SHELL_SELFTESTS — review only the top commit until #161 merges. Co-Authored-By: Claude Fable 5 --- .github/scripts/unaddressed-findings.sh | 37 ++++-- selftest/test_findings_reply_narrowing.sh | 130 ++++++++++++++++++++++ selftest/test_workflow_guards.py | 1 + 3 files changed, 160 insertions(+), 8 deletions(-) create mode 100755 selftest/test_findings_reply_narrowing.sh diff --git a/.github/scripts/unaddressed-findings.sh b/.github/scripts/unaddressed-findings.sh index 9f206a0..e98c5b3 100755 --- a/.github/scripts/unaddressed-findings.sh +++ b/.github/scripts/unaddressed-findings.sh @@ -18,8 +18,9 @@ # `create_inline_comment` + `gh pr comment`, so a finding can never block a # merge. Detection is the available lever. # -# Signal: an INLINE review comment newer than the newest commit. Inline comments -# are reserved for "issues you're 80%+ sure are real bugs" (claude-review.yml +# Signal: an INLINE review comment newer than the newest commit (human threaded +# REPLIES excluded — see the reply rule in check_pr). Inline comments are +# reserved for "issues you're 80%+ sure are real bugs" (claude-review.yml # prompt), so they carry far less noise than top-level summaries — the bot posts # a top-level line on every run, including "No issues found". Top-level comments # count only when they match a finding marker and no clean marker. @@ -454,8 +455,9 @@ CLEAN_RE='(No issues found|Skipped:|Bugbot is not enab|Coverage Floor|claude-aut # Residual gap: a bot that asserts a finding ONLY in unstructured prose while # also emitting a clean marker, no VERDICT: REGRESSION and no `regression:` # line. Codex itemizes and stamps a trailer, so this is narrow — but do NOT -# assume claude[bot] findings always arrive as inline comments (which this -# filter never touches: author-agnostic, no clean-marker check at all). On +# assume claude[bot] findings always arrive as inline comments (which never see +# CLEAN_RE or the severity override; their only filter is the human-reply +# exclusion in check_pr). On # wxa-jake-ai#1054 claude[bot] posted its finding TOP-LEVEL, as a `regression:` # line, with zero inline comments on the PR. The top-level path is load-bearing # for both bots. A live instance of the residual gap: the codex comment on that @@ -554,18 +556,37 @@ check_pr() { # each as an independent input and the per-page results concatenate, so no # reduction is needed for those. - # Inline comments strictly newer than the newest commit. + # Inline comments strictly newer than the newest commit. Author-agnostic at + # TOP LEVEL — a human's own inline comment on the diff is a deliberate + # code-level finding, same as a bot's. HUMAN THREADED REPLIES are the one + # exclusion: replying to a finding's thread is how a fix is RECORDED + # ("Fixed in "), and that reply necessarily post-dates the commit it + # cites, so every resolution round trips the newer-than-last-commit signal + # and the report re-flags resolved threads forever. Measured on wxa_vpn#1523: + # the report's top three hits were the PR's own "Fixed in 38774043" replies, + # posted 7-12s after the fix commit — 5/5 false positives carried + # in_reply_to_id, 0/14 genuine bot findings did. BOT replies still count: a + # reviewer bot answering a thread with "still broken after the fix" is a + # live finding, and keeping them cost zero false positives in that corpus. + # Accepted residual: a human typing a NEW finding into an existing thread is + # skipped — the operator wrote it, so the operator already knows it, and the + # alternative re-flags every resolved thread on every sweep, which is the + # cry-wolf failure this file argues twice over is the more expensive one. local late_inline late_inline=$(printf '%s' "$inline" | jq -r --arg last "$last" ' - [ .[] | select(.created_at > $last) ] + [ .[] | select(.created_at > $last) + | select(.in_reply_to_id == null + or ((.user.login // "") | endswith("[bot]"))) ] | .[] | [.created_at, .user.login, (.path // "-"), ((.body // "") | gsub("\n"; " ") | .[0:90])] | @tsv' 2>/dev/null) # Top-level findings count only from review BOTS. Humans post round-summary # comments ("REVIEW-LOOP: round 7 — 1 finding, fixed in 2a5937e") that match # the finding markers while actually reporting a FIX — counting those cost a - # false positive on wxa-secrets#27. Inline comments stay author-agnostic: a - # human's inline comment is a deliberate code-level finding either way. + # false positive on wxa-secrets#27. Top-level inline comments stay + # author-agnostic — a human's own comment on the diff is a deliberate + # code-level finding; the one inline exclusion is human REPLIES, per the + # reply rule above. # # `canon` is the normalizer described at _BLOCK_PREFIX above: per line, strip # markdown block markers, then reduce any emphasis spelling of `regression:` diff --git a/selftest/test_findings_reply_narrowing.sh b/selftest/test_findings_reply_narrowing.sh new file mode 100755 index 0000000..eb34d6b --- /dev/null +++ b/selftest/test_findings_reply_narrowing.sh @@ -0,0 +1,130 @@ +#!/usr/bin/env bash +# Behavioral test for the human-reply narrowing in the unaddressed-findings +# detector (wxa_vpn#1392's false-positive half, measured on wxa_vpn#1523). +# +# THE DEFECT. The detector's inline signal was "any inline comment newer than +# the newest commit". A threaded REPLY that RESOLVES a finding ("Fixed in +# ") necessarily post-dates the commit it cites — the author pushes, then +# replies citing the push — so every resolution round tripped the signal, and +# the report re-flagged resolved threads forever. +# +# MEASURED, wxa_vpn#1523 (2026-08-14). The report's top three hits were the +# PR's own "Fixed in 38774043" replies, posted 7-12s after the fix commit. +# Across the PR: 5/5 false positives carried in_reply_to_id; 0/14 genuine bot +# findings did. A sweeper that cries wolf gets ignored — the detector's own +# header argues twice over that this costs more than the miss it trades +# against, and this gate's output is quoted verbatim into automerge-withheld +# comments, so a false flag there declines arms on clean PRs. +# +# THE NARROWING. Drop inline comments that are BOTH a reply (in_reply_to_id +# set) AND human. Everything else is untouched: +# * human TOP-LEVEL inline comments still count (author-agnostic — a +# human's own comment on the diff is a deliberate finding); +# * BOT replies still count (a reviewer bot answering a thread with "still +# broken after the fix" is a live finding). +# Accepted residual: a human typing a NEW finding into an existing thread is +# skipped — the operator wrote it, so the operator already knows it. +# +# Cases (all through the vendored twin's own --fixture seam): +# S1. structural: the narrowing keys on in_reply_to_id == null OR bot login +# R1. THE #1523 SHAPE: human reply "Fixed in " after the last commit +# ⇒ rc=0, no unaddressed findings +# R2. over-correction guard: a BOT reply after the last commit ⇒ rc=1 +# R3. over-correction guard: a human TOP-LEVEL inline comment ⇒ rc=1 +# (the narrowing must not eat author-agnosticism at top level) +# R4. mixed thread: human resolution replies + one genuine late bot +# finding ⇒ rc=1, report names the bot line and NOT the replies +# R5. legacy fixture shape (no in_reply_to_id field at all) ⇒ still +# flagged — absent and null must read the same, so old fixtures and +# REST payloads that omit the field keep today's behavior +# +# Run from the repo root: +# bash selftest/test_findings_reply_narrowing.sh +set -euo pipefail + +CHECKER=.github/scripts/unaddressed-findings.sh +failed=0 +T=$(mktemp -d) +trap 'rm -rf "$T"' EXIT + +# S1. Structural pin: the exclusion must be the null-or-bot shape. A rewrite +# to `has("in_reply_to_id") | not` would split absent from null (R5 pins the +# behavior, this names the intended spelling), and dropping the bot escape +# would eat R2 silently on fixtures that never exercise bot replies. +if grep -q 'in_reply_to_id == null' "$CHECKER" \ + && grep -A1 'in_reply_to_id == null' "$CHECKER" | grep -q 'endswith("\[bot\]")'; then + echo "✓ S1 narrowing is 'not a reply OR a bot' (null-check + bot escape present)" +else + echo "✗ S1 narrowing shape missing or rewritten — expected in_reply_to_id == null with a [bot] escape" + failed=1 +fi + +FX="$T/fixture" +mkdir -p "$FX" +LAST='2026-01-01T00:00:00Z' +AFTER='2026-01-01T01:00:00Z' +printf '{"merged_at": null, "last_commit": "%s"}' "$LAST" > "$FX/meta.json" +echo '[]' > "$FX/issue.json" + +run_case() { # $1 expected-rc, $2 name, $3 inline-json + printf '%s' "$3" > "$FX/inline.json" + set +e + OUT=$(bash "$CHECKER" --fixture "$FX" o/r 1 2>&1); RC=$? + set -e + if [ "$RC" -eq "$1" ]; then + echo "✓ $2" + else + echo "✗ $2 — rc=$RC, expected $1" + printf '%s\n' "$OUT" | head -4 | sed 's/^/ /' + failed=1 + fi +} + +# R1. THE #1523 SHAPE. +run_case 0 "R1 human 'Fixed in ' reply is not a finding (rc=0)" '[ + {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"infra/x.yml", + "in_reply_to_id":3787072809, + "body":"Fixed in 38774043 — the docstring now records the inversion."}]' + +# R2. Bot reply still counts. +run_case 1 "R2 a BOT reply in a thread still counts (rc=1)" '[ + {"created_at":"'"$AFTER"'","user":{"login":"claude[bot]"},"path":"infra/x.yml", + "in_reply_to_id":3787072809, + "body":"Still broken after the fix: the tuple is empty on 3.12."}]' + +# R3. Human top-level inline still counts. +run_case 1 "R3 a human TOP-LEVEL inline comment still counts (rc=1)" '[ + {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"deploy.yml", + "body":"This breaks rollback — the old image tag is gone by then."}]' + +# R4. Mixed: replies must not mask (or appear beside) the real finding. +printf '%s' '[ + {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"infra/x.yml", + "in_reply_to_id":9,"body":"Fixed in 38774043."}, + {"created_at":"'"$AFTER"'","user":{"login":"claude[bot]"},"path":"docker-compose.yml", + "body":"The compose rationale is now inverted by the trust change."}]' > "$FX/inline.json" +set +e +OUT=$(bash "$CHECKER" --fixture "$FX" o/r 1 2>&1); RC=$? +set -e +if [ "$RC" -eq 1 ] && printf '%s' "$OUT" | grep -q "compose rationale" \ + && ! printf '%s' "$OUT" | grep -q "Fixed in 38774043"; then + echo "✓ R4 mixed thread: bot finding reported, human replies absent from the report" +else + echo "✗ R4 mixed thread — rc=$RC; report must name the bot line and omit the replies:" + printf '%s\n' "$OUT" | head -6 | sed 's/^/ /' + failed=1 +fi + +# R5. Legacy shape: no in_reply_to_id field anywhere (old fixtures, and REST +# payloads omit the field on top-level comments rather than null-ing it). +run_case 1 "R5 field-absent inline comment keeps today's behavior (rc=1)" '[ + {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"deploy.yml", + "body":"Real bug here."}]' + +echo +if [ "$failed" -eq 0 ]; then + echo "PASS — findings reply narrowing" +else + echo "FAIL — findings reply narrowing" +fi +exit "$failed" diff --git a/selftest/test_workflow_guards.py b/selftest/test_workflow_guards.py index 80f0af3..21ced3f 100644 --- a/selftest/test_workflow_guards.py +++ b/selftest/test_workflow_guards.py @@ -37,6 +37,7 @@ # the completeness guard below found both on its first execution. "selftest/test_claude_review_max_turns_type.sh", "selftest/test_codex_verdict_gate.sh", + "selftest/test_findings_reply_narrowing.sh", "selftest/test_pr_files_listing.sh", "selftest/test_prettier_scope_failsafe.sh", "selftest/test_prettier_symlink_filter.sh", From 70eaf8dc9d58e619d62a21f0636e7febfad332f2 Mon Sep 17 00:00:00 2001 From: topcoder1 Date: Fri, 14 Aug 2026 18:48:38 -0700 Subject: [PATCH 2/5] =?UTF-8?q?fix(findings):=20round-2=20review=20fixes?= =?UTF-8?q?=20=E2=80=94=20null-user=20replies=20count;=20explicit-null=20+?= =?UTF-8?q?=20harness=20cleanups?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from #162's Claude Review, all addressed: 1. PRODUCTION: a reply whose .user is null (deleted/suspended account) read login "" via `// ""`, failed the [bot] test, and was DROPPED — an unknown author silently converted into a false negative. The exclusion now drops KNOWN-human replies only: `.in_reply_to_id == null or (.user.login == null) or (.user.login | endswith("[bot]"))`. The report's login column renders "-" for a null login instead of jq's "null". New R2b pins it; mutation (restoring the `// ""` spelling) fails R2b + S1. 2. TEST GAP: R5 covered field-absent only; new R6 covers the explicit `"in_reply_to_id": null` spelling, so a future has()-based rewrite cannot split the two. 3. HARNESS: run_case now documents that it leaves $OUT for content assertions and returns the case verdict; R4 uses it instead of hand-rolling a second harness (rc via R4, report content via R4b). Twin transplanted to dotclaude byte-identically (verified); its regression suite grew the matching two cases. shellcheck clean; all sibling selftests pass. Co-Authored-By: Claude Fable 5 --- .github/scripts/unaddressed-findings.sh | 18 ++++-- selftest/test_findings_reply_narrowing.sh | 77 ++++++++++++++++------- 2 files changed, 65 insertions(+), 30 deletions(-) diff --git a/.github/scripts/unaddressed-findings.sh b/.github/scripts/unaddressed-findings.sh index e98c5b3..459eafe 100755 --- a/.github/scripts/unaddressed-findings.sh +++ b/.github/scripts/unaddressed-findings.sh @@ -568,16 +568,22 @@ check_pr() { # in_reply_to_id, 0/14 genuine bot findings did. BOT replies still count: a # reviewer bot answering a thread with "still broken after the fix" is a # live finding, and keeping them cost zero false positives in that corpus. - # Accepted residual: a human typing a NEW finding into an existing thread is - # skipped — the operator wrote it, so the operator already knows it, and the - # alternative re-flags every resolved thread on every sweep, which is the - # cry-wolf failure this file argues twice over is the more expensive one. + # A reply whose author is UNKNOWN (deleted/suspended account -> .user null) + # also still counts: the exclusion drops only a KNOWN-human reply, because + # reading an absent login as "not a bot" would silently convert an unknown + # author into a false negative — the fail direction this detector must + # never take. Accepted residual: a human typing a NEW finding into an + # existing thread is skipped — the operator wrote it, so the operator + # already knows it, and the alternative re-flags every resolved thread on + # every sweep, which is the cry-wolf failure this file argues twice over is + # the more expensive one. local late_inline late_inline=$(printf '%s' "$inline" | jq -r --arg last "$last" ' [ .[] | select(.created_at > $last) | select(.in_reply_to_id == null - or ((.user.login // "") | endswith("[bot]"))) ] - | .[] | [.created_at, .user.login, (.path // "-"), + or (.user.login == null) + or (.user.login | endswith("[bot]"))) ] + | .[] | [.created_at, (.user.login // "-"), (.path // "-"), ((.body // "") | gsub("\n"; " ") | .[0:90])] | @tsv' 2>/dev/null) # Top-level findings count only from review BOTS. Humans post round-summary diff --git a/selftest/test_findings_reply_narrowing.sh b/selftest/test_findings_reply_narrowing.sh index eb34d6b..171430a 100755 --- a/selftest/test_findings_reply_narrowing.sh +++ b/selftest/test_findings_reply_narrowing.sh @@ -26,10 +26,15 @@ # skipped — the operator wrote it, so the operator already knows it. # # Cases (all through the vendored twin's own --fixture seam): -# S1. structural: the narrowing keys on in_reply_to_id == null OR bot login +# S1. structural: the narrowing keys on in_reply_to_id == null OR unknown +# author OR bot login # R1. THE #1523 SHAPE: human reply "Fixed in " after the last commit # ⇒ rc=0, no unaddressed findings # R2. over-correction guard: a BOT reply after the last commit ⇒ rc=1 +# R2b. a reply whose author is UNKNOWN (.user null — deleted/suspended +# account) ⇒ rc=1. Reading an absent login as "not a bot" would +# silently convert an unknown author into a false negative; the +# exclusion drops KNOWN-human replies only. # R3. over-correction guard: a human TOP-LEVEL inline comment ⇒ rc=1 # (the narrowing must not eat author-agnosticism at top level) # R4. mixed thread: human resolution replies + one genuine late bot @@ -37,6 +42,10 @@ # R5. legacy fixture shape (no in_reply_to_id field at all) ⇒ still # flagged — absent and null must read the same, so old fixtures and # REST payloads that omit the field keep today's behavior +# R6. explicit `"in_reply_to_id": null` (the OTHER spelling of "not a +# reply" — REST omits the field, but a cached/proxied payload may +# null it) ⇒ still flagged; pins that a `has()` rewrite cannot split +# the two spellings # # Run from the repo root: # bash selftest/test_findings_reply_narrowing.sh @@ -47,15 +56,17 @@ failed=0 T=$(mktemp -d) trap 'rm -rf "$T"' EXIT -# S1. Structural pin: the exclusion must be the null-or-bot shape. A rewrite -# to `has("in_reply_to_id") | not` would split absent from null (R5 pins the -# behavior, this names the intended spelling), and dropping the bot escape -# would eat R2 silently on fixtures that never exercise bot replies. +# S1. Structural pin: the exclusion must be the null-or-unknown-or-bot shape. +# A rewrite to `has("in_reply_to_id") | not` would split absent from null (R5 +# and R6 pin the behavior, this names the intended spelling); dropping the bot +# escape would eat R2 silently on fixtures that never exercise bot replies; +# dropping the unknown-author arm would eat R2b the same way. if grep -q 'in_reply_to_id == null' "$CHECKER" \ - && grep -A1 'in_reply_to_id == null' "$CHECKER" | grep -q 'endswith("\[bot\]")'; then - echo "✓ S1 narrowing is 'not a reply OR a bot' (null-check + bot escape present)" + && grep -A2 'in_reply_to_id == null' "$CHECKER" | grep -q '\.user\.login == null' \ + && grep -A2 'in_reply_to_id == null' "$CHECKER" | grep -q 'endswith("\[bot\]")'; then + echo "✓ S1 narrowing is 'not a reply OR unknown author OR a bot' (all three arms present)" else - echo "✗ S1 narrowing shape missing or rewritten — expected in_reply_to_id == null with a [bot] escape" + echo "✗ S1 narrowing shape missing or rewritten — expected in_reply_to_id == null, a login == null arm, and a [bot] escape" failed=1 fi @@ -66,6 +77,9 @@ AFTER='2026-01-01T01:00:00Z' printf '{"merged_at": null, "last_commit": "%s"}' "$LAST" > "$FX/meta.json" echo '[]' > "$FX/issue.json" +# Runs the checker on an inline.json fixture and asserts the exit code. +# Leaves the report in $OUT so callers can additionally assert on its CONTENT +# (R4 does) without hand-rolling a second harness. run_case() { # $1 expected-rc, $2 name, $3 inline-json printf '%s' "$3" > "$FX/inline.json" set +e @@ -73,11 +87,12 @@ run_case() { # $1 expected-rc, $2 name, $3 inline-json set -e if [ "$RC" -eq "$1" ]; then echo "✓ $2" - else - echo "✗ $2 — rc=$RC, expected $1" - printf '%s\n' "$OUT" | head -4 | sed 's/^/ /' - failed=1 + return 0 fi + echo "✗ $2 — rc=$RC, expected $1" + printf '%s\n' "$OUT" | head -4 | sed 's/^/ /' + failed=1 + return 1 } # R1. THE #1523 SHAPE. @@ -92,27 +107,33 @@ run_case 1 "R2 a BOT reply in a thread still counts (rc=1)" '[ "in_reply_to_id":3787072809, "body":"Still broken after the fix: the tuple is empty on 3.12."}]' +# R2b. Unknown-author reply counts (deleted/suspended account -> .user null). +run_case 1 "R2b a reply with a null .user still counts (rc=1)" '[ + {"created_at":"'"$AFTER"'","user":null,"path":"infra/x.yml", + "in_reply_to_id":3787072809, + "body":"The retry loop still deadlocks when the queue is empty."}]' + # R3. Human top-level inline still counts. run_case 1 "R3 a human TOP-LEVEL inline comment still counts (rc=1)" '[ {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"deploy.yml", "body":"This breaks rollback — the old image tag is gone by then."}]' # R4. Mixed: replies must not mask (or appear beside) the real finding. -printf '%s' '[ +# Exit code through the shared harness; the report-content assertions read +# the $OUT it leaves behind. +if run_case 1 "R4 mixed thread flags (rc=1)" '[ {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"infra/x.yml", "in_reply_to_id":9,"body":"Fixed in 38774043."}, {"created_at":"'"$AFTER"'","user":{"login":"claude[bot]"},"path":"docker-compose.yml", - "body":"The compose rationale is now inverted by the trust change."}]' > "$FX/inline.json" -set +e -OUT=$(bash "$CHECKER" --fixture "$FX" o/r 1 2>&1); RC=$? -set -e -if [ "$RC" -eq 1 ] && printf '%s' "$OUT" | grep -q "compose rationale" \ - && ! printf '%s' "$OUT" | grep -q "Fixed in 38774043"; then - echo "✓ R4 mixed thread: bot finding reported, human replies absent from the report" -else - echo "✗ R4 mixed thread — rc=$RC; report must name the bot line and omit the replies:" - printf '%s\n' "$OUT" | head -6 | sed 's/^/ /' - failed=1 + "body":"The compose rationale is now inverted by the trust change."}]'; then + if printf '%s' "$OUT" | grep -q "compose rationale" \ + && ! printf '%s' "$OUT" | grep -q "Fixed in 38774043"; then + echo "✓ R4b report names the bot finding and omits the human replies" + else + echo "✗ R4b report must name the bot line and omit the replies:" + printf '%s\n' "$OUT" | head -6 | sed 's/^/ /' + failed=1 + fi fi # R5. Legacy shape: no in_reply_to_id field anywhere (old fixtures, and REST @@ -121,6 +142,14 @@ run_case 1 "R5 field-absent inline comment keeps today's behavior (rc=1)" '[ {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"deploy.yml", "body":"Real bug here."}]' +# R6. The OTHER spelling of "not a reply": the field present and explicitly +# null. jq reads absent and null identically through `.in_reply_to_id`, and +# this pins that a future `has("in_reply_to_id")` rewrite cannot split them. +run_case 1 "R6 explicit in_reply_to_id:null keeps today's behavior (rc=1)" '[ + {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"deploy.yml", + "in_reply_to_id":null, + "body":"Real bug here."}]' + echo if [ "$failed" -eq 0 ]; then echo "PASS — findings reply narrowing" From 20e0a107118cf82ec18fde1cdd51e8ae3b72da8c Mon Sep 17 00:00:00 2001 From: topcoder1 Date: Fri, 14 Aug 2026 19:23:15 -0700 Subject: [PATCH 3/5] =?UTF-8?q?fix(findings):=20round-3=20review=20fixes?= =?UTF-8?q?=20=E2=80=94=20late=5Fissue=20null-user=20crash=20guard;=20suit?= =?UTF-8?q?e=20no=20longer=20aborts=20mid-run?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of #162's review flagged 5, all addressed: 1. PRODUCTION (the big one, pre-existing): late_issue's `select(.user.login | endswith("[bot]"))` was a jq TYPE ERROR on a null-user comment (deleted/suspended account). With stderr discarded the abort emptied late_issue and silenced EVERY top-level finding on the PR — exit 0 — because one unrelated comment lost its author. Now guarded with `// ""`: an unknown author falls to the path's DEFAULT (top-level counts KNOWN bots only — the mirror of inline, whose default is to count, so its unknown authors do). R7 pins the crash case (mutation: reverting the guard fails R7 with rc=0 — the silenced-findings behavior exactly); R8 pins the direction. The TSV login column renders "-" for null. 2. HARNESS under set -e: run_case's round-2 `return 1` made a bare call abort the suite at the first failing case — my own mutation output was truncated by exactly this and I read it as complete. run_case now always returns 0; the verdict lives in `failed`. Proven: the R7 mutation run prints all 11 cases and still exits FAIL. 3. R4b content assertions now run UNCONDITIONALLY — gating them on R4's rc skipped the only content check exactly when the checker misbehaves. 4. S1 greps each arm independently instead of through a -A line window that broke on any reformat. Twin transplanted to dotclaude byte-identically; its suite grew the two matching cases (55 pass). shellcheck clean. Co-Authored-By: Claude Fable 5 --- .github/scripts/unaddressed-findings.sh | 14 +++- selftest/test_findings_reply_narrowing.sh | 79 ++++++++++++++++------- 2 files changed, 68 insertions(+), 25 deletions(-) diff --git a/.github/scripts/unaddressed-findings.sh b/.github/scripts/unaddressed-findings.sh index 459eafe..b3bc82e 100755 --- a/.github/scripts/unaddressed-findings.sh +++ b/.github/scripts/unaddressed-findings.sh @@ -594,6 +594,16 @@ check_pr() { # code-level finding; the one inline exclusion is human REPLIES, per the # reply rule above. # + # The `// ""` on the login below is load-bearing twice over. A comment + # whose author account was deleted/suspended arrives with `.user: null`, + # and `null | endswith(...)` is a jq TYPE ERROR — with stderr discarded + # that would abort the whole program, empty late_issue, and silence EVERY + # top-level finding on the PR because one unrelated comment lost its + # author (exit 0, the fail direction this detector must never take). With + # the guard, an unknown author simply falls to this path's DEFAULT: it is + # not a known bot, so it does not count — the mirror of the inline path, + # whose default is to count and whose unknown authors therefore do. + # # `canon` is the normalizer described at _BLOCK_PREFIX above: per line, strip # markdown block markers, then reduce any emphasis spelling of `regression:` # and the opening wrapper punctuation after it to one canonical form. Both @@ -630,14 +640,14 @@ check_pr() { --arg blk "$_BLOCK_PREFIX" --arg mark "$_MARKER_CANON" ' def canon: split("\n") | map(sub($blk; "") | sub($mark; "regression: "; "i")) | join("\n"); [ .[] | select(.created_at > $last) - | select(.user.login | endswith("[bot]")) + | select((.user.login // "") | endswith("[bot]")) | select(((.body // "") | canon) as $body | ((.body // "")) as $raw | ($body | test($find; "i")) and ((($raw | test($clean; "i")) | not) or ($raw | test($sev; "i")) or ($raw | test($rmark; "i")))) ] - | .[] | [.created_at, .user.login, "-", + | .[] | [.created_at, (.user.login // "-"), "-", ((.body // "") | gsub("\n"; " ") | .[0:90])] | @tsv' 2>/dev/null) local all diff --git a/selftest/test_findings_reply_narrowing.sh b/selftest/test_findings_reply_narrowing.sh index 171430a..1824bd5 100755 --- a/selftest/test_findings_reply_narrowing.sh +++ b/selftest/test_findings_reply_narrowing.sh @@ -46,6 +46,12 @@ # reply" — REST omits the field, but a cached/proxied payload may # null it) ⇒ still flagged; pins that a `has()` rewrite cannot split # the two spellings +# R7. TOP-LEVEL crash guard: one null-user issue comment must not abort +# the jq program and silence every other top-level finding (with +# stderr discarded, that abort read as "no findings" — exit 0) +# R8. direction pin: a null-user TOP-LEVEL comment falls to that path's +# default (only KNOWN bots count), the mirror of R2b on the inline +# path (default is to count, so unknown authors do) # # Run from the repo root: # bash selftest/test_findings_reply_narrowing.sh @@ -60,10 +66,13 @@ trap 'rm -rf "$T"' EXIT # A rewrite to `has("in_reply_to_id") | not` would split absent from null (R5 # and R6 pin the behavior, this names the intended spelling); dropping the bot # escape would eat R2 silently on fixtures that never exercise bot replies; -# dropping the unknown-author arm would eat R2b the same way. +# dropping the unknown-author arm would eat R2b the same way. Each arm is +# grepped INDEPENDENTLY, not through a -A line window: the window broke as +# soon as the arms spanned a different number of lines, and R1-R6 already +# prove the arms sit in the same select — this only names the spelling. if grep -q 'in_reply_to_id == null' "$CHECKER" \ - && grep -A2 'in_reply_to_id == null' "$CHECKER" | grep -q '\.user\.login == null' \ - && grep -A2 'in_reply_to_id == null' "$CHECKER" | grep -q 'endswith("\[bot\]")'; then + && grep -qF '(.user.login == null)' "$CHECKER" \ + && grep -qF '(.user.login | endswith("[bot]"))' "$CHECKER"; then echo "✓ S1 narrowing is 'not a reply OR unknown author OR a bot' (all three arms present)" else echo "✗ S1 narrowing shape missing or rewritten — expected in_reply_to_id == null, a login == null arm, and a [bot] escape" @@ -77,22 +86,27 @@ AFTER='2026-01-01T01:00:00Z' printf '{"merged_at": null, "last_commit": "%s"}' "$LAST" > "$FX/meta.json" echo '[]' > "$FX/issue.json" -# Runs the checker on an inline.json fixture and asserts the exit code. -# Leaves the report in $OUT so callers can additionally assert on its CONTENT -# (R4 does) without hand-rolling a second harness. -run_case() { # $1 expected-rc, $2 name, $3 inline-json +# Runs the checker on inline.json (+ optional issue.json) fixtures and asserts +# the exit code. ALWAYS returns 0: this file runs under `set -e`, so a nonzero +# return from a bare `run_case` call would abort the suite at the first +# failing case and silently skip every later one — the per-case verdict lives +# in `failed` (the suite's exit), and the report stays in $OUT so callers can +# additionally assert on its CONTENT (R4b does) without hand-rolling a second +# harness. +run_case() { # $1 expected-rc, $2 name, $3 inline-json, $4 issue-json (optional) printf '%s' "$3" > "$FX/inline.json" + printf '%s' "${4:-[]}" > "$FX/issue.json" set +e OUT=$(bash "$CHECKER" --fixture "$FX" o/r 1 2>&1); RC=$? set -e if [ "$RC" -eq "$1" ]; then echo "✓ $2" - return 0 + else + echo "✗ $2 — rc=$RC, expected $1" + printf '%s\n' "$OUT" | head -4 | sed 's/^/ /' + failed=1 fi - echo "✗ $2 — rc=$RC, expected $1" - printf '%s\n' "$OUT" | head -4 | sed 's/^/ /' - failed=1 - return 1 + return 0 } # R1. THE #1523 SHAPE. @@ -120,20 +134,21 @@ run_case 1 "R3 a human TOP-LEVEL inline comment still counts (rc=1)" '[ # R4. Mixed: replies must not mask (or appear beside) the real finding. # Exit code through the shared harness; the report-content assertions read -# the $OUT it leaves behind. -if run_case 1 "R4 mixed thread flags (rc=1)" '[ +# the $OUT it leaves behind and run UNCONDITIONALLY — gating them on R4's +# verdict would silently skip the only content check exactly when the +# checker misbehaves, which is when its report matters most. +run_case 1 "R4 mixed thread flags (rc=1)" '[ {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"infra/x.yml", "in_reply_to_id":9,"body":"Fixed in 38774043."}, {"created_at":"'"$AFTER"'","user":{"login":"claude[bot]"},"path":"docker-compose.yml", - "body":"The compose rationale is now inverted by the trust change."}]'; then - if printf '%s' "$OUT" | grep -q "compose rationale" \ - && ! printf '%s' "$OUT" | grep -q "Fixed in 38774043"; then - echo "✓ R4b report names the bot finding and omits the human replies" - else - echo "✗ R4b report must name the bot line and omit the replies:" - printf '%s\n' "$OUT" | head -6 | sed 's/^/ /' - failed=1 - fi + "body":"The compose rationale is now inverted by the trust change."}]' +if printf '%s' "$OUT" | grep -q "compose rationale" \ + && ! printf '%s' "$OUT" | grep -q "Fixed in 38774043"; then + echo "✓ R4b report names the bot finding and omits the human replies" +else + echo "✗ R4b report must name the bot line and omit the replies:" + printf '%s\n' "$OUT" | head -6 | sed 's/^/ /' + failed=1 fi # R5. Legacy shape: no in_reply_to_id field anywhere (old fixtures, and REST @@ -150,6 +165,24 @@ run_case 1 "R6 explicit in_reply_to_id:null keeps today's behavior (rc=1)" '[ "in_reply_to_id":null, "body":"Real bug here."}]' +# R7. THE CRASH CASE, on the TOP-LEVEL path: one null-user comment anywhere +# in the issue-comment list must not silence the OTHER findings. Pre-guard, +# `null | endswith` was a jq type error, stderr was discarded, late_issue +# came back empty and a genuine bot P1 in the same list vanished — exit 0. +run_case 1 "R7 a null-user issue comment does not silence a bot finding (rc=1)" '[]' '[ + {"created_at":"'"$AFTER"'","user":null, + "body":"thanks, closing the loop"}, + {"created_at":"'"$AFTER"'","user":{"login":"github-actions[bot]"}, + "body":"- [P1] Token check is skippable — auth.py:31"}]' + +# R8. Direction pin for the same guard: a null-user TOP-LEVEL comment falls +# to this path's default — top-level counts only KNOWN bots, so an unknown +# author does not count (the mirror of R2b, where inline's default is to +# count and unknown authors therefore do). +run_case 0 "R8 a null-user issue comment alone is not a finding (rc=0)" '[]' '[ + {"created_at":"'"$AFTER"'","user":null, + "body":"- [P1] Token check is skippable — auth.py:31"}]' + echo if [ "$failed" -eq 0 ]; then echo "PASS — findings reply narrowing" From 2a1ec125617eec4fa6613f9ac3ea2032fd7435a0 Mon Sep 17 00:00:00 2001 From: topcoder1 Date: Fri, 14 Aug 2026 19:49:05 -0700 Subject: [PATCH 4/5] =?UTF-8?q?test(findings):=20bind=20R4's=20report=20to?= =?UTF-8?q?=20R4=5FOUT=20=E2=80=94=20round-3=20review=20fix?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R4b read the $OUT global with no ordering guard, so any run_case call inserted between R4 and R4b would silently redirect the content assertions at someone else's report. Bound immediately after R4; both branches read R4_OUT. Co-Authored-By: Claude Fable 5 --- selftest/test_findings_reply_narrowing.sh | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/selftest/test_findings_reply_narrowing.sh b/selftest/test_findings_reply_narrowing.sh index 1824bd5..8ea205c 100755 --- a/selftest/test_findings_reply_narrowing.sh +++ b/selftest/test_findings_reply_narrowing.sh @@ -136,18 +136,21 @@ run_case 1 "R3 a human TOP-LEVEL inline comment still counts (rc=1)" '[ # Exit code through the shared harness; the report-content assertions read # the $OUT it leaves behind and run UNCONDITIONALLY — gating them on R4's # verdict would silently skip the only content check exactly when the -# checker misbehaves, which is when its report matters most. +# checker misbehaves, which is when its report matters most. $OUT is bound +# to R4_OUT immediately, so a case inserted between R4 and R4b later cannot +# silently redirect the content assertions at someone else's report. run_case 1 "R4 mixed thread flags (rc=1)" '[ {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"infra/x.yml", "in_reply_to_id":9,"body":"Fixed in 38774043."}, {"created_at":"'"$AFTER"'","user":{"login":"claude[bot]"},"path":"docker-compose.yml", "body":"The compose rationale is now inverted by the trust change."}]' -if printf '%s' "$OUT" | grep -q "compose rationale" \ - && ! printf '%s' "$OUT" | grep -q "Fixed in 38774043"; then +R4_OUT=$OUT +if printf '%s' "$R4_OUT" | grep -q "compose rationale" \ + && ! printf '%s' "$R4_OUT" | grep -q "Fixed in 38774043"; then echo "✓ R4b report names the bot finding and omits the human replies" else echo "✗ R4b report must name the bot line and omit the replies:" - printf '%s\n' "$OUT" | head -6 | sed 's/^/ /' + printf '%s\n' "$R4_OUT" | head -6 | sed 's/^/ /' failed=1 fi From 9766cc09acbed506918e49b021e75fab5aea4152 Mon Sep 17 00:00:00 2001 From: topcoder1 Date: Sat, 15 Aug 2026 11:47:45 -0700 Subject: [PATCH 5/5] =?UTF-8?q?fix(findings):=20structural=20null-safety?= =?UTF-8?q?=20in=20the=20narrowing;=20S1=20verifies=20the=20connector=20?= =?UTF-8?q?=E2=80=94=20round-4=20review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from #162's post-rebase review, both addressed: 1. The null test and the bot test were SIBLING arms of the outer `or`, so null-safety was POSITIONAL: it held only because the null arm happened to be evaluated first. A refactor swapping them would send null into `endswith` — a jq type error `2>/dev/null` swallows — emptying late_inline and silently vanishing every inline finding on the PR (exit 0). Now ONE pipeline on the login: `(.user.login | . == null or endswith("[bot]"))`. The short-circuit is structural with respect to the OUTER arms. Measured while proving it: swapping the two tests INSIDE that pipeline (`endswith or . == null`) still sends null into endswith first — the inner order remains load-bearing — so S1 pins the exact inner spelling too, and R2b catches it at runtime (both fail under that mutation). 2. S1 checked arm PRESENCE but not the CONNECTOR: with `or`→`and`, no comment can satisfy "is a reply" and "is not a reply", late_inline is always empty, and the sweep passes every PR — S1 still printed ✓. Now S1 requires the `or` adjacent to the reply test and rejects any `and` on that line. Mutation-proven: or→and fails S1 + R2/R2b/R3/R5/R6; reverting to sibling arms fails S1; inner swap fails S1 + R2b. shellcheck clean. Co-Authored-By: Claude Fable 5 --- .github/scripts/unaddressed-findings.sh | 12 +++++++-- selftest/test_findings_reply_narrowing.sh | 30 ++++++++++++++--------- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/.github/scripts/unaddressed-findings.sh b/.github/scripts/unaddressed-findings.sh index b3bc82e..7d7274b 100755 --- a/.github/scripts/unaddressed-findings.sh +++ b/.github/scripts/unaddressed-findings.sh @@ -577,12 +577,20 @@ check_pr() { # already knows it, and the alternative re-flags every resolved thread on # every sweep, which is the cry-wolf failure this file argues twice over is # the more expensive one. + # + # The null test and the bot test are ONE pipeline expression on the login + # (`. == null or endswith`), not two sibling arms of the outer `or`. As + # siblings the null-safety was POSITIONAL — it held only because the null + # arm happened to be evaluated first, and swapping them in a refactor would + # send null into `endswith`, a jq TYPE ERROR that `2>/dev/null` swallows: + # late_inline comes back empty and every inline finding on the PR silently + # vanishes (exit 0). Inside one pipeline the short-circuit is structural: + # `. == null` guards `endswith` no matter how the outer arms are arranged. local late_inline late_inline=$(printf '%s' "$inline" | jq -r --arg last "$last" ' [ .[] | select(.created_at > $last) | select(.in_reply_to_id == null - or (.user.login == null) - or (.user.login | endswith("[bot]"))) ] + or (.user.login | . == null or endswith("[bot]"))) ] | .[] | [.created_at, (.user.login // "-"), (.path // "-"), ((.body // "") | gsub("\n"; " ") | .[0:90])] | @tsv' 2>/dev/null) diff --git a/selftest/test_findings_reply_narrowing.sh b/selftest/test_findings_reply_narrowing.sh index 8ea205c..1228b2e 100755 --- a/selftest/test_findings_reply_narrowing.sh +++ b/selftest/test_findings_reply_narrowing.sh @@ -26,8 +26,9 @@ # skipped — the operator wrote it, so the operator already knows it. # # Cases (all through the vendored twin's own --fixture seam): -# S1. structural: the narrowing keys on in_reply_to_id == null OR unknown -# author OR bot login +# S1. structural: the narrowing keys on in_reply_to_id == null OR (one +# login pipeline: null OR bot) — the connector is `or`, and the null +# test guards `endswith` structurally, not by arm order # R1. THE #1523 SHAPE: human reply "Fixed in " after the last commit # ⇒ rc=0, no unaddressed findings # R2. over-correction guard: a BOT reply after the last commit ⇒ rc=1 @@ -62,20 +63,27 @@ failed=0 T=$(mktemp -d) trap 'rm -rf "$T"' EXIT -# S1. Structural pin: the exclusion must be the null-or-unknown-or-bot shape. +# S1. Structural pin: the exclusion must be the null-or-(unknown-or-bot) shape. # A rewrite to `has("in_reply_to_id") | not` would split absent from null (R5 # and R6 pin the behavior, this names the intended spelling); dropping the bot # escape would eat R2 silently on fixtures that never exercise bot replies; -# dropping the unknown-author arm would eat R2b the same way. Each arm is -# grepped INDEPENDENTLY, not through a -A line window: the window broke as -# soon as the arms spanned a different number of lines, and R1-R6 already -# prove the arms sit in the same select — this only names the spelling. +# dropping the unknown-author test would eat R2b the same way. Two more things +# it pins, both review catches: the null test and the bot test must be ONE +# pipeline on the login (`. == null or endswith`) so null-safety is structural +# rather than dependent on which sibling arm is evaluated first; and the +# connector between the reply test and the login pipeline must be `or` — with +# `and`, no comment can satisfy both (a reply is not a non-reply), late_inline +# is always empty, and the sweep passes every PR. R3/R5 catch that at runtime; +# S1 is the structural claim, so it must not certify a shape it did not check. +# Each piece is grepped INDEPENDENTLY, not through a -A line window: the +# window broke as soon as the arms spanned a different number of lines. if grep -q 'in_reply_to_id == null' "$CHECKER" \ - && grep -qF '(.user.login == null)' "$CHECKER" \ - && grep -qF '(.user.login | endswith("[bot]"))' "$CHECKER"; then - echo "✓ S1 narrowing is 'not a reply OR unknown author OR a bot' (all three arms present)" + && grep -qF '(.user.login | . == null or endswith("[bot]"))' "$CHECKER" \ + && grep -A1 'in_reply_to_id == null$' "$CHECKER" | grep -q '^[[:space:]]*or (\.user\.login |' \ + && ! grep -q 'in_reply_to_id == null.*\band\b' "$CHECKER"; then + echo "✓ S1 narrowing is 'not a reply OR (unknown author OR bot)' — one login pipeline, joined by or" else - echo "✗ S1 narrowing shape missing or rewritten — expected in_reply_to_id == null, a login == null arm, and a [bot] escape" + echo "✗ S1 narrowing shape missing or rewritten — expected 'in_reply_to_id == null' joined by 'or' to ONE login pipeline '(.user.login | . == null or endswith(\"[bot]\"))'" failed=1 fi