feat(loop): comment-fix — automated bounded fix attempts for unresolved PR review threads (issue #96, part 2)

.claude/commands/pr-loop.md

@@ -23,12 +23,13 @@ Prompt to use (the tick logic, with adaptive STEP 0):
 
 > Run one tick of the autonomous PR loop. ALL `gh` interaction (yours and every agent's) MUST run as the bot via `.claude/scripts/bot-gh.sh` — never bare `gh`; only `git` commits/pushes stay as the owner.
 >
-> STEP 0 — run the tick. Invoke, as a REAL bash tool call, exactly: `bash ${CLAUDE_PLUGIN_ROOT:-.claude}/scripts/loop-tick.sh`. This one script runs census, then notify-poll.sh, merge-ready.sh, pr-feedback.sh, and pr-ci-fix.sh, IN ORDER, with their full output, and ends with exactly one machine-readable verdict line as the LAST line of output: `action=none`, `action=advance issue=N`, `action=feedback pr=N`, or `action=ci-fix pr=N`. That verdict line is the SOLE source of truth for what to do next: never hand-count open PRs, planned issues, feedback PRs, or CI-red PRs yourself, and never skip this invocation because the tick "looks quiet" — it must run, and its output must be read, on every single tick with no exceptions.
+> STEP 0 — run the tick. Invoke, as a REAL bash tool call, exactly: `bash ${CLAUDE_PLUGIN_ROOT:-.claude}/scripts/loop-tick.sh`. This one script runs census, then notify-poll.sh, merge-ready.sh, pr-feedback.sh, pr-comment-fix.sh, and pr-ci-fix.sh, IN ORDER, with their full output, and ends with exactly one machine-readable verdict line as the LAST line of output: `action=none`, `action=advance issue=N`, `action=feedback pr=N`, `action=comment-fix pr=N`, or `action=ci-fix pr=N`. That verdict line is the SOLE source of truth for what to do next: never hand-count open PRs, planned issues, feedback PRs, comment-fix PRs, or CI-red PRs yourself, and never skip this invocation because the tick "looks quiet" — it must run, and its output must be read, on every single tick with no exceptions.
 >
-> CADENCE: the script's `=== 1/5 loop-census.sh ===` section includes a line `cadence=FAST|WATCH|IDLE cron=<expr>` — this is the desired cadence; consume it as-is, do NOT re-derive it from counts. If this cron job's current schedule differs from that `cron=<expr>`, CronDelete this job and CronCreate a durable replacement with this SAME prompt at the desired schedule.
+> CADENCE: the script's `=== 1/6 loop-census.sh ===` section includes a line `cadence=FAST|WATCH|IDLE cron=<expr>` — this is the desired cadence; consume it as-is, do NOT re-derive it from counts. If this cron job's current schedule differs from that `cron=<expr>`, CronDelete this job and CronCreate a durable replacement with this SAME prompt at the desired schedule.
 >
-> Then obey the verdict line (the tick already ran poll/merge/feedback-detection/ci-fix-detection above — do not re-run those scripts). Precedence when more than one is ready: `feedback` > `ci-fix` > `advance` (a PR with BOTH unaddressed feedback and failing CI is handled as feedback, never ci-fix):
+> Then obey the verdict line (the tick already ran poll/merge/feedback-detection/comment-fix-detection/ci-fix-detection above — do not re-run those scripts). Precedence when more than one is ready: `feedback` > `comment-fix` > `ci-fix` > `advance` (a PR with unaddressed feedback outranks everything; a PR with an unresolved qualifying review-comment thread outranks a CI fix or a fresh advance):
 > - `action=feedback pr=N` → address PR N's feedback: run orchestrator → worktree implementer → reviewer lenses (per .claude/gates.json) on the SAME branch, push to update the PR in place, and post the `<!-- claude-addressed -->` marker comment via bot-gh.sh. Do NOT merge.
+> - `action=comment-fix pr=N` → address PR N's unresolved review-comment thread(s): label the PR `claude-comment-fixing` via bot-gh.sh first (in-flight guard), then run orchestrator → worktree implementer on the SAME branch (checkout the PR's existing branch, do NOT create a new one) → reviewer lenses (per .claude/gates.json), push to update the PR in place. For each thread you actually addressed, resolve it on GitHub and post a bot comment containing `<!-- claude-comment-addressed:<thread-id>:<attempt> -->` for each (the real thread id and attempt number from the `5/6 pr-comment-fix.sh` section above) so pr-comment-fix.sh's cursor recognizes it as addressed. Do NOT merge, and do NOT force-push.
 > - `action=ci-fix pr=N` → fix PR N's failing CI: label the PR `claude-ci-fixing` via bot-gh.sh first (in-flight guard), then run orchestrator → worktree implementer on the SAME branch (checkout the PR's existing branch, do NOT create a new one) → reviewer lenses (per .claude/gates.json) to fix the failure, push to update the PR in place. After pushing, query the PR's current head SHA (`bot-gh.sh pr view N --json headRefOid`) and post a bot comment containing exactly `<!-- claude-ci-addressed:<head-sha> -->` (the real SHA substituted in) so pr-ci-fix.sh's cursor recognizes this head as already addressed. Do NOT merge, and do NOT force-push.
 > - `action=advance issue=N` → advance issue N through the orchestrator (scope → worktree implementer → gate.sh gates → reviewer lenses → bot PR). One issue in flight at a time. `backlog` issues are owner-unapproved: never pick them, and if you file an issue yourself, label it `backlog` — NEVER `planned` (that label is the owner's formal approval and is assigned by the owner alone; see docs/USAGE.md → "Autonomous loop & the issue queue").
 > - `action=none` → reply exactly one line: "No actionable activity." This is the ONLY path to that phrase — never reply it without loop-tick.sh having actually been invoked (and its output read) earlier in this same turn.

.claude/gates.json

@@ -73,6 +73,9 @@
   "_notify_note": "issue #99 — needs-human push-notification seam, read by .claude/scripts/notify.sh (GATES_FILE-aware; same empty-means-skip convention as `gates`). Empty string = disabled (offline/CI-safe no-op) — the default; fill in a command to enable push notifications when the autonomous loop blocks on YOU (PR ready for review, CHANGES_REQUESTED addressed and awaiting re-review, attempt-budget/stall escalation, ...). Contract: severity/title/body-line reach the command BOTH as positional args ($1/$2/$3) and as NOTIFY_SEVERITY/NOTIFY_TITLE/NOTIFY_BODY env vars; throttled to one notification per (kind,target) per window (default 1800s — see notify.sh). Example commands (uncomment/adapt ONE):\n  ntfy:        curl -s -d \"$NOTIFY_BODY\" -H \"Title: $NOTIFY_TITLE\" -H \"Priority: $NOTIFY_SEVERITY\" ntfy.sh/<your-topic>\n  notify-send: notify-send \"$NOTIFY_TITLE\" \"$NOTIFY_BODY\"\n  webhook:     curl -s -X POST -H 'Content-Type: application/json' -d \"{\\\"severity\\\":\\\"$NOTIFY_SEVERITY\\\",\\\"title\\\":\\\"$NOTIFY_TITLE\\\",\\\"body\\\":\\\"$NOTIFY_BODY\\\"}\" https://example.invalid/hook",
   "notify": "",
 
+  "_commentFix_note": "issue #96 (part 2) — bot-commenter allowlist for the comment-fix reaction, read by .claude/scripts/pr-comment-fix.sh. commentFix.botAllowlist: an array of bot GitHub logins whose inline PR review comments qualify a thread for automated comment-fix, in ADDITION to the repo owner (who always qualifies, regardless of this list). Empty array (the default, BOTH shipped adapters) = disabled — only the owner's own review threads are ever comment-fixed. Fill in bot logins (e.g. a linter/reviewer bot account) to let their unresolved review threads trigger the same automated fix-attempt path.",
+  "commentFix": { "botAllowlist": [] },
+
   "_plan_note": "issue #100 — optional spec/plan gate before implementation starts. plan.gate: off (default; today's single-pass advance behavior, unchanged) | label (gate only planned issues that ALSO carry a plan-first label) | always (gate every planned+module:* issue). Unknown/missing value falls back to off. When gated, the loop posts ONE structured plan comment on the issue (marked `<!-- plan-gate:plan -->`) and labels it plan-review + needs-human instead of implementing; the owner reviews the comment and either replaces plan-review with plan-approved (approve — the next tick implements with the plan injected into the implementer and reviewers as authoritative scope) or removes plan-review (request changes — the loop re-plans). Owner-only approval is NOT identity-enforced — same limitation as the `planned` label itself (see docs/USAGE.md). Read by loop-census.sh/loop-tick.sh/loop-event.sh.",
   "plan": { "gate": "off" }
 }

