From 83dec489173f08a7f1d1821bdb9f312a08328dd8 Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Tue, 4 Aug 2026 14:03:59 +0200 Subject: [PATCH 1/2] fix(loop): census_error surfacing + pr-rebase gh field fix (issue #187) Two defects let the autonomous loop fail silently: 1. pr-rebase.sh requested `baseRefOid` in a `gh pr list --json` field list. That field does not exist on `gh pr list` in gh 2.46.0 (it is a GraphQL PullRequest field, surfaced only via `gh pr view`), so gh printed "Unknown JSON field" and exited non-zero on EVERY invocation. loop-census.sh swallowed that via `| grep -c . || true`, pinning rebase_prs=0 forever: the whole #96-part-3 rebase feature was a permanent no-op, plus stderr garbage on every tick. Every PR in this query shares `--base "$base"`, so the PR-level baseRefOid GitHub would report is by definition that base branch's current tip; fetch it once via `gh api repos/OWNER/REPO/commits/BASE --jq .sha` instead. 2. Nothing distinguished "census failed" from "nothing to do". Census now emits a greppable `census_error=` stdout contract line whenever a gh/parse step genuinely fails or degrades, previously unguarded gh calls no longer abort the script mid-contract under `set -euo pipefail`, loop-tick.sh folds the stages into the tick record's `reason` field, and cockpit.sh renders `reason` in the loop-health panel. A healthy tick emits zero census_error lines and is byte-identical to before. Co-Authored-By: Claude Sonnet 5 --- .claude/scripts/cockpit.sh | 14 ++- .claude/scripts/cockpit.test.sh | 39 +++++++ .claude/scripts/loop-census.sh | 151 ++++++++++++++++++++++++++-- .claude/scripts/loop-census.test.sh | 94 +++++++++++++++++ .claude/scripts/loop-tick.sh | 25 ++++- .claude/scripts/loop-tick.test.sh | 48 +++++++++ .claude/scripts/pr-rebase.sh | 39 +++++-- .claude/scripts/pr-rebase.test.sh | 117 ++++++++++++++++++--- 8 files changed, 489 insertions(+), 38 deletions(-) diff --git a/.claude/scripts/cockpit.sh b/.claude/scripts/cockpit.sh index 43cf72c..0ccca4f 100755 --- a/.claude/scripts/cockpit.sh +++ b/.claude/scripts/cockpit.sh @@ -783,6 +783,16 @@ function renderLoopHealth() { const intervalSec = CADENCE_INTERVAL_SECONDS[cadence]; html += `

Last tick: ${esc(last.ts)} · verdict ${esc(last.verdict)}

`; + // reason (issue #187): loop-tick.sh's tick record has always carried a + // `reason` field (issue #95, spend-ceiling diagnostics), but this panel + // never rendered it -- cockpit-visibility gap that let a census_error- + // driven action=none look IDENTICAL to a genuinely idle repo. Render it + // whenever non-empty, regardless of WHICH mechanism populated it + // (spend-ceiling OR census_error), so an operator glancing at the panel + // sees not just "action=none" but WHY. + if (last.reason) { + html += `

Reason: ${esc(last.reason)}

`; + } html += `

Cadence: ${esc(cadence || "(unknown)")}`; if (intervalSec) html += ` (every ${intervalSec}s)`; html += `

`; @@ -796,11 +806,11 @@ function renderLoopHealth() { html += `

STALLED — no tick in over ${intervalSec * 2}s (cadence ${esc(cadence)})

`; } - html += ``; + html += `
TimeVerdictCadence
`; const historyStop = Math.max(0, ticks.length - VERDICT_HISTORY_N); for (let i = ticks.length - 1; i >= historyStop; i--) { const t = ticks[i]; - html += ``; + html += ``; } html += `
TimeVerdictCadenceReason
${esc(t.ts)}${esc(t.verdict)}${esc(t.cadence)}
${esc(t.ts)}${esc(t.verdict)}${esc(t.cadence)}${t.reason ? esc(t.reason) : ""}
`; diff --git a/.claude/scripts/cockpit.test.sh b/.claude/scripts/cockpit.test.sh index 65afd1b..509ca8a 100755 --- a/.claude/scripts/cockpit.test.sh +++ b/.claude/scripts/cockpit.test.sh @@ -323,6 +323,45 @@ check "verdict-history default cap (10) does not truncate a shorter (5-tick) his } ' "$html_history_default" +# --------------------------------------------------------------------------- +# 2e. census_error reason surfacing (issue #187): a tick record whose +# `reason` field is populated (either the pre-existing spend-ceiling +# diagnostic, issue #95, or the new census_error fold, issue #187) must +# render a visible "Reason:" line under "Last tick", AND the verdict- +# history table must carry the SAME value in its own Reason column. A +# tick record with NO reason (the common/healthy case) must render +# neither — regression control mirroring section 2a's plain fixture. +# --------------------------------------------------------------------------- +mkdir -p "$work/fixtures-census-error" +echo "[]" >"$work/fixtures-census-error/issues.json" +echo "[]" >"$work/fixtures-census-error/prs.json" +: >"$work/fixtures-census-error/events.jsonl" +cat > "$work/fixtures-census-error/loop-ticks.jsonl" <<'EOF' +{"ts":"2026-01-01T00:00:00Z","verdict":"action=none","cadence":"IDLE","action":"none","issue":"","pr":""} +{"ts":"2026-01-01T00:15:00Z","verdict":"action=none","cadence":"IDLE","action":"none","issue":"","pr":"","reason":"census_error:rebase_prs"} +EOF +html_census_error="$work/cockpit-census-error.html" +COCKPIT_NOW="2026-01-01T00:15:30Z" bash "$cockpit" --fixtures "$work/fixtures-census-error" "$html_census_error" >/dev/null 2>"$work/stderr-census-error.log" +check "census_error reason rendered under 'Last tick'" grep -qF '

Reason: census_error:rebase_prs

' "$html_census_error" +check "census_error reason also rendered in the verdict-history table's Reason column" 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("loop-health section not found"); + if (!m[0].includes("Reason")) throw new Error("Reason column header missing"); + if (!m[0].includes("census_error:rebase_prs")) throw new Error("Reason cell missing/wrong for the census_error tick"); + if (!/<\/td><\/tr>/.test(m[0])) throw new Error("expected an EMPTY Reason cell for the reason-less tick"); +' "$html_census_error" + +# Regression control: NO reason on the last tick -> no "Reason:" paragraph +# at all (unchanged from before this feature). +cat > "$work/fixtures-census-error/loop-ticks.jsonl" <<'EOF' +{"ts":"2026-01-01T00:15:00Z","verdict":"action=none","cadence":"IDLE","action":"none","issue":"","pr":""} +EOF +html_no_reason="$work/cockpit-no-reason.html" +COCKPIT_NOW="2026-01-01T00:15:30Z" bash "$cockpit" --fixtures "$work/fixtures-census-error" "$html_no_reason" >/dev/null 2>"$work/stderr-no-reason.log" +check "no Reason paragraph when the last tick has no reason (regression control)" bash -c '! grep -q "unavailable\">Reason:" "$1"' _ "$html_no_reason" + # --------------------------------------------------------------------------- # 2d. Spend ceilings sub-panel (issue #95): stop-after countdown, today's # action count vs the daily ceiling, and per-issue attempts vs the diff --git a/.claude/scripts/loop-census.sh b/.claude/scripts/loop-census.sh index 30eb308..ab31747 100644 --- a/.claude/scripts/loop-census.sh +++ b/.claude/scripts/loop-census.sh @@ -4,6 +4,27 @@ # whether it can ACT — so the actionability check is a single pre-approvable # command instead of a discipline the tick can silently skip: # +# census_error= zero or more (issue #187) — a gh/parse step +# genuinely failed or degraded THIS TICK +# instead of silently zeroing a real count or +# aborting the whole script before any other +# contract line printed. Printed inline, near +# the (possibly degraded) line it would +# otherwise have fed — greppable via +# `^census_error=`. is one of: +# repo-derive, open_prs, open_pr_branches, +# open_issue_set, feedback_prs, ci_fix_prs, +# comment_fix_prs, rebase_prs, planned. NOT +# emitted for the documented, deliberate +# empty-means-ok degrades (the milestone REST +# fetch, the STALE-MERGED-REMOTE per-branch +# merged-PR lookup) — see their own comments +# below for why those stay silent. A repo with +# a genuinely healthy tick prints ZERO +# census_error lines, same as today. +# loop-tick.sh folds this into a non-empty +# tick reason (see its own header) rather than +# a bare action=none with an empty reason. # open_prs=N open PRs against the adapter's base branch # feedback_prs=N bot PRs with unaddressed CHANGES_REQUESTED (pr-feedback.sh) # ci_fix_prs=N bot PRs with a failing CI check on the current @@ -274,7 +295,26 @@ set -euo pipefail . "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/resolve-roots.sh" # Route EVERY gh call through the bot identity (see bot-gh.sh). gh() { bash "$script_dir/bot-gh.sh" "$@"; } -repo="${1:-$(gh repo view --json nameWithOwner -q .nameWithOwner)}" + +# --- census_error mechanism (issue #187) ------------------------------------ +# `census_error=` — a single-line, greppable (`^census_error=`) signal +# printed INLINE (near the contract line it would otherwise have fed) +# whenever a gh/parse step genuinely fails or degrades this tick, instead of +# EITHER (a) silently zeroing a real count (the historical bug: pr-rebase.sh +# hitting an invalid `gh pr list --json` field, swallowed by loop-census.sh's +# own `| grep -c . || true` down to rebase_prs=0 — read by the loop forever +# after as "nothing to rebase") OR (b) aborting this whole script under +# `set -euo pipefail` before any contract line prints at all (the caller, +# loop-tick.sh, then sees an empty/partial census_out and can silently +# downgrade to `action=none` with an EMPTY reason — indistinguishable from +# "nothing to do"). loop-tick.sh greps these out and folds them into a +# non-empty tick reason (see loop-tick.sh's own header comment). This is NOT +# for the documented, deliberate empty-means-ok degrades (e.g. the milestone +# REST fetch below, or the STALE-MERGED-REMOTE per-branch lookup) — only for +# a failure that would otherwise zero out or abort past a REAL signal. +census_error() { + echo "census_error=$1" +} gates_rel="${GATES_FILE:-.claude/gates.json}" case "$gates_rel" in /*) gates="$gates_rel" ;; *) gates="$root/$gates_rel" ;; esac @@ -326,6 +366,32 @@ main_head=$(git -C "$root" symbolic-ref --quiet --short HEAD 2>/dev/null || true [ -n "$main_head" ] || main_head="detached" echo "main_head=$main_head" +# repo (issue #187): derived from $1, falling back to `gh repo view` only +# when no explicit repo was passed. Guarded — an unguarded `gh repo view` +# failure here used to abort this entire script under `set -euo pipefail` +# BEFORE main_dirty=/main_head= even printed (they're computed above this +# line precisely so they survive a repo-derivation failure too). Everything +# below this point needs a real $repo, so on failure this prints a minimal, +# internally-consistent (never partial/ambiguous) degraded contract plus +# census_error=repo-derive, and exits 0 rather than crashing or continuing +# with gh calls against an empty/invalid repo. +if [ -n "${1:-}" ]; then + repo="$1" +else + if ! repo="$(gh repo view --json nameWithOwner -q .nameWithOwner 2>/dev/null)" || [ -z "$repo" ]; then + census_error "repo-derive" + echo "open_prs=0" + echo "feedback_prs=0" + echo "ci_fix_prs=0" + echo "comment_fix_prs=0" + echo "rebase_prs=0" + echo "planned_issues=0" + echo "advance_ready=none" + echo "cadence=IDLE cron=*/15 * * * *" + exit 0 + fi +fi + # Adapter-derived facts: base branch + the module:* label set. base=$(node -e 'const g=require(process.argv[1]); console.log((g.merge&&g.merge.baseBranch)||"main")' "$gates") module_labels=$(node -e 'const g=require(process.argv[1]); console.log(g.modules.map(m=>"module:"+m.name).join("\n"))' "$gates") @@ -496,19 +562,45 @@ get_blocked_by() { ' "$json" 2>/dev/null || true } -open_prs=$(gh pr list -R "$repo" --state open --base "$base" --json number --jq 'length') +# open_prs (issue #187): guarded — an unguarded `gh pr list` failure here +# used to abort this ENTIRE script under `set -euo pipefail` (the "daemon +# ticks die to action=none" symptom: loop-tick.sh's census_out ends up +# empty/partial, advance_ready defaults to "none" with no way to tell that +# apart from a genuinely idle repo). On failure: census_error, and open_prs +# degrades to -1 (a value that is neither `-eq 0` nor `-ge 1`, so every +# downstream open_prs comparison below takes its conservative/"don't act on +# unknown data" branch instead of hitting a bash arithmetic error on a +# non-numeric value). +if ! open_prs=$(gh pr list -R "$repo" --state open --base "$base" --json number --jq 'length'); then + census_error "open_prs" + open_prs=-1 +fi echo "open_prs=$open_prs" # Head branch names of every open PR (against base) — used below to tell # in_flight (branch exists, no PR yet) apart from already-at-PR-stage. -open_pr_branches=$(gh pr list -R "$repo" --state open --base "$base" --json headRefName --jq '.[].headRefName') +# Guarded (issue #187): failure degrades to open_pr_branches="" (every +# candidate branch just looks like it has no open PR yet, i.e. in_flight — +# the conservative direction, same as the pre-existing "no open PRs" case) +# instead of aborting the whole script. +if ! open_pr_branches=$(gh pr list -R "$repo" --state open --base "$base" --json headRefName --jq '.[].headRefName'); then + census_error "open_pr_branches" + open_pr_branches="" +fi # All open issue numbers (bounded --limit, matching cockpit.sh's own --state # open fetch) — used to decide whether a candidate's "Blocked by #N" target # is still open. `|| true` guards a transient gh failure from wedging the # whole census; an empty set just makes is_open_issue always report false, # 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 +# census_error (issue #187): this degrade is silent on PURPOSE for a +# genuinely empty open-issue set, but a genuine gh FAILURE still needs to be +# told apart from "repo has zero open issues" — rc is checked explicitly +# rather than folded into the `|| true`. +if ! open_issue_set=$(gh issue list -R "$repo" --state open --json number --jq '.[].number' --limit 200 2>/dev/null); then + census_error "open_issue_set" + open_issue_set="" +fi # 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 @@ -517,16 +609,45 @@ open_issue_set=$(gh issue list -R "$repo" --state open --json number --jq '.[].n # 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) +# +# Each of the four PR-event scripts below is guarded the SAME way (issue +# #187): the sibling script's OWN exit status is checked (not just its +# stdout line count) so a genuine failure inside it -- e.g. pr-rebase.sh's +# `gh pr list` erroring on an invalid --json field, the historical bug this +# issue fixes -- surfaces as census_error=_prs instead of silently +# reporting 0 (indistinguishable from "nothing to do" for that PR-event +# type, forever). stderr is left un-redirected (same as before) so the +# underlying gh/script error is still visible to a human tailing the loop. +if feedback_raw=$(PR_FEEDBACK_COUNT_ONLY=1 bash "$script_dir/pr-feedback.sh" "$repo"); then + feedback_prs=$(printf '%s\n' "$feedback_raw" | grep -c . || true) +else + census_error "feedback_prs" + feedback_prs=0 +fi echo "feedback_prs=$feedback_prs" -ci_fix_prs=$(bash "$script_dir/pr-ci-fix.sh" "$repo" | grep -c . || true) +if cifix_raw=$(bash "$script_dir/pr-ci-fix.sh" "$repo"); then + ci_fix_prs=$(printf '%s\n' "$cifix_raw" | grep -c . || true) +else + census_error "ci_fix_prs" + ci_fix_prs=0 +fi echo "ci_fix_prs=$ci_fix_prs" -comment_fix_prs=$(bash "$script_dir/pr-comment-fix.sh" "$repo" | grep -c . || true) +if commentfix_raw=$(bash "$script_dir/pr-comment-fix.sh" "$repo"); then + comment_fix_prs=$(printf '%s\n' "$commentfix_raw" | grep -c . || true) +else + census_error "comment_fix_prs" + comment_fix_prs=0 +fi echo "comment_fix_prs=$comment_fix_prs" -rebase_prs=$(bash "$script_dir/pr-rebase.sh" "$repo" | grep -c . || true) +if rebase_raw=$(bash "$script_dir/pr-rebase.sh" "$repo"); then + rebase_prs=$(printf '%s\n' "$rebase_raw" | grep -c . || true) +else + census_error "rebase_prs" + rebase_prs=0 +fi echo "rebase_prs=$rebase_prs" # Open `planned` issues carrying any of the adapter's module labels, ordered @@ -539,7 +660,14 @@ echo "rebase_prs=$rebase_prs" # (never a separate per-milestone issue query — see the gh 2.4.0 note in # MILESTONE SCOPING above), placed BEFORE title so title stays the LAST TSV # field (may contain spaces) and is left untouched by this transform. -planned=$(gh issue list -R "$repo" --state open --label planned --json number,title,labels,milestone \ +# Guarded (issue #187): this pipeline used to be a bare, unguarded +# assignment -- a `gh issue list` failure (under `pipefail`, the pipeline's +# exit status is gh's, even though the downstream awk/sort/cut all succeed +# on empty input) aborted this whole script before ANY contract line below +# this point could print. On failure, degrade to planned="" (identical to +# "repo has zero planned issues" downstream) plus census_error, rather than +# crashing. +if ! planned=$(gh issue list -R "$repo" --state open --label planned --json number,title,labels,milestone \ --jq '.[] | [.number, ([.labels[].name]|join(",")), (.milestone.title // ""), .title] | @tsv' \ | awk -F'\t' 'BEGIN { OFS = "\t" } { @@ -552,7 +680,10 @@ planned=$(gh issue list -R "$repo" --state open --label planned --json number,ti print rank OFS $0 }' \ | sort -t $'\t' -k1,1n -k2,2n \ - | cut -f2-) + | cut -f2-); then + census_error "planned" + planned="" +fi # --- current-milestone detection (issue #174) ------------------------------- # Walk the version-sorted open milestones ascending; the FIRST one with at diff --git a/.claude/scripts/loop-census.test.sh b/.claude/scripts/loop-census.test.sh index ed42f46..d65c811 100644 --- a/.claude/scripts/loop-census.test.sh +++ b/.claude/scripts/loop-census.test.sh @@ -1620,6 +1620,100 @@ check "worktree-marker: zero in_flight lines total (both issues already have ope check "worktree-marker: zero stalled lines total" bash -c \ '[ "$(printf "%s\n" "$1" | grep -c "^stalled=")" -eq 0 ]' _ "$outWtMarker" +# --------------------------------------------------------------------------- +# census_error (issue #187): a gh/parse step failing mid-tick must surface a +# visible `census_error=` line rather than a silently zeroed count, +# AND census must still complete (emit every later contract line) rather +# than aborting under `set -euo pipefail`. Two failures are injected in ONE +# fixture run: +# - the CORE `gh pr list --json number` open-PR-count call, which was +# UNGUARDED before this fix -- a failure there used to abort the whole +# script (the "daemon ticks die to action=none" class of bug: no +# stdout at all, so loop-tick.sh's census_out ends up empty and +# advance_ready defaults to "none" with no way to tell that apart from +# a genuinely idle repo); +# - pr-rebase.sh itself failing, reproducing the ACTUAL #187 production +# bug directly (an invalid `--json` field made gh exit non-zero, +# silently swallowed by census's old `| grep -c . || true` down to +# rebase_prs=0 forever). +# --------------------------------------------------------------------------- +fixtureCensusErr="$work/fixture-census-error" +scriptsCensusErr="$fixtureCensusErr/.claude/scripts" +mkdir -p "$scriptsCensusErr" +cp "$census_src" "$scriptsCensusErr/loop-census.sh" +cp "$resolve_roots_src" "$scriptsCensusErr/resolve-roots.sh" + +cat > "$fixtureCensusErr/.claude/gates.json" <<'EOF' +{ + "modules": [{ "name": "test", "path": ".", "description": "", "owner": "" }], + "merge": { "baseBranch": "main" } +} +EOF + +cat > "$scriptsCensusErr/pr-feedback.sh" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +cat > "$scriptsCensusErr/pr-ci-fix.sh" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +cat > "$scriptsCensusErr/pr-comment-fix.sh" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +# The actual #187 production bug, reproduced directly: pr-rebase.sh itself +# fails (mirrors gh exiting non-zero on the invalid baseRefOid --json field). +cat > "$scriptsCensusErr/pr-rebase.sh" <<'EOF' +#!/usr/bin/env bash +echo "fake-pr-rebase.sh: simulated Unknown JSON field failure" >&2 +exit 1 +EOF + +cat > "$scriptsCensusErr/bot-gh.sh" <<'EOF' +#!/usr/bin/env bash +case "$1" in + repo) echo "acme/repo" ;; + pr) + if printf '%s\n' "$*" | grep -q -- '--json number '; then + echo "fake-bot-gh.sh: simulated gh pr list failure" >&2 + exit 1 + elif printf '%s\n' "$*" | grep -q 'headRefName'; then + : # no open PRs + else + echo "fake-bot-gh.sh: unexpected pr subcommand: $*" >&2 + exit 1 + fi + ;; + issue) + printf '5\tplanned,module:test\t\tCandidate five\n' + ;; + *) echo "fake-bot-gh.sh: unhandled args: $*" >&2; exit 1 ;; +esac +EOF +chmod +x "$scriptsCensusErr"/*.sh + +git -C "$fixtureCensusErr" init -q -b main +git -C "$fixtureCensusErr" -c user.email=t@e.st -c user.name=t commit -q --allow-empty -m init + +outCensusErr="$(env -u GATES_FILE bash "$scriptsCensusErr/loop-census.sh" "acme/repo" 2>/dev/null)" +rcCensusErr=$? + +check "census_error: script exits 0 despite the open_prs gh failure (no set -e crash)" \ + bash -c '[ "$1" -eq 0 ]' _ "$rcCensusErr" +check "census_error=open_prs line emitted" \ + bash -c 'printf "%s\n" "$1" | grep -qx "census_error=open_prs"' _ "$outCensusErr" +check "open_prs degrades to -1 (never a bare/unset value that would break later -eq/-ge comparisons)" \ + bash -c 'printf "%s\n" "$1" | grep -qx "open_prs=-1"' _ "$outCensusErr" +check "census_error=rebase_prs line emitted (the actual #187 production bug, reproduced)" \ + bash -c 'printf "%s\n" "$1" | grep -qx "census_error=rebase_prs"' _ "$outCensusErr" +check "rebase_prs=0 still printed (never a bare/unset value)" \ + bash -c 'printf "%s\n" "$1" | grep -qx "rebase_prs=0"' _ "$outCensusErr" +check "census still reaches planned_issues= despite both failures (never aborts early)" \ + bash -c 'printf "%s\n" "$1" | grep -qx "planned_issues=1"' _ "$outCensusErr" +check "advance_ready=none when open_prs is unknown (conservative -- never advances on bad data)" \ + bash -c 'printf "%s\n" "$1" | grep -qx "advance_ready=none"' _ "$outCensusErr" + echo "" if [ "$fail" -eq 0 ]; then echo "loop-census.test.sh: PASS ($ok checks)" diff --git a/.claude/scripts/loop-tick.sh b/.claude/scripts/loop-tick.sh index 6cee424..b1d9a42 100644 --- a/.claude/scripts/loop-tick.sh +++ b/.claude/scripts/loop-tick.sh @@ -275,6 +275,17 @@ in_flight_issues="$(printf '%s\n' "$census_out" | sed -n 's/^in_flight=//p')" # e.g. "cadence=FAST cron=* * * * *"; keep only the leading token. cadence="$(printf '%s\n' "$census_out" | sed -n 's/^cadence=\([A-Za-z]*\).*/\1/p' | tail -1)" +# census_error (issue #187): zero or more `census_error=` lines from +# loop-census.sh (a gh/parse step that failed or degraded THIS TICK -- see +# loop-census.sh's own header). Folded into a comma-joined, non-empty +# ceiling_reason-style value below and persisted on the tick record's +# `reason` field, so a tick that lands on action=none BECAUSE census +# couldn't get real data is never indistinguishable, on the tick log/cockpit, +# from a tick that landed on action=none because there was genuinely nothing +# to do (an empty `reason` on a bare `action=none` meant exactly that before +# this fix). +census_error_stages="$(printf '%s\n' "$census_out" | sed -n 's/^census_error=//p' | paste -sd, - 2>/dev/null || true)" + # --- Parse pr-feedback.sh's TSV (num, branch, reviewer, changes_requested_at) -- # Lowest-numbered PR wins when several need feedback addressed. feedback_line="$(printf '%s\n' "$feedback_out" | awk -F'\t' 'NF>=2 && $1 ~ /^[0-9]+$/ {print $1"\t"$2}' | sort -t $'\t' -k1,1n | head -1)" @@ -1027,8 +1038,20 @@ fi echo "$verdict" +# tick_reason (issue #187): ceiling_reason (existing, issue #95) always wins +# when set -- a spend-ceiling breach is a MORE specific/actionable reason +# than "census degraded". Otherwise, when census reported one or more +# census_error= lines this tick, fold them into the reason so the +# tick record's `reason` field is non-empty even on a bare `action=none` -- +# see the census_error_stages comment above for why an empty reason there is +# the exact bug this closes. +tick_reason="$ceiling_reason" +if [ -z "$tick_reason" ] && [ -n "$census_error_stages" ]; then + tick_reason="census_error:$census_error_stages" +fi + # Persist the tick record (issue #85) AFTER the verdict has been echoed, and # writing to the FILE ONLY -- never stdout -- so the verdict line above stays # the last line of this script's stdout. Best-effort: never allowed to affect # the exit status set below. -write_tick_record "$verdict" "$cadence" "$ceiling_reason" || true +write_tick_record "$verdict" "$cadence" "$tick_reason" || true diff --git a/.claude/scripts/loop-tick.test.sh b/.claude/scripts/loop-tick.test.sh index dfe9619..7020e55 100644 --- a/.claude/scripts/loop-tick.test.sh +++ b/.claude/scripts/loop-tick.test.sh @@ -964,6 +964,54 @@ out25="$(env -u GATES_FILE CLAUDE_EVENTS_FILE="$events25" bash "$dir25/loop-tick check "scenario 25 (real loop-census.sh reports advance_ready=none, issue 77 in_flight+stalled)" bash -c 'printf "%s\n" "$1" | grep -qx "advance_ready=none" && printf "%s\n" "$1" | grep -qx "in_flight=77" && printf "%s\n" "$1" | grep -q "^stalled=77 "' _ "$out25" check "scenario 25 (end-to-end reachability via REAL census): verdict is action=resume issue=77 branch=feat/issue-77-x" bash -c '[ "$(printf "%s\n" "$1" | tail -1)" = "action=resume issue=77 branch=feat/issue-77-x" ]' _ "$out25" +# --------------------------------------------------------------------------- +# 26. census_error surfacing (issue #187): when loop-census.sh's output +# includes one or more `census_error=` lines (a gh/parse step +# failed or degraded that tick), a resulting action=none verdict must +# land on the tick record with a NON-EMPTY `reason` -- never the bare, +# empty-reason action=none that's indistinguishable from "nothing to +# do" (the exact #187 bug this closes). Two census_error lines are +# fed in to also confirm they're comma-joined, not just the first one. +# --------------------------------------------------------------------------- +dir26="$(new_fixture scenario26 'census_error=open_prs +open_prs=-1 +census_error=rebase_prs +feedback_prs=0 +ci_fix_prs=0 +comment_fix_prs=0 +rebase_prs=0 +planned_issues=0 +advance_ready=none +cadence=IDLE cron=*/15 * * * *' '')" +ticks26="$work/scenario26-ticks.jsonl" +out26="$(CLAUDE_TICKS_FILE="$ticks26" run_tick "$dir26")" +check "scenario 26: verdict is still action=none (no data to act on)" bash -c '[ "$(printf "%s\n" "$1" | tail -1)" = "action=none" ]' _ "$out26" +check "scenario 26: tick record's reason is non-empty and names BOTH census_error stages" node -e ' + const fs = require("fs"); + const obj = JSON.parse(fs.readFileSync(process.argv[1], "utf8").trim()); + if (obj.action !== "none") throw new Error("action mismatch: " + JSON.stringify(obj)); + if (!obj.reason) throw new Error("expected a NON-EMPTY reason, got " + JSON.stringify(obj)); + if (obj.reason !== "census_error:open_prs,rebase_prs") throw new Error("reason mismatch: " + JSON.stringify(obj)); +' "$ticks26" + +# A HEALTHY tick (zero census_error lines) must keep reason="" exactly as +# before this feature -- regression control for scenario 26 above. +dir26b="$(new_fixture scenario26b 'open_prs=0 +feedback_prs=0 +ci_fix_prs=0 +comment_fix_prs=0 +rebase_prs=0 +planned_issues=0 +advance_ready=none +cadence=IDLE cron=*/15 * * * *' '')" +ticks26b="$work/scenario26b-ticks.jsonl" +out26b="$(CLAUDE_TICKS_FILE="$ticks26b" run_tick "$dir26b")" +check "scenario 26b (no census_error lines): reason stays empty, unchanged from before this feature" node -e ' + const fs = require("fs"); + const obj = JSON.parse(fs.readFileSync(process.argv[1], "utf8").trim()); + if (obj.reason !== "") throw new Error("expected empty reason, got " + JSON.stringify(obj)); +' "$ticks26b" + echo "" if [ "$fail" -eq 0 ]; then echo "loop-tick.test.sh: PASS ($ok checks)" diff --git a/.claude/scripts/pr-rebase.sh b/.claude/scripts/pr-rebase.sh index 600e9bd..b175c7d 100644 --- a/.claude/scripts/pr-rebase.sh +++ b/.claude/scripts/pr-rebase.sh @@ -47,7 +47,8 @@ # posts a bot comment containing # `` after EVERY attempt # (both a clean rebase+force-push AND an aborted conflicting one — see -# loop-event.sh's rebase prompt). For the PR's CURRENT `baseRefOid` (base_sha): +# loop-event.sh's rebase prompt). For the base branch's CURRENT tip commit +# (base_sha — GitHub's own `baseRefOid`, fetched once per run; see below): # - no marker at all for this EXACT base_sha -> next attempt is 1, this PR # is a fresh candidate; # - the highest-attempt marker found for this EXACT base_sha is K -> next @@ -60,7 +61,7 @@ # A marker's base_sha is compared for EXACT equality only — a marker posted # against an OLDER base_sha never counts toward the CURRENT base_sha's # budget. This is what makes the budget self-resetting: once a NEW sibling PR -# merges into base, `baseRefOid` changes, every marker on file was written +# merges into base, its tip commit (base_sha) changes, every marker on file was written # against the now-stale base_sha, so the count for the fresh base_sha starts # back at zero and this PR is eligible again (attempt=1) even if it had # previously exhausted its budget against the OLD base_sha and been labeled @@ -101,6 +102,23 @@ gates_rel="${GATES_FILE:-.claude/gates.json}" case "$gates_rel" in /*) gates="$gates_rel" ;; *) gates="$root/$gates_rel" ;; esac base="$(node -e 'const g=require(process.argv[1]); console.log((g.merge&&g.merge.baseBranch)||"main")' "$gates" 2>/dev/null || echo main)" +# Base branch tip SHA (issue #187): `baseRefOid` is NOT a valid `gh pr list +# --json` field on the installed gh (2.46.0) -- it only exists on the +# GraphQL PullRequest type, which this gh has no `gh api graphql` support +# for either. Requesting it in the `--json` list below made gh print +# "Unknown JSON field" + exit non-zero on EVERY invocation, which the caller +# (loop-census.sh) silently swallowed to rebase_prs=0 -- a permanent no-op. +# Fix: every PR this script queries shares the SAME base branch ($base, +# enforced by --base below), so the PR-level baseRefOid GitHub would report +# is, by definition, identical to that base branch's own current tip -- +# fetch it ONCE via a plain REST call instead of a per-PR graphql round +# trip. A failed fetch degrades to base_sha="" (never aborts): no marker +# comment will ever match an empty sha, so every CONFLICTING PR is simply +# treated as attempt=1 (fresh) for this tick rather than crashing the whole +# script -- the anti-livelock budget still applies correctly on the NEXT +# tick once the fetch succeeds. +base_sha="$(gh api "repos/$repo/commits/$base" --jq .sha 2>/dev/null)" || base_sha="" + # Feedback, comment-fix, AND ci-fix candidates OUTRANK rebase (precedence) — # exclude their PR numbers up front so a PR that qualifies for any of them # never shows up here at all. @@ -133,14 +151,16 @@ is_ci_fix_candidate() { } gh pr list -R "$repo" --state open --base "$base" \ - --json number,headRefName,author,labels,mergeable,baseRefOid,headRefOid \ + --json number,headRefName,author,labels,mergeable,headRefOid \ --jq '.[] | select(.author.login=="'"$bot"'")' \ | while IFS= read -r pr_json; do [ -z "$pr_json" ] && continue - # One node call per PR: parse number/branch/head/base/labels and compute - # the guard-skip + qualifies decisions together, so the rest of this loop - # body only branches on plain shell values. Extracted via `cut -f`, NOT + # One node call per PR: parse number/branch/head/labels and compute the + # guard-skip + qualifies decisions together, so the rest of this loop + # body only branches on plain shell values. base_sha is NOT part of this + # per-PR parse (see the single shared $base_sha fetched once above, not + # a per-PR JSON field — issue #187). Extracted via `cut -f`, NOT # `IFS=$'\t' read` — bash's `read` classifies tab as "IFS whitespace" # REGARDLESS of what IFS is set to, so it silently collapses consecutive # tabs (an empty field would swallow the NEXT field too); `cut` never @@ -150,14 +170,13 @@ gh pr list -R "$repo" --state open --base "$base" \ const labels = (p.labels || []).map((l) => l.name); const guardSkip = labels.includes("needs-human") || labels.includes("claude-rebasing"); const qualifies = p.mergeable === "CONFLICTING"; - console.log([p.number, p.headRefName || "", p.headRefOid || "", p.baseRefOid || "", guardSkip ? 1 : 0, qualifies ? 1 : 0].join("\t")); + console.log([p.number, p.headRefName || "", p.headRefOid || "", guardSkip ? 1 : 0, qualifies ? 1 : 0].join("\t")); ')" num="$(printf '%s' "$parsed" | cut -f1)" branch="$(printf '%s' "$parsed" | cut -f2)" head_sha="$(printf '%s' "$parsed" | cut -f3)" - base_sha="$(printf '%s' "$parsed" | cut -f4)" - guard_skip="$(printf '%s' "$parsed" | cut -f5)" - qualifies="$(printf '%s' "$parsed" | cut -f6)" + guard_skip="$(printf '%s' "$parsed" | cut -f4)" + qualifies="$(printf '%s' "$parsed" | cut -f5)" [ -z "${num:-}" ] && continue [ "$guard_skip" = "1" ] && continue diff --git a/.claude/scripts/pr-rebase.test.sh b/.claude/scripts/pr-rebase.test.sh index 8814363..5e064e3 100644 --- a/.claude/scripts/pr-rebase.test.sh +++ b/.claude/scripts/pr-rebase.test.sh @@ -69,6 +69,15 @@ EOF # 18: mergeable=UNKNOWN -> NOT emitted (still computing) # 19: mergeable=CONFLICTING, marker attempt=2 for a DIFFERENT (old) base_sha # -> EMITTED attempt=1 (reset) +# +# Issue #187: `baseRefOid` is not a valid `gh pr list --json` field on the +# real gh 2.46.0 -- the PR list response below no longer carries it (mirrors +# the production fix). Instead, base_sha comes from ONE fake +# `gh api repos/acme/repo/commits/main --jq .sha` call, simulated below to +# return "base-new" -- the SAME value for every PR, exactly like the real +# per-run fetch (every PR here shares base=main). The reset scenario (PR 19) +# is exercised by giving ONLY that PR's marker comment an OLDER base_sha +# ("base-old"), distinct from the current "base-new". cat > "$scripts_dir/bot-gh.sh" <<'BOTGH' #!/usr/bin/env bash # Log every invocation (mirrors pr-comment-fix.test.sh's gh-call-log @@ -89,17 +98,17 @@ case "$1" in esac if printf '%s\n' "$*" | grep -q 'headRefOid'; then cat <<'JSON' -{"number":10,"headRefName":"feat/issue-10-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","baseRefOid":"base1","headRefOid":"sha10"} -{"number":11,"headRefName":"feat/issue-11-a","author":{"login":"testbot"},"labels":[],"mergeable":"MERGEABLE","baseRefOid":"base1","headRefOid":"sha11"} -{"number":12,"headRefName":"feat/issue-12-a","author":{"login":"testbot"},"labels":[{"name":"needs-human"}],"mergeable":"CONFLICTING","baseRefOid":"base1","headRefOid":"sha12"} -{"number":13,"headRefName":"feat/issue-13-a","author":{"login":"testbot"},"labels":[{"name":"claude-rebasing"}],"mergeable":"CONFLICTING","baseRefOid":"base1","headRefOid":"sha13"} -{"number":14,"headRefName":"feat/issue-14-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","baseRefOid":"base1","headRefOid":"sha14"} -{"number":15,"headRefName":"feat/issue-15-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","baseRefOid":"base1","headRefOid":"sha15"} -{"number":20,"headRefName":"feat/issue-20-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","baseRefOid":"base1","headRefOid":"sha20"} -{"number":16,"headRefName":"feat/issue-16-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","baseRefOid":"base1","headRefOid":"sha16"} -{"number":17,"headRefName":"feat/issue-17-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","baseRefOid":"base1","headRefOid":"sha17"} -{"number":18,"headRefName":"feat/issue-18-a","author":{"login":"testbot"},"labels":[],"mergeable":"UNKNOWN","baseRefOid":"base1","headRefOid":"sha18"} -{"number":19,"headRefName":"feat/issue-19-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","baseRefOid":"base-new","headRefOid":"sha19"} +{"number":10,"headRefName":"feat/issue-10-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","headRefOid":"sha10"} +{"number":11,"headRefName":"feat/issue-11-a","author":{"login":"testbot"},"labels":[],"mergeable":"MERGEABLE","headRefOid":"sha11"} +{"number":12,"headRefName":"feat/issue-12-a","author":{"login":"testbot"},"labels":[{"name":"needs-human"}],"mergeable":"CONFLICTING","headRefOid":"sha12"} +{"number":13,"headRefName":"feat/issue-13-a","author":{"login":"testbot"},"labels":[{"name":"claude-rebasing"}],"mergeable":"CONFLICTING","headRefOid":"sha13"} +{"number":14,"headRefName":"feat/issue-14-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","headRefOid":"sha14"} +{"number":15,"headRefName":"feat/issue-15-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","headRefOid":"sha15"} +{"number":20,"headRefName":"feat/issue-20-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","headRefOid":"sha20"} +{"number":16,"headRefName":"feat/issue-16-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","headRefOid":"sha16"} +{"number":17,"headRefName":"feat/issue-17-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","headRefOid":"sha17"} +{"number":18,"headRefName":"feat/issue-18-a","author":{"login":"testbot"},"labels":[],"mergeable":"UNKNOWN","headRefOid":"sha18"} +{"number":19,"headRefName":"feat/issue-19-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","headRefOid":"sha19"} JSON else echo "fake-bot-gh.sh: unexpected pr subcommand: $*" >&2 @@ -108,6 +117,9 @@ JSON ;; api) case "$*" in + *"repos/acme/repo/commits/main --jq .sha"*) + echo "base-new" + ;; *"-X POST"*"/issues/17/labels --input -") touch "$label_marker17" ;; @@ -118,10 +130,10 @@ JSON [ -f "$label_marker17" ] && echo "needs-human" ;; *"issues/16/comments"*) - echo '[{"user":{"login":"testbot"},"body":""}]' + echo '[{"user":{"login":"testbot"},"body":""}]' ;; *"issues/17/comments"*) - echo '[{"user":{"login":"testbot"},"body":""}]' + echo '[{"user":{"login":"testbot"},"body":""}]' ;; *"issues/19/comments"*) echo '[{"user":{"login":"testbot"},"body":""}]' @@ -159,7 +171,7 @@ chmod +x "$scripts_dir/pr-ci-fix.sh" out="$(env -u GATES_FILE BOT_LOGIN=testbot bash "$scripts_dir/pr-rebase.sh" "acme/repo")" check "PR 10 (CONFLICTING, no marker): emitted attempt=1" bash -c ' - printf "%s\n" "$1" | grep -qF "$(printf "10\tfeat/issue-10-a\tsha10\tbase1\t1")"' _ "$out" + printf "%s\n" "$1" | grep -qF "$(printf "10\tfeat/issue-10-a\tsha10\tbase-new\t1")"' _ "$out" check "PR 11 (MERGEABLE): NOT emitted" bash -c '! printf "%s\n" "$1" | grep -qE "^11\b"' _ "$out" check "PR 12 (needs-human guard): NOT emitted" bash -c '! printf "%s\n" "$1" | grep -qE "^12\b"' _ "$out" check "PR 13 (claude-rebasing in-flight guard): NOT emitted" bash -c '! printf "%s\n" "$1" | grep -qE "^13\b"' _ "$out" @@ -167,7 +179,7 @@ check "PR 14 (also a feedback candidate -- precedence): NOT emitted" bash -c '! check "PR 15 (also a comment-fix candidate -- precedence): NOT emitted" bash -c '! printf "%s\n" "$1" | grep -qE "^15\b"' _ "$out" check "PR 20 (also a ci-fix candidate -- precedence): NOT emitted" bash -c '! printf "%s\n" "$1" | grep -qE "^20\b"' _ "$out" check "PR 16 (marker attempt=1 for SAME base_sha): emitted attempt=2" bash -c ' - printf "%s\n" "$1" | grep -qF "$(printf "16\tfeat/issue-16-a\tsha16\tbase1\t2")"' _ "$out" + printf "%s\n" "$1" | grep -qF "$(printf "16\tfeat/issue-16-a\tsha16\tbase-new\t2")"' _ "$out" check "PR 17 (marker attempt=2 for SAME base_sha -- budget exhausted): NOT emitted (escalated)" bash -c '! printf "%s\n" "$1" | grep -qE "^17\b"' _ "$out" check "PR 18 (UNKNOWN mergeable, still computing): NOT emitted" bash -c '! printf "%s\n" "$1" | grep -qE "^18\b"' _ "$out" check "PR 19 (marker for a DIFFERENT/old base_sha -- reset): emitted attempt=1" bash -c ' @@ -185,6 +197,81 @@ check "PR 16 (attempt 2, budget not yet exhausted): needs-human label NOT applie check "PR 19 (reset after base change): needs-human label NOT applied" \ bash -c '! grep -qF -- "-X POST repos/acme/repo/issues/19/labels --input -" "$1"' _ "$gh_log1" +# --------------------------------------------------------------------------- +# fixture2 (issue #187): the base-branch-tip fetch (`gh api +# repos/.../commits/main --jq .sha`) itself fails (transient gh/network +# error) -- the script must NOT abort (no `set -e` crash under +# `set -euo pipefail`) and must still emit the CONFLICTING PR, just with an +# empty base_sha field, degrading exactly like every other guarded gh call +# in this script. +# --------------------------------------------------------------------------- +fixture2="$work/fixture2" +scripts_dir2="$fixture2/.claude/scripts" +mkdir -p "$scripts_dir2" "$fixture2/.claude/state" +cp "$src" "$scripts_dir2/pr-rebase.sh" +cp "$resolve_roots_src" "$scripts_dir2/resolve-roots.sh" +cp "$script_dir/needs-human.sh" "$scripts_dir2/needs-human.sh" +cp "$script_dir/notify.sh" "$scripts_dir2/notify.sh" +cp "$script_dir/log-event.sh" "$scripts_dir2/log-event.sh" + +cat > "$fixture2/.claude/gates.json" <<'EOF' +{ + "modules": [{ "name": "test", "path": ".", "description": "", "owner": "" }], + "merge": { "baseBranch": "main" } +} +EOF + +cat > "$scripts_dir2/bot-gh.sh" <<'BOTGH' +#!/usr/bin/env bash +log_dir="$(cd "$(dirname "$0")" && pwd)" +printf '%s\n' "$*" >> "$log_dir/gh-calls.log" +case "$1" in + repo) echo "acme/repo" ;; + pr) + case "$2" in + comment) exit 0 ;; + esac + if printf '%s\n' "$*" | grep -q 'headRefOid'; then + echo '{"number":30,"headRefName":"feat/issue-30-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","headRefOid":"sha30"}' + else + echo "fake-bot-gh.sh: unexpected pr subcommand: $*" >&2 + exit 1 + fi + ;; + api) + case "$*" in + *"repos/acme/repo/commits/main --jq .sha"*) + echo "fake-bot-gh.sh: simulated transient failure fetching base tip" >&2 + exit 1 + ;; + *"issues/"*"/comments"*) echo '[]' ;; + *) echo "fake-bot-gh.sh: unhandled api call: $*" >&2; exit 1 ;; + esac + ;; + *) echo "fake-bot-gh.sh: unhandled args: $*" >&2; exit 1 ;; +esac +BOTGH +chmod +x "$scripts_dir2/bot-gh.sh" + +cat > "$scripts_dir2/pr-feedback.sh" <<'EOF' +#!/usr/bin/env bash +EOF +chmod +x "$scripts_dir2/pr-feedback.sh" +cat > "$scripts_dir2/pr-comment-fix.sh" <<'EOF' +#!/usr/bin/env bash +EOF +chmod +x "$scripts_dir2/pr-comment-fix.sh" +cat > "$scripts_dir2/pr-ci-fix.sh" <<'EOF' +#!/usr/bin/env bash +EOF +chmod +x "$scripts_dir2/pr-ci-fix.sh" + +out2="$(env -u GATES_FILE BOT_LOGIN=testbot bash "$scripts_dir2/pr-rebase.sh" "acme/repo")" +rc2=$? +check "base-tip fetch failure: script exits 0 (no set -e crash)" bash -c '[ "$1" -eq 0 ]' _ "$rc2" +check "base-tip fetch failure: PR 30 still emitted, attempt=1, base_sha empty" bash -c ' + printf "%s\n" "$1" | grep -qF "$(printf "30\tfeat/issue-30-a\tsha30\t\t1")"' _ "$out2" + echo "" if [ "$fail" -eq 0 ]; then echo "pr-rebase.test.sh: PASS ($ok checks)" From 12c9eb334ceb0209265f2eae3ab719548c1a6cea Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:29:45 +0200 Subject: [PATCH 2/2] fix(loop): fail-closed rebase base-sha fetch + open_prs value validation (issue #187) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening on top of 83dec48's census_error surfacing, addressing review-lens findings: - pr-rebase.sh: a failed base-branch-tip fetch now degrades FAIL-CLOSED (checked exit 1, no candidates emitted) instead of fail-open with an empty base_sha — the old behavior would silently re-emit every CONFLICTING PR as a fresh attempt=1 candidate on every tick of a fetch outage, defeating the anti-livelock budget. loop-census.sh's existing exit-code guard now surfaces census_error=rebase_prs for this case. - loop-census.sh: open_prs is now value-validated, not just exit-code checked — a gh SUCCESS with empty/non-numeric stdout (malformed --jq result) previously sailed past the guard and crashed the later `-eq 0` comparison with a bash "integer expression expected" error instead of a clean census_error. - pr-rebase.test.sh's gh stub now validates the --json field list against gh 2.46.0's real allowlist, so a regression that reintroduces the invalid `baseRefOid` field fails the suite instead of silently passing. - loop-daemon.test.sh: run_daemon_once_stripped_path now uses the same curated PATH as every other helper (previously a bare /usr/bin:/bin, which can resolve the host's real systemd-run/systemctl and hang when this suite runs from inside an active pr-loop-driver-* unit). - Test coverage: per-stage census_error fixtures for every remaining census stage (repo-derive, open_pr_branches, open_issue_set, feedback_prs, ci_fix_prs, comment_fix_prs, planned), the open_prs non-numeric-success case, and a ceiling-vs-census_error precedence regression guard (ceiling_reason always wins). Co-Authored-By: Claude Sonnet 5 --- .claude/scripts/loop-ceilings.test.sh | 40 +++++ .claude/scripts/loop-census.sh | 16 +- .claude/scripts/loop-census.test.sh | 240 ++++++++++++++++++++++++++ .claude/scripts/loop-daemon.test.sh | 14 +- .claude/scripts/pr-rebase.sh | 43 +++-- .claude/scripts/pr-rebase.test.sh | 60 ++++++- 6 files changed, 388 insertions(+), 25 deletions(-) diff --git a/.claude/scripts/loop-ceilings.test.sh b/.claude/scripts/loop-ceilings.test.sh index e301b22..9e40a97 100644 --- a/.claude/scripts/loop-ceilings.test.sh +++ b/.claude/scripts/loop-ceilings.test.sh @@ -483,6 +483,46 @@ dir13="$(new_fixture scenario13 "$CENSUS_READY_42" "$FEEDBACK_PR_17" 0 "$CIFIX_P out13="$(run_tick "$dir13")" check "scenario 13 (feedback beats an also-ready ci-fix): verdict is action=feedback pr=17, not ci-fix" bash -c '[ "$(verdict_of "$1")" = "action=feedback pr=17" ]' _ "$out13" +# --------------------------------------------------------------------------- +# 14. tick_reason precedence (issue #187, non-blocking follow-up): when a +# spend-ceiling breach (ceiling_reason, issue #95) AND one or more +# census_error= lines (issue #187) are BOTH present on the SAME +# tick, loop-tick.sh's own comment above tick_reason ("ceiling_reason +# ALWAYS wins when set -- a spend-ceiling breach is a MORE specific/ +# actionable reason than 'census degraded'") documents ceiling_reason as +# strictly higher precedence. Exercised by combining scenario 2's +# past-expiry fixture (ceiling_reason=expired) with a census fixture that +# ALSO emits census_error=open_prs -- the tick record's reason must be +# EXACTLY "expired", never "census_error:open_prs" and never a +# concatenation of both. +# --------------------------------------------------------------------------- +CENSUS_READY_42_WITH_ERROR='census_error=open_prs +open_prs=-1 +feedback_prs=0 +planned_issues=1 +issue=42 branch=none title=Do the thing +advance_ready=42 +cadence=FAST cron=* * * * *' +dir14="$(new_fixture scenario14 "$CENSUS_READY_42_WITH_ERROR" "" 1)" +node -e ' + const fs = require("fs"); + const dir = process.argv[1] + "/../state"; + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(dir + "/loop-arming.json", JSON.stringify({ + armed_at: "2020-01-01T00:00:00Z", expires_at: "2020-01-08T00:00:00Z", + stop_after_days: 7, notified_expired: false, notice_issue: null, + })); +' "$dir14" +ticks14="$work/scenario14-ticks.jsonl" +out14="$(CLAUDE_TICKS_FILE="$ticks14" run_tick "$dir14")" +check "scenario 14 (ceiling + census_error both fire): verdict is still action=none" bash -c '[ "$(verdict_of "$1")" = "action=none" ]' _ "$out14" +check "scenario 14: tick record reason is EXACTLY ceiling_reason (expired), not census_error, when both fire" node -e ' + const fs = require("fs"); + const obj = JSON.parse(fs.readFileSync(process.argv[1], "utf8").trim()); + if (obj.action !== "none") throw new Error("action mismatch: " + JSON.stringify(obj)); + if (obj.reason !== "expired") throw new Error("expected reason=\"expired\" (ceiling_reason must win over census_error per documented precedence), got " + JSON.stringify(obj)); +' "$ticks14" + echo "" if [ "$fail" -eq 0 ]; then echo "loop-ceilings.test.sh: PASS ($ok checks)" diff --git a/.claude/scripts/loop-census.sh b/.claude/scripts/loop-census.sh index ab31747..071c99b 100644 --- a/.claude/scripts/loop-census.sh +++ b/.claude/scripts/loop-census.sh @@ -571,7 +571,21 @@ get_blocked_by() { # downstream open_prs comparison below takes its conservative/"don't act on # unknown data" branch instead of hitting a bash arithmetic error on a # non-numeric value). -if ! open_prs=$(gh pr list -R "$repo" --state open --base "$base" --json number --jq 'length'); then +# +# The exit-code check alone is not enough (issue #187 follow-up): a gh +# SUCCESS with empty/non-numeric stdout (a malformed --jq result, or a gh +# version whose `length` output isn't a bare integer) would sail past `if ! +# open_prs=$(...)` unnoticed and leave open_prs="" — silently breaking the +# LATER `[ "$open_prs" -eq 0 ]` comparison with a bash "integer expression +# expected" error instead of a clean census_error. Validate the VALUE too, +# reusing the same `case ... ''|*[!0-9]*)` numeric-guard idiom already used +# for merged_count below — any non-digit-only result (including empty) is +# treated exactly like a hard gh failure. +if open_prs=$(gh pr list -R "$repo" --state open --base "$base" --json number --jq 'length'); then + case "$open_prs" in + ''|*[!0-9]*) census_error "open_prs"; open_prs=-1 ;; + esac +else census_error "open_prs" open_prs=-1 fi diff --git a/.claude/scripts/loop-census.test.sh b/.claude/scripts/loop-census.test.sh index d65c811..dbbcd10 100644 --- a/.claude/scripts/loop-census.test.sh +++ b/.claude/scripts/loop-census.test.sh @@ -243,6 +243,8 @@ check "issue=42 branch line shows the origin-prefixed remote-tracking name" bash check "ci_fix_prs=0 counted (no-op pr-ci-fix.sh stub, issue #96)" bash -c 'printf "%s\n" "$1" | grep -qx "ci_fix_prs=0"' _ "$out" check "comment_fix_prs=0 counted (no-op pr-comment-fix.sh stub, issue #96 part 2)" bash -c 'printf "%s\n" "$1" | grep -qx "comment_fix_prs=0"' _ "$out" check "rebase_prs=0 counted (no-op pr-rebase.sh stub, issue #96 part 3)" bash -c 'printf "%s\n" "$1" | grep -qx "rebase_prs=0"' _ "$out" +check "healthy census run (issue #187): zero census_error= lines emitted (nominal fixture must never false-positive)" bash -c \ + '[ "$(printf "%s\n" "$1" | grep -c "^census_error=")" -eq 0 ]' _ "$out" # --------------------------------------------------------------------------- # ci_fix_prs (issue #96): loop-census.sh must surface pr-ci-fix.sh's own @@ -1714,6 +1716,244 @@ check "census still reaches planned_issues= despite both failures (never aborts check "advance_ready=none when open_prs is unknown (conservative -- never advances on bad data)" \ bash -c 'printf "%s\n" "$1" | grep -qx "advance_ready=none"' _ "$outCensusErr" +# --------------------------------------------------------------------------- +# census_error: repo-derive (issue #187 tests-lens fix, blocker #3). Distinct +# from every other stage: `gh repo view` is only ever called when NO repo is +# passed as $1, and a failure there degrades to a MINIMAL, self-consistent +# contract (every count zeroed/none, cadence=IDLE) printed BEFORE any other +# gh call is attempted, then exits 0 -- not a partial run of the other 8 +# stages. Asserted specifically, as its own fixture, rather than folded into +# the table-driven block below. +# --------------------------------------------------------------------------- +dirRepoDerive="$work/repo-derive" +scriptsRepoDerive="$dirRepoDerive/.claude/scripts" +mkdir -p "$scriptsRepoDerive" +cp "$census_src" "$scriptsRepoDerive/loop-census.sh" +cp "$resolve_roots_src" "$scriptsRepoDerive/resolve-roots.sh" +cat > "$dirRepoDerive/.claude/gates.json" <<'EOF' +{ + "modules": [{ "name": "test", "path": ".", "description": "", "owner": "" }], + "merge": { "baseBranch": "main" } +} +EOF +for stub in pr-feedback pr-ci-fix pr-comment-fix pr-rebase; do + cat > "$scriptsRepoDerive/$stub.sh" <&2 +exit 1 +EOF +done +cat > "$scriptsRepoDerive/bot-gh.sh" <<'EOF' +#!/usr/bin/env bash +case "$1" in + repo) echo "fake-bot-gh.sh: simulated gh repo view failure" >&2; exit 1 ;; + *) echo "fake-bot-gh.sh: should NEVER be called past repo-derive: $*" >&2; exit 1 ;; +esac +EOF +chmod +x "$scriptsRepoDerive"/*.sh +git -C "$dirRepoDerive" init -q -b main +git -C "$dirRepoDerive" -c user.email=t@e.st -c user.name=t commit -q --allow-empty -m init + +# NOTE: no repo arg passed -- forces loop-census.sh down the `gh repo view` +# fallback path (the only way repo-derive's failure branch can be reached). +outRepoDerive="$(env -u GATES_FILE bash "$scriptsRepoDerive/loop-census.sh" 2>/dev/null)" +rcRepoDerive=$? +check "census_error=repo-derive: script exits 0 (degraded contract printed, not a set -e crash)" \ + bash -c '[ "$1" -eq 0 ]' _ "$rcRepoDerive" +check "census_error=repo-derive line emitted" \ + bash -c 'printf "%s\n" "$1" | grep -qx "census_error=repo-derive"' _ "$outRepoDerive" +check "census_error=repo-derive: minimal degraded contract -- open_prs=0" \ + bash -c 'printf "%s\n" "$1" | grep -qx "open_prs=0"' _ "$outRepoDerive" +check "census_error=repo-derive: minimal degraded contract -- rebase_prs=0" \ + bash -c 'printf "%s\n" "$1" | grep -qx "rebase_prs=0"' _ "$outRepoDerive" +check "census_error=repo-derive: minimal degraded contract -- advance_ready=none" \ + bash -c 'printf "%s\n" "$1" | grep -qx "advance_ready=none"' _ "$outRepoDerive" +check "census_error=repo-derive: minimal degraded contract -- cadence=IDLE" \ + bash -c 'printf "%s\n" "$1" | grep -q "^cadence=IDLE"' _ "$outRepoDerive" +check "census_error=repo-derive: exactly one census_error= line (the early-exit itself, nothing downstream ran)" \ + bash -c '[ "$(printf "%s\n" "$1" | grep -c "^census_error=")" -eq 1 ]' _ "$outRepoDerive" +check "census_error=repo-derive: none of the sibling PR-event scripts were ever invoked (true early exit)" \ + bash -c '! printf "%s\n" "$1" | grep -q "should NEVER be invoked"' _ "$outRepoDerive" + +# --------------------------------------------------------------------------- +# census_error: remaining stage coverage (issue #187 tests-lens fix, blocker +# #3) -- open_pr_branches, open_issue_set, feedback_prs, ci_fix_prs, +# comment_fix_prs, planned. (open_prs and rebase_prs are already covered +# above/in pr-rebase.test.sh; repo-derive is covered separately above because +# of its distinct early-exit contract.) One parameterized fixture generator, +# table-driven over the 6 remaining stages: BREAK_STAGE selects exactly ONE +# call to fail per run, every other call succeeds normally, so a genuine +# failure is attributable to ONLY that stage -- and the "exactly one +# census_error= line" assertion below actively guards against a broken stage +# ALSO false-positiving some other, unrelated stage. +# --------------------------------------------------------------------------- +build_stage_error_fixture() { + # $1 = dir name under $work -> prints fixture root + local dir="$work/$1" + local scripts="$dir/.claude/scripts" + mkdir -p "$scripts" + cp "$census_src" "$scripts/loop-census.sh" + cp "$resolve_roots_src" "$scripts/resolve-roots.sh" + cat > "$dir/.claude/gates.json" <<'EOF' +{ + "modules": [{ "name": "test", "path": ".", "description": "", "owner": "" }], + "merge": { "baseBranch": "main" } +} +EOF + git -C "$dir" init -q -b main + git -C "$dir" -c user.email=t@e.st -c user.name=t commit -q --allow-empty -m init + printf '%s\n' "$dir" +} + +write_breakable_stub() { + # $1=path $2=BREAK_STAGE value that makes this stub fail; anything else + # (including BREAK_STAGE unset/a DIFFERENT stage under test) succeeds as a + # plain no-op, exactly like the nominal fixture1 stubs above. + local path="$1" stage="$2" + { + printf '#!/usr/bin/env bash\n' + printf 'if [ "${BREAK_STAGE:-}" = "%s" ]; then\n' "$stage" + printf ' echo "simulated failure (BREAK_STAGE=%s)" >&2\n' "$stage" + printf ' exit 1\n' + printf 'fi\n' + printf 'exit 0\n' + } > "$path" + chmod +x "$path" +} + +for stage in open_pr_branches open_issue_set feedback_prs ci_fix_prs comment_fix_prs planned; do + dirStage="$(build_stage_error_fixture "stage-$stage")" + scriptsStage="$dirStage/.claude/scripts" + write_breakable_stub "$scriptsStage/pr-feedback.sh" "feedback_prs" + write_breakable_stub "$scriptsStage/pr-ci-fix.sh" "ci_fix_prs" + write_breakable_stub "$scriptsStage/pr-comment-fix.sh" "comment_fix_prs" + write_breakable_stub "$scriptsStage/pr-rebase.sh" "rebase_prs" + # bot-gh.sh: nominal (all-succeeding) responses for every gh call + # loop-census.sh makes on a healthy tick, EXCEPT the one selected by + # $BREAK_STAGE, which fails exactly like a real transient gh error would. + cat > "$scriptsStage/bot-gh.sh" <<'EOF' +#!/usr/bin/env bash +case "$1" in + repo) echo "acme/repo" ;; + pr) + if printf '%s\n' "$*" | grep -q -- '--state merged'; then + echo 0 # stale-merged-remote per-branch check (not under test; unreached here anyway) + elif printf '%s\n' "$*" | grep -q -- '--json number '; then + echo 0 # open_prs count (covered by a separate fixture, not this table) + elif printf '%s\n' "$*" | grep -q 'headRefName'; then + if [ "${BREAK_STAGE:-}" = "open_pr_branches" ]; then + echo "simulated open_pr_branches failure" >&2 + exit 1 + fi + : # nominal: no open PR branches + else + echo "fake-bot-gh.sh: unexpected pr subcommand: $*" >&2 + exit 1 + fi + ;; + issue) + case "$2" in + list) + if printf '%s\n' "$*" | grep -q -- '--label'; then + if [ "${BREAK_STAGE:-}" = "planned" ]; then + echo "simulated planned failure" >&2 + exit 1 + fi + printf '5\tplanned,module:test\t\tCandidate five\n' + else + if [ "${BREAK_STAGE:-}" = "open_issue_set" ]; then + echo "simulated open_issue_set failure" >&2 + exit 1 + fi + printf '99\n' + fi + ;; + view) echo '{"body":""}' ;; + *) echo "fake-bot-gh.sh: unhandled issue subcmd: $*" >&2; exit 1 ;; + esac + ;; + api) exit 1 ;; # milestones REST fetch -- degrades gracefully (`|| true` in loop-census.sh) + *) echo "fake-bot-gh.sh: unhandled args: $*" >&2; exit 1 ;; +esac +EOF + chmod +x "$scriptsStage/bot-gh.sh" + + outStage="$(env -u GATES_FILE BREAK_STAGE="$stage" bash "$scriptsStage/loop-census.sh" "acme/repo" 2>/dev/null)" + rcStage=$? + check "census_error stage coverage ($stage): script exits 0 (no set -e crash)" \ + bash -c '[ "$1" -eq 0 ]' _ "$rcStage" + check "census_error stage coverage ($stage): census_error=$stage line emitted" bash -c \ + "printf '%s\n' \"\$1\" | grep -qx 'census_error=$stage'" _ "$outStage" + check "census_error stage coverage ($stage): exactly one census_error= line (no cross-stage false-positive)" bash -c \ + '[ "$(printf "%s\n" "$1" | grep -c "^census_error=")" -eq 1 ]' _ "$outStage" + check "census_error stage coverage ($stage): census still reaches planned_issues= (never aborts early)" \ + bash -c 'printf "%s\n" "$1" | grep -q "^planned_issues="' _ "$outStage" + check "census_error stage coverage ($stage): census still reaches advance_ready= (never aborts early)" \ + bash -c 'printf "%s\n" "$1" | grep -q "^advance_ready="' _ "$outStage" +done + +# --------------------------------------------------------------------------- +# open_prs: gh SUCCEEDS (rc=0) but returns empty/non-numeric stdout (issue +# #187 follow-up, correctness-lens finding). The exit-code-only guard above +# would sail right past this — open_prs="" with no census_error — and the +# LATER `[ "$open_prs" -eq 0 ]` comparison would then blow up with a bash +# "integer expression expected" error instead of a clean, greppable signal. +# Reuses build_stage_error_fixture's nominal shape; only bot-gh.sh's `pr +# list --json number ` response differs (rc=0, stdout is the literal string +# "null" -- a real-world shape a malformed --jq expression could produce). +# --------------------------------------------------------------------------- +dirOpenPrsGarbage="$(build_stage_error_fixture "stage-open_prs_garbage")" +scriptsOpenPrsGarbage="$dirOpenPrsGarbage/.claude/scripts" +write_breakable_stub "$scriptsOpenPrsGarbage/pr-feedback.sh" "__never__" +write_breakable_stub "$scriptsOpenPrsGarbage/pr-ci-fix.sh" "__never__" +write_breakable_stub "$scriptsOpenPrsGarbage/pr-comment-fix.sh" "__never__" +write_breakable_stub "$scriptsOpenPrsGarbage/pr-rebase.sh" "__never__" +cat > "$scriptsOpenPrsGarbage/bot-gh.sh" <<'EOF' +#!/usr/bin/env bash +case "$1" in + repo) echo "acme/repo" ;; + pr) + if printf '%s\n' "$*" | grep -q -- '--json number '; then + echo "null" # rc=0, but non-numeric -- simulates a malformed --jq result + elif printf '%s\n' "$*" | grep -q 'headRefName'; then + : # nominal: no open PR branches + else + echo "fake-bot-gh.sh: unexpected pr subcommand: $*" >&2 + exit 1 + fi + ;; + issue) + case "$2" in + list) + if printf '%s\n' "$*" | grep -q -- '--label'; then + printf '5\tplanned,module:test\t\tCandidate five\n' + else + printf '99\n' + fi + ;; + view) echo '{"body":""}' ;; + *) echo "fake-bot-gh.sh: unhandled issue subcmd: $*" >&2; exit 1 ;; + esac + ;; + api) exit 1 ;; + *) echo "fake-bot-gh.sh: unhandled args: $*" >&2; exit 1 ;; +esac +EOF +chmod +x "$scriptsOpenPrsGarbage/bot-gh.sh" + +outOpenPrsGarbage="$(env -u GATES_FILE bash "$scriptsOpenPrsGarbage/loop-census.sh" "acme/repo" 2>&1)" +rcOpenPrsGarbage=$? +check "open_prs non-numeric success: script exits 0 (no bash 'integer expression expected' crash)" \ + bash -c '[ "$1" -eq 0 ]' _ "$rcOpenPrsGarbage" +check "open_prs non-numeric success: census_error=open_prs line emitted (value validated, not just exit code)" \ + bash -c 'printf "%s\n" "$1" | grep -qx "census_error=open_prs"' _ "$outOpenPrsGarbage" +check "open_prs non-numeric success: open_prs degrades to -1 (never the bare non-numeric value)" \ + bash -c 'printf "%s\n" "$1" | grep -qx "open_prs=-1"' _ "$outOpenPrsGarbage" +check "open_prs non-numeric success: no bash 'integer expression expected' error leaked into output" \ + bash -c '! printf "%s\n" "$1" | grep -q "integer expression expected"' _ "$outOpenPrsGarbage" +check "open_prs non-numeric success: census still reaches advance_ready= (never aborts early)" \ + bash -c 'printf "%s\n" "$1" | grep -q "^advance_ready="' _ "$outOpenPrsGarbage" + echo "" if [ "$fail" -eq 0 ]; then echo "loop-census.test.sh: PASS ($ok checks)" diff --git a/.claude/scripts/loop-daemon.test.sh b/.claude/scripts/loop-daemon.test.sh index c496459..48dac78 100644 --- a/.claude/scripts/loop-daemon.test.sh +++ b/.claude/scripts/loop-daemon.test.sh @@ -263,8 +263,18 @@ run_daemon_once_stripped_path() { # that has NEITHER node NOR claude (mirroring pr-loop.service's minimal # systemd PATH before issue #107's baked-PATH fix), to exercise main()'s # startup ensure_claude_on_path nvm fallback instead of the fixture's own - # bin/ dir. - ( cd "$1" && PATH="/usr/bin:/bin" NVM_DIR="$2" LOOP_DAEMON_MAX_ITERATIONS=1 LOOP_DAEMON_SLEEP_FAST=0 LOOP_DAEMON_SLEEP_WATCH=0 LOOP_DAEMON_SLEEP_IDLE=0 LOOP_DAEMON_SLEEP_FALLBACK=0 bash .claude/scripts/loop-daemon.sh ) + # bin/ dir. Uses $curated_bin (NOT a bare "/usr/bin:/bin") for the exact + # same reason as run_daemon_once/run_daemon_once_env: a bare /usr/bin:/bin + # still resolves the HOST's real systemd-run/systemctl if present, and this + # test runs FROM INSIDE a real pr-loop-driver-issue* systemd unit (the + # autonomous loop dogfoods its own test gate), so main()'s startup re-attach + # (issue #119 pt 3 -- list_active_driver_units + wait_for_driver_unit) would + # see that genuinely-active host unit and block forever in its reattach poll + # loop, hanging this whole test suite. $curated_bin already omits + # node/claude/systemd-run/systemctl (see its build comment above), so it + # preserves this scenario's PATH-resolution purpose while restoring the + # systemd isolation every other helper here relies on. + ( cd "$1" && PATH="$curated_bin" NVM_DIR="$2" LOOP_DAEMON_MAX_ITERATIONS=1 LOOP_DAEMON_SLEEP_FAST=0 LOOP_DAEMON_SLEEP_WATCH=0 LOOP_DAEMON_SLEEP_IDLE=0 LOOP_DAEMON_SLEEP_FALLBACK=0 bash .claude/scripts/loop-daemon.sh ) } # --------------------------------------------------------------------------- diff --git a/.claude/scripts/pr-rebase.sh b/.claude/scripts/pr-rebase.sh index b175c7d..193d94f 100644 --- a/.claude/scripts/pr-rebase.sh +++ b/.claude/scripts/pr-rebase.sh @@ -104,20 +104,35 @@ base="$(node -e 'const g=require(process.argv[1]); console.log((g.merge&&g.merge # Base branch tip SHA (issue #187): `baseRefOid` is NOT a valid `gh pr list # --json` field on the installed gh (2.46.0) -- it only exists on the -# GraphQL PullRequest type, which this gh has no `gh api graphql` support -# for either. Requesting it in the `--json` list below made gh print -# "Unknown JSON field" + exit non-zero on EVERY invocation, which the caller -# (loop-census.sh) silently swallowed to rebase_prs=0 -- a permanent no-op. -# Fix: every PR this script queries shares the SAME base branch ($base, -# enforced by --base below), so the PR-level baseRefOid GitHub would report -# is, by definition, identical to that base branch's own current tip -- -# fetch it ONCE via a plain REST call instead of a per-PR graphql round -# trip. A failed fetch degrades to base_sha="" (never aborts): no marker -# comment will ever match an empty sha, so every CONFLICTING PR is simply -# treated as attempt=1 (fresh) for this tick rather than crashing the whole -# script -- the anti-livelock budget still applies correctly on the NEXT -# tick once the fetch succeeds. -base_sha="$(gh api "repos/$repo/commits/$base" --jq .sha 2>/dev/null)" || base_sha="" +# GraphQL PullRequest type. `gh api graphql` DOES work on this gh (verified +# directly; the installed gh has no problem executing a graphql query) -- +# but a REST one-shot call here is still strictly the better choice, not +# because graphql is unavailable: every PR this script queries shares the +# SAME base branch ($base, enforced by --base below), so the PR-level +# baseRefOid GitHub would report is, by definition, identical to that base +# branch's own current tip -- one plain REST call replaces what would +# otherwise be a per-PR graphql round trip. Requesting `baseRefOid` in the +# `--json` list below (the pre-#187 bug) made gh print "Unknown JSON field" +# + exit non-zero on EVERY invocation, which the caller (loop-census.sh) +# silently swallowed to rebase_prs=0 -- a permanent no-op. +# +# Fail-CLOSED on a failed fetch, NOT fail-open: an empty base_sha can never +# match a marker's `` +# ([^:]+ never matches an empty string), so treating base_sha="" as a green +# light would silently re-emit EVERY CONFLICTING PR as a fresh attempt=1 +# candidate on every tick of a fetch outage -- the driver would then post an +# unreadable `claude-rebase-attempted::1` marker and the anti-livelock +# budget above would stop applying without any signal that anything was +# wrong. Emit NOTHING instead and exit non-zero, so loop-census.sh's guard +# (the same exit-code-checked pattern used for every sibling PR-event call -- +# see its own comment) surfaces `census_error=rebase_prs`: the loop sees +# "census degraded" rather than misreading a fetch outage as "nothing to +# rebase". This is a deliberate, checked `exit 1`, not an uncaught `set -e` +# crash -- the "never abort the script" property is preserved. +if ! base_sha="$(gh api "repos/$repo/commits/$base" --jq .sha 2>/dev/null)"; then + echo "pr-rebase.sh: failed to fetch base branch tip sha for '$base' -- degrading to zero rebase candidates this tick (fail-closed, issue #187)" >&2 + exit 1 +fi # Feedback, comment-fix, AND ci-fix candidates OUTRANK rebase (precedence) — # exclude their PR numbers up front so a PR that qualifies for any of them diff --git a/.claude/scripts/pr-rebase.test.sh b/.claude/scripts/pr-rebase.test.sh index 5e064e3..88128b6 100644 --- a/.claude/scripts/pr-rebase.test.sh +++ b/.claude/scripts/pr-rebase.test.sh @@ -77,7 +77,22 @@ EOF # return "base-new" -- the SAME value for every PR, exactly like the real # per-run fetch (every PR here shares base=main). The reset scenario (PR 19) # is exercised by giving ONLY that PR's marker comment an OLDER base_sha -# ("base-old"), distinct from the current "base-new". +# ("base-old"), distinct from the current "base-new"). +# +# `--json` field validation (issue #187 tests-lens fix): a prior version of +# this stub dispatched on a bare `grep -q 'headRefOid'` and returned canned +# JSON for ANY `--json` field list containing that string -- so re-adding the +# invalid `baseRefOid` field back into pr-rebase.sh's --json list (the exact +# production defect this issue fixed) still made the suite go green, because +# the stub never actually validated the field list the way real gh does. +# `valid_pr_json_fields` below is the EXACT `gh pr list --json ` +# allowlist reported by the real installed gh (2.46.0; confirm with +# `gh pr list --json bogus`) -- deliberately NOT including `baseRefOid` +# (which only exists on the GraphQL PullRequest type, never on `gh pr list`). +# Any `--json` field outside this list makes the stub fail exactly like real +# gh does ("Unknown JSON field: ..." + exit non-zero), so a future +# reintroduction of `baseRefOid` here fails this suite instead of silently +# passing. cat > "$scripts_dir/bot-gh.sh" <<'BOTGH' #!/usr/bin/env bash # Log every invocation (mirrors pr-comment-fix.test.sh's gh-call-log @@ -90,12 +105,38 @@ cat > "$scripts_dir/bot-gh.sh" <<'BOTGH' log_dir="$(cd "$(dirname "$0")" && pwd)" label_marker17="$log_dir/labeled-17.marker" printf '%s\n' "$*" >> "$log_dir/gh-calls.log" +# Real gh 2.46.0's `gh pr list --json ` allowlist (confirm with +# `gh pr list --json bogus`), deliberately NOT including `baseRefOid` (which +# only exists on the GraphQL PullRequest type, never on `gh pr list`). +valid_pr_json_fields=" additions assignees author autoMergeRequest baseRefName body changedFiles closed closedAt comments commits createdAt deletions files headRefName headRefOid headRepository headRepositoryOwner id isCrossRepository isDraft labels latestReviews maintainerCanModify mergeCommit mergeStateStatus mergeable mergedAt mergedBy milestone number potentialMergeCommit projectCards projectItems reactionGroups reviewDecision reviewRequests reviews state statusCheckRollup title updatedAt url " +validate_json_fields() { + # $1 = comma-separated --json field list; mimics real gh's per-field + # "Unknown JSON field" rejection (issue #187) so a bogus/invalid field + # (e.g. a reintroduced baseRefOid) fails the SAME way real gh 2.46.0 does. + local field + IFS=',' read -ra _fields <<< "$1" + for field in "${_fields[@]}"; do + case "$valid_pr_json_fields" in + *" $field "*) : ;; + *) echo "Unknown JSON field: \"$field\"" >&2; return 1 ;; + esac + done +} case "$1" in repo) echo "acme/repo" ;; pr) case "$2" in comment) exit 0 ;; # needs_human_flag's `gh pr comment` esac + json_fields="" + prev="" + for a in "$@"; do + [ "$prev" = "--json" ] && json_fields="$a" + prev="$a" + done + if [ -n "$json_fields" ]; then + validate_json_fields "$json_fields" || exit 1 + fi if printf '%s\n' "$*" | grep -q 'headRefOid'; then cat <<'JSON' {"number":10,"headRefName":"feat/issue-10-a","author":{"login":"testbot"},"labels":[],"mergeable":"CONFLICTING","headRefOid":"sha10"} @@ -200,10 +241,14 @@ check "PR 19 (reset after base change): needs-human label NOT applied" \ # --------------------------------------------------------------------------- # fixture2 (issue #187): the base-branch-tip fetch (`gh api # repos/.../commits/main --jq .sha`) itself fails (transient gh/network -# error) -- the script must NOT abort (no `set -e` crash under -# `set -euo pipefail`) and must still emit the CONFLICTING PR, just with an -# empty base_sha field, degrading exactly like every other guarded gh call -# in this script. +# error). This must degrade FAIL-CLOSED: no `set -e` crash (a deliberate, +# checked `exit 1`, not an uncaught failure), but ALSO no candidates emitted +# -- an empty base_sha would otherwise re-emit the CONFLICTING PR as a fresh +# attempt=1 candidate every tick of the outage (the OLD, fail-open +# behavior this fixture used to assert before issue #187's fail-closed fix). +# The script's non-zero exit is what lets loop-census.sh's guard (same +# exit-code-checked pattern as every sibling PR-event call) surface +# census_error=rebase_prs instead of silently reporting "nothing to do". # --------------------------------------------------------------------------- fixture2="$work/fixture2" scripts_dir2="$fixture2/.claude/scripts" @@ -268,9 +313,8 @@ chmod +x "$scripts_dir2/pr-ci-fix.sh" out2="$(env -u GATES_FILE BOT_LOGIN=testbot bash "$scripts_dir2/pr-rebase.sh" "acme/repo")" rc2=$? -check "base-tip fetch failure: script exits 0 (no set -e crash)" bash -c '[ "$1" -eq 0 ]' _ "$rc2" -check "base-tip fetch failure: PR 30 still emitted, attempt=1, base_sha empty" bash -c ' - printf "%s\n" "$1" | grep -qF "$(printf "30\tfeat/issue-30-a\tsha30\t\t1")"' _ "$out2" +check "base-tip fetch failure: script exits non-zero (fail-closed, issue #187 -- lets loop-census.sh surface census_error=rebase_prs)" bash -c '[ "$1" -ne 0 ]' _ "$rc2" +check "base-tip fetch failure: NO candidates emitted (fail-closed -- PR 30 must NOT be re-emitted as a bare attempt=1)" bash -c '[ -z "$1" ]' _ "$out2" echo "" if [ "$fail" -eq 0 ]; then