From c384c4db4b550ce3c3604d1f7252383e3e6e4e77 Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Thu, 16 Jul 2026 17:59:32 +0200 Subject: [PATCH 1/2] =?UTF-8?q?feat(loop):=20needs-human=20signal=20?= =?UTF-8?q?=E2=80=94=20one=20label,=20one=20notify=20seam,=20cockpit=20str?= =?UTF-8?q?ip=20(issue=20#99)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates the loop's ad-hoc needs-human escalations (attempt-budget, stall) into one shared helper (needs-human.sh: needs_human_flag/needs_human_clear), adds a throttled push-notification seam (notify.sh, empty command = offline no-op), wires it into new block-on-owner points (PR ready for review / re-approval needed in merge-ready.sh, CHANGES_REQUESTED-addressed-awaiting- re-review in pr-feedback.sh) with clears firing on merge/re-dispatch, and surfaces everything as a "Needs you" strip at the top of the cockpit dashboard. Seeds the needs-human label alongside backlog/planned. --- .claude/gates.json | 5 +- .claude/scripts/cockpit.sh | 95 ++++++++++++- .claude/scripts/cockpit.test.sh | 75 ++++++++++ .claude/scripts/loop-ceilings.test.sh | 6 + .claude/scripts/loop-tick.sh | 37 +++-- .claude/scripts/loop-tick.test.sh | 8 ++ .claude/scripts/merge-ready.sh | 48 +++++++ .claude/scripts/needs-human.sh | 95 +++++++++++++ .claude/scripts/needs-human.test.sh | 165 ++++++++++++++++++++++ .claude/scripts/notify.sh | 194 ++++++++++++++++++++++++++ .claude/scripts/notify.test.sh | 139 ++++++++++++++++++ .claude/scripts/pr-feedback.sh | 26 ++++ .claude/scripts/seed-issues.sh | 7 + .claude/self/gates.json | 5 +- .claude/skills/setup/SKILL.md | 3 +- 15 files changed, 890 insertions(+), 18 deletions(-) create mode 100644 .claude/scripts/needs-human.sh create mode 100755 .claude/scripts/needs-human.test.sh create mode 100644 .claude/scripts/notify.sh create mode 100755 .claude/scripts/notify.test.sh diff --git a/.claude/gates.json b/.claude/gates.json index feb1b14..22fabc2 100644 --- a/.claude/gates.json +++ b/.claude/gates.json @@ -68,5 +68,8 @@ "policy": "pr-per-agent", "_policy_options": "pr-per-agent | orchestrated-sequential-merge", "baseBranch": "main" - } + }, + + "_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/\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": "" } diff --git a/.claude/scripts/cockpit.sh b/.claude/scripts/cockpit.sh index ffa302e..8a0550c 100755 --- a/.claude/scripts/cockpit.sh +++ b/.claude/scripts/cockpit.sh @@ -55,6 +55,15 @@ # appended near the end of ; cockpit-serve.sh (serve mode) injects # ITS OWN separate SSE/refresh client script by string-replacing , # so the live-stream code never ships in this static output. +# +# Issue #99 adds a "Needs you" strip at the VERY TOP of (before every +# other section): anything labeled `needs-human` (issues OR PRs — see +# needs-human.sh) or awaiting your first/re-review (a PR with passing CI that +# the owner hasn't approved/rejected yet), grouped by reason, with links. An +# all-clear message renders when nothing qualifies. Sourced from the SAME +# issues/prs arrays every other section already fetches (no extra gh call) — +# the live PR fetch now also asks for `labels` alongside the fields it always +# fetched, so a needs-human-labeled PR is visible without a second round trip. set -uo pipefail # Two-root derivation (issue #63): script_dir = sibling scripts, root = consumer project. @@ -169,7 +178,7 @@ prs_unavailable=0 if [ -n "$fixtures" ]; then if [ -f "$fixtures/prs.json" ]; then cp "$fixtures/prs.json" "$tmpdir/prs.json"; else echo "[]" >"$tmpdir/prs.json"; fi else - if ! gh pr list --state open --limit 200 --json number,title,url,headRefName,reviewDecision,statusCheckRollup >"$tmpdir/prs.json" 2>"$tmpdir/prs.err"; then + if ! gh pr list --state open --limit 200 --json number,title,url,headRefName,reviewDecision,statusCheckRollup,labels >"$tmpdir/prs.json" 2>"$tmpdir/prs.err"; then prs_unavailable=1 fi if [ "$prs_unavailable" -eq 0 ] && ! valid_json "$tmpdir/prs.json"; then prs_unavailable=1; fi @@ -385,6 +394,14 @@ function moduleLabelsOf(issue) { return (issue.labels || []).map((l) => l.name).filter((n) => typeof n === "string" && n.startsWith("module:")); } +// hasLabel: works for both the issues.json and prs.json label shapes (an +// array of {name} objects — the same `gh ... --json ...,labels` shape both +// fetches above already use), so the SAME helper serves renderNeedsYou() +// below for either an issue or a PR object. +function hasLabel(obj, name) { + return (obj.labels || []).some((l) => l && l.name === name); +} + // ---- Live worker progress section (issue #52, grouped by task in #92) ----- // Derive the CURRENT state per worker keyed by (role, task): keep the LATEST // event (by file order, i.e. append order) per key. No event log, or an @@ -819,6 +836,73 @@ function renderRouting() { return html; } +// ---- "Needs you" strip (issue #99) ----------------------------------------- +// The FIRST thing the dashboard answers: does anything need the owner right +// now? Two reasons, each its own group (grouped/annotated per the issue's +// acceptance criteria), sourced from the SAME issues/prs arrays every other +// section already fetched (no extra gh call): +// - "needs-human": an issue OR PR carrying the `needs-human` label (see +// needs-human.sh — the loop's escalation/PR-review/re-review points all +// apply this label through the one shared helper). +// - "awaiting your review": a PR with passing CI whose review decision is +// neither APPROVED nor CHANGES_REQUESTED (i.e. REVIEW_REQUIRED or no +// review yet) — the owner hasn't weighed in yet. CHANGES_REQUESTED is +// deliberately excluded here: that PR is in the BOT's court (pr-feedback.sh +// dispatches a fix), not the owner's, until it's addressed (which is when +// it picks up the `needs-human` label instead — see pr-feedback.sh). +// Degrades to an empty group set (never a crash) when issues/prs are +// unavailable, matching every other section's degrade contract; renders a +// clear all-clear state when the total across both groups is zero. +function renderNeedsYou() { + const groups = new Map(); // reason -> item[] + const push = (reason, item) => { + if (!groups.has(reason)) groups.set(reason, []); + groups.get(reason).push(item); + }; + + if (!issuesUnavailable) { + for (const issue of issues) { + if (hasLabel(issue, "needs-human")) { + push("needs-human", { num: issue.number, url: issue.url, title: issue.title }); + } + } + } + if (!prsUnavailable) { + for (const pr of prs) { + if (hasLabel(pr, "needs-human")) { + push("needs-human", { num: pr.number, url: pr.url, title: pr.title }); + continue; + } + const ci = ciBadge(pr.statusCheckRollup); + const rd = pr.reviewDecision; + if (ci.label === "passing" && rd !== "APPROVED" && rd !== "CHANGES_REQUESTED") { + push("awaiting your review", { num: pr.number, url: pr.url, title: pr.title }); + } + } + } + + const total = [...groups.values()].reduce((acc, list) => acc + list.length, 0); + let html = `
`; + if (total === 0) { + html += `

Needs you

all clear — nothing needs you right now

`; + } else { + html += `

Needs you (${total})

`; + // Stable group order regardless of Map insertion order: needs-human first + // (the more urgent/explicit signal), then awaiting-review. + for (const reason of ["needs-human", "awaiting your review"]) { + const list = groups.get(reason); + if (!list || list.length === 0) continue; + html += `

${esc(reason)}

`; + } + } + html += `
`; + return html; +} + // ---- Active worktrees section --------------------------------------------------- function renderWorktrees() { let html = `

Active worktrees

`; @@ -882,8 +966,10 @@ const html = ` h2 { margin-top: 0; border-bottom: 1px solid var(--border); padding-bottom: 0.4rem; } h3 { margin-bottom: 0.3rem; color: var(--text-dim); cursor: pointer; user-select: none; } #issues h3:hover { color: var(--link); } - ul.issue-list, ul.pr-list { list-style: none; padding-left: 0; } - ul.issue-list li, ul.pr-list li { padding: 0.4rem 0; border-bottom: 1px dashed var(--border); } + ul.issue-list, ul.pr-list, ul.needs-you-list { list-style: none; padding-left: 0; } + ul.issue-list li, ul.pr-list li, ul.needs-you-list li { padding: 0.4rem 0; border-bottom: 1px dashed var(--border); } + #needs-you { margin-bottom: 1rem; } + .needs-you-clear { font-size: 1rem; padding: 0.3rem 0.8rem; } .rel { font-size: 0.85rem; color: var(--text-dim); margin-top: 0.2rem; } .badge { display: inline-block; padding: 0.1rem 0.5rem; border-radius: 4px; font-size: 0.8rem; margin-left: 0.3rem; } .badge.good { background: var(--good-bg); color: var(--good-fg); } @@ -905,7 +991,8 @@ const html = `

Cockpit

-

Generated ${esc(generatedAt)} · read-only Phase 1 snapshot (issue #51) + Phase 2 live progress (issue #52) + Phase 3a serve/theme/filter (issue #69) + loop health panel (issue #85) · re-run cockpit.sh to refresh (or run cockpit-serve.sh for live auto-update)

+

Generated ${esc(generatedAt)} · read-only Phase 1 snapshot (issue #51) + Phase 2 live progress (issue #52) + Phase 3a serve/theme/filter (issue #69) + loop health panel (issue #85) + needs-you strip (issue #99) · re-run cockpit.sh to refresh (or run cockpit-serve.sh for live auto-update)

+${renderNeedsYou()} ${renderLiveProgress()} ${renderLoopHealth()} ${renderIssues()} diff --git a/.claude/scripts/cockpit.test.sh b/.claude/scripts/cockpit.test.sh index 8dd7fbd..feb6735 100755 --- a/.claude/scripts/cockpit.test.sh +++ b/.claude/scripts/cockpit.test.sh @@ -375,6 +375,81 @@ check "no spend-ceiling state files: stop-after degrades to 'not armed yet'" gre check "no spend-ceiling state files: per-issue attempts degrades to 'none tracked yet'" grep -qF 'Per-issue attempts: none tracked yet' "$html_ceilings_missing" check "no spend-ceiling state files: today's actions default to 0 / adapter ceiling" grep -qF '0 / 50' "$html_ceilings_missing" +# --------------------------------------------------------------------------- +# 2e. "Needs you" strip (issue #99): the VERY FIRST section in , sourced +# from the SAME issues.json/prs.json every other section already reads +# (no extra gh call). Two groups: +# - "needs-human": an issue OR PR carrying the `needs-human` label. +# - "awaiting your review": a PR with passing CI and a review decision +# that is neither APPROVED nor CHANGES_REQUESTED (the latter is the +# BOT's court via pr-feedback.sh, not the owner's). +# PR 200 (APPROVED, passing) and PR 201 (CHANGES_REQUESTED, failing) must +# NOT appear in either group. +# --------------------------------------------------------------------------- +mkdir -p "$work/fixtures-needs-you" +cat > "$work/fixtures-needs-you/issues.json" <<'EOF' +[ + {"number":106,"title":"Issue needing a human","url":"https://example.com/106","labels":[{"name":"needs-human"}],"body":""}, + {"number":100,"title":"Ordinary issue","url":"https://example.com/100","labels":[],"body":""} +] +EOF +cat > "$work/fixtures-needs-you/prs.json" <<'EOF' +[ + {"number":200,"title":"Approved PR","url":"https://example.com/pr/200","headRefName":"feat/x","reviewDecision":"APPROVED","statusCheckRollup":[{"conclusion":"SUCCESS","status":"COMPLETED","name":"build"}],"labels":[]}, + {"number":201,"title":"Changes requested PR","url":"https://example.com/pr/201","headRefName":"feat/y","reviewDecision":"CHANGES_REQUESTED","statusCheckRollup":[{"conclusion":"FAILURE","status":"COMPLETED","name":"test"}],"labels":[]}, + {"number":202,"title":"Needs-human PR","url":"https://example.com/pr/202","headRefName":"feat/z","reviewDecision":null,"statusCheckRollup":[{"conclusion":"SUCCESS","status":"COMPLETED","name":"build"}],"labels":[{"name":"needs-human"}]}, + {"number":203,"title":"Awaiting review PR","url":"https://example.com/pr/203","headRefName":"feat/w","reviewDecision":"REVIEW_REQUIRED","statusCheckRollup":[{"conclusion":"SUCCESS","status":"COMPLETED","name":"build"}],"labels":[]} +] +EOF +: >"$work/fixtures-needs-you/events.jsonl" +: >"$work/fixtures-needs-you/loop-ticks.jsonl" +html_needs_you="$work/cockpit-needs-you.html" +bash "$cockpit" --fixtures "$work/fixtures-needs-you" "$html_needs_you" >/dev/null 2>"$work/stderr-needs-you.log" +check "needs-you generator run exits 0" [ -s "$html_needs_you" ] +check "needs-you section is the FIRST section in (before live/issues/prs)" node -e ' + const fs = require("fs"); + const html = fs.readFileSync(process.argv[1], "utf8"); + const bodyIdx = html.indexOf(""); + const needsYouIdx = html.indexOf("
"); + } +' "$html_needs_you" +check "needs-you total count is 3 (issue 106 + PR 202 + PR 203)" grep -qF '

Needs you (3)

' "$html_needs_you" +needs_you_section="$(node -e ' + const fs = require("fs"); + const html = fs.readFileSync(process.argv[1], "utf8"); + const m = html.match(/
[\s\S]*?<\/section>/); + if (!m) throw new Error("needs-you section not found"); + process.stdout.write(m[0]); +' "$html_needs_you")" +check "needs-human group heading present" bash -c 'printf "%s" "$1" | grep -qF "

needs-human

"' _ "$needs_you_section" +check "awaiting-your-review group heading present" bash -c 'printf "%s" "$1" | grep -qF "

awaiting your review

"' _ "$needs_you_section" +check "needs-human group lists issue #106" bash -c 'printf "%s" "$1" | grep -qF "#106 Issue needing a human"' _ "$needs_you_section" +check "needs-human group lists PR #202 (not the awaiting-review group)" bash -c 'printf "%s" "$1" | grep -qF "#202 Needs-human PR"' _ "$needs_you_section" +check "awaiting-your-review group lists PR #203" bash -c 'printf "%s" "$1" | grep -qF "#203 Awaiting review PR"' _ "$needs_you_section" +check "approved PR #200 does NOT appear in the needs-you strip" bash -c '! printf "%s" "$1" | grep -qF "#200"' _ "$needs_you_section" +check "changes-requested PR #201 does NOT appear in the needs-you strip (bot's court, not owner's)" bash -c '! printf "%s" "$1" | grep -qF "#201"' _ "$needs_you_section" +check "ordinary issue #100 does NOT appear in the needs-you strip" bash -c '! printf "%s" "$1" | grep -qF "#100"' _ "$needs_you_section" + +# All-clear state: nothing labeled needs-human, nothing awaiting review. +mkdir -p "$work/fixtures-needs-you-clear" +echo "[]" >"$work/fixtures-needs-you-clear/issues.json" +cat > "$work/fixtures-needs-you-clear/prs.json" <<'EOF' +[{"number":300,"title":"All good PR","url":"https://example.com/pr/300","headRefName":"feat/all-good","reviewDecision":"APPROVED","statusCheckRollup":[{"conclusion":"SUCCESS","status":"COMPLETED","name":"build"}],"labels":[]}] +EOF +: >"$work/fixtures-needs-you-clear/events.jsonl" +: >"$work/fixtures-needs-you-clear/loop-ticks.jsonl" +html_needs_you_clear="$work/cockpit-needs-you-clear.html" +bash "$cockpit" --fixtures "$work/fixtures-needs-you-clear" "$html_needs_you_clear" >/dev/null 2>"$work/stderr-needs-you-clear.log" +check "all-clear generator run exits 0" [ -s "$html_needs_you_clear" ] +check "all-clear message rendered when nothing needs the owner" grep -qF 'all clear — nothing needs you right now' "$html_needs_you_clear" +check "all-clear run: no needs-human group heading" bash -c '! grep -qF "

needs-human

" "$1"' _ "$html_needs_you_clear" +check "all-clear run: no awaiting-your-review group heading" bash -c '! grep -qF "

awaiting your review

" "$1"' _ "$html_needs_you_clear" + # --------------------------------------------------------------------------- # 3. GATES_FILE override is honored (self-host adapter), still with fixtures # (no gh/network either way). diff --git a/.claude/scripts/loop-ceilings.test.sh b/.claude/scripts/loop-ceilings.test.sh index 17cb1f4..59a98c3 100644 --- a/.claude/scripts/loop-ceilings.test.sh +++ b/.claude/scripts/loop-ceilings.test.sh @@ -47,6 +47,12 @@ new_fixture() { rm -rf "$work/$name/.claude/state" # loop-tick.sh must mkdir -p it itself cp "$loop_tick_src" "$dir/loop-tick.sh" cp "$resolve_roots_src" "$dir/resolve-roots.sh" + # needs-human.sh/notify.sh (issue #99): loop-tick.sh sources needs-human.sh + # unconditionally when present -- copy the REAL implementations so the + # attempt-budget escalation scenarios (5, 6) exercise the real seam, with + # gh calls still landing only in this fixture's own logging bot-gh.sh. + cp "$script_dir/needs-human.sh" "$dir/needs-human.sh" + cp "$script_dir/notify.sh" "$dir/notify.sh" cat > "$dir/loop-census.sh" <= $per_issue_attempts)" if [ "$attempts_escalated" != "1" ]; then - gh label create "needs-human" --color b60205 --description "Loop attempt budget exhausted -- needs a human" --force >/dev/null 2>&1 || true body="This ${attempt_escalate_kind} has ping-ponged through $attempts_now advance/feedback dispatches for issue #$attempt_issue without landing (budget.per_issue_attempts=$per_issue_attempts). The loop will not retry it automatically -- labeling \`needs-human\`. Address it by hand, then either close it out or clear its entry in .claude/state/loop-issue-attempts.json to let the loop resume." - if [ "$attempt_escalate_kind" = "pr" ]; then - gh pr edit "$attempt_escalate_num" --add-label needs-human >/dev/null 2>&1 || true - gh pr comment "$attempt_escalate_num" --body "$body" >/dev/null 2>&1 || true - else - gh issue edit "$attempt_escalate_num" --add-label needs-human >/dev/null 2>&1 || true - gh issue comment "$attempt_escalate_num" --body "$body" >/dev/null 2>&1 || true - fi + needs_human_flag "${attempt_escalate_kind}:${attempt_escalate_num}" "attempt-budget" "high" \ + "Loop attempt budget exhausted for issue #$attempt_issue" "$body" tmp_att="$(mktemp "$state_dir/.loop-issue-attempts.json.XXXXXX")" if CLAUDE_ATT_KEY="$attempt_issue" CLAUDE_ATT_COUNT="$attempts_now" node -e ' const fs = require("fs"); @@ -760,10 +776,9 @@ else fi else write_resume_state "$resume_issue" "$resume_count" "1" - gh label create "needs-human" --color b60205 --description "Loop attempt budget exhausted -- needs a human" --force >/dev/null 2>&1 || true body="Issue #$resume_issue's feat/issue-$resume_issue-* branch has stalled and already been resumed $resume_count times without landing a PR. The loop will not retry it automatically -- labeling \`needs-human\`. Address it by hand, then either close it out or clear its entry in .claude/state/loop-resume-attempts.json to let the loop resume." - gh issue edit "$resume_issue" --add-label needs-human >/dev/null 2>&1 || true - gh issue comment "$resume_issue" --body "$body" >/dev/null 2>&1 || true + needs_human_flag "issue:$resume_issue" "stall" "high" \ + "Issue #$resume_issue stalled -- resume attempts exhausted" "$body" echo "# advance refused: issue=$resume_issue exhausted its 2 resume attempts -- escalated to needs-human" log_loop_event "$resume_issue" "escalated-to-needs-human" "issue=$resume_issue escalated to needs-human after $resume_count resumes" verdict="action=none" diff --git a/.claude/scripts/loop-tick.test.sh b/.claude/scripts/loop-tick.test.sh index 2aa589f..62df439 100644 --- a/.claude/scripts/loop-tick.test.sh +++ b/.claude/scripts/loop-tick.test.sh @@ -44,6 +44,12 @@ new_fixture() { rm -rf "$work/$name/.claude/state" # loop-tick.sh must mkdir -p it itself cp "$loop_tick_src" "$dir/loop-tick.sh" cp "$resolve_roots_src" "$dir/resolve-roots.sh" + # needs-human.sh/notify.sh (issue #99): loop-tick.sh sources needs-human.sh + # unconditionally if present; copy the REAL implementations so escalation + # scenarios exercise the real seam (gh calls still land in the fixture's + # own fake/logging bot-gh.sh, never real network). + cp "$script_dir/needs-human.sh" "$dir/needs-human.sh" + cp "$script_dir/notify.sh" "$dir/notify.sh" cat > "$dir/loop-census.sh" < "$dir/.claude/gates.json" <<'EOF' { "modules": [{ "name": "test", "path": ".", "description": "", "owner": "" }], diff --git a/.claude/scripts/merge-ready.sh b/.claude/scripts/merge-ready.sh index 74e1ee1..5315161 100644 --- a/.claude/scripts/merge-ready.sh +++ b/.claude/scripts/merge-ready.sh @@ -32,6 +32,15 @@ export PATH="$HOME/.local/bin:$PATH" # Route EVERY gh call (list/view/merge) through the bot identity (see bot-gh.sh). gh() { bash "$script_dir/bot-gh.sh" "$@"; } repo="${1:-$(gh repo view --json nameWithOwner -q .nameWithOwner)}" + +# needs_human_flag/needs_human_clear (issue #99): the ONE shared label+notify +# seam for "PR ready-for-review" / "re-approve current head" -- see the +# per-PR loop below. Sourced AFTER the `gh` wrapper above so both functions +# call the bot identity; guarded (not a bare `&&`) so a missing file under +# `set -e` never aborts the script (see needs-human.sh's own header for why +# every statement in it is written the same defensive way). +# shellcheck source=needs-human.sh +if [ -f "$script_dir/needs-human.sh" ]; then . "$script_dir/needs-human.sh"; fi owner="${MERGE_APPROVER:-${repo%%/*}}" # the approver whose APPROVED review authorizes a merge gates="$root/.claude/gates.json" base="$(node -e "try{const g=require('$gates');process.stdout.write((g.merge&&g.merge.baseBranch)||'main')}catch(e){process.stdout.write('main')}")" @@ -79,9 +88,48 @@ for n in $(gh pr list -R "$repo" --base "$base" --state open --json number -q '. verdict="$(printf '%s' "$data" | decide)" title="$(printf '%s' "$data" | node -e 'process.stdout.write((JSON.parse(require("fs").readFileSync(0,"utf8")).title)||"")')" head_branch="$(printf '%s' "$data" | node -e 'process.stdout.write((JSON.parse(require("fs").readFileSync(0,"utf8")).headRefName)||"")')" + + # needs-human (issue #99): a PR is genuinely blocked on the OWNER for + # exactly two of decide()'s skip reasons -- no review submitted yet, or a + # stale approval that no longer covers the current head (new commits + # pushed since). Every other reason (draft/base mismatch/conflicts/CI + # pending-or-failing, and owner-review=CHANGES_REQUESTED -- that one is + # pr-feedback.sh's job to dispatch a bot fix for, not the owner's) is NOT + # an owner-blocking wait, so any earlier "ready for review" flag on this PR + # is cleared. Both calls are best-effort no-ops when the corresponding + # helper function isn't defined (needs-human.sh missing from a fixture). + case "$verdict" in + SKIP:no-owner-review|SKIP:approval-stale*) + if command -v needs_human_flag >/dev/null 2>&1; then + needs_human_flag "pr:$n" "pr-review" "low" \ + "PR #$n ready for your review" "$title (${verdict#SKIP:})" + fi + ;; + *) + if command -v needs_human_clear >/dev/null 2>&1; then + needs_human_clear "pr:$n" "pr-review" + fi + ;; + esac + if [ "$verdict" = "MERGE" ]; then if gh pr merge "$n" -R "$repo" --merge --delete-branch >/dev/null 2>&1; then echo "{\"pr\":$n,\"action\":\"merged\",\"title\":\"$title\"}"; merged=$((merged+1)) + # needs-human (issue #99): the PR just merged -- the clearest possible + # "this block-on-owner condition just resolved" signal. Clear any + # needs-human flag on the PR itself (belt-and-suspenders; it's about to + # be closed anyway) AND on the issue it was cut from (feat/issue-N-* or + # fix/issue-N-*), since loop-tick.sh's attempt-budget/stall escalations + # both flag the ISSUE, not the PR. + if command -v needs_human_clear >/dev/null 2>&1; then + needs_human_clear "pr:$n" "pr-review" + needs_human_clear "pr:$n" "changes-requested" + merged_issue_num="$(printf '%s\n' "$head_branch" | sed -n 's/.*issue-\([0-9][0-9]*\).*/\1/p')" + if [ -n "$merged_issue_num" ]; then + needs_human_clear "issue:$merged_issue_num" "attempt-budget" + needs_human_clear "issue:$merged_issue_num" "stall" + fi + fi # Auto-cleanup (issue #91): the merged branch's local worktree + local # branch are now stale. worktree-cleanup.sh applies its OWN safety # rails (worker-path naming, clean tree, fully merged into $base) and diff --git a/.claude/scripts/needs-human.sh b/.claude/scripts/needs-human.sh new file mode 100644 index 0000000..a837ba8 --- /dev/null +++ b/.claude/scripts/needs-human.sh @@ -0,0 +1,95 @@ +#!/usr/bin/env bash +# needs-human.sh — the ONE shared "needs-human" seam (issue #99). SOURCE this +# (never execute it) from a script that already defines `gh` the way every +# loop script does: +# gh() { bash "$script_dir/bot-gh.sh" "$@"; } +# needs_human_flag/needs_human_clear below call that `gh` function directly, +# so sourcing this file WITHOUT `gh` already defined will make every call a +# plain (probably missing) `gh` binary — fine in a fixture that expects zero +# gh side effects, but callers that want real label/comment behavior must +# define `gh` first, exactly like they already do for their own gh calls. +# +# This file intentionally does NOT `set -e`/`set -u`/etc — it is SOURCED into +# the caller's shell, and changing the caller's shell options out from under +# it would be a much bigger foot-gun than the small amount of defensiveness +# lost by not doing so here. Every statement in both functions below already +# ends in `|| true` for exactly this reason: they must be safe to call from a +# caller running under `set -e` (pr-feedback.sh, merge-ready.sh) AND one that +# isn't (loop-tick.sh). +# +# needs_human_flag TARGET KIND SEVERITY TITLE BODY +# TARGET: "issue:" or "pr:" — which GitHub object to label/comment on. +# KIND: a short escalation key, e.g. "attempt-budget", "stall", +# "pr-review", "changes-requested", "expired", "daily-ceiling" — +# combined with TARGET as notify.sh's throttle key (see notify.sh), +# so distinct escalation kinds on the SAME target notify +# independently, while repeats of the SAME kind on the SAME target +# stay throttled to notify.sh's window. +# SEVERITY/TITLE/BODY: passed straight through to notify.sh; BODY is also +# posted as a comment on TARGET (skipped when BODY is empty). +# Idempotent: `gh label create --force` never fails if the label already +# exists; adding an already-present label is a no-op on GitHub's side. +# +# needs_human_clear TARGET KIND +# Removes the needs-human label from TARGET (best-effort — a target that +# was never labeled just no-ops) and clears notify.sh's throttle entry for +# (KIND,TARGET), so a FUTURE flag of the same kind on the same target +# notifies immediately instead of staying throttled from the episode that +# just cleared. Call this from the success point where the corresponding +# block-on-owner condition resolves (PR merged, approval given, issue +# advanced, budget/attempts counter manually reset, etc). +# +# Both functions are best-effort throughout: a gh/notify failure here must +# NEVER break the calling script. +needs_human_script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# $1 = "issue:42" or "pr:17" -> sets NH_TYPE ("issue"|"pr") and NH_NUM. +_needs_human_split_target() { + local t="$1" + NH_TYPE="${t%%:*}" + NH_NUM="${t#*:}" +} + +needs_human_flag() { + local target="$1" kind="$2" severity="$3" title="$4" body="$5" + _needs_human_split_target "$target" + + gh label create "needs-human" --color b60205 \ + --description "Loop is blocked on owner judgment -- see the issue/PR body/comments" \ + --force >/dev/null 2>&1 || true + + # NOTE: every conditional below ends in `|| true` on the OUTSIDE of the + # `[ ... ] && { ... }` too (not just inside the braces) -- under a caller + # running `set -e` (pr-feedback.sh, merge-ready.sh), a bare + # `[ -n "$body" ] && { ...; }` statement whose test is FALSE evaluates the + # whole statement to non-zero and would trip errexit right here. + case "$NH_TYPE" in + pr) + gh pr edit "$NH_NUM" --add-label needs-human >/dev/null 2>&1 || true + { [ -n "$body" ] && gh pr comment "$NH_NUM" --body "$body" >/dev/null 2>&1; } || true + ;; + issue) + gh issue edit "$NH_NUM" --add-label needs-human >/dev/null 2>&1 || true + { [ -n "$body" ] && gh issue comment "$NH_NUM" --body "$body" >/dev/null 2>&1; } || true + ;; + *) ;; + esac + + local body_line + body_line="$(printf '%s\n' "$body" | head -1)" + bash "$needs_human_script_dir/notify.sh" "$severity" "$title" "$body_line" \ + --kind "$kind" --target "$target" >/dev/null 2>&1 || true +} + +needs_human_clear() { + local target="$1" kind="$2" + _needs_human_split_target "$target" + + case "$NH_TYPE" in + pr) gh pr edit "$NH_NUM" --remove-label needs-human >/dev/null 2>&1 || true ;; + issue) gh issue edit "$NH_NUM" --remove-label needs-human >/dev/null 2>&1 || true ;; + *) ;; + esac + + bash "$needs_human_script_dir/notify.sh" --clear --kind "$kind" --target "$target" >/dev/null 2>&1 || true +} diff --git a/.claude/scripts/needs-human.test.sh b/.claude/scripts/needs-human.test.sh new file mode 100755 index 0000000..be767e7 --- /dev/null +++ b/.claude/scripts/needs-human.test.sh @@ -0,0 +1,165 @@ +#!/usr/bin/env bash +# needs-human.test.sh — offline smoke test for needs-human.sh (issue #99), +# the ONE shared label+notify seam for the loop's block-on-owner points. +# +# Sources the REAL needs-human.sh (+ notify.sh, next to it) with a stubbed +# `gh` shell FUNCTION (never a network call) that logs every invocation, so +# needs_human_flag/needs_human_clear exercise their real gh call sequence +# with zero network/tokens. notify.sh's own configured command is also a +# local file write, never gh/network. +# +# Exit 0 on success, non-zero if any assertion fails. Runnable bare: +# bash .claude/scripts/needs-human.test.sh +set -uo pipefail + +# Isolate from the CALLER's environment (mirrors cockpit.test.sh/notify.test.sh): +# this test is wired into .claude/self/checks.sh's `test` case, which itself +# often runs under `GATES_FILE=.claude/self/gates.json` (the self-host loop). +# An ambient GATES_FILE would silently redirect notify.sh's config lookup +# (called by needs_human_flag/needs_human_clear below) onto the SELF adapter +# instead of each fixture's own hand-written .claude/gates.json. +unset GATES_FILE + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +work="$(mktemp -d "${TMPDIR:-/tmp}/needs-human-test.XXXXXX")" +trap 'rm -rf "$work"' EXIT + +fail=0 +ok=0 +check() { + local desc="$1"; shift + if "$@"; then + ok=$((ok + 1)) + echo "ok - $desc" + else + fail=1 + echo "FAIL - $desc" + fi +} + +# $1 = fixture name, $2 = notify command (may be empty). Prints the fixture +# root dir (a throwaway /.claude/{scripts,state} tree). +new_fixture() { + local name="$1" notify_cmd="$2" + local dir="$work/$name" + local scripts="$dir/.claude/scripts" + mkdir -p "$scripts" "$dir/.claude/state" + cp "$script_dir/needs-human.sh" "$scripts/needs-human.sh" + cp "$script_dir/notify.sh" "$scripts/notify.sh" + cp "$script_dir/resolve-roots.sh" "$scripts/resolve-roots.sh" + chmod +x "$scripts"/*.sh + CLAUDE_NOTIFY_CMD="$notify_cmd" node -e ' + const fs = require("fs"); + fs.writeFileSync(process.argv[1], JSON.stringify({ notify: process.env.CLAUDE_NOTIFY_CMD })); + ' "$dir/.claude/gates.json" + printf '%s\n' "$dir" +} + +# --------------------------------------------------------------------------- +# 1. needs_human_flag on an ISSUE target: label create + issue edit +# --add-label + issue comment (in that order), all via the stubbed `gh`. +# --------------------------------------------------------------------------- +dir1="$(new_fixture scenario1 "")" +gh_log1="$work/scenario1-gh.log" +out1="$(bash -c ' + gh() { printf "%s\n" "$*" >> "'"$gh_log1"'"; } + . "'"$dir1"'/.claude/scripts/needs-human.sh" + needs_human_flag "issue:42" "attempt-budget" "high" "Attempt budget exhausted" "Please look at issue 42" +' 2>&1)" +rc1=$? +check "scenario 1: needs_human_flag exits 0" [ "$rc1" -eq 0 ] +check "scenario 1: label create is the FIRST gh call" bash -c 'head -1 "$1" | grep -q "^label create needs-human"' _ "$gh_log1" +check "scenario 1: issue edit --add-label needs-human is the SECOND gh call" bash -c 'sed -n 2p "$1" | grep -qF "issue edit 42 --add-label needs-human"' _ "$gh_log1" +check "scenario 1: issue comment with the body is the THIRD gh call" bash -c 'sed -n 3p "$1" | grep -qF "issue comment 42 --body Please look at issue 42"' _ "$gh_log1" +check "scenario 1: exactly 3 gh calls (no extra side effects)" bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 3 ]' _ "$gh_log1" + +# --------------------------------------------------------------------------- +# 2. needs_human_flag on a PR target: `pr edit`/`pr comment`, not `issue *`. +# --------------------------------------------------------------------------- +dir2="$(new_fixture scenario2 "")" +gh_log2="$work/scenario2-gh.log" +bash -c ' + gh() { printf "%s\n" "$*" >> "'"$gh_log2"'"; } + . "'"$dir2"'/.claude/scripts/needs-human.sh" + needs_human_flag "pr:17" "pr-review" "low" "PR ready" "please review PR 17" +' >/dev/null 2>&1 +check "scenario 2: PR target uses 'pr edit', not 'issue edit'" bash -c 'grep -q "^pr edit 17 --add-label needs-human" "$1" && ! grep -q "^issue edit" "$1"' _ "$gh_log2" +check "scenario 2: PR target posts via 'pr comment'" grep -qF "pr comment 17 --body please review PR 17" "$gh_log2" + +# --------------------------------------------------------------------------- +# 3. needs_human_clear removes the label (the OPPOSITE gh call from flag) and +# never posts a comment. +# --------------------------------------------------------------------------- +dir3="$(new_fixture scenario3 "")" +gh_log3="$work/scenario3-gh.log" +bash -c ' + gh() { printf "%s\n" "$*" >> "'"$gh_log3"'"; } + . "'"$dir3"'/.claude/scripts/needs-human.sh" + needs_human_clear "issue:99" "stall" +' >/dev/null 2>&1 +check "scenario 3: clear removes the label via 'issue edit --remove-label'" grep -qF "issue edit 99 --remove-label needs-human" "$gh_log3" +check "scenario 3: clear never posts a comment" bash -c '! grep -q "^issue comment" "$1"' _ "$gh_log3" +check "scenario 3: clear never (re-)creates the label" bash -c '! grep -q "^label create" "$1"' _ "$gh_log3" + +# --------------------------------------------------------------------------- +# 4. Idempotent add/remove round-trip driven through notify.sh's real +# throttle state: flag -> clear -> flag again fires the SAME (kind,target) +# notification TWICE (clear resets the throttle so the second flag isn't +# silently swallowed) — proves flag/clear are a genuine round trip, not +# just gh label bookkeeping with a notify path that's accidentally inert. +# --------------------------------------------------------------------------- +fired4="$work/scenario4-fired.txt" +dir4="$(new_fixture scenario4 "printf 'fired\n' >> $fired4")" +gh_log4="$work/scenario4-gh.log" +bash -c ' + gh() { printf "%s\n" "$*" >> "'"$gh_log4"'"; } + . "'"$dir4"'/.claude/scripts/needs-human.sh" + needs_human_flag "issue:7" "attempt-budget" "high" "T" "B" + needs_human_clear "issue:7" "attempt-budget" + needs_human_flag "issue:7" "attempt-budget" "high" "T2" "B2" +' >/dev/null 2>&1 +check "scenario 4: label add appears TWICE (flag, clear, flag again)" bash -c '[ "$(grep -c "add-label needs-human" "$1")" -eq 2 ]' _ "$gh_log4" +check "scenario 4: label remove appears ONCE (the clear in between)" bash -c '[ "$(grep -c "remove-label needs-human" "$1")" -eq 1 ]' _ "$gh_log4" +check "scenario 4: notify fired TWICE (clear reset the throttle between the two flags)" bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 2 ]' _ "$fired4" + +# --------------------------------------------------------------------------- +# 5. Without an intervening clear, a SECOND flag for the SAME (kind,target) +# is throttled by notify.sh (still re-applies the gh label, but does not +# re-notify) -- the "one notification per (kind,target) per window" +# contract this issue's acceptance criteria calls for. +# --------------------------------------------------------------------------- +fired5="$work/scenario5-fired.txt" +dir5="$(new_fixture scenario5 "printf 'fired\n' >> $fired5")" +bash -c ' + gh() { :; } + . "'"$dir5"'/.claude/scripts/needs-human.sh" + needs_human_flag "issue:8" "attempt-budget" "high" "T" "B" + needs_human_flag "issue:8" "attempt-budget" "high" "T again" "B again" +' >/dev/null 2>&1 +check "scenario 5: repeated flag with no clear in between notifies only ONCE" bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 1 ]' _ "$fired5" + +# --------------------------------------------------------------------------- +# 6. A failing `gh` (offline/unauthenticated) never crashes the caller +# (best-effort contract) -- `gh` here always returns non-zero, standing in +# for "gh unreachable", WITHOUT ever falling through to a real gh binary +# that might be on this machine's PATH (a bare undefined `gh` would risk +# exactly that -- this stub is deliberate, not a shortcut). +# --------------------------------------------------------------------------- +dir6="$(new_fixture scenario6 "")" +out6="$(bash -c ' + gh() { return 1; } + . "'"$dir6"'/.claude/scripts/needs-human.sh" + needs_human_flag "issue:1" "attempt-budget" "high" "T" "B" + echo "SURVIVED" +' 2>&1)" +check "scenario 6: caller survives (prints SURVIVED) even when every gh call fails" bash -c 'printf "%s\n" "$1" | grep -q "SURVIVED"' _ "$out6" + +echo "" +if [ "$fail" -eq 0 ]; then + echo "needs-human.test.sh: PASS ($ok checks)" + exit 0 +else + echo "needs-human.test.sh: FAIL (see FAIL lines above)" + exit 1 +fi diff --git a/.claude/scripts/notify.sh b/.claude/scripts/notify.sh new file mode 100644 index 0000000..b880b68 --- /dev/null +++ b/.claude/scripts/notify.sh @@ -0,0 +1,194 @@ +#!/usr/bin/env bash +# notify.sh — generic push-notification seam for the loop (issue #99). +# +# Reads an adapter-configured shell command from gates.json's `notify` key +# (same GATES_FILE override + empty-means-skip convention as every other +# gate/budget knob — see gate.sh and loop-tick.sh's STEP 0). When that +# command is empty (the default in BOTH shipped adapters — the owner picks +# their own notifier), this is a SILENT no-op that exits 0: offline/CI-safe +# by construction, and the hard acceptance criterion for this issue. +# +# Usage: +# notify.sh <body-line> [--kind <kind>] [--target <target>] [--window <seconds>] +# notify.sh --clear --kind <kind> --target <target> +# +# Contract with the configured command: severity/title/body reach it BOTH +# ways, so a one-liner (`notify-send "$1" "$2"`) and an env-reading script +# (`curl ... -d "$NOTIFY_BODY"`) are equally easy to wire up: +# - positional args $1/$2/$3 = severity/title/body-line +# - env vars NOTIFY_SEVERITY / NOTIFY_TITLE / NOTIFY_BODY = the same three +# Example commands (all empty by default; pick ONE in your adapter): +# ntfy: "curl -s -d \"$NOTIFY_BODY\" -H \"Title: $NOTIFY_TITLE\" -H \"Priority: $NOTIFY_SEVERITY\" ntfy.sh/<your-topic>" +# notify-send: "notify-send \"$NOTIFY_TITLE\" \"$NOTIFY_BODY\"" +# webhook curl: "curl -s -X POST -H 'Content-Type: application/json' -d \"{\\\"severity\\\":\\\"$NOTIFY_SEVERITY\\\",\\\"title\\\":\\\"$NOTIFY_TITLE\\\",\\\"body\\\":\\\"$NOTIFY_BODY\\\"}\" https://example.invalid/hook" +# +# Throttling: at most one notification per (kind, target) per cadence window +# (default ${NOTIFY_THROTTLE_SECONDS:-1800}s = 30 minutes; override per-call +# with --window, or globally with $NOTIFY_THROTTLE_SECONDS), so a WATCH-cadence +# loop re-checking a still-blocked condition every few minutes doesn't nag. +# State: <root>/.claude/state/notify-throttle.json (gitignored, mirrors every +# other loop state file), keyed by "<kind>:<target>" -> last-fired timestamp. +# Override the state file with CLAUDE_NOTIFY_THROTTLE_FILE (tests). kind/target +# default to "general"/the title when the caller doesn't pass them, so +# throttling always has SOME key rather than silently never throttling. +# +# --clear --kind K --target T: removes the (kind,target) throttle entry with +# NO notification sent — used by needs-human.sh's needs_human_clear so a +# FUTURE re-flag of the same (kind,target) notifies immediately instead of +# staying throttled from the episode that just cleared. +# +# Guards state read/write failures (missing/read-only .claude/state/) so a +# broken state dir degrades to "always notify" rather than crashing — this +# must never break the calling script, matching log-event.sh's contract. +# Every side effect below is best-effort; the configured command's own exit +# status is never propagated (a flaky notifier must never fail the loop). +set -uo pipefail + +# Two-root derivation (issue #63): script_dir = sibling scripts, root = +# consumer project. Never fails (log-event.sh-style — this script must not +# block the caller even if resolve-roots.sh is somehow missing). +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" 2>/dev/null && pwd)" +if [ -f "$script_dir/resolve-roots.sh" ]; then + # shellcheck source=resolve-roots.sh + . "$script_dir/resolve-roots.sh" 2>/dev/null || true +fi +root="${root:-$(pwd)}" + +# --------------------------------------------------------------------------- +# Args +# --------------------------------------------------------------------------- +clear_mode=0 +severity="" title="" body="" +kind="" target="" window="" + +if [ "${1:-}" = "--clear" ]; then + clear_mode=1 + shift +else + severity="${1:-}"; title="${2:-}"; body="${3:-}" + # Consume up to 3 positional args (whichever actually exist) before parsing + # flags — avoids `shift 3` erroring out when fewer than 3 were passed. + for _ in 1 2 3; do + [ $# -gt 0 ] && shift + done +fi + +while [ $# -gt 0 ]; do + case "${1:-}" in + --kind) kind="${2:-}"; shift 2 ;; + --target) target="${2:-}"; shift 2 ;; + --window) window="${2:-}"; shift 2 ;; + *) shift ;; + esac +done + +case "$window" in ''|*[!0-9]*) window="${NOTIFY_THROTTLE_SECONDS:-1800}" ;; esac +case "$window" in ''|*[!0-9]*) window=1800 ;; esac + +throttle_key="${kind:-general}:${target:-${title:-untitled}}" + +state_dir="$root/.claude/state" +throttle_file="${CLAUDE_NOTIFY_THROTTLE_FILE:-$state_dir/notify-throttle.json}" + +now_iso="${NOTIFY_NOW:-$(date -u +%FT%TZ 2>/dev/null)}" + +# --------------------------------------------------------------------------- +# --clear: drop the throttle entry, no notification, always exit 0. +# --------------------------------------------------------------------------- +if [ "$clear_mode" -eq 1 ]; then + if [ -f "$throttle_file" ]; then + tmp="$(mktemp "$state_dir/.notify-throttle.json.XXXXXX" 2>/dev/null || true)" + if [ -n "$tmp" ] && CLAUDE_NH_KEY="$throttle_key" node -e ' + const fs = require("fs"); + const file = process.argv[1], tmp = process.argv[2]; + const key = process.env.CLAUDE_NH_KEY; + let j = {}; + try { j = JSON.parse(fs.readFileSync(file, "utf8")); } catch (e) {} + delete j[key]; + fs.writeFileSync(tmp, JSON.stringify(j, null, 2) + "\n"); + ' "$throttle_file" "$tmp" 2>/dev/null; then + mv -f "$tmp" "$throttle_file" 2>/dev/null || rm -f "$tmp" 2>/dev/null + else + [ -n "$tmp" ] && rm -f "$tmp" 2>/dev/null + fi + fi + exit 0 +fi + +# --------------------------------------------------------------------------- +# Read the adapter-configured command. Empty (both shipped adapters ship it +# empty by default) -> silent no-op, exit 0. This is the hard offline/CI-safe +# acceptance criterion for issue #99. +# --------------------------------------------------------------------------- +gates_rel="${GATES_FILE:-.claude/gates.json}" +case "$gates_rel" in /*) gates_path="$gates_rel" ;; *) gates_path="$root/$gates_rel" ;; esac + +cmd="$(node -e ' + const fs = require("fs"); + try { + const g = JSON.parse(fs.readFileSync(process.argv[1], "utf8")); + process.stdout.write((g && typeof g.notify === "string") ? g.notify : ""); + } catch (e) { process.stdout.write(""); } +' "$gates_path" 2>/dev/null)" + +[ -n "$cmd" ] || exit 0 + +# --------------------------------------------------------------------------- +# Throttle check: skip (silently, exit 0) when this (kind,target) fired within +# the window. A missing/unreadable/unwritable state dir degrades to "always +# notify" (never throttled) rather than blocking the notification. +# --------------------------------------------------------------------------- +if [ -f "$throttle_file" ]; then + should_skip="$(CLAUDE_NH_KEY="$throttle_key" CLAUDE_NH_NOW="$now_iso" CLAUDE_NH_WINDOW="$window" node -e ' + const fs = require("fs"); + const key = process.env.CLAUDE_NH_KEY; + // NOTE: `|| 1800` would treat a legitimate --window 0 as falsy and + // silently override it back to 1800 -- use Number.isFinite instead so + // 0 (never throttle) is respected. + const windowParsed = parseInt(process.env.CLAUDE_NH_WINDOW, 10); + const window = Number.isFinite(windowParsed) && windowParsed >= 0 ? windowParsed : 1800; + let j = {}; + try { j = JSON.parse(fs.readFileSync(process.argv[1], "utf8")); } catch (e) { j = {}; } + const last = j[key]; + if (!last) { console.log("0"); process.exit(0); } + const lastMs = Date.parse(last); + const nowMs = Date.parse(process.env.CLAUDE_NH_NOW); + if (!Number.isFinite(lastMs) || !Number.isFinite(nowMs)) { console.log("0"); process.exit(0); } + console.log((nowMs - lastMs) < window * 1000 ? "1" : "0"); + ' "$throttle_file" 2>/dev/null || echo "0")" + if [ "$should_skip" = "1" ]; then + exit 0 + fi +fi + +# --------------------------------------------------------------------------- +# Fire: run the configured command with severity/title/body available both as +# positional args ($1/$2/$3) and as NOTIFY_SEVERITY/NOTIFY_TITLE/NOTIFY_BODY +# env vars. Never propagate its exit status — a flaky/misconfigured notifier +# must never fail the caller. +# --------------------------------------------------------------------------- +NOTIFY_SEVERITY="$severity" NOTIFY_TITLE="$title" NOTIFY_BODY="$body" \ + bash -c "$cmd" -- "$severity" "$title" "$body" >/dev/null 2>&1 || true + +# --------------------------------------------------------------------------- +# Record the fire so the throttle window applies to the NEXT call. Best +# effort: a failure here (read-only/missing state dir) never affects the exit +# status — the notification already fired above. +# --------------------------------------------------------------------------- +mkdir -p "$state_dir" 2>/dev/null || exit 0 +tmp="$(mktemp "$state_dir/.notify-throttle.json.XXXXXX" 2>/dev/null || true)" +if [ -n "$tmp" ] && CLAUDE_NH_KEY="$throttle_key" CLAUDE_NH_NOW="$now_iso" node -e ' + const fs = require("fs"); + const file = process.argv[1], tmp = process.argv[2]; + const key = process.env.CLAUDE_NH_KEY; + let j = {}; + try { j = JSON.parse(fs.readFileSync(file, "utf8")); } catch (e) {} + j[key] = process.env.CLAUDE_NH_NOW; + fs.writeFileSync(tmp, JSON.stringify(j, null, 2) + "\n"); +' "$throttle_file" "$tmp" 2>/dev/null; then + mv -f "$tmp" "$throttle_file" 2>/dev/null || rm -f "$tmp" 2>/dev/null +else + [ -n "$tmp" ] && rm -f "$tmp" 2>/dev/null +fi + +exit 0 diff --git a/.claude/scripts/notify.test.sh b/.claude/scripts/notify.test.sh new file mode 100755 index 0000000..0bf3d23 --- /dev/null +++ b/.claude/scripts/notify.test.sh @@ -0,0 +1,139 @@ +#!/usr/bin/env bash +# notify.test.sh — offline smoke test for notify.sh (issue #99). +# +# Every scenario runs against a throwaway <fixture>/.claude/ tree (real +# notify.sh + resolve-roots.sh, a hand-written gates.json) so root/GATES_FILE +# resolution matches production exactly, with zero network/gh calls: notify.sh +# never calls gh at all, so this is just shell + a configured shell command +# that writes to a temp file. +# +# Exit 0 on success, non-zero if any assertion fails. Runnable bare: +# bash .claude/scripts/notify.test.sh +set -uo pipefail + +# Isolate from the CALLER's environment (mirrors cockpit.test.sh): this test +# is wired into .claude/self/checks.sh's `test` case, which itself often runs +# under `GATES_FILE=.claude/self/gates.json` (the self-host loop). Since env +# vars set before a command propagate to every child process, an ambient +# GATES_FILE would silently redirect notify.sh's config lookup onto the SELF +# adapter instead of each fixture's own hand-written .claude/gates.json below. +unset GATES_FILE + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +notify_src="$script_dir/notify.sh" +resolve_roots_src="$script_dir/resolve-roots.sh" + +work="$(mktemp -d "${TMPDIR:-/tmp}/notify-test.XXXXXX")" +trap 'rm -rf "$work"' EXIT + +fail=0 +ok=0 +check() { + local desc="$1"; shift + if "$@"; then + ok=$((ok + 1)) + echo "ok - $desc" + else + fail=1 + echo "FAIL - $desc" + fi +} + +# $1 = fixture name, $2 = notify command (may be empty). Prints the fixture's +# .claude/scripts dir and writes a matching .claude/gates.json. +new_fixture() { + local name="$1" notify_cmd="$2" + local dir="$work/$name" + local scripts="$dir/.claude/scripts" + mkdir -p "$scripts" + cp "$notify_src" "$scripts/notify.sh" + cp "$resolve_roots_src" "$scripts/resolve-roots.sh" + chmod +x "$scripts"/*.sh + CLAUDE_NOTIFY_CMD="$notify_cmd" node -e ' + const fs = require("fs"); + fs.writeFileSync(process.argv[1], JSON.stringify({ notify: process.env.CLAUDE_NOTIFY_CMD })); + ' "$dir/.claude/gates.json" + printf '%s\n' "$scripts" +} + +run_notify() { + # $1=scripts dir, rest = notify.sh args. Runs with a throwaway throttle + # state file scoped to THIS fixture (never the real .claude/state/). + local scripts="$1"; shift + CLAUDE_NOTIFY_THROTTLE_FILE="$scripts/../state/notify-throttle.json" bash "$scripts/notify.sh" "$@" +} + +# --------------------------------------------------------------------------- +# 1. Empty command -> silent no-op, exit 0, no throttle state written. +# --------------------------------------------------------------------------- +dir1="$(new_fixture scenario1 "")" +out1="$(run_notify "$dir1" high "Some title" "Some body" --kind test --target issue:1)" +rc1=$? +check "scenario 1 (empty command): exit 0" [ "$rc1" -eq 0 ] +check "scenario 1: no output" [ -z "$out1" ] +check "scenario 1: no throttle state file created" [ ! -f "$dir1/../state/notify-throttle.json" ] + +# --------------------------------------------------------------------------- +# 2. Configured command fires exactly once; an IMMEDIATE second call for the +# SAME (kind,target) is throttled (no second fire). +# --------------------------------------------------------------------------- +fired2="$work/scenario2-fired.txt" +dir2="$(new_fixture scenario2 "printf '%s|%s|%s|%s\n' \"\$1\" \"\$2\" \"\$3\" \"\$NOTIFY_SEVERITY\" >> $fired2")" +run_notify "$dir2" high "First title" "First body" --kind foo --target issue:1 >/dev/null +check "scenario 2: command fired exactly once" bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 1 ]' _ "$fired2" +check "scenario 2: severity/title/body reached the command as positional args AND env var" \ + grep -qF "high|First title|First body|high" "$fired2" + +run_notify "$dir2" high "Second title" "Second body" --kind foo --target issue:1 >/dev/null +check "scenario 2: immediate second call for the SAME (kind,target) is throttled (still exactly 1 fire)" \ + bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 1 ]' _ "$fired2" + +# A DIFFERENT (kind,target) is a distinct throttle bucket -> fires independently. +run_notify "$dir2" high "Third title" "Third body" --kind foo --target issue:2 >/dev/null +check "scenario 2: a different target is an independent throttle bucket (now 2 fires total)" \ + bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 2 ]' _ "$fired2" + +# --------------------------------------------------------------------------- +# 3. --window overrides the default throttle window: a call using a 0-second +# window is effectively never throttled. +# --------------------------------------------------------------------------- +fired3="$work/scenario3-fired.txt" +dir3="$(new_fixture scenario3 "printf 'fired\n' >> $fired3")" +run_notify "$dir3" low "T" "B" --kind zero-window --target issue:9 --window 0 >/dev/null +run_notify "$dir3" low "T" "B" --kind zero-window --target issue:9 --window 0 >/dev/null +check "scenario 3: --window 0 never throttles (both calls fired)" bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 2 ]' _ "$fired3" + +# --------------------------------------------------------------------------- +# 4. --clear removes the throttle entry with NO notification, so a +# subsequent call for the same (kind,target) fires immediately again. +# --------------------------------------------------------------------------- +fired4="$work/scenario4-fired.txt" +dir4="$(new_fixture scenario4 "printf 'fired\n' >> $fired4")" +run_notify "$dir4" high "T" "B" --kind clear-me --target issue:5 >/dev/null +check "scenario 4: first call fired" bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 1 ]' _ "$fired4" +run_notify "$dir4" --clear --kind clear-me --target issue:5 +rc4clear=$? +check "scenario 4: --clear exits 0 and does NOT itself fire the command" bash -c '[ "'"$rc4clear"'" -eq 0 ] && [ "$(wc -l < "$1" | tr -d " ")" -eq 1 ]' _ "$fired4" +run_notify "$dir4" high "T2" "B2" --kind clear-me --target issue:5 >/dev/null +check "scenario 4: after --clear, the SAME (kind,target) fires again immediately (not throttled)" \ + bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 2 ]' _ "$fired4" + +# --------------------------------------------------------------------------- +# 5. Missing/unwritable state dir degrades gracefully (never crashes, never +# blocks the notification) -- guards the "read-only state dir" contract. +# --------------------------------------------------------------------------- +fired5="$work/scenario5-fired.txt" +dir5="$(new_fixture scenario5 "printf 'fired\n' >> $fired5")" +out5="$(CLAUDE_NOTIFY_THROTTLE_FILE="/nonexistent-dir-$$/notify-throttle.json" bash "$dir5/notify.sh" high T B --kind x --target y 2>&1)" +rc5=$? +check "scenario 5 (unwritable throttle path): exit 0 despite being unable to persist state" [ "$rc5" -eq 0 ] +check "scenario 5: the notification still fired despite the degraded state path" bash -c '[ -s "$1" ]' _ "$fired5" + +echo "" +if [ "$fail" -eq 0 ]; then + echo "notify.test.sh: PASS ($ok checks)" + exit 0 +else + echo "notify.test.sh: FAIL (see FAIL lines above)" + exit 1 +fi diff --git a/.claude/scripts/pr-feedback.sh b/.claude/scripts/pr-feedback.sh index 5159fba..bb9253d 100644 --- a/.claude/scripts/pr-feedback.sh +++ b/.claude/scripts/pr-feedback.sh @@ -24,6 +24,15 @@ repo="${1:-$(gh repo view --json nameWithOwner -q .nameWithOwner)}" bot="${BOT_LOGIN:-robercano-ghbot}" marker="<!-- claude-addressed -->" +# needs_human_flag/needs_human_clear (issue #99): the ONE shared label+notify +# seam for "CHANGES_REQUESTED round-trip done, owner re-review needed" (see +# the per-PR loop below). Sourced AFTER the `gh` wrapper above; guarded (not +# a bare `&&`) so a missing file under `set -e` never aborts the script. +# Bash functions are inherited by the `| while read; do ... done` subshell +# below, so defining these at top level is enough for the loop to use them. +# shellcheck source=needs-human.sh +if [ -f "$script_dir/needs-human.sh" ]; then . "$script_dir/needs-human.sh"; fi + gh pr list -R "$repo" --state open \ --json number,headRefName,author,labels \ --jq '.[] | select(.author.login=="'"$bot"'") | [.number, .headRefName, ([.labels[].name]|join(","))] | @tsv' \ @@ -42,6 +51,23 @@ gh pr list -R "$repo" --state open \ 2>/dev/null || true) if [ -z "$ta" ] || [[ "$tcr" > "$ta" ]]; then + # Needs bot action, not owner action -- ball is NOT in the owner's + # court right now, so clear any earlier "awaiting re-review" flag + # (issue #99). Best-effort no-op when needs-human.sh isn't sourced. + if command -v needs_human_clear >/dev/null 2>&1; then + needs_human_clear "pr:$num" "changes-requested" + fi printf '%s\t%s\t%s\t%s\n' "$num" "$branch" "$reviewer" "$tcr" + elif command -v needs_human_flag >/dev/null 2>&1; then + # Addressed (marker comment is newer than the last CHANGES_REQUESTED + # review) but GitHub still reports reviewDecision=CHANGES_REQUESTED + # until the owner submits a fresh review (see the file header) -- this + # IS the "round-trip done, re-review needed" block-on-owner point + # (issue #99). Cleared above the moment a FRESH CHANGES_REQUESTED + # arrives (back in the bot's court), or by merge-ready.sh once the PR + # merges. + needs_human_flag "pr:$num" "changes-requested" "low" \ + "PR #$num addressed feedback -- ready for re-review" \ + "$reviewer's changes-requested review was addressed; awaiting re-review." fi done diff --git a/.claude/scripts/seed-issues.sh b/.claude/scripts/seed-issues.sh index 945096b..dd2ba9c 100755 --- a/.claude/scripts/seed-issues.sh +++ b/.claude/scripts/seed-issues.sh @@ -53,6 +53,13 @@ gh label create "type:infra" --color b60205 --description "Repo-wide tooling/i # is what makes it loop-eligible (together with a module:* label). gh label create "backlog" --color bfd4f2 --description "Filed, not yet approved by the owner — the loop must NOT pick it up" --force >/dev/null 2>&1 && echo " label ✓ backlog" || true gh label create "planned" --color 0e8a16 --description "Owner-approved for the autonomous loop (assigned ONLY by the owner)" --force >/dev/null 2>&1 && echo " label ✓ planned" || true +# needs-human (issue #99): the loop's push-attention signal — applied by +# .claude/scripts/needs-human.sh at every block-on-owner point (attempt- +# budget/stall escalation, PR ready for review, CHANGES_REQUESTED addressed +# and awaiting re-review, ...) and removed once that condition clears. +# Seeded here too (idempotent --force) so a fresh repo has it before the loop +# ever needs it; needs-human.sh ALSO creates it lazily on first use either way. +gh label create "needs-human" --color b60205 --description "Loop is blocked on owner judgment -- see the issue/PR body/comments" --force >/dev/null 2>&1 && echo " label ✓ needs-human" || true # --- helper: create an issue unless an exact-title match already exists ------- existing="$(gh issue list --state all --limit 500 --json title -q '.[].title' 2>/dev/null)" diff --git a/.claude/self/gates.json b/.claude/self/gates.json index 87a0a0b..2e4a5bb 100644 --- a/.claude/self/gates.json +++ b/.claude/self/gates.json @@ -47,5 +47,8 @@ "stall_minutes": 30 }, - "merge": { "policy": "pr-per-agent", "baseBranch": "main" } + "merge": { "policy": "pr-per-agent", "baseBranch": "main" }, + + "_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`/`budget`). Empty string here = disabled (offline/CI-safe no-op); left EMPTY deliberately for this repo's own dogfooding — the owner picks a default later. Contract: severity/title/body-line reach the configured command BOTH as positional args ($1/$2/$3) and as NOTIFY_SEVERITY/NOTIFY_TITLE/NOTIFY_BODY env vars. Called by .claude/scripts/needs-human.sh's needs_human_flag() at every loop block-on-owner point (attempt-budget/stall escalation in loop-tick.sh, PR-ready-for-review/re-approval-needed in merge-ready.sh, CHANGES_REQUESTED-addressed-awaiting-re-review in pr-feedback.sh), 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": "" } diff --git a/.claude/skills/setup/SKILL.md b/.claude/skills/setup/SKILL.md index 861f182..53ed893 100644 --- a/.claude/skills/setup/SKILL.md +++ b/.claude/skills/setup/SKILL.md @@ -105,9 +105,10 @@ repo-level override elsewhere in `.gitignore` could still un-ignore one). ## 6. Create the module + approval labels For every module `name`: `bash ${CLAUDE_PLUGIN_ROOT:-.claude}/scripts/bot-gh.sh label create "module:<name>" --description "<desc>" --force`. -Also create the approval-workflow pair (if `gh label create` is unavailable in the installed gh, use `bot-gh.sh api repos/<owner>/<repo>/labels -f name=... -f color=... -f description=...`): +Also create the approval-workflow pair PLUS the needs-human signal label (if `gh label create` is unavailable in the installed gh, use `bot-gh.sh api repos/<owner>/<repo>/labels -f name=... -f color=... -f description=...`): - `backlog` (color `bfd4f2`) — "Filed, not yet approved by the owner — the loop must NOT pick it up" - `planned` (color `0e8a16`) — "Owner-approved for the autonomous loop (assigned ONLY by the owner)" +- `needs-human` (color `b60205`) — "Loop is blocked on owner judgment -- see the issue/PR body/comments" (issue #99: applied/removed by `.claude/scripts/needs-human.sh` at every block-on-owner point — PR ready for review, CHANGES_REQUESTED addressed and awaiting re-review, attempt-budget/stall escalation. Surfaced as a "Needs you" strip at the top of the cockpit dashboard, and optionally pushed via `.claude/scripts/notify.sh` if the adapter's `notify` command is configured.) Report created vs already-existing. Remind: **an issue is only loop-eligible once the OWNER labels it `planned` and it carries a `module:*` label**; issues agents file must be labelled `backlog`. From e17d2976268651c081eb60aacb9bb7ce25a54fb6 Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Thu, 16 Jul 2026 18:21:53 +0200 Subject: [PATCH 2/2] fix(loop): stop needs-human comment spam, keep census read-only (issue #99 re-review) Both reviewers rejected the original needs-human-signal work: - needs_human_flag now gates the GitHub comment on a FRESH escalation episode, derived from whether the needs-human label is already present on the target (read via `gh pr/issue view --json labels` before mutating). Label add + throttled notify still run every call; only the comment is first-transition-only, so a persisting block-on-owner condition no longer spams a fresh comment every loop tick. Fails open (comment still fires) if the label read itself fails. - pr-feedback.sh gains PR_FEEDBACK_COUNT_ONLY=1, which suppresses every needs_human_flag/needs_human_clear side effect while printing the identical TSV. loop-census.sh's feedback_prs= counter now sets this, so a read-only census can no longer mutate GitHub state; loop-tick.sh's real dispatch invocation is unchanged. - merge-ready.sh's needs-human comment body no longer leaks the raw "SKIP:..." verdict token. - cockpit.sh's "awaiting your review" strip now also includes PRs with no CI checks configured (previously silently omitted). Added merge-ready.test.sh and pr-feedback.test.sh running the REAL scripts (not fakes) against a stubbed bot-gh.sh, asserting the needs_human_flag/clear gh-call sequence including the new comment-gating and count-only behavior. Extended needs-human.test.sh (episode-gating scenarios), loop-ceilings.test.sh (updated gh-call counts for the new label-presence read), and cockpit.test.sh (no-checks PR coverage). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --- .claude/scripts/cockpit.sh | 18 +- .claude/scripts/cockpit.test.sh | 14 +- .claude/scripts/loop-ceilings.test.sh | 4 +- .claude/scripts/loop-census.sh | 9 +- .claude/scripts/merge-ready.sh | 9 +- .claude/scripts/merge-ready.test.sh | 231 +++++++++++++++++++++++ .claude/scripts/needs-human.sh | 48 ++++- .claude/scripts/needs-human.test.sh | 77 +++++++- .claude/scripts/pr-feedback.sh | 20 +- .claude/scripts/pr-feedback.test.sh | 261 ++++++++++++++++++++++++++ 10 files changed, 662 insertions(+), 29 deletions(-) create mode 100755 .claude/scripts/merge-ready.test.sh create mode 100755 .claude/scripts/pr-feedback.test.sh diff --git a/.claude/scripts/cockpit.sh b/.claude/scripts/cockpit.sh index 8a0550c..fb1e841 100755 --- a/.claude/scripts/cockpit.sh +++ b/.claude/scripts/cockpit.sh @@ -844,12 +844,16 @@ function renderRouting() { // - "needs-human": an issue OR PR carrying the `needs-human` label (see // needs-human.sh — the loop's escalation/PR-review/re-review points all // apply this label through the one shared helper). -// - "awaiting your review": a PR with passing CI whose review decision is -// neither APPROVED nor CHANGES_REQUESTED (i.e. REVIEW_REQUIRED or no -// review yet) — the owner hasn't weighed in yet. CHANGES_REQUESTED is -// deliberately excluded here: that PR is in the BOT's court (pr-feedback.sh -// dispatches a fix), not the owner's, until it's addressed (which is when -// it picks up the `needs-human` label instead — see pr-feedback.sh). +// - "awaiting your review": a PR with passing CI, OR with NO CI checks at +// all (ciBadge's "no checks" — a PR the adapter has no checks configured +// for is not stuck on a red/pending build; it's simply waiting on the +// owner, same as a "passing" one), whose review decision is neither +// APPROVED nor CHANGES_REQUESTED (i.e. REVIEW_REQUIRED or no review yet) +// — the owner hasn't weighed in yet. CHANGES_REQUESTED is deliberately +// excluded here: that PR is in the BOT's court (pr-feedback.sh dispatches +// a fix), not the owner's, until it's addressed (which is when it picks +// up the `needs-human` label instead — see pr-feedback.sh). A "failing" +// or "pending" PR is also excluded: that one's blocked on CI, not on you. // Degrades to an empty group set (never a crash) when issues/prs are // unavailable, matching every other section's degrade contract; renders a // clear all-clear state when the total across both groups is zero. @@ -875,7 +879,7 @@ function renderNeedsYou() { } const ci = ciBadge(pr.statusCheckRollup); const rd = pr.reviewDecision; - if (ci.label === "passing" && rd !== "APPROVED" && rd !== "CHANGES_REQUESTED") { + if ((ci.label === "passing" || ci.label === "no checks") && rd !== "APPROVED" && rd !== "CHANGES_REQUESTED") { push("awaiting your review", { num: pr.number, url: pr.url, title: pr.title }); } } diff --git a/.claude/scripts/cockpit.test.sh b/.claude/scripts/cockpit.test.sh index feb6735..0a658c0 100755 --- a/.claude/scripts/cockpit.test.sh +++ b/.claude/scripts/cockpit.test.sh @@ -380,9 +380,11 @@ check "no spend-ceiling state files: today's actions default to 0 / adapter ceil # from the SAME issues.json/prs.json every other section already reads # (no extra gh call). Two groups: # - "needs-human": an issue OR PR carrying the `needs-human` label. -# - "awaiting your review": a PR with passing CI and a review decision -# that is neither APPROVED nor CHANGES_REQUESTED (the latter is the -# BOT's court via pr-feedback.sh, not the owner's). +# - "awaiting your review": a PR with passing CI (OR no CI checks +# configured at all -- re-review finding, a review-ready PR with zero +# checks must not be silently omitted) and a review decision that is +# neither APPROVED nor CHANGES_REQUESTED (the latter is the BOT's +# court via pr-feedback.sh, not the owner's). # PR 200 (APPROVED, passing) and PR 201 (CHANGES_REQUESTED, failing) must # NOT appear in either group. # --------------------------------------------------------------------------- @@ -398,7 +400,8 @@ cat > "$work/fixtures-needs-you/prs.json" <<'EOF' {"number":200,"title":"Approved PR","url":"https://example.com/pr/200","headRefName":"feat/x","reviewDecision":"APPROVED","statusCheckRollup":[{"conclusion":"SUCCESS","status":"COMPLETED","name":"build"}],"labels":[]}, {"number":201,"title":"Changes requested PR","url":"https://example.com/pr/201","headRefName":"feat/y","reviewDecision":"CHANGES_REQUESTED","statusCheckRollup":[{"conclusion":"FAILURE","status":"COMPLETED","name":"test"}],"labels":[]}, {"number":202,"title":"Needs-human PR","url":"https://example.com/pr/202","headRefName":"feat/z","reviewDecision":null,"statusCheckRollup":[{"conclusion":"SUCCESS","status":"COMPLETED","name":"build"}],"labels":[{"name":"needs-human"}]}, - {"number":203,"title":"Awaiting review PR","url":"https://example.com/pr/203","headRefName":"feat/w","reviewDecision":"REVIEW_REQUIRED","statusCheckRollup":[{"conclusion":"SUCCESS","status":"COMPLETED","name":"build"}],"labels":[]} + {"number":203,"title":"Awaiting review PR","url":"https://example.com/pr/203","headRefName":"feat/w","reviewDecision":"REVIEW_REQUIRED","statusCheckRollup":[{"conclusion":"SUCCESS","status":"COMPLETED","name":"build"}],"labels":[]}, + {"number":204,"title":"No-checks PR awaiting review","url":"https://example.com/pr/204","headRefName":"feat/v","reviewDecision":null,"statusCheckRollup":[],"labels":[]} ] EOF : >"$work/fixtures-needs-you/events.jsonl" @@ -418,7 +421,7 @@ check "needs-you section is the FIRST section in <body> (before live/issues/prs) throw new Error("needs-you is not the first section after <body>"); } ' "$html_needs_you" -check "needs-you total count is 3 (issue 106 + PR 202 + PR 203)" grep -qF '<h2>Needs you (3)</h2>' "$html_needs_you" +check "needs-you total count is 4 (issue 106 + PR 202 + PR 203 + PR 204)" grep -qF '<h2>Needs you (4)</h2>' "$html_needs_you" needs_you_section="$(node -e ' const fs = require("fs"); const html = fs.readFileSync(process.argv[1], "utf8"); @@ -431,6 +434,7 @@ check "awaiting-your-review group heading present" bash -c 'printf "%s" "$1" | g check "needs-human group lists issue #106" bash -c 'printf "%s" "$1" | grep -qF "<a href=\"https://example.com/106\">#106</a> Issue needing a human"' _ "$needs_you_section" check "needs-human group lists PR #202 (not the awaiting-review group)" bash -c 'printf "%s" "$1" | grep -qF "<a href=\"https://example.com/pr/202\">#202</a> Needs-human PR"' _ "$needs_you_section" check "awaiting-your-review group lists PR #203" bash -c 'printf "%s" "$1" | grep -qF "<a href=\"https://example.com/pr/203\">#203</a> Awaiting review PR"' _ "$needs_you_section" +check "awaiting-your-review group ALSO lists PR #204 (no CI checks configured, must not be silently omitted)" bash -c 'printf "%s" "$1" | grep -qF "<a href=\"https://example.com/pr/204\">#204</a> No-checks PR awaiting review"' _ "$needs_you_section" check "approved PR #200 does NOT appear in the needs-you strip" bash -c '! printf "%s" "$1" | grep -qF "#200"' _ "$needs_you_section" check "changes-requested PR #201 does NOT appear in the needs-you strip (bot's court, not owner's)" bash -c '! printf "%s" "$1" | grep -qF "#201"' _ "$needs_you_section" check "ordinary issue #100 does NOT appear in the needs-you strip" bash -c '! printf "%s" "$1" | grep -qF "#100"' _ "$needs_you_section" diff --git a/.claude/scripts/loop-ceilings.test.sh b/.claude/scripts/loop-ceilings.test.sh index 59a98c3..7fca7e2 100644 --- a/.claude/scripts/loop-ceilings.test.sh +++ b/.claude/scripts/loop-ceilings.test.sh @@ -226,7 +226,7 @@ node -e ' out5="$(run_tick "$dir5")" check "scenario 5 (attempts 5 >= budget 5): verdict is action=none" bash -c '[ "$(verdict_of "$1")" = "action=none" ]' _ "$out5" check "scenario 5: diagnostic cites the attempt budget" bash -c 'printf "%s\n" "$1" | grep -q "attempt budget exceeded for issue=42"' _ "$out5" -check "scenario 5: exactly 3 gh calls (label create, issue edit, issue comment)" bash -c '[ "$(gh_calls "$1" | wc -l | tr -d " ")" -eq 3 ]' _ "$dir5" +check "scenario 5: exactly 4 gh calls (label-presence read, label create, issue edit, issue comment)" bash -c '[ "$(gh_calls "$1" | wc -l | tr -d " ")" -eq 4 ]' _ "$dir5" check "scenario 5: the issue itself (not a PR) was labeled needs-human" bash -c 'gh_calls "$1" | grep -q "^issue edit 42 --add-label needs-human"' _ "$dir5" check "scenario 5: escalated is now persisted true, attempts unchanged at 5" node -e ' const fs = require("fs"); @@ -235,7 +235,7 @@ check "scenario 5: escalated is now persisted true, attempts unchanged at 5" nod ' "$dir5/../state/loop-issue-attempts.json" out5b="$(run_tick "$dir5")" check "scenario 5b (still over budget, second tick): verdict is still action=none" bash -c '[ "$(verdict_of "$1")" = "action=none" ]' _ "$out5b" -check "scenario 5b: no additional gh calls (escalated guard held) -- still exactly 3" bash -c '[ "$(gh_calls "$1" | wc -l | tr -d " ")" -eq 3 ]' _ "$dir5" +check "scenario 5b: no additional gh calls (escalated guard held) -- still exactly 4" bash -c '[ "$(gh_calls "$1" | wc -l | tr -d " ")" -eq 4 ]' _ "$dir5" # --------------------------------------------------------------------------- # 6. Per-issue attempt budget applies across advance AND feedback phases of diff --git a/.claude/scripts/loop-census.sh b/.claude/scripts/loop-census.sh index 91250ec..238f878 100644 --- a/.claude/scripts/loop-census.sh +++ b/.claude/scripts/loop-census.sh @@ -192,7 +192,14 @@ open_pr_branches=$(gh pr list -R "$repo" --state open --base "$base" --json head # i.e. the blocking gate degrades to a no-op (same as before this feature). open_issue_set=$(gh issue list -R "$repo" --state open --json number --jq '.[].number' --limit 200 2>/dev/null) || true -feedback_prs=$(bash "$script_dir/pr-feedback.sh" "$repo" | grep -c . || true) +# PR_FEEDBACK_COUNT_ONLY=1 (issue #99 re-review finding #2): census is a +# read-only report -- it must NEVER mutate GitHub state. pr-feedback.sh's own +# per-PR loop calls needs_human_flag/needs_human_clear (label/comment/notify) +# as a side effect of its real dispatch role; count-only mode suppresses all +# of that while still printing the identical TSV this line counts. The real, +# side-effecting invocation stays in loop-tick.sh, which actually dispatches +# fixes for the PRs this counts. +feedback_prs=$(PR_FEEDBACK_COUNT_ONLY=1 bash "$script_dir/pr-feedback.sh" "$repo" | grep -c . || true) echo "feedback_prs=$feedback_prs" # Open `planned` issues carrying any of the adapter's module labels, ascending. diff --git a/.claude/scripts/merge-ready.sh b/.claude/scripts/merge-ready.sh index 5315161..fa7af23 100644 --- a/.claude/scripts/merge-ready.sh +++ b/.claude/scripts/merge-ready.sh @@ -101,8 +101,15 @@ for n in $(gh pr list -R "$repo" --base "$base" --state open --json number -q '. case "$verdict" in SKIP:no-owner-review|SKIP:approval-stale*) if command -v needs_human_flag >/dev/null 2>&1; then + # Human-readable reason, not the raw "SKIP:..." verdict token (non- + # blocking re-review nit): only these two verdicts reach this branch, + # so a simple case is enough -- no need to reformat the token itself. + reason_text="not yet reviewed" + case "$verdict" in + SKIP:approval-stale*) reason_text="approval is stale -- please re-review the current head" ;; + esac needs_human_flag "pr:$n" "pr-review" "low" \ - "PR #$n ready for your review" "$title (${verdict#SKIP:})" + "PR #$n ready for your review" "$title ($reason_text)" fi ;; *) diff --git a/.claude/scripts/merge-ready.test.sh b/.claude/scripts/merge-ready.test.sh new file mode 100755 index 0000000..5c4e2b6 --- /dev/null +++ b/.claude/scripts/merge-ready.test.sh @@ -0,0 +1,231 @@ +#!/usr/bin/env bash +# merge-ready.test.sh — offline smoke test for the REAL merge-ready.sh (issue +# #99 re-review finding #3). Every other suite that touches merge-ready.sh +# (loop-tick.test.sh) stubs it with a fake `echo` — this test runs the ACTUAL +# script, with a stubbed bot-gh.sh answering canned `gh pr list`/`pr view`/ +# `pr merge` JSON per verdict path, and asserts the resulting +# needs_human_flag/needs_human_clear gh-call sequence (label add/remove, +# comment, `pr merge`) that flows through the real needs-human.sh seam this +# script sources. +# +# Exit 0 on success, non-zero if any assertion fails. Runnable bare: +# bash .claude/scripts/merge-ready.test.sh +set -uo pipefail + +# Isolate from the CALLER's environment, matching needs-human.test.sh / +# loop-census.test.sh: an ambient GATES_FILE (e.g. from a self-host gate run) +# would leak into every fixture's own gates.json lookup below. +unset GATES_FILE + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +merge_ready_src="$script_dir/merge-ready.sh" + +work="$(mktemp -d "${TMPDIR:-/tmp}/merge-ready-test.XXXXXX")" +trap 'rm -rf "$work"' EXIT + +fail=0 +ok=0 +check() { + local desc="$1"; shift + if "$@"; then + ok=$((ok + 1)) + echo "ok - $desc" + else + fail=1 + echo "FAIL - $desc" + fi +} + +# new_fixture: a throwaway <dir>/.claude/{scripts,state} tree with the REAL +# merge-ready.sh + resolve-roots.sh + needs-human.sh + notify.sh copied in +# (never reimplemented), a minimal gates.json (base=main, notify command a +# local file append so the notify.sh leg of the seam is also exercised +# offline), and a no-op worktree-cleanup.sh (its own behavior is covered by +# worktree-cleanup.test.sh; here it must just not blow up merge-ready.sh's +# `while read` over its stdout when a PR merges). +new_fixture() { + local name="$1" + local dir="$work/$name" + local scripts="$dir/.claude/scripts" + mkdir -p "$scripts" "$dir/.claude/state" + cp "$merge_ready_src" "$scripts/merge-ready.sh" + cp "$script_dir/resolve-roots.sh" "$scripts/resolve-roots.sh" + cp "$script_dir/needs-human.sh" "$scripts/needs-human.sh" + cp "$script_dir/notify.sh" "$scripts/notify.sh" + chmod +x "$scripts"/*.sh + cat > "$dir/.claude/gates.json" <<EOF +{ "merge": { "baseBranch": "main" }, "notify": "printf 'fired\\n' >> $work/$name-notify-fired.txt" } +EOF + cat > "$scripts/worktree-cleanup.sh" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF + chmod +x "$scripts/worktree-cleanup.sh" + printf '%s\n' "$dir" +} + +# --------------------------------------------------------------------------- +# A. SKIP:no-owner-review, run TWICE in a row (simulating two loop ticks with +# the PR still unreviewed) -- proves the real merge-ready.sh wiring only +# posts ONE GitHub comment across the whole episode (issue #99 re-review +# finding #1), while the label is (re-)applied and the verdict is skipped +# both times. The fake bot-gh.sh tracks "was the label already applied?" +# via a marker FILE (persists across the two invocations, exactly like a +# real needs-human label persists across real loop ticks), so the second +# run's `_needs_human_already_labeled` read reports "yes" and the comment +# is skipped on that second run. +# --------------------------------------------------------------------------- +dirA="$(new_fixture scenarioA)" +gh_logA="$work/scenarioA-gh.log" +labeled_markerA="$work/scenarioA-labeled.marker" +cat > "$dirA/.claude/scripts/bot-gh.sh" <<EOF +#!/usr/bin/env bash +printf '%s\n' "\$*" >> "$gh_logA" +case "\$1" in + pr) + case "\$2" in + list) + if printf '%s\n' "\$*" | grep -q -- '--json number'; then + echo "10" + fi + ;; + view) + if printf '%s\n' "\$*" | grep -q -- '--json labels'; then + [ -f "$labeled_markerA" ] && printf 'needs-human\n' + else + cat <<'JSON' +{"number":10,"title":"Add widget","isDraft":false,"baseRefName":"main","headRefName":"feat/issue-10-widget","mergeable":"MERGEABLE","reviews":[],"statusCheckRollup":[],"commits":[{"committedDate":"2026-01-01T00:00:00Z"}]} +JSON + fi + ;; + edit) + if printf '%s\n' "\$*" | grep -q -- '--add-label needs-human'; then + touch "$labeled_markerA" + fi + ;; + comment) : ;; + merge) exit 1 ;; + *) : ;; + esac + ;; + label) : ;; + issue) : ;; + *) echo "unhandled: \$*" >&2; exit 1 ;; +esac +EOF +chmod +x "$dirA/.claude/scripts/bot-gh.sh" + +outA1="$(env -u GATES_FILE bash "$dirA/.claude/scripts/merge-ready.sh" "acme/repo" 2>&1)" +outA2="$(env -u GATES_FILE bash "$dirA/.claude/scripts/merge-ready.sh" "acme/repo" 2>&1)" + +check "A: first run's verdict is skip:no-owner-review" bash -c 'printf "%s\n" "$1" | grep -q "\"reason\":\"no-owner-review\""' _ "$outA1" +check "A: second run's verdict is ALSO skip:no-owner-review (still unreviewed)" bash -c 'printf "%s\n" "$1" | grep -q "\"reason\":\"no-owner-review\""' _ "$outA2" +check "A: needs-human label add attempted on BOTH runs" bash -c '[ "$(grep -c "pr edit 10 --add-label needs-human" "$1")" -eq 2 ]' _ "$gh_logA" +check "A: exactly ONE comment across BOTH runs (episode-gated, finding #1)" bash -c '[ "$(grep -c "pr comment 10 --body" "$1")" -eq 1 ]' _ "$gh_logA" +check "A: no merge was attempted (skip path)" bash -c '! grep -q "^pr merge" "$1"' _ "$gh_logA" + +# --------------------------------------------------------------------------- +# B. SKIP:approval-stale (an APPROVED review exists but predates the PR's +# latest commit -- a push landed after the approval) -- same flag path, +# different verdict reason. Single run: proves the OTHER skip-reason that +# triggers needs_human_flag, distinct from "no review at all". +# --------------------------------------------------------------------------- +dirB="$(new_fixture scenarioB)" +gh_logB="$work/scenarioB-gh.log" +cat > "$dirB/.claude/scripts/bot-gh.sh" <<EOF +#!/usr/bin/env bash +printf '%s\n' "\$*" >> "$gh_logB" +case "\$1" in + pr) + case "\$2" in + list) + if printf '%s\n' "\$*" | grep -q -- '--json number'; then + echo "11" + fi + ;; + view) + if printf '%s\n' "\$*" | grep -q -- '--json labels'; then + : # never labeled yet -- fresh episode + else + cat <<'JSON' +{"number":11,"title":"Fix bug","isDraft":false,"baseRefName":"main","headRefName":"fix/issue-11-bug","mergeable":"MERGEABLE","reviews":[{"author":{"login":"acme"},"state":"APPROVED","submittedAt":"2026-01-01T00:00:00Z"}],"statusCheckRollup":[],"commits":[{"committedDate":"2026-01-02T00:00:00Z"}]} +JSON + fi + ;; + edit) : ;; + comment) : ;; + merge) exit 1 ;; + *) : ;; + esac + ;; + label) : ;; + issue) : ;; + *) echo "unhandled: \$*" >&2; exit 1 ;; +esac +EOF +chmod +x "$dirB/.claude/scripts/bot-gh.sh" +outB="$(env -u GATES_FILE bash "$dirB/.claude/scripts/merge-ready.sh" "acme/repo" 2>&1)" + +check "B: verdict is skip:approval-stale" bash -c 'printf "%s\n" "$1" | grep -q "approval-stale"' _ "$outB" +check "B: label add attempted (fresh episode)" grep -q "pr edit 11 --add-label needs-human" "$gh_logB" +check "B: comment posted (fresh episode, ready-for-review body)" bash -c 'grep -q "pr comment 11 --body" "$1"' _ "$gh_logB" + +# --------------------------------------------------------------------------- +# C. Successful MERGE: owner-approved, CI-green, head_branch matches +# feat/issue-<N>-*. Asserts the on-merge clear fan-out: pr:12's pr-review +# AND changes-requested flags clear, AND (via the head_branch->issue-number +# regex) issue:77's attempt-budget AND stall flags clear too. +# --------------------------------------------------------------------------- +dirC="$(new_fixture scenarioC)" +gh_logC="$work/scenarioC-gh.log" +cat > "$dirC/.claude/scripts/bot-gh.sh" <<EOF +#!/usr/bin/env bash +printf '%s\n' "\$*" >> "$gh_logC" +case "\$1" in + pr) + case "\$2" in + list) + if printf '%s\n' "\$*" | grep -q -- '--json number'; then + echo "12" + fi + ;; + view) + if printf '%s\n' "\$*" | grep -q -- '--json labels'; then + : # not relevant on the merge path + else + cat <<'JSON' +{"number":12,"title":"Ship feature","isDraft":false,"baseRefName":"main","headRefName":"feat/issue-77-thing","mergeable":"MERGEABLE","reviews":[{"author":{"login":"acme"},"state":"APPROVED","submittedAt":"2026-01-02T00:00:00Z"}],"statusCheckRollup":[],"commits":[{"committedDate":"2026-01-01T00:00:00Z"}]} +JSON + fi + ;; + merge) exit 0 ;; + edit) : ;; + comment) : ;; + *) : ;; + esac + ;; + label) : ;; + issue) : ;; + *) echo "unhandled: \$*" >&2; exit 1 ;; +esac +EOF +chmod +x "$dirC/.claude/scripts/bot-gh.sh" +outC="$(env -u GATES_FILE bash "$dirC/.claude/scripts/merge-ready.sh" "acme/repo" 2>&1)" + +check "C: PR merged" bash -c 'printf "%s\n" "$1" | grep -q "\"action\":\"merged\""' _ "$outC" +check "C: gh pr merge invoked with --merge --delete-branch" grep -q "pr merge 12 -R acme/repo --merge --delete-branch" "$gh_logC" +check "C: no comment posted on the merge path (verdict never hits the flag case)" bash -c '! grep -q "^pr comment" "$1"' _ "$gh_logC" +check "C: pr:12 pr-review/changes-requested cleared -- 'pr edit 12 --remove-label needs-human' appears 3x (case-default once, success block twice)" \ + bash -c '[ "$(grep -c "pr edit 12 --remove-label needs-human" "$1")" -eq 3 ]' _ "$gh_logC" +check "C: head_branch->issue-number regex clears issue 77's attempt-budget AND stall (2x 'issue edit 77 --remove-label needs-human')" \ + bash -c '[ "$(grep -c "issue edit 77 --remove-label needs-human" "$1")" -eq 2 ]' _ "$gh_logC" +check "C: no needs-human label/comment ever added on the merge path" bash -c '! grep -q "add-label" "$1"' _ "$gh_logC" + +echo "" +if [ "$fail" -eq 0 ]; then + echo "merge-ready.test.sh: PASS ($ok checks)" + exit 0 +else + echo "merge-ready.test.sh: FAIL (see FAIL lines above)" + exit 1 +fi diff --git a/.claude/scripts/needs-human.sh b/.claude/scripts/needs-human.sh index a837ba8..2b8d8e3 100644 --- a/.claude/scripts/needs-human.sh +++ b/.claude/scripts/needs-human.sh @@ -26,10 +26,27 @@ # independently, while repeats of the SAME kind on the SAME target # stay throttled to notify.sh's window. # SEVERITY/TITLE/BODY: passed straight through to notify.sh; BODY is also -# posted as a comment on TARGET (skipped when BODY is empty). +# posted as a comment on TARGET -- but ONLY on a FRESH escalation +# episode (see below); always skipped when BODY is empty. # Idempotent: `gh label create --force` never fails if the label already # exists; adding an already-present label is a no-op on GitHub's side. # +# Comment is first-transition-only (issue #99 re-review finding #1): BEFORE +# (re-)adding the label, this reads TARGET's CURRENT labels from GitHub. If +# `needs-human` is already present, this call is a REPEAT of an ongoing +# escalation episode -- the label add + notify (still throttled by +# notify.sh) still run, but the COMMENT is skipped, so a persisting +# block-on-owner condition doesn't spam a fresh GitHub comment every tick +# (loop-tick.sh/loop-census.sh invoke the callers of this seam every few +# minutes for as long as the condition holds). If `needs-human` is ABSENT, +# this is a fresh episode (first flag ever, or a prior episode was cleared) +# and the comment posts. Deriving "fresh episode" from GitHub's own label +# state (rather than a local state file) makes this self-heal across +# restarts/redeploys with no extra state to keep in sync. A failed label +# read (offline/unauthenticated) is treated as "not already labeled" so the +# comment still fires -- fail toward the OLD (safe, if noisier) behavior, +# never toward silently swallowing an escalation. +# # needs_human_clear TARGET KIND # Removes the needs-human label from TARGET (best-effort — a target that # was never labeled just no-ops) and clears notify.sh's throttle entry for @@ -50,10 +67,35 @@ _needs_human_split_target() { NH_NUM="${t#*:}" } +# _needs_human_already_labeled: prints "yes" if TARGET (NH_TYPE/NH_NUM, must +# already be split) currently carries the needs-human label on GitHub, "no" +# otherwise -- including on any read failure (offline/unauthenticated/missing +# gh), so callers fail toward still posting the comment (the old behavior) +# rather than toward silently swallowing a fresh escalation. Read-only: never +# mutates anything. +_needs_human_already_labeled() { + local out="" + case "$NH_TYPE" in + pr) out="$(gh pr view "$NH_NUM" --json labels -q '.labels[].name' 2>/dev/null)" || out="" ;; + issue) out="$(gh issue view "$NH_NUM" --json labels -q '.labels[].name' 2>/dev/null)" || out="" ;; + *) out="" ;; + esac + if printf '%s\n' "$out" | grep -qx "needs-human"; then + printf 'yes\n' + else + printf 'no\n' + fi +} + needs_human_flag() { local target="$1" kind="$2" severity="$3" title="$4" body="$5" _needs_human_split_target "$target" + # Read BEFORE mutating: this call's own label add below must not make + # itself look like a "repeat" episode. + local fresh_episode="yes" + { [ "$(_needs_human_already_labeled)" = "yes" ] && fresh_episode="no"; } || true + gh label create "needs-human" --color b60205 \ --description "Loop is blocked on owner judgment -- see the issue/PR body/comments" \ --force >/dev/null 2>&1 || true @@ -66,11 +108,11 @@ needs_human_flag() { case "$NH_TYPE" in pr) gh pr edit "$NH_NUM" --add-label needs-human >/dev/null 2>&1 || true - { [ -n "$body" ] && gh pr comment "$NH_NUM" --body "$body" >/dev/null 2>&1; } || true + { [ "$fresh_episode" = "yes" ] && [ -n "$body" ] && gh pr comment "$NH_NUM" --body "$body" >/dev/null 2>&1; } || true ;; issue) gh issue edit "$NH_NUM" --add-label needs-human >/dev/null 2>&1 || true - { [ -n "$body" ] && gh issue comment "$NH_NUM" --body "$body" >/dev/null 2>&1; } || true + { [ "$fresh_episode" = "yes" ] && [ -n "$body" ] && gh issue comment "$NH_NUM" --body "$body" >/dev/null 2>&1; } || true ;; *) ;; esac diff --git a/.claude/scripts/needs-human.test.sh b/.claude/scripts/needs-human.test.sh index be767e7..261845f 100755 --- a/.claude/scripts/needs-human.test.sh +++ b/.claude/scripts/needs-human.test.sh @@ -57,8 +57,10 @@ new_fixture() { } # --------------------------------------------------------------------------- -# 1. needs_human_flag on an ISSUE target: label create + issue edit -# --add-label + issue comment (in that order), all via the stubbed `gh`. +# 1. needs_human_flag on an ISSUE target, FRESH episode (stub `gh` returns no +# labels, so the pre-flight presence check reports "not already labeled"): +# label-presence read + label create + issue edit --add-label + issue +# comment (in that order), all via the stubbed `gh`. # --------------------------------------------------------------------------- dir1="$(new_fixture scenario1 "")" gh_log1="$work/scenario1-gh.log" @@ -69,10 +71,11 @@ out1="$(bash -c ' ' 2>&1)" rc1=$? check "scenario 1: needs_human_flag exits 0" [ "$rc1" -eq 0 ] -check "scenario 1: label create is the FIRST gh call" bash -c 'head -1 "$1" | grep -q "^label create needs-human"' _ "$gh_log1" -check "scenario 1: issue edit --add-label needs-human is the SECOND gh call" bash -c 'sed -n 2p "$1" | grep -qF "issue edit 42 --add-label needs-human"' _ "$gh_log1" -check "scenario 1: issue comment with the body is the THIRD gh call" bash -c 'sed -n 3p "$1" | grep -qF "issue comment 42 --body Please look at issue 42"' _ "$gh_log1" -check "scenario 1: exactly 3 gh calls (no extra side effects)" bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 3 ]' _ "$gh_log1" +check "scenario 1: label-presence check is the FIRST gh call" bash -c 'head -1 "$1" | grep -q "^issue view 42 --json labels"' _ "$gh_log1" +check "scenario 1: label create is the SECOND gh call" bash -c 'sed -n 2p "$1" | grep -q "^label create needs-human"' _ "$gh_log1" +check "scenario 1: issue edit --add-label needs-human is the THIRD gh call" bash -c 'sed -n 3p "$1" | grep -qF "issue edit 42 --add-label needs-human"' _ "$gh_log1" +check "scenario 1: issue comment with the body is the FOURTH gh call" bash -c 'sed -n 4p "$1" | grep -qF "issue comment 42 --body Please look at issue 42"' _ "$gh_log1" +check "scenario 1: exactly 4 gh calls (no extra side effects)" bash -c '[ "$(wc -l < "$1" | tr -d " ")" -eq 4 ]' _ "$gh_log1" # --------------------------------------------------------------------------- # 2. needs_human_flag on a PR target: `pr edit`/`pr comment`, not `issue *`. @@ -155,6 +158,68 @@ out6="$(bash -c ' ' 2>&1)" check "scenario 6: caller survives (prints SURVIVED) even when every gh call fails" bash -c 'printf "%s\n" "$1" | grep -q "SURVIVED"' _ "$out6" +# --------------------------------------------------------------------------- +# 7. Comment episode-gating (issue #99 re-review finding #1): a REPEAT flag +# call on a target that ALREADY carries the needs-human label (per the +# stubbed `gh issue view --json labels` read) still (re-)applies the label +# but SKIPS the comment -- this is the fix for the "fresh GitHub comment +# every tick" bug the reviewer reproduced. +# --------------------------------------------------------------------------- +dir7="$(new_fixture scenario7 "")" +gh_log7="$work/scenario7-gh.log" +bash -c ' + gh() { + printf "%s\n" "$*" >> "'"$gh_log7"'" + case "$*" in + "issue view 50 --json labels -q .labels[].name") printf "needs-human\n" ;; + *) : ;; + esac + } + . "'"$dir7"'/.claude/scripts/needs-human.sh" + needs_human_flag "issue:50" "attempt-budget" "high" "T" "already labeled body" +' >/dev/null 2>&1 +check "scenario 7: label is still (re-)applied when already present" grep -qF "issue edit 50 --add-label needs-human" "$gh_log7" +check "scenario 7: comment is SKIPPED because the label was already present" bash -c '! grep -q "^issue comment" "$1"' _ "$gh_log7" + +# --------------------------------------------------------------------------- +# 8. Same target shape, label ABSENT (a fresh episode -- e.g. right after a +# needs_human_clear, or the very first flag ever) -- comment posts. +# --------------------------------------------------------------------------- +dir8="$(new_fixture scenario8 "")" +gh_log8="$work/scenario8-gh.log" +bash -c ' + gh() { + printf "%s\n" "$*" >> "'"$gh_log8"'" + case "$*" in + "issue view 51 --json labels -q .labels[].name") printf "some-other-label\n" ;; + *) : ;; + esac + } + . "'"$dir8"'/.claude/scripts/needs-human.sh" + needs_human_flag "issue:51" "attempt-budget" "high" "T" "fresh episode body" +' >/dev/null 2>&1 +check "scenario 8: comment posts on a fresh episode (needs-human label absent)" grep -qF "issue comment 51 --body fresh episode body" "$gh_log8" + +# --------------------------------------------------------------------------- +# 9. A failing label READ (offline/unauthenticated `gh issue view`) fails +# OPEN toward posting the comment -- the old, safe (if noisier) behavior +# -- never toward silently swallowing a genuinely fresh escalation. +# --------------------------------------------------------------------------- +dir9="$(new_fixture scenario9 "")" +gh_log9="$work/scenario9-gh.log" +bash -c ' + gh() { + printf "%s\n" "$*" >> "'"$gh_log9"'" + case "$*" in + "issue view 52 --json labels -q .labels[].name") return 1 ;; + *) : ;; + esac + } + . "'"$dir9"'/.claude/scripts/needs-human.sh" + needs_human_flag "issue:52" "attempt-budget" "high" "T" "fail-open body" +' >/dev/null 2>&1 +check "scenario 9: a failing label read fails OPEN -- comment still posts" grep -qF "issue comment 52 --body fail-open body" "$gh_log9" + echo "" if [ "$fail" -eq 0 ]; then echo "needs-human.test.sh: PASS ($ok checks)" diff --git a/.claude/scripts/pr-feedback.sh b/.claude/scripts/pr-feedback.sh index bb9253d..38b00b3 100644 --- a/.claude/scripts/pr-feedback.sh +++ b/.claude/scripts/pr-feedback.sh @@ -13,6 +13,15 @@ # # Repo derived from the git remote; override with $1. Bot login via $BOT_LOGIN. # Invoke as `bash .claude/scripts/pr-feedback.sh` (pre-approve that exact command). +# +# PR_FEEDBACK_COUNT_ONLY=1 (issue #99 re-review finding #2): a PURE counting +# mode that suppresses every needs_human_flag/needs_human_clear call (no +# label add/remove, no comment, no notify) while still printing the exact +# same TSV lines. loop-census.sh sets this when it shells out here ONLY to +# `grep -c .` the count for its feedback_prs= line -- a read-only census must +# never have side effects on GitHub state. loop-tick.sh's own invocation +# (which dispatches real fixes) does NOT set this, so it keeps flagging/ +# clearing exactly as before. set -euo pipefail # Two-root derivation (issue #63): script_dir = sibling scripts, root = consumer project. @@ -32,6 +41,7 @@ marker="<!-- claude-addressed -->" # below, so defining these at top level is enough for the loop to use them. # shellcheck source=needs-human.sh if [ -f "$script_dir/needs-human.sh" ]; then . "$script_dir/needs-human.sh"; fi +count_only="${PR_FEEDBACK_COUNT_ONLY:-0}" gh pr list -R "$repo" --state open \ --json number,headRefName,author,labels \ @@ -53,19 +63,21 @@ gh pr list -R "$repo" --state open \ if [ -z "$ta" ] || [[ "$tcr" > "$ta" ]]; then # Needs bot action, not owner action -- ball is NOT in the owner's # court right now, so clear any earlier "awaiting re-review" flag - # (issue #99). Best-effort no-op when needs-human.sh isn't sourced. - if command -v needs_human_clear >/dev/null 2>&1; then + # (issue #99). Best-effort no-op when needs-human.sh isn't sourced, and + # SKIPPED entirely in count-only mode (finding #2 -- a pure counter must + # not mutate GitHub state). + if [ "$count_only" != "1" ] && command -v needs_human_clear >/dev/null 2>&1; then needs_human_clear "pr:$num" "changes-requested" fi printf '%s\t%s\t%s\t%s\n' "$num" "$branch" "$reviewer" "$tcr" - elif command -v needs_human_flag >/dev/null 2>&1; then + elif [ "$count_only" != "1" ] && command -v needs_human_flag >/dev/null 2>&1; then # Addressed (marker comment is newer than the last CHANGES_REQUESTED # review) but GitHub still reports reviewDecision=CHANGES_REQUESTED # until the owner submits a fresh review (see the file header) -- this # IS the "round-trip done, re-review needed" block-on-owner point # (issue #99). Cleared above the moment a FRESH CHANGES_REQUESTED # arrives (back in the bot's court), or by merge-ready.sh once the PR - # merges. + # merges. Skipped entirely in count-only mode (finding #2). needs_human_flag "pr:$num" "changes-requested" "low" \ "PR #$num addressed feedback -- ready for re-review" \ "$reviewer's changes-requested review was addressed; awaiting re-review." diff --git a/.claude/scripts/pr-feedback.test.sh b/.claude/scripts/pr-feedback.test.sh new file mode 100755 index 0000000..b72daa9 --- /dev/null +++ b/.claude/scripts/pr-feedback.test.sh @@ -0,0 +1,261 @@ +#!/usr/bin/env bash +# pr-feedback.test.sh — offline smoke test for the REAL pr-feedback.sh (issue +# #99 re-review finding #4). loop-tick.test.sh/loop-census.test.sh only ever +# stub pr-feedback.sh out entirely — this test runs the ACTUAL script, with a +# stubbed bot-gh.sh answering canned `pr list`/`api .../reviews`/`api +# .../comments` output per scenario, and asserts: +# - an UNADDRESSED changes-requested PR is listed in the TSV AND clears any +# earlier "awaiting re-review" needs-human flag (ball is in the bot's +# court, not the owner's) +# - an ADDRESSED PR (marker comment newer than the last CHANGES_REQUESTED +# review) is NOT listed, and instead FLAGS needs-human (owner's turn) -- +# but only posts the GitHub comment on a FRESH escalation episode; a +# REPEAT run with the label already applied skips the comment (issue #99 +# re-review finding #1, exercised here through pr-feedback.sh's own +# wiring, not just needs-human.sh's generic seam) +# - a PR already labeled `claude-addressing` is skipped before any review/ +# comment lookup at all +# - PR_FEEDBACK_COUNT_ONLY=1 (finding #2) preserves the exact same TSV +# output while suppressing every label/comment/notify side effect, on +# BOTH the unaddressed and addressed paths -- this is the mode +# loop-census.sh now uses so a read-only census never mutates GitHub +# state. +# +# Exit 0 on success, non-zero if any assertion fails. Runnable bare: +# bash .claude/scripts/pr-feedback.test.sh +set -uo pipefail + +unset GATES_FILE + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +pr_feedback_src="$script_dir/pr-feedback.sh" + +work="$(mktemp -d "${TMPDIR:-/tmp}/pr-feedback-test.XXXXXX")" +trap 'rm -rf "$work"' EXIT + +fail=0 +ok=0 +check() { + local desc="$1"; shift + if "$@"; then + ok=$((ok + 1)) + echo "ok - $desc" + else + fail=1 + echo "FAIL - $desc" + fi +} + +# new_fixture: a throwaway <dir>/.claude/{scripts,state} tree with the REAL +# pr-feedback.sh + resolve-roots.sh + needs-human.sh + notify.sh copied in. +new_fixture() { + local name="$1" + local dir="$work/$name" + local scripts="$dir/.claude/scripts" + mkdir -p "$scripts" "$dir/.claude/state" + cp "$pr_feedback_src" "$scripts/pr-feedback.sh" + cp "$script_dir/resolve-roots.sh" "$scripts/resolve-roots.sh" + cp "$script_dir/needs-human.sh" "$scripts/needs-human.sh" + cp "$script_dir/notify.sh" "$scripts/notify.sh" + chmod +x "$scripts"/*.sh + cat > "$dir/.claude/gates.json" <<EOF +{ "notify": "printf 'fired\\n' >> $work/$name-notify-fired.txt" } +EOF + printf '%s\n' "$dir" +} + +# --------------------------------------------------------------------------- +# A. UNADDRESSED changes-requested PR (no "claude-addressed" marker at all): +# listed in the TSV, AND clears any earlier "awaiting re-review" flag +# (ball is in the bot's court right now, not the owner's). +# --------------------------------------------------------------------------- +dirA="$(new_fixture scenarioA)" +gh_logA="$work/scenarioA-gh.log" +cat > "$dirA/.claude/scripts/bot-gh.sh" <<EOF +#!/usr/bin/env bash +printf '%s\n' "\$*" >> "$gh_logA" +case "\$1" in + pr) + case "\$2" in + list) printf '20\tfeat/issue-20-x\t\n' ;; + view) : ;; # not queried on this path before the clear + edit) : ;; + comment) : ;; + *) : ;; + esac + ;; + api) + case "\$2" in + repos/*/pulls/*/reviews) printf '2026-02-01T00:00:00Z\treviewer1\n' ;; + repos/*/issues/*/comments) : ;; # no addressed-marker comment at all + *) : ;; + esac + ;; + label) : ;; + issue) : ;; + *) echo "unhandled: \$*" >&2; exit 1 ;; +esac +EOF +chmod +x "$dirA/.claude/scripts/bot-gh.sh" +outA="$(env -u GATES_FILE bash "$dirA/.claude/scripts/pr-feedback.sh" "acme/repo" 2>&1)" + +check "A: unaddressed PR 20 IS listed in the TSV" bash -c 'printf "%s\n" "$1" | grep -qF "20 feat/issue-20-x reviewer1 2026-02-01T00:00:00Z"' _ "$outA" +check "A: earlier 'awaiting re-review' flag is CLEARED (ball back in bot's court)" grep -q "pr edit 20 --remove-label needs-human" "$gh_logA" +check "A: no comment posted on the clear path" bash -c '! grep -q "^pr comment" "$1"' _ "$gh_logA" + +# --------------------------------------------------------------------------- +# A-count-only. Same PR/situation, PR_FEEDBACK_COUNT_ONLY=1 -- the census's +# read-only invocation (issue #99 re-review finding #2): TSV output must be +# IDENTICAL, but the clear's gh label mutation must NOT happen at all. +# --------------------------------------------------------------------------- +gh_logAc="$work/scenarioA-count-gh.log" +cat > "$dirA/.claude/scripts/bot-gh.sh" <<EOF +#!/usr/bin/env bash +printf '%s\n' "\$*" >> "$gh_logAc" +case "\$1" in + pr) + case "\$2" in + list) printf '20\tfeat/issue-20-x\t\n' ;; + *) : ;; + esac + ;; + api) + case "\$2" in + repos/*/pulls/*/reviews) printf '2026-02-01T00:00:00Z\treviewer1\n' ;; + repos/*/issues/*/comments) : ;; + *) : ;; + esac + ;; + label) : ;; + issue) : ;; + *) echo "unhandled: \$*" >&2; exit 1 ;; +esac +EOF +chmod +x "$dirA/.claude/scripts/bot-gh.sh" +outAc="$(env -u GATES_FILE PR_FEEDBACK_COUNT_ONLY=1 bash "$dirA/.claude/scripts/pr-feedback.sh" "acme/repo" 2>&1)" + +check "A-count-only: TSV output identical to the real (non-count-only) run" \ + bash -c '[ "$1" = "$2" ]' _ "$outA" "$outAc" +check "A-count-only: NO gh label mutation at all (pure counter, finding #2)" bash -c '! grep -q "pr edit" "$1"' _ "$gh_logAc" + +# --------------------------------------------------------------------------- +# B. ADDRESSED PR (marker comment newer than the last CHANGES_REQUESTED +# review): NOT listed; instead FLAGS needs-human (owner's turn to +# re-review). Run TWICE in a row (simulating two loop ticks with the same +# still-addressed state) via a label marker FILE that persists across runs +# -- proves the REAL pr-feedback.sh wiring posts the comment only on the +# FIRST (fresh) episode and skips it on the repeat (issue #99 re-review +# finding #1). +# --------------------------------------------------------------------------- +dirB="$(new_fixture scenarioB)" +gh_logB="$work/scenarioB-gh.log" +labeled_markerB="$work/scenarioB-labeled.marker" +cat > "$dirB/.claude/scripts/bot-gh.sh" <<EOF +#!/usr/bin/env bash +printf '%s\n' "\$*" >> "$gh_logB" +case "\$1" in + pr) + case "\$2" in + list) printf '21\tfeat/issue-21-y\t\n' ;; + view) + if printf '%s\n' "\$*" | grep -q -- '--json labels'; then + [ -f "$labeled_markerB" ] && printf 'needs-human\n' + fi + ;; + edit) + if printf '%s\n' "\$*" | grep -q -- '--add-label needs-human'; then + touch "$labeled_markerB" + fi + ;; + comment) : ;; + *) : ;; + esac + ;; + api) + case "\$2" in + repos/*/pulls/*/reviews) printf '2026-02-01T00:00:00Z\treviewer2\n' ;; + repos/*/issues/*/comments) printf '2026-02-02T00:00:00Z\n' ;; # marker AFTER the CR + *) : ;; + esac + ;; + label) : ;; + issue) : ;; + *) echo "unhandled: \$*" >&2; exit 1 ;; +esac +EOF +chmod +x "$dirB/.claude/scripts/bot-gh.sh" +outB1="$(env -u GATES_FILE bash "$dirB/.claude/scripts/pr-feedback.sh" "acme/repo" 2>&1)" +outB2="$(env -u GATES_FILE bash "$dirB/.claude/scripts/pr-feedback.sh" "acme/repo" 2>&1)" + +check "B: addressed PR 21 is NOT listed on run 1" bash -c '[ -z "$1" ]' _ "$outB1" +check "B: addressed PR 21 is NOT listed on run 2 either" bash -c '[ -z "$1" ]' _ "$outB2" +check "B: label add attempted on BOTH runs" bash -c '[ "$(grep -c "pr edit 21 --add-label needs-human" "$1")" -eq 2 ]' _ "$gh_logB" +check "B: exactly ONE comment across BOTH runs (episode-gated, finding #1)" bash -c '[ "$(grep -c "pr comment 21 --body" "$1")" -eq 1 ]' _ "$gh_logB" + +# --------------------------------------------------------------------------- +# B-count-only. Same addressed-PR situation, PR_FEEDBACK_COUNT_ONLY=1 -- no +# label add, no comment, no notify at all (finding #2). +# --------------------------------------------------------------------------- +gh_logBc="$work/scenarioB-count-gh.log" +cat > "$dirB/.claude/scripts/bot-gh.sh" <<EOF +#!/usr/bin/env bash +printf '%s\n' "\$*" >> "$gh_logBc" +case "\$1" in + pr) + case "\$2" in + list) printf '21\tfeat/issue-21-y\t\n' ;; + *) : ;; + esac + ;; + api) + case "\$2" in + repos/*/pulls/*/reviews) printf '2026-02-01T00:00:00Z\treviewer2\n' ;; + repos/*/issues/*/comments) printf '2026-02-02T00:00:00Z\n' ;; + *) : ;; + esac + ;; + label) : ;; + issue) : ;; + *) echo "unhandled: \$*" >&2; exit 1 ;; +esac +EOF +chmod +x "$dirB/.claude/scripts/bot-gh.sh" +outBc="$(env -u GATES_FILE PR_FEEDBACK_COUNT_ONLY=1 bash "$dirB/.claude/scripts/pr-feedback.sh" "acme/repo" 2>&1)" + +check "B-count-only: still not listed (same as non-count-only)" bash -c '[ -z "$1" ]' _ "$outBc" +check "B-count-only: no label add, no comment at all" bash -c '! grep -qE "add-label|^pr comment" "$1"' _ "$gh_logBc" + +# --------------------------------------------------------------------------- +# C. A PR already labeled `claude-addressing` is skipped entirely -- not even +# the reviews/comments lookup runs (the early `continue`). +# --------------------------------------------------------------------------- +dirC="$(new_fixture scenarioC)" +gh_logC="$work/scenarioC-gh.log" +cat > "$dirC/.claude/scripts/bot-gh.sh" <<EOF +#!/usr/bin/env bash +printf '%s\n' "\$*" >> "$gh_logC" +case "\$1" in + pr) + case "\$2" in + list) printf '22\tfeat/issue-22-z\tclaude-addressing\n' ;; + *) : ;; + esac + ;; + api) echo "should not be called: \$*" >&2; exit 1 ;; + *) : ;; +esac +EOF +chmod +x "$dirC/.claude/scripts/bot-gh.sh" +outC="$(env -u GATES_FILE bash "$dirC/.claude/scripts/pr-feedback.sh" "acme/repo" 2>&1)" + +check "C: claude-addressing-labeled PR 22 is NOT listed" bash -c '[ -z "$1" ]' _ "$outC" +check "C: no 'api' lookups happened at all (early continue)" bash -c '! grep -q "^api" "$1"' _ "$gh_logC" + +echo "" +if [ "$fail" -eq 0 ]; then + echo "pr-feedback.test.sh: PASS ($ok checks)" + exit 0 +else + echo "pr-feedback.test.sh: FAIL (see FAIL lines above)" + exit 1 +fi