.claude/scripts/loop-ceilings.test.sh

@@ -41,9 +41,11 @@ check() {
 # all, matching loop-tick.test.sh's own "gh must never be invoked" contract
 # for scenarios that expect zero gh side effects). $5=fake_cifix (TSV body,
 # issue #96 -- defaults to empty, i.e. no ci-fix candidates, so every existing
-# 3/4-arg call site keeps working unchanged).
+# 3/4-arg call site keeps working unchanged). $6=fake_commentfix (TSV body,
+# issue #96 part 2 -- defaults to empty, i.e. no comment-fix candidates, so
+# every existing 3/4/5-arg call site keeps working unchanged).
 new_fixture() {
-  local name="$1" fake_census="$2" fake_feedback="$3" with_gh="${4:-0}" fake_cifix="${5:-}"
+  local name="$1" fake_census="$2" fake_feedback="$3" with_gh="${4:-0}" fake_cifix="${5:-}" fake_commentfix="${6:-}"
   local dir="$work/$name/.claude/scripts"
   mkdir -p "$dir" "$work/$name/.claude/state" 2>/dev/null
   rm -rf "$work/$name/.claude/state"   # loop-tick.sh must mkdir -p it itself
@@ -81,6 +83,12 @@ EOF
 cat <<'CIFIX'
 $fake_cifix
 CIFIX
+EOF
+  cat > "$dir/pr-comment-fix.sh" <<EOF
+#!/usr/bin/env bash
+cat <<'COMMENTFIX'
+$fake_commentfix
+COMMENTFIX
 EOF
   chmod +x "$dir"/*.sh
 

.claude/scripts/loop-census.sh

@@ -10,6 +10,11 @@
 #                               head, not already a feedback candidate, not
 #                               already addressed for that head (pr-ci-fix.sh,
 #                               issue #96)
+#   comment_fix_prs=N            bot PRs with an unresolved, qualifying (owner
+#                               or allowlisted-bot) review-comment thread, not
+#                               already a feedback candidate, not already
+#                               addressed for its current state
+#                               (pr-comment-fix.sh, issue #96 part 2)
 #   planned_issues=N            open issues labelled `planned` AND one of the
 #                               adapter's module:* labels, one detail line each:
 #     issue=<n> branch=<feat/issue-n-* or none> title=<title>
@@ -310,6 +315,9 @@ echo "feedback_prs=$feedback_prs"
 ci_fix_prs=$(bash "$script_dir/pr-ci-fix.sh" "$repo" | grep -c . || true)
 echo "ci_fix_prs=$ci_fix_prs"
 
+comment_fix_prs=$(bash "$script_dir/pr-comment-fix.sh" "$repo" | grep -c . || true)
+echo "comment_fix_prs=$comment_fix_prs"
+
 # Open `planned` issues carrying any of the adapter's module labels, ascending.
 planned=$(gh issue list -R "$repo" --state open --label planned --json number,title,labels \
   --jq '.[] | [.number, ([.labels[].name]|join(",")), .title] | @tsv' | sort -n)
@@ -461,7 +469,7 @@ fi
 echo "advance_ready=$advance_ready"
 
 # Desired cadence per the loop policy: FAST only when the loop can ACT now.
-if [ "$feedback_prs" -ge 1 ] || [ "$ci_fix_prs" -ge 1 ] || { [ "$open_prs" -eq 0 ] && [ "$planned_count" -ge 1 ]; }; then
+if [ "$feedback_prs" -ge 1 ] || [ "$comment_fix_prs" -ge 1 ] || [ "$ci_fix_prs" -ge 1 ] || { [ "$open_prs" -eq 0 ] && [ "$planned_count" -ge 1 ]; }; then
   echo 'cadence=FAST cron=* * * * *'
 elif [ "$open_prs" -ge 1 ]; then
   echo 'cadence=WATCH cron=*/5 * * * *'

.claude/scripts/loop-census.test.sh

@@ -111,6 +111,10 @@ cat > "$scripts_dir/pr-ci-fix.sh" <<'EOF'
 #!/usr/bin/env bash
 exit 0
 EOF
+cat > "$scripts_dir/pr-comment-fix.sh" <<'EOF'
+#!/usr/bin/env bash
+exit 0
+EOF
 
 # Fake bot-gh.sh: no network, no real `gh` — dispatches on the subcommand and
 # a `--json` marker to canned, fixture-appropriate output.
@@ -204,6 +208,7 @@ check "exactly one in_flight line total (only issue 42 qualifies)" bash -c '[ "$
 check "planned_issues=4 counted" bash -c 'printf "%s\n" "$1" | grep -qx "planned_issues=4"' _ "$out"
 check "issue=42 branch line shows the origin-prefixed remote-tracking name" bash -c 'printf "%s\n" "$1" | grep -q "^issue=42 branch=origin/feat/issue-42-y"' _ "$out"
 check "ci_fix_prs=0 counted (no-op pr-ci-fix.sh stub, issue #96)" bash -c 'printf "%s\n" "$1" | grep -qx "ci_fix_prs=0"' _ "$out"
+check "comment_fix_prs=0 counted (no-op pr-comment-fix.sh stub, issue #96 part 2)" bash -c 'printf "%s\n" "$1" | grep -qx "comment_fix_prs=0"' _ "$out"
 
 # ---------------------------------------------------------------------------
 # ci_fix_prs (issue #96): loop-census.sh must surface pr-ci-fix.sh's own
@@ -229,6 +234,19 @@ git -C "$fixture" -c [email protected] -c user.name=t commit -q -m "swap in ci_f
 outCiFix="$(env -u GATES_FILE bash "$scripts_dir/loop-census.sh" "acme/repo")"
 check "ci_fix_prs=2 counted when pr-ci-fix.sh reports two candidates" bash -c 'printf "%s\n" "$1" | grep -qx "ci_fix_prs=2"' _ "$outCiFix"
 
+# ---------------------------------------------------------------------------
+# comment_fix_prs (issue #96 part 2): same wrapping contract as ci_fix_prs
+# above, exercised against pr-comment-fix.sh instead.
+# ---------------------------------------------------------------------------
+cat > "$scripts_dir/pr-comment-fix.sh" <<'EOF'
+#!/usr/bin/env bash
+printf '20\tfeat/issue-20-a\tTABC:1\tsha20\n'
+EOF
+git -C "$fixture" add .claude/scripts/pr-comment-fix.sh
+git -C "$fixture" -c [email protected] -c user.name=t commit -q -m "swap in comment_fix_prs stub (test fixture)"
+outCommentFix="$(env -u GATES_FILE bash "$scripts_dir/loop-census.sh" "acme/repo")"
+check "comment_fix_prs=1 counted when pr-comment-fix.sh reports one candidate" bash -c 'printf "%s\n" "$1" | grep -qx "comment_fix_prs=1"' _ "$outCommentFix"
+
 # ---------------------------------------------------------------------------
 # driver_unit_active guard (issue #119 post-review finding #5): loop-census.sh
 # must never report advance_ready for an issue whose transient driver unit
@@ -263,6 +281,10 @@ EOF
   cat > "$scripts/pr-ci-fix.sh" <<'EOF'
 #!/usr/bin/env bash
 exit 0
+EOF
+  cat > "$scripts/pr-comment-fix.sh" <<'EOF'
+#!/usr/bin/env bash
+exit 0
 EOF
   cat > "$scripts/bot-gh.sh" <<'EOF'
 #!/usr/bin/env bash
@@ -354,7 +376,11 @@ EOF
 #!/usr/bin/env bash
 exit 0
 EOF
-  chmod +x "$scripts/pr-feedback.sh" "$scripts/pr-ci-fix.sh" "$scripts/cockpit.sh" "$scripts/loop-census.sh"
+  cat > "$scripts/pr-comment-fix.sh" <<'EOF'
+#!/usr/bin/env bash
+exit 0
+EOF
+  chmod +x "$scripts/pr-feedback.sh" "$scripts/pr-ci-fix.sh" "$scripts/pr-comment-fix.sh" "$scripts/cockpit.sh" "$scripts/loop-census.sh"
   git -C "$dir" init -q -b main
   git -C "$dir" -c [email protected] -c user.name=t commit -q --allow-empty -m init
 }
@@ -598,6 +624,10 @@ cat > "$scriptsStall/pr-ci-fix.sh" <<'EOF'
 #!/usr/bin/env bash
 exit 0
 EOF
+cat > "$scriptsStall/pr-comment-fix.sh" <<'EOF'
+#!/usr/bin/env bash
+exit 0
+EOF
 cat > "$scriptsStall/bot-gh.sh" <<'EOF'
 #!/usr/bin/env bash
 case "$1" in

.claude/scripts/loop-daemon.sh

@@ -373,46 +373,52 @@ verify_and_classify_post_exit() {
   printf '%s' "$out"
 }
 
-# --- transient systemd unit naming (issue #119 pt 1; issue #96 pt ci-fix) ---
+# --- transient systemd unit naming (issue #119 pt 1; issue #96 pt ci-fix;
+# issue #96 part 2 pt comment-fix) -------------------------------------------
 # $1=verdict -> "pr-loop-driver-issue<N>" for "advance issue=N",
-# "pr-loop-driver-pr<N>" for "feedback pr=N", or
-# "pr-loop-driver-cifix-pr<N>" for "ci-fix pr=N". Used both to SPAWN the unit
-# (run_driver) and, in reverse (verdict_from_unit_name below), to recover the
-# verdict from a unit already running when this daemon process starts up
-# (reattach_orphaned_drivers) — the two must stay exact inverses of each other.
-# ci-fix gets its OWN distinct unit name (not "pr-loop-driver-pr<N>") so a
-# feedback driver and a ci-fix driver on the SAME PR can never collide in
-# naming or reattach — the verdict precedence (feedback > ci-fix) makes both
-# firing for the same PR in the same tick impossible, but a feedback driver
-# from a PRIOR tick could still be finishing up while a later tick, after that
-# feedback was addressed, dispatches a ci-fix driver for the identical PR
-# number; distinct unit names keep those two spawns/reattaches from ever being
+# "pr-loop-driver-pr<N>" for "feedback pr=N", "pr-loop-driver-cifix-pr<N>"
+# for "ci-fix pr=N", or "pr-loop-driver-commentfix-pr<N>" for
+# "comment-fix pr=N". Used both to SPAWN the unit (run_driver) and, in reverse
+# (verdict_from_unit_name below), to recover the verdict from a unit already
+# running when this daemon process starts up (reattach_orphaned_drivers) — the
+# two must stay exact inverses of each other. ci-fix and comment-fix EACH get
+# their OWN distinct unit name (neither reuses "pr-loop-driver-pr<N>") so a
+# feedback driver, a comment-fix driver, and a ci-fix driver on the SAME PR
+# can never collide in naming or reattach — the verdict precedence
+# (feedback > comment-fix > ci-fix) makes more than one of these firing for
+# the same PR in the SAME tick impossible, but a driver from a PRIOR tick
+# could still be finishing up while a LATER tick, after that reaction was
+# addressed, dispatches a DIFFERENT kind of driver for the identical PR
+# number; distinct unit names keep those spawns/reattaches from ever being
 # confused with each other.
 driver_unit_name() {
   case "$1" in
-    "advance issue="*)  printf 'pr-loop-driver-issue%s' "${1#advance issue=}" ;;
-    "ci-fix pr="*)       printf 'pr-loop-driver-cifix-pr%s' "${1#ci-fix pr=}" ;;
-    "feedback pr="*)    printf 'pr-loop-driver-pr%s' "${1#feedback pr=}" ;;
-    *)                  printf 'pr-loop-driver-unknown' ;;
+    "advance issue="*)     printf 'pr-loop-driver-issue%s' "${1#advance issue=}" ;;
+    "comment-fix pr="*)    printf 'pr-loop-driver-commentfix-pr%s' "${1#comment-fix pr=}" ;;
+    "ci-fix pr="*)         printf 'pr-loop-driver-cifix-pr%s' "${1#ci-fix pr=}" ;;
+    "feedback pr="*)       printf 'pr-loop-driver-pr%s' "${1#feedback pr=}" ;;
+    *)                     printf 'pr-loop-driver-unknown' ;;
   esac
 }
 
 # --- reverse of driver_unit_name: unit name -> verdict (issue #119 pt 3) ----
 # Prints nothing (not an error) for a unit name that doesn't match the
 # expected naming convention — reattach_orphaned_drivers skips those rather
