fix(findings): stop counting human resolution replies as findings

[topcoder1]

Manual merge (ci-workflows policy). Stacked on #161 — based on its branch so the diff shows only this change; GitHub retargets to main when #161 merges. Merge order: #161 first, then this.

Paired PR: the detector is a vendored twin — the byte-identical body ships to topcoder1/dotclaude (templates/ci-workflows/scripts/bb-unaddressed-findings.sh) in dotclaude's fix/findings-reply-narrowing PR. Merge both or neither; identity below the header verified both directions.

HUMAN_READABLE_SUMMARY: The findings detector flagged the author's own "Fixed in <sha>" inline replies as unaddressed findings, because a resolution reply always post-dates the commit it cites. It now skips inline comments that are both threaded replies and human-authored. Human top-level comments and bot replies still count.

---

The defect, 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 they cited. Across the PR: 5/5 false positives carried in_reply_to_id; 0/14 genuine bot findings did. A resolution reply necessarily post-dates the commit it cites — author pushes, then replies citing the push — so every resolution round trips the newer-than-last-commit signal, forever.

This reaches the automerge gate too: the 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.

The narrowing

select(.in_reply_to_id == null or ((.user.login // "") | endswith("[bot]")))

Kept deliberately: human top-level inline comments (author-agnosticism — a human's own comment on the diff is a deliberate finding) and bot replies ("still broken after the fix" is a live finding; keeping them cost zero false positives in the corpus). Accepted residual: a human typing a new finding into an existing thread is skipped — the operator wrote it, so the operator knows it.

Tests — proven to bite

selftest/test_findings_reply_narrowing.sh (6 checks), CI-wired via _SHELL_SELFTESTS. Three mutations, each caught by its intended cases:

Mutation	Result
remove the narrowing (pre-fix state)	R1 + R4 fail — the #1523 shape flags again
drop the bot escape (exclude all replies)	R2 fails
over-correct to bots-only inline	R3 + R5 fail

R5 pins that a field-absent comment (legacy fixtures; REST omits the field on top-level comments) reads the same as null. Full wrapper suite: 46 pass. shellcheck clean on both files. dotclaude side: 3 new regression cases, mutation-proven there too (the bot-reply guard survives the removal mutation by design — it pins the over-correction direction).

🤖 Generated with Claude Code

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[github-actions]

Codex review

Coverage exercises the narrowed reply behavior, new test-state mutations are asserted via exit status, and function comments remain consistent.
Coverage exercises the narrowed reply behavior, new test-state mutations are asserted via exit status, and function comments remain consistent.

_{Model: gpt-5.6-sol · CLI: codex-cli 0.147.0 · reasoning: low}

[github-actions]

Coverage Floor — mode: enforce

metric	value
measured	100.0%
floor (current)	99.0%
target	100.0%
last bumped	2026-05-12

[claude]

Bug: null .user on a bot reply causes a false negative

When GitHub returns .user: null for a deleted or suspended account, a comment that is a threaded reply (in_reply_to_id non-null) silently fails both branches of the or and is excluded as if it were a human reply:

1. .in_reply_to_id == null → false (it is non-null)
2. (.user.login // "") → jq null-propagates .user.login to null, then null // "" = "", and "" | endswith("[bot]") → false

Result: a genuine bot finding posted as a threaded reply ("still broken after the fix") is dropped with no error. The 2>/dev/null ensures jq emits no diagnostic.

The corpus data (0/14 real bot findings were replies) makes this rare, but a bot reply that lands while the account is being rotated/suspended is exactly the scenario the bot-reply keep-rule exists for.

Suggested change

	or ((.user.login // "") | endswith("[bot]"))) ]
	| select(.in_reply_to_id == null
	or (((.user // {}).login // "") | endswith("[bot]"))) ]

(.user // {}).login avoids the null-propagation path while keeping the // "" fallback for a null login field on an existing user object.

[topcoder1]

Fixed in 583241d — null-user replies now count (drops KNOWN-human replies only, new R2b + S1 arm), explicit-null pinned as R6, and R4 rides the shared run_case harness via its exposed $OUT (R4/R4b). Twin transplanted to dotclaude byte-identically.

[claude]

Test gap: R5 only tests field-absent, never explicit "in_reply_to_id": null

R5's stated purpose is proving "absent and null must read the same." But both R3 and R5 use fixtures where the key is entirely absent — neither tests a payload with "in_reply_to_id": null (explicit JSON null).

The distinction matters: == null treats absent and explicit null identically, but a future rewrite to has("in_reply_to_id") | not would handle them differently — has() returns true when the key exists even with a null value, so an explicit-null comment would be excluded as a reply instead of flagged as a finding. No R* fixture would catch that regression; S1's source grep only guards the literal == null text.

GitHub's REST API omits in_reply_to_id for top-level comments rather than setting it to null, so the immediate risk is low — but the intent documented in the comment is to cover both spellings.

Add a case:

run_case 1 "R5b explicit in_reply_to_id:null is not a reply (rc=1)" '[
  {"created_at":"'"$AFTER"'","user":{"login":"topcoder1"},"path":"deploy.yml",
   "in_reply_to_id":null,
   "body":"Real bug here — explicit null."}]'

[topcoder1]

Fixed in 583241d — null-user replies now count (drops KNOWN-human replies only, new R2b + S1 arm), explicit-null pinned as R6, and R4 rides the shared run_case harness via its exposed $OUT (R4/R4b). Twin transplanted to dotclaude byte-identically.

[claude]

Duplication: R4 hand-rolls the run_case harness because run_case doesn't expose $OUT

R4 needs to grep the output body (to verify it names the bot line and omits the human reply), so it re-implements the set +e / OUT=$(bash ...) / RC=$? / set -e block that run_case already encapsulates. If the checker path, the --fixture signature, or the 2>&1 redirect ever changes, R4's copy must be updated independently — with no cross-reference to point the editor there.

run_case could accept an optional fourth argument — a body-grep pattern — and do its own grep -q check internally, or it could expose $OUT via a nameref or a global. Either approach would let R4 be expressed as a single run_case call with an extra assertion, keeping all five cases uniform:

run_case_body() {  # $1 expected-rc, $2 name, $3 inline-json, $4 must-match, $5 must-not-match
  printf '%s' "$3" > "$FX/inline.json"
  set +e; OUT=$(bash "$CHECKER" --fixture "$FX" o/r 1 2>&1); RC=$?; set -e
  local ok=1
  [ "$RC" -eq "$1" ] || ok=0
  [ -z "${4:-}" ] || printf '%s' "$OUT" | grep -q "$4" || ok=0
  [ -z "${5:-}" ] || { printf '%s' "$OUT" | grep -q "$5" && ok=0; }
  if [ "$ok" -eq 1 ]; then echo "✓ $2"; else
    echo "✗ $2 — rc=$RC, expected $1"; printf '%s\n' "$OUT" | head -6 | sed 's/^/    /'; failed=1; fi
}

[topcoder1]

Fixed in 583241d — null-user replies now count (drops KNOWN-human replies only, new R2b + S1 arm), explicit-null pinned as R6, and R4 rides the shared run_case harness via its exposed $OUT (R4/R4b). Twin transplanted to dotclaude byte-identically.

[claude]

Flagged 3 issues inline — 1 production null-user bug in unaddressed-findings.sh, 1 test gap (explicit-null fixture missing), and 1 test-harness duplication in R4.

[github-actions]

Codex review

The changed behavior is covered, introduces no production state mutation, and matches the updated function comments.
The changed behavior is covered, introduces no production state mutation, and matches the updated function comments.

_{Model: gpt-5.6-sol · CLI: codex-cli 0.147.0 · reasoning: low}

[claude]

Bug: set -euo pipefail + bare run_case calls — test exits on first failure, all subsequent cases skipped

run_case returns 1 when the checker's actual exit code doesn't match the expected one. This call (and R2, R2b, R3, R5, R6 at lines 105, 111, 117, 141, 148) is a bare statement under set -e, so bash treats the non-zero return as a fatal error and exits immediately. The remaining test cases are never run, failed stops accumulating, and the "FAIL — findings reply narrowing" summary at line 157 never prints. CI gets the right signal (non-zero exit) but the diagnostic output is cut short.

R4 at line 124 is correctly guarded with if run_case ...; then and doesn't share this problem.

Fix — use || to absorb the return without exiting:

Suggested change

	run_case 0 "R1 human 'Fixed in <sha>' reply is not a finding (rc=0)" '[
	run_case 0 "R1 human 'Fixed in <sha>' 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."}]' || true

Apply the same || true (or || :) pattern to the R2, R2b, R3, R5, R6 calls. The failed=1 inside run_case accumulates the count before returning, so the final exit "$failed" still reflects all failures.

[topcoder1]

Fixed in c8ef8ee — run_case always returns 0 under set -e (the R7 mutation run now prints all 11 cases and still exits FAIL), R4b runs unconditionally against $OUT, and S1 greps each arm independently with no -A window. The two top-level findings (late_issue null guard) are also in c8ef8ee: guarded with // "", crash pinned by R7 (mutation-proven), direction pinned by R8; twin transplanted byte-identically.

[claude]

Bug: R4b content assertions silently skipped when R4 fails

R4b — the only test that checks report content (bot finding appears, human reply does not) — is nested inside the same if run_case 1 ...; then block that gates on R4's exit code. If a bug causes check_pr to exit with rc=2 instead of rc=1 (e.g., a fixture error or an assertion in the checker), run_case returns 1, the outer if is false, and the two grep -q assertions at lines 129-130 are silently skipped. failed is not incremented for that gap, and the test reports PASS for a regression that lets human replies appear in the report.

Consider promoting R4b to an unconditional check, or at minimum incrementing failed when R4 itself fails in a way that prevents R4b from running:

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."}]'; 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
else
  echo "✗ R4b skipped — R4 itself failed; cannot verify report content"
  failed=1
fi

[topcoder1]

Fixed in c8ef8ee — run_case always returns 0 under set -e (the R7 mutation run now prints all 11 cases and still exits FAIL), R4b runs unconditionally against $OUT, and S1 greps each arm independently with no -A window. The two top-level findings (late_issue null guard) are also in c8ef8ee: guarded with // "", crash pinned by R7 (mutation-proven), direction pinned by R8; twin transplanted byte-identically.

[claude]

PLAUSIBLE: S1's grep -A2 window is fragile — reformatting or a 4th condition triggers a false alarm

The three or arms in the checker's jq filter currently occupy exactly 3 lines, so grep -A2 (which prints the matching line plus 2 after it) just barely captures all three. If a formatter inserts a blank line between the second and third arms, or if a fourth condition is added on its own line, endswith("[bot]") falls outside the window and S1 fails — not because the logic is wrong, but because the window is too narrow.

Since R2 and R2b already exercise the bot-reply and null-user paths behaviorally, S1's value is mainly as a named-shape pin. Consider anchoring it on the whole block with a wider context or a single grep -c count, or simply drop S1 and rely on R2/R2b to catch arm-removal regressions.

Alternative that avoids the line-count fragility:

if grep -q 'in_reply_to_id == null' "$CHECKER" \
   && grep -q '\.user\.login == null' "$CHECKER" \
   && grep -q 'endswith("\[bot\]")' "$CHECKER"; then

(Three independent greps — order and co-location are not checked, but R2/R2b behaviorally cover those invariants.)

[topcoder1]

Fixed in c8ef8ee — run_case always returns 0 under set -e (the R7 mutation run now prints all 11 cases and still exits FAIL), R4b runs unconditionally against $OUT, and S1 greps each arm independently with no -A window. The two top-level findings (late_issue null guard) are also in c8ef8ee: guarded with // "", crash pinned by R7 (mutation-proven), direction pinned by R8; twin transplanted byte-identically.

[claude]

Flagged 5 issues (3 inline) — 2 additional findings on unchanged lines couldn't be posted inline: late_issue in unaddressed-findings.sh lacks the same null guard on .user.login that this PR added to late_inline (lines 633/640), which can cause a type error that silences all findings and exits 0.

[github-actions]

Codex review

Relevant tests pass; new test-state mutations are asserted through the suite exit status, and check_pr remains consistent with its comments.
Relevant tests pass; new test-state mutations are asserted through the suite exit status, and check_pr remains consistent with its comments.

_{Model: gpt-5.6-sol · CLI: codex-cli 0.147.0 · reasoning: low}