-# than guessing. The `pr-loop-driver-cifix-pr*` arm MUST be checked before
-# `pr-loop-driver-pr*` would even matter for disambiguation (it doesn't here —
-# "cifix-pr..." never matches the "pr..." prefix pattern either way — but the
-# ordering keeps the two ci-fix/feedback arms visually adjacent to their
-# distinct name shapes in driver_unit_name above, so the pairing stays obvious
-# on read).
+# than guessing. The `pr-loop-driver-cifix-pr*`/`pr-loop-driver-commentfix-pr*`
+# arms MUST be checked before `pr-loop-driver-pr*` would even matter for
+# disambiguation (it doesn't here — neither "cifix-pr..." nor
+# "commentfix-pr..." ever matches the plain "pr..." prefix pattern either
+# way — but the ordering keeps all three non-advance arms visually adjacent to
+# their distinct name shapes in driver_unit_name above, so the pairing stays
+# obvious on read).
 verdict_from_unit_name() {
   local unit="${1%.service}"
   case "$unit" in
-    pr-loop-driver-issue*)    printf 'advance issue=%s' "${unit#pr-loop-driver-issue}" ;;
-    pr-loop-driver-cifix-pr*) printf 'ci-fix pr=%s' "${unit#pr-loop-driver-cifix-pr}" ;;
-    pr-loop-driver-pr*)       printf 'feedback pr=%s' "${unit#pr-loop-driver-pr}" ;;
-    *)                        : ;;
+    pr-loop-driver-issue*)      printf 'advance issue=%s' "${unit#pr-loop-driver-issue}" ;;
+    pr-loop-driver-cifix-pr*)   printf 'ci-fix pr=%s' "${unit#pr-loop-driver-cifix-pr}" ;;
+    pr-loop-driver-commentfix-pr*) printf 'comment-fix pr=%s' "${unit#pr-loop-driver-commentfix-pr}" ;;
+    pr-loop-driver-pr*)         printf 'feedback pr=%s' "${unit#pr-loop-driver-pr}" ;;
+    *)                          : ;;
   esac
 }
 
@@ -679,7 +685,7 @@ run_once() {
     none|"")
       : # nothing actionable — no driver spawned
       ;;
-    "advance issue="*|"feedback pr="*|"ci-fix pr="*)
+    "advance issue="*|"feedback pr="*|"comment-fix pr="*|"ci-fix pr="*)
       local model prompt_file
       model="$(printf '%s\n' "$out" | sed -n 's/^loop-event: model=//p' | tail -1)"
       model="${model:-${LOOP_MODEL:-sonnet}}"