From 10abb0d9281c293e01acd6e397a9949f668a127d Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Wed, 8 Jul 2026 15:47:42 +0200 Subject: [PATCH 1/2] feat(loop): harden the tick with loop-tick.sh + in-flight detection + spawn lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapses the multi-step PR-loop tick (census, poll, merge, feedback-check) into one script that computes a single machine-readable verdict (action=none/advance/feedback) instead of leaving that arithmetic to be re-derived from a prompt on every firing — the tick used to drift under smaller/cheaper models (fabricated step output, double-spawned orchestrators). loop-census.sh now reports in_flight=N (a branch exists but no PR yet), and loop-tick.sh adds a self-healing spawn lock (.claude/state/loop-advance.lock) so a second tick can't double-spawn an orchestrator for an issue already mid-flight. Fixes #81. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_0135JVnX98DjPxTRzQZRy82q --- .claude/scripts/loop-census.sh | 111 ++++++++++++++++++ .claude/scripts/loop-tick.sh | 132 +++++++++++++++++++++ .claude/scripts/loop-tick.test.sh | 189 ++++++++++++++++++++++++++++++ docs/GETTING_STARTED.md | 8 ++ docs/USAGE.md | 29 ++++- 5 files changed, 463 insertions(+), 6 deletions(-) create mode 100644 .claude/scripts/loop-census.sh create mode 100644 .claude/scripts/loop-tick.sh create mode 100644 .claude/scripts/loop-tick.test.sh diff --git a/.claude/scripts/loop-census.sh b/.claude/scripts/loop-census.sh new file mode 100644 index 0000000..6e0d510 --- /dev/null +++ b/.claude/scripts/loop-census.sh @@ -0,0 +1,111 @@ +#!/usr/bin/env bash +# loop-census.sh — one-shot STEP 0 census for the PR loop (base and self-hosted). +# Prints, as stable key=value telemetry, everything a tick needs to decide +# whether it can ACT — so the actionability check is a single pre-approvable +# command instead of a discipline the tick can silently skip: +# +# open_prs=N open PRs against the adapter's base branch +# feedback_prs=N bot PRs with unaddressed CHANGES_REQUESTED (pr-feedback.sh) +# planned_issues=N open issues labelled `planned` AND one of the +# adapter's module:* labels, one detail line each: +# issue= branch= title= +# in_flight=<n> one line PER planned issue that has a +# feat/issue-n-* branch (local or remote) but NO +# open PR for it yet — i.e. work has started but +# hasn't reached PR stage. A tick uses this to +# avoid double-spawning an orchestrator for an +# issue that already has a worktree in progress. +# advance_ready=<n|none> lowest-numbered planned issue with no branch, +# only when open_prs=0 (the ADVANCE precondition) +# cadence=FAST|WATCH|IDLE cron=<expr> desired cadence per the loop policy +# +# The module label set is derived from $GATES_FILE (default .claude/gates.json) +# → modules[].name, so the same script serves the self-hosted loop +# (GATES_FILE=.claude/self/gates.json) and downstream adopters. +# +# WHY THIS EXISTS (issue: loop stalled 13h with two planned issues): ticks that +# "optimized" STEP 0 away — or piped the cursor-advancing notify-poll.sh through +# `tail -1` — reported "No actionable activity" while ADVANCE work sat ready. +# A tick may claim "No actionable activity" ONLY when this census prints zeros. +# +# Repo derived from the git remote; override with $1. Bot login via $BOT_LOGIN. +# Invoke as `bash .claude/scripts/loop-census.sh` (pre-approve that exact +# command). Read-only: advances no cursor, mutates nothing — safe to re-run. +set -euo pipefail + +# Two-root derivation (issue #63): script_dir = sibling scripts, root = consumer project. +# shellcheck source=resolve-roots.sh +. "$(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)}" + +gates_rel="${GATES_FILE:-.claude/gates.json}" +case "$gates_rel" in /*) gates="$gates_rel" ;; *) gates="$root/$gates_rel" ;; esac + +# 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") + +open_prs=$(gh pr list -R "$repo" --state open --base "$base" --json number --jq 'length') +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') + +feedback_prs=$(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. +planned=$(gh issue list -R "$repo" --state open --label planned --json number,title,labels \ + --jq '.[] | [.number, ([.labels[].name]|join(",")), .title] | @tsv' | sort -n) + +planned_count=0 +advance_ready="none" +detail="" +in_flight="" +while IFS=$'\t' read -r num labels title; do + [ -z "${num:-}" ] && continue + hit=0 + while IFS= read -r ml; do + case ",$labels," in *",$ml,"*) hit=1; break;; esac + done <<< "$module_labels" + [ "$hit" -eq 1 ] || continue + planned_count=$((planned_count + 1)) + # Existing feat/issue-<n>-* branch (local or remote) means it's already in flight. + branch=$(git -C "$root" branch -a --list "*feat/issue-$num-*" | head -1 | sed 's/^[* ]*//;s|^remotes/||') + [ -n "$branch" ] || branch="none" + detail+="issue=$num branch=$branch title=$title"$'\n' + if [ "$advance_ready" = "none" ] && [ "$branch" = "none" ] && [ "$open_prs" -eq 0 ]; then + advance_ready="$num" + fi + # in_flight: a branch exists for this issue but no open PR carries it yet + # (branch may be printed with a "origin/" remote prefix above; strip it — + # or match it as a "/"-suffix — before comparing against headRefName, which + # is always the bare branch name). + if [ "$branch" != "none" ]; then + has_open_pr=0 + while IFS= read -r b; do + [ -z "$b" ] && continue + case "$branch" in + "$b"|*"/$b") has_open_pr=1; break ;; + esac + done <<< "$open_pr_branches" + [ "$has_open_pr" -eq 1 ] || in_flight+="in_flight=$num"$'\n' + fi +done <<< "$planned" + +echo "planned_issues=$planned_count" +[ -n "$detail" ] && printf '%s' "$detail" +[ -n "$in_flight" ] && printf '%s' "$in_flight" +echo "advance_ready=$advance_ready" + +# Desired cadence per the loop policy: FAST only when the loop can ACT now. +if [ "$feedback_prs" -ge 1 ] || { [ "$open_prs" -eq 0 ] && [ "$planned_count" -ge 1 ]; }; then + echo 'cadence=FAST cron=* * * * *' +elif [ "$open_prs" -ge 1 ]; then + echo 'cadence=WATCH cron=*/5 * * * *' +else + echo 'cadence=IDLE cron=*/15 * * * *' +fi diff --git a/.claude/scripts/loop-tick.sh b/.claude/scripts/loop-tick.sh new file mode 100644 index 0000000..3cd5bd3 --- /dev/null +++ b/.claude/scripts/loop-tick.sh @@ -0,0 +1,132 @@ +#!/usr/bin/env bash +# loop-tick.sh — one-shot orchestration tick for the autonomous PR loop. +# +# Runs the loop's four step scripts, IN ORDER, with their FULL output +# preserved (never swallowed or `tail -1`'d), then emits exactly one +# machine-readable verdict line as the LAST line of output: +# action=none +# action=advance issue=N +# action=feedback pr=N +# +# WHY THIS EXISTS (issue #81): the tick used to be a multi-step PROMPT +# (.claude/commands/pr-loop.md) that a model re-derived, from scratch, every +# firing. Repetition is exactly where smaller/cheaper models drift — a +# Haiku-driven tick has been observed to stop invoking the step scripts and +# fabricate their output, and to double-spawn an orchestrator for the same +# issue because it misread an in-flight worktree as hung. Collapsing the +# whole tick to ONE script plus one conditional spawn (of the ADVANCE/FEEDBACK +# work itself) makes the protocol immune to that drift: the verdict line is +# computed by shell/node logic, not recalled by the model from a prompt. +# +# This script does NOT reimplement census, polling, merge, or feedback-detection +# logic — it calls the existing sibling scripts and only adds the verdict +# arithmetic + the spawn lock (see .claude/state/loop-advance.lock below). +# +# Precedence: unaddressed CHANGES_REQUESTED feedback (pr-feedback.sh) always +# wins over ADVANCE — a human is waiting on a reply. When multiple PRs need +# feedback addressed, the lowest-numbered PR is picked. ADVANCE additionally +# requires: census says advance_ready=N (already means zero open PRs + a +# planned+module issue + no existing branch), N is not census's in_flight=N +# (a feat/issue-N-* branch with no open PR — someone/something is already +# mid-flight on it), and the spawn lock (below) is not already held for N. +# +# Spawn lock: .claude/state/loop-advance.lock (root-relative; .claude/state/ +# is already gitignored). Written the moment this script emits +# `action=advance issue=N`, so a SECOND tick — fired before the first +# implementer has even pushed a branch — is refused by this script's own +# logic rather than by model discipline. Format: one line, +# `issue=N ts=<UTC ISO-8601>`. INVARIANT: the lock for issue N is considered +# released once EITHER (a) an open PR now exists for N, or (b) no +# feat/issue-N-* branch exists at all — i.e. census no longer reports N as +# advance_ready or in_flight. This script self-heals: on every run it checks +# the held lock (if any) against the FRESH census output and clears it if it +# no longer qualifies, so a stale lock (e.g. left behind by a crashed +# orchestrator) never permanently blocks the issue. Written atomically +# (temp file + mv) to avoid a torn read from a concurrent tick. +# +# Repo derived from the git remote; override with $1. Bot login via +# $BOT_LOGIN (passed through to the step scripts). Honors $GATES_FILE exactly +# like the sibling scripts (loop-census.sh reads it directly; the others fall +# back to the default adapter). +# +# Invoke as `bash .claude/scripts/loop-tick.sh` (pre-approve that exact +# command). Safe to run: this script itself only reads and computes a +# verdict + lock file — its only SIDE EFFECTS are the ones already documented +# on the step scripts it calls (notify-poll.sh advances its cursor; +# merge-ready.sh merges owner-approved, CI-green PRs and fast-forwards a +# clean local checkout on main). It never itself opens a PR, merges, or +# spawns an agent — it only tells the caller which single action to take. +set -uo pipefail + +# Two-root derivation (issue #63): script_dir = sibling scripts, root = consumer project. +# shellcheck source=resolve-roots.sh +. "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/resolve-roots.sh" +# Route EVERY gh call (ours and the step scripts') through the bot identity. +gh() { bash "$script_dir/bot-gh.sh" "$@"; } +repo="${1:-$(gh repo view --json nameWithOwner -q .nameWithOwner)}" + +echo "=== 1/4 loop-census.sh ===" +census_out="$(bash "$script_dir/loop-census.sh" "$repo")" +printf '%s\n' "$census_out" + +echo "=== 2/4 notify-poll.sh ===" +bash "$script_dir/notify-poll.sh" "$repo" + +echo "=== 3/4 merge-ready.sh ===" +bash "$script_dir/merge-ready.sh" "$repo" + +echo "=== 4/4 pr-feedback.sh ===" +feedback_out="$(bash "$script_dir/pr-feedback.sh" "$repo")" +printf '%s\n' "$feedback_out" + +echo "=== verdict ===" + +# --- Parse census telemetry needed for the verdict ------------------------- +advance_ready="$(printf '%s\n' "$census_out" | sed -n 's/^advance_ready=//p' | tail -1)" +advance_ready="${advance_ready:-none}" +in_flight_issues="$(printf '%s\n' "$census_out" | sed -n 's/^in_flight=//p')" + +# --- Parse pr-feedback.sh's TSV (num, branch, reviewer, changes_requested_at) -- +# Lowest-numbered PR wins when several need feedback addressed. +feedback_pr="$(printf '%s\n' "$feedback_out" | awk -F'\t' 'NF>=1 && $1 ~ /^[0-9]+$/ {print $1}' | sort -n | head -1)" + +# --- Spawn lock: read + self-heal against the FRESH census above ----------- +state_dir="$root/.claude/state" +lock_file="$state_dir/loop-advance.lock" +mkdir -p "$state_dir" + +lock_issue="" +if [ -f "$lock_file" ]; then + lock_issue="$(sed -n 's/^issue=\([0-9][0-9]*\).*/\1/p' "$lock_file" | head -1)" +fi + +if [ -n "$lock_issue" ]; then + still_qualifies=0 + [ "$lock_issue" = "$advance_ready" ] && still_qualifies=1 + printf '%s\n' "$in_flight_issues" | grep -qx "$lock_issue" && still_qualifies=1 + if [ "$still_qualifies" -eq 0 ]; then + echo "# lock self-heal: cleared stale spawn lock for issue=$lock_issue (no longer advance_ready/in_flight — open PR exists or branch is gone)" + rm -f "$lock_file" + lock_issue="" + fi +fi + +# --- Decide the verdict ----------------------------------------------------- +if [ -n "$feedback_pr" ]; then + echo "action=feedback pr=$feedback_pr" +elif [ "$advance_ready" != "none" ] && [ -n "$advance_ready" ]; then + if printf '%s\n' "$in_flight_issues" | grep -qx "$advance_ready"; then + echo "# advance refused: issue=$advance_ready is in_flight (a feat/issue-$advance_ready-* branch already exists with no open PR)" + echo "action=none" + elif [ "$lock_issue" = "$advance_ready" ]; then + echo "# advance refused: spawn lock already held for issue=$advance_ready ($(cat "$lock_file" 2>/dev/null))" + echo "action=none" + else + tmp="$(mktemp "$state_dir/.loop-advance.lock.XXXXXX")" + printf 'issue=%s ts=%s\n' "$advance_ready" "$(date -u +%FT%TZ)" > "$tmp" + mv -f "$tmp" "$lock_file" + echo "action=advance issue=$advance_ready" + fi +else + echo "action=none" +fi diff --git a/.claude/scripts/loop-tick.test.sh b/.claude/scripts/loop-tick.test.sh new file mode 100644 index 0000000..8b7b6ad --- /dev/null +++ b/.claude/scripts/loop-tick.test.sh @@ -0,0 +1,189 @@ +#!/usr/bin/env bash +# loop-tick.test.sh — offline smoke test for loop-tick.sh (issue #81). +# +# loop-tick.sh's own logic is just: run its four sibling step scripts, parse +# census/pr-feedback output, and emit one verdict line (plus the spawn lock). +# So this test doesn't touch real gh/network — it builds a throwaway +# .claude/scripts/ directory containing the REAL loop-tick.sh + resolve-roots.sh +# next to FAKE loop-census.sh / notify-poll.sh / merge-ready.sh / pr-feedback.sh +# that print canned, scripted output, then asserts the final verdict line and +# the spawn-lock file behavior for each scenario. +# +# Exit 0 on success, non-zero if any assertion fails. Runnable bare: +# bash .claude/scripts/loop-tick.test.sh +set -uo pipefail + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +loop_tick_src="$script_dir/loop-tick.sh" +resolve_roots_src="$script_dir/resolve-roots.sh" + +work="$(mktemp -d "${TMPDIR:-/tmp}/loop-tick-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 +} + +# Build one fresh fake "consumer project" per scenario: <fixture>/.claude/scripts/. +# fake_census / fake_feedback are the exact stdout the corresponding real +# script would print; notify-poll.sh and merge-ready.sh are stubbed to just +# print a marker line (their output is passed through, never parsed). +new_fixture() { + local name="$1" fake_census="$2" fake_feedback="$3" + local dir="$work/$name/.claude/scripts" + mkdir -p "$dir" "$work/$name/.claude/state" 2>/dev/null + rm -rf "$work/$name/.claude/state" # loop-tick.sh must mkdir -p it itself + cp "$loop_tick_src" "$dir/loop-tick.sh" + cp "$resolve_roots_src" "$dir/resolve-roots.sh" + + cat > "$dir/loop-census.sh" <<EOF +#!/usr/bin/env bash +cat <<'CENSUS' +$fake_census +CENSUS +EOF + cat > "$dir/notify-poll.sh" <<'EOF' +#!/usr/bin/env bash +echo "CURSOR=fake NOW=fake" +echo "=== fake notify-poll output ===" +EOF + cat > "$dir/merge-ready.sh" <<'EOF' +#!/usr/bin/env bash +echo "=== merge-ready: merged=0 skipped=0 ===" +EOF + cat > "$dir/pr-feedback.sh" <<EOF +#!/usr/bin/env bash +cat <<'FEEDBACK' +$fake_feedback +FEEDBACK +EOF + chmod +x "$dir"/*.sh + printf '%s\n' "$dir" +} + +run_tick() { + # $1 = fixture script_dir; repo passed explicitly so loop-tick.sh's own gh() + # (bot-gh.sh) is never invoked (bot-gh.sh doesn't even exist in the fixture). + bash "$1/loop-tick.sh" "acme/repo" +} + +last_line() { tail -1; } + +# --------------------------------------------------------------------------- +# 1. Nothing actionable -> action=none. +# --------------------------------------------------------------------------- +dir1="$(new_fixture scenario1 'open_prs=0 +feedback_prs=0 +planned_issues=0 +advance_ready=none +cadence=IDLE cron=*/15 * * * *' '')" +out1="$(run_tick "$dir1")" +check "scenario 1 (nothing actionable): verdict is action=none" bash -c '[ "$(printf "%s\n" "$1" | tail -1)" = "action=none" ]' _ "$out1" +check "scenario 1: no spawn lock left behind" [ ! -e "$dir1/../state/loop-advance.lock" ] + +# --------------------------------------------------------------------------- +# 2. Advance-ready issue, no feedback, nothing in flight, no lock held -> +# action=advance issue=N, and the lock file is written. +# --------------------------------------------------------------------------- +dir2="$(new_fixture scenario2 'open_prs=0 +feedback_prs=0 +planned_issues=1 +issue=42 branch=none title=Do the thing +advance_ready=42 +cadence=FAST cron=* * * * *' '')" +out2="$(run_tick "$dir2")" +check "scenario 2 (advance ready): verdict is action=advance issue=42" bash -c '[ "$(printf "%s\n" "$1" | tail -1)" = "action=advance issue=42" ]' _ "$out2" +lock2="$dir2/../state/loop-advance.lock" +check "scenario 2: spawn lock file was written" [ -f "$lock2" ] +check "scenario 2: lock file records issue=42" grep -q '^issue=42 ts=' "$lock2" + +# --------------------------------------------------------------------------- +# 3. Same fixture, SECOND tick while the lock from scenario 2's issue is still +# held -> downgraded to action=none (never re-emits action=advance for the +# same issue while a first spawn is still in flight). +# --------------------------------------------------------------------------- +out3="$(run_tick "$dir2")" +check "scenario 3 (lock already held): verdict downgrades to action=none" bash -c '[ "$(printf "%s\n" "$1" | tail -1)" = "action=none" ]' _ "$out3" +check "scenario 3: a diagnostic line explains the refusal" bash -c 'printf "%s\n" "$1" | grep -q "spawn lock already held for issue=42"' _ "$out3" +check "scenario 3: the lock file is untouched (still issue=42)" grep -q '^issue=42 ts=' "$lock2" + +# --------------------------------------------------------------------------- +# 4. Feedback PR present takes priority over an ALSO-ready advance -> pick the +# LOWEST-numbered feedback PR, never action=advance. +# --------------------------------------------------------------------------- +dir4="$(new_fixture scenario4 'open_prs=0 +feedback_prs=1 +planned_issues=1 +issue=7 branch=none title=Some issue +advance_ready=7 +cadence=FAST cron=* * * * *' "9 feat/issue-9-x owner 2026-01-01T00:00:00Z +5 feat/issue-5-y owner 2026-01-01T00:00:00Z")" +out4="$(run_tick "$dir4")" +check "scenario 4 (feedback beats advance): verdict is action=feedback pr=5 (lowest)" bash -c '[ "$(printf "%s\n" "$1" | tail -1)" = "action=feedback pr=5" ]' _ "$out4" +check "scenario 4: no spawn lock written (advance never attempted)" [ ! -e "$dir4/../state/loop-advance.lock" ] + +# --------------------------------------------------------------------------- +# 5. in_flight refusal: advance_ready=N but census ALSO reports N as in_flight +# (defensive check — real census never produces both for the same issue, +# but loop-tick.sh must still refuse rather than double-spawn). +# --------------------------------------------------------------------------- +dir5="$(new_fixture scenario5 'open_prs=0 +feedback_prs=0 +planned_issues=1 +issue=8 branch=feat/issue-8-x title=In flight thing +in_flight=8 +advance_ready=8 +cadence=FAST cron=* * * * *' '')" +out5="$(run_tick "$dir5")" +check "scenario 5 (in_flight): verdict downgrades to action=none" bash -c '[ "$(printf "%s\n" "$1" | tail -1)" = "action=none" ]' _ "$out5" +check "scenario 5: diagnostic cites in_flight" bash -c 'printf "%s\n" "$1" | grep -q "in_flight"' _ "$out5" +check "scenario 5: no spawn lock written" [ ! -e "$dir5/../state/loop-advance.lock" ] + +# --------------------------------------------------------------------------- +# 6. Self-heal: a stale lock for issue 3 (no longer advance_ready/in_flight in +# the fresh census — e.g. its PR landed) must be cleared automatically, and +# a DIFFERENT now-ready issue can still be picked up in the SAME tick. +# --------------------------------------------------------------------------- +dir6="$(new_fixture scenario6 'open_prs=0 +feedback_prs=0 +planned_issues=1 +issue=9 branch=none title=Fresh issue +advance_ready=9 +cadence=FAST cron=* * * * *' '')" +lock6="$dir6/../state/loop-advance.lock" +mkdir -p "$(dirname "$lock6")" +printf 'issue=3 ts=2020-01-01T00:00:00Z\n' > "$lock6" +out6="$(run_tick "$dir6")" +check "scenario 6 (self-heal): verdict advances the NEW issue 9" bash -c '[ "$(printf "%s\n" "$1" | tail -1)" = "action=advance issue=9" ]' _ "$out6" +check "scenario 6: self-heal diagnostic mentions the cleared stale issue=3" bash -c 'printf "%s\n" "$1" | grep -q "cleared stale spawn lock for issue=3"' _ "$out6" +check "scenario 6: lock file now records the NEW issue=9, not the stale 3" grep -q '^issue=9 ts=' "$lock6" + +# --------------------------------------------------------------------------- +# 7. All four step scripts' full output is preserved (never swallowed). +# --------------------------------------------------------------------------- +check "all four labeled step headers appear in the tick's output" bash -c ' + printf "%s\n" "$1" | grep -q "1/4 loop-census.sh" && + printf "%s\n" "$1" | grep -q "2/4 notify-poll.sh" && + printf "%s\n" "$1" | grep -q "3/4 merge-ready.sh" && + printf "%s\n" "$1" | grep -q "4/4 pr-feedback.sh" +' _ "$out1" +check "notify-poll.sh full output line passed through, not swallowed" bash -c 'printf "%s\n" "$1" | grep -qF "fake notify-poll output"' _ "$out1" +check "merge-ready.sh full output line passed through, not swallowed" bash -c 'printf "%s\n" "$1" | grep -qF "merge-ready: merged=0 skipped=0"' _ "$out1" + +echo "" +if [ "$fail" -eq 0 ]; then + echo "loop-tick.test.sh: PASS ($ok checks)" + exit 0 +else + echo "loop-tick.test.sh: FAIL (see FAIL lines above)" + exit 1 +fi diff --git a/docs/GETTING_STARTED.md b/docs/GETTING_STARTED.md index 411ec47..118f501 100644 --- a/docs/GETTING_STARTED.md +++ b/docs/GETTING_STARTED.md @@ -184,6 +184,14 @@ model (the `module:*` opt-in queue + the owner-approval merge gate) that this ch set branch protection / required status checks on `merge.baseBranch` if your plan supports it. 7. **Arm the loop** — run **`/orchestrator:pr-loop`**. It self-adjusts cadence (FAST when there's ≥1 open PR or ≥1 open `module:*` issue, else IDLE) but the cron is session-scoped, so re-run it at the start of each session. + **Pick the right model for each side of the loop:** run the tick session on **Sonnet** — the ticks are + repetitive, and that repetition is exactly where smaller models drift (a Haiku-driven tick session has been + observed to stop running the step scripts and fabricate their output, and to double-spawn orchestrators for + one issue). `.claude/scripts/loop-tick.sh` hardens the tick itself — one script computes the census/feedback/ + advance verdict and a self-healing spawn lock, instead of a model re-deriving it from a prompt every firing + (see [`USAGE.md`](USAGE.md) → "Model selection") — but the driving session still needs Sonnet to read that + verdict and act on it. Use **Fable or Opus** for the owner-side judgment + work — scoping, planning, and filing issues — then let the loop execute the approved queue. 8. *(optional)* **Hardening** — `/orchestrator:harden` for the bypass + strict-sandbox profile, see [`HARDENING.md`](HARDENING.md). diff --git a/docs/USAGE.md b/docs/USAGE.md index 195e026..f10e7e4 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -120,6 +120,13 @@ With `pr-per-agent`, the standing loop per ticket looks like: haven't reviewed — pushing after approval requires re-approval. Uses ambient `gh` auth (merging is an owner action; only PR *creation* uses the bot). +**Or run it as one script:** `.claude/scripts/loop-tick.sh` runs the census (`loop-census.sh`) plus all three +scripts above, IN ORDER, with their full output preserved, and prints exactly one machine-readable verdict line +at the end — `action=none`, `action=advance issue=N`, or `action=feedback pr=N` — collapsing the whole tick +into a single pre-approvable command. It also owns a self-healing spawn lock +(`.claude/state/loop-advance.lock`) so a second tick fired before the first ADVANCE has even reached PR stage +never double-spawns an orchestrator for the same issue; see the script's header comment for the full contract. + With all three wired, the loop runs hands-off: **add issues → review → approve → it merges and advances**. A natural step 4 is to start the next `module:*` issue only when **no PRs are open**, so work stays serialized (one issue in flight) and bounded. Caveats: cron jobs fire only while Claude Code is running, @@ -162,16 +169,26 @@ decide what the loop actually touches: labelled it `planned`. Commenting "approved" on an issue does nothing — nothing watches issue text; the `planned` label is the only approval signal. +**Model selection.** Drive the loop's tick sessions with **Sonnet**. Ticks are cheap but highly +repetitive, and repetition is where smaller models degrade: a Haiku-driven tick session has been observed +to stop invoking the step scripts entirely — fabricating census/merge output from the pattern of earlier +quiet ticks (missing an owner approval and a `planned` issue for hours) — and to misread an in-flight +orchestration as hung, double-spawning orchestrators for the same issue. Reserve **Fable or Opus** for +the owner-side judgment work: scoping, planning, and filing issues. (Script-side hardening that reduces +the tick's model-dependence landed in issue #81 as `.claude/scripts/loop-tick.sh`: it computes the +census/feedback/advance verdict and the spawn lock in shell, rather than leaving that arithmetic to be +re-derived from a prompt every firing — Sonnet remains the recommended driver for the session that invokes +it, since the driving session still has to read the verdict and act on it, e.g. spawning the orchestrator.) + > Historical note: before the `planned` label existed, the `module:*` label alone was the opt-in queue. > If a repo predates the split, treat `module:*`-only issues as `backlog` until the owner adds `planned`. -**Self-hosting this repo's own backlog?** **`.claude/self/pr-loop-self.md`** runs the -same loop mechanics self-hosted, against this repo's own `.claude`/`docs`/`examples`/`.github` backlog, using +**Self-hosting this repo's own backlog?** **`.claude/self/pr-loop-self.md`** runs the same loop mechanics +self-hosted, against this repo's own `.claude`/`docs`/`examples`/`.github` backlog, using **`.claude/self/gates.json`** as the adapter (module map, gates, review lenses) instead of the placeholder -`.claude/gates.json` above. This file lives under `.claude/self/`, not `.claude/commands/`, so it is **not** -shipped to downstream plugin consumers and is not a registered slash command — ask Claude to read and follow -it directly (e.g. "read and run `.claude/self/pr-loop-self.md`"). See `.claude/self/README.md` for the full -self-adapter contract. +`.claude/gates.json` above. It lives under `.claude/self/` (not `.claude/commands/`), so it is self-hosting-only +— not a registered slash command and never packaged to downstream installs of the plugin; ask Claude to read and +follow it directly. See `.claude/self/README.md` for the self-adapter contract. New project? Wire this up with the **[new-project configuration checklist](GETTING_STARTED.md#new-project-configuration-checklist)**. From e43e75f7b9b131feb782eadebb5e84e18abd1b02 Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Wed, 8 Jul 2026 16:11:35 +0200 Subject: [PATCH 2/2] fix(loop): close spawn-lock deadlock, TOCTOU race, and census SIGPIPE (issue #81 re-review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of the loop-tick hardening (10abb0d) found the spawn lock's own INVARIANT backwards: it was retained precisely when advance_ready==lock_issue, i.e. the no-branch-yet state census reports for a crashed spawn — so a crashed orchestrator wedged the issue forever with only manual `rm` as recovery. - loop-tick.sh: the lock is now released when (a) an open PR exists, (b) a branch exists (in_flight — the pre-branch race is over), or (c) the lock is older than LOCK_TTL_SECONDS=900 with the issue STILL advance_ready and no branch — treated as a crashed spawn and cleared to allow re-advance. The 15-minute TTL comfortably exceeds a real orchestrator's branch-push latency. - loop-tick.sh: the whole read-check-write critical section around the lock is now serialized with `flock` on a dedicated .claude/state/loop-advance.flock, closing a TOCTOU race where two overlapping ticks could both observe "no lock" and both emit `action=advance issue=N` (atomic temp+mv only prevented a torn read, not two racing readers). - loop-census.sh: `git branch -a --list ... | head -1` under `set -euo pipefail` could abort the whole script on git's SIGPIPE (exit 141) when head closes the pipe early; `|| true` absorbs the non-fatal pipeline failure. Reproduced and verified against a repo with 5000 matching branches. - docs/USAGE.md: reconciled the spawn-lock description with the corrected invariant. Tests: loop-tick.test.sh gained a TTL self-heal scenario (crashed-spawn recovery) and a real concurrent-process scenario proving `flock` serializes two overlapping ticks; scenario 7's step-passthrough assertions now check a BODY line from each fake fixture (not just the header banner), so silently swallowing loop-census.sh's or pr-feedback.sh's output fails loudly. Added loop-census.test.sh: runs the REAL loop-census.sh against a real git repo with local/remote-tracking branches to directly cover in_flight detection, including the "issue-4 vs issue-42" prefix-collision guard and the "remotes/origin/" stripping/suffix-match on both branch-exists paths. Every new/changed assertion was mutation-checked (reverted to the buggy behavior) to confirm it fails loudly; all failure modes reproduced deterministically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0135JVnX98DjPxTRzQZRy82q --- .claude/scripts/loop-census.sh | 6 +- .claude/scripts/loop-census.test.sh | 174 ++++++++++++++++++++++++++++ .claude/scripts/loop-tick.sh | 86 ++++++++++++-- .claude/scripts/loop-tick.test.sh | 61 ++++++++++ docs/USAGE.md | 7 +- 5 files changed, 321 insertions(+), 13 deletions(-) create mode 100644 .claude/scripts/loop-census.test.sh diff --git a/.claude/scripts/loop-census.sh b/.claude/scripts/loop-census.sh index 6e0d510..8b44c4f 100644 --- a/.claude/scripts/loop-census.sh +++ b/.claude/scripts/loop-census.sh @@ -74,7 +74,11 @@ while IFS=$'\t' read -r num labels title; do [ "$hit" -eq 1 ] || continue planned_count=$((planned_count + 1)) # Existing feat/issue-<n>-* branch (local or remote) means it's already in flight. - branch=$(git -C "$root" branch -a --list "*feat/issue-$num-*" | head -1 | sed 's/^[* ]*//;s|^remotes/||') + # NOTE: `| head -1` can make `git` see SIGPIPE (exit 141) if head closes the + # pipe before git finishes writing; under `set -euo pipefail` that would abort + # this whole script. `|| true` on the assignment absorbs that non-fatal + # pipeline failure — the captured output (head's one line) is unaffected. + branch=$(git -C "$root" branch -a --list "*feat/issue-$num-*" | head -1 | sed 's/^[* ]*//;s|^remotes/||') || true [ -n "$branch" ] || branch="none" detail+="issue=$num branch=$branch title=$title"$'\n' if [ "$advance_ready" = "none" ] && [ "$branch" = "none" ] && [ "$open_prs" -eq 0 ]; then diff --git a/.claude/scripts/loop-census.test.sh b/.claude/scripts/loop-census.test.sh new file mode 100644 index 0000000..ca8e611 --- /dev/null +++ b/.claude/scripts/loop-census.test.sh @@ -0,0 +1,174 @@ +#!/usr/bin/env bash +# loop-census.test.sh — offline smoke test for loop-census.sh's in_flight +# detection (issue #81 re-review, finding 5). +# +# loop-tick.test.sh exercises loop-tick.sh against a FAKE loop-census.sh that +# just echoes canned `in_flight=N` lines — it never runs loop-census.sh's own +# branch-detection algorithm. This test closes that gap: it runs the REAL +# loop-census.sh (+ real resolve-roots.sh) against a REAL git repo with real +# local and remote-tracking branches, stubbing only `gh` (via a fake +# bot-gh.sh) and pr-feedback.sh (no network, no gh CLI required), and asserts +# on the actual `in_flight=`/`branch=` lines the real algorithm prints. +# +# Specifically covers the two failure modes called out in re-review: +# +# - PREFIX COLLISION: issue 4 has NO branch of its own, while issue 42 and +# issue 43 DO (as "issue-4" is a literal prefix of "issue-42"/"issue-43"). +# A glob without the trailing "-" (`*feat/issue-4*` instead of +# `*feat/issue-4-*`) would make `git branch -a --list` for issue 4 also +# match issue 42's/43's branches; since issue 4 has no LOCAL branch of +# its own to sort first, `head -1` would then wrongly attribute one of +# THEIR branches to issue 4. Asserted directly: issue 4 must come back +# branch=none despite 42/43 existing. +# +# - "remotes/origin/" HANDLING: issue 42's and 43's branches exist ONLY as +# remote-tracking refs (pushed, then the local branch deleted), so +# `git branch -a` reports them as "remotes/origin/feat/issue-4N-*". +# Issue 43 additionally already has an open PR under its BARE branch +# name (`feat/issue-43-z`, no "origin/" prefix, matching a real +# `headRefName`) — that must still register as "already has a PR" (not +# in_flight) via the "*/<bare>" suffix rule, not just an exact-string +# match; issue 100's LOCAL (non-remote) branch with an open PR is the +# control for the exact-match path. +# +# Exit 0 on success, non-zero if any assertion fails. Runnable bare: +# bash .claude/scripts/loop-census.test.sh +set -uo pipefail + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +census_src="$script_dir/loop-census.sh" +resolve_roots_src="$script_dir/resolve-roots.sh" + +work="$(mktemp -d "${TMPDIR:-/tmp}/loop-census-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 +} + +# --------------------------------------------------------------------------- +# Build one fixture: a real git repo (fixture root = census's $root) with: +# - issue 4: NO branch at all -> branch=none. Must not be fooled by +# issue 42's/43's branches, whose names have "issue-4" as a +# literal prefix. +# - issue 42: a REMOTE-tracking-only branch feat/issue-42-y (pushed, local +# copy deleted), no open PR for it -> MUST be in_flight. +# - issue 43: a REMOTE-tracking-only branch feat/issue-43-z, which +# ALREADY has an open PR under its bare name -> must NOT be +# in_flight (the "remotes/origin/" strip + "*/<bare>" suffix +# match on the POSITIVE path). +# - issue 100: a LOCAL branch feat/issue-100-w, which ALREADY has an open +# PR under its bare (exact, no prefix) name -> must NOT be +# in_flight (the plain exact-match control case). +# --------------------------------------------------------------------------- +fixture="$work/fixture1" +scripts_dir="$fixture/.claude/scripts" +mkdir -p "$scripts_dir" +cp "$census_src" "$scripts_dir/loop-census.sh" +cp "$resolve_roots_src" "$scripts_dir/resolve-roots.sh" + +# Minimal adapter: one module, so "module:test" is the only label census cares +# about; base branch is "main" to match the repo below. +cat > "$fixture/.claude/gates.json" <<'EOF' +{ + "modules": [{ "name": "test", "path": ".", "description": "", "owner": "" }], + "merge": { "baseBranch": "main" } +} +EOF + +# pr-feedback.sh is exercised by its own test (loop-tick.test.sh); here it's +# just a no-op stub so census's feedback_prs line is deterministic (0). +cat > "$scripts_dir/pr-feedback.sh" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF + +# Fake bot-gh.sh: no network, no real `gh` — dispatches on the subcommand and +# a `--json` marker to canned, fixture-appropriate output. +# - `pr list ... --json headRefName ...`: bare branch names of open PRs — +# issues 43 and 100 already have one; issue 42 does not (issue 4 has no +# branch, so it can't have a PR either). +# - `pr list ... --json number ...`: open PR count (2, matching above). +# - `issue list ...`: TSV `num<TAB>labels<TAB>title` +# for the four planned+module:test issues. +cat > "$scripts_dir/bot-gh.sh" <<'EOF' +#!/usr/bin/env bash +case "$1" in + repo) echo "acme/repo" ;; + pr) + if printf '%s\n' "$*" | grep -q 'headRefName'; then + printf '%s\n' "feat/issue-43-z" + printf '%s\n' "feat/issue-100-w" + else + echo 2 + fi + ;; + issue) + printf '4\tplanned,module:test\tIssue four\n' + printf '42\tplanned,module:test\tIssue forty two\n' + printf '43\tplanned,module:test\tIssue forty three\n' + printf '100\tplanned,module:test\tIssue one hundred\n' + ;; + *) echo "fake-bot-gh.sh: unhandled args: $*" >&2; exit 1 ;; +esac +EOF +chmod +x "$scripts_dir"/*.sh + +# Real git repo at the fixture root (census does `git -C "$root" branch -a`). +git -C "$fixture" init -q -b main +git -C "$fixture" -c user.email=t@e.st -c user.name=t commit -q --allow-empty -m init + +# Bare "remote" so `git branch -a` prints genuine "remotes/origin/..." lines. +remote="$work/remote.git" +git init -q --bare "$remote" +git -C "$fixture" remote add origin "$remote" + +# issue 4: deliberately NO branch at all (see the prefix-collision note above). + +# issue 42 and 43: pushed to origin, then the LOCAL copy is deleted so only +# the "remotes/origin/..." remote-tracking ref remains — this is the case +# census's "strip remotes/ or match as /-suffix" logic exists for. +git -C "$fixture" branch feat/issue-42-y main >/dev/null +git -C "$fixture" push -q origin feat/issue-42-y >/dev/null 2>&1 +git -C "$fixture" branch -D feat/issue-42-y >/dev/null + +git -C "$fixture" branch feat/issue-43-z main >/dev/null +git -C "$fixture" push -q origin feat/issue-43-z >/dev/null 2>&1 +git -C "$fixture" branch -D feat/issue-43-z >/dev/null + +# issue 100: LOCAL-only branch (never pushed) — exact-match control. +git -C "$fixture" branch feat/issue-100-w main >/dev/null + +# Unset GATES_FILE explicitly: loop-census.sh reads it straight from the +# environment, and this test may itself be run from inside a gate invocation +# that exports GATES_FILE=.claude/self/gates.json for the OUTER repo — which +# would leak in here and make census look for a gates.json this fixture never +# created. Force it back to the fixture's own default-relative gates.json. +out="$(env -u GATES_FILE bash "$scripts_dir/loop-census.sh" "acme/repo")" + +check "issue 4 (no branch at all) reports branch=none" bash -c 'printf "%s\n" "$1" | grep -q "^issue=4 branch=none"' _ "$out" +check "issue 4 is NOT in_flight (no branch to be in flight with)" bash -c '! printf "%s\n" "$1" | grep -qx "in_flight=4"' _ "$out" +check "issue 42 (remote-only branch, no open PR) IS in_flight" bash -c 'printf "%s\n" "$1" | grep -qx "in_flight=42"' _ "$out" +check "issue 43 (remote-only branch, already has an open PR via origin/ strip+suffix match) is NOT in_flight" bash -c '! printf "%s\n" "$1" | grep -qx "in_flight=43"' _ "$out" +check "issue 100 (local branch, already has an open PR, exact-match control) is NOT in_flight" bash -c '! printf "%s\n" "$1" | grep -qx "in_flight=100"' _ "$out" +check "exactly one in_flight line total (only issue 42 qualifies)" bash -c '[ "$(printf "%s\n" "$1" | grep -c "^in_flight=")" -eq 1 ]' _ "$out" +check "planned_issues=4 counted" bash -c 'printf "%s\n" "$1" | grep -qx "planned_issues=4"' _ "$out" +check "issue=42 branch line shows the origin-prefixed remote-tracking name" bash -c 'printf "%s\n" "$1" | grep -q "^issue=42 branch=origin/feat/issue-42-y"' _ "$out" + +echo "" +if [ "$fail" -eq 0 ]; then + echo "loop-census.test.sh: PASS ($ok checks)" + exit 0 +else + echo "loop-census.test.sh: FAIL (see FAIL lines above)" + exit 1 +fi diff --git a/.claude/scripts/loop-tick.sh b/.claude/scripts/loop-tick.sh index 3cd5bd3..c44de55 100644 --- a/.claude/scripts/loop-tick.sh +++ b/.claude/scripts/loop-tick.sh @@ -35,14 +35,36 @@ # `action=advance issue=N`, so a SECOND tick — fired before the first # implementer has even pushed a branch — is refused by this script's own # logic rather than by model discipline. Format: one line, -# `issue=N ts=<UTC ISO-8601>`. INVARIANT: the lock for issue N is considered -# released once EITHER (a) an open PR now exists for N, or (b) no -# feat/issue-N-* branch exists at all — i.e. census no longer reports N as -# advance_ready or in_flight. This script self-heals: on every run it checks -# the held lock (if any) against the FRESH census output and clears it if it -# no longer qualifies, so a stale lock (e.g. left behind by a crashed -# orchestrator) never permanently blocks the issue. Written atomically -# (temp file + mv) to avoid a torn read from a concurrent tick. +# `issue=N ts=<UTC ISO-8601>`. +# +# INVARIANT (corrected — see issue #81 re-review): a lock for issue N is +# held to cover exactly the narrow window between "this tick just emitted +# action=advance issue=N" and "an orchestrator has pushed feat/issue-N-*". +# While that window is open, census reports N as advance_ready (no branch +# yet) — the SAME signal that means "N still needs advancing" — so the two +# cannot be told apart by advance_ready alone. The lock is released as soon +# as EITHER: +# (a) an open PR now exists for N (census no longer reports N as +# advance_ready — feedback/merge scripts own N from here), OR +# (b) a feat/issue-N-* branch now exists with no open PR yet (census +# reports N as in_flight) — the orchestrator got at least as far as +# pushing a branch, so the pre-branch race this lock guards against is +# over; a second tick would refuse to re-advance N anyway once it's +# in_flight, OR +# (c) the lock is older than LOCK_TTL_SECONDS and N is STILL +# advance_ready with no branch — this can only mean the spawn that +# should have created the branch crashed (or never started) before +# reaching (b), so a lock stuck in this state is treated as a crashed +# spawn and cleared to let a later tick re-advance N. +# This script self-heals: on every run it checks the held lock (if any) +# against the FRESH census output plus the TTL above and clears it whenever +# it no longer qualifies, so a crashed orchestrator never permanently wedges +# the issue. Written atomically (temp file + mv) to avoid a torn read, and +# the whole read-check-write critical section is additionally serialized +# with `flock` (a separate .claude/state/loop-advance.flock) so two ticks +# racing each other cannot both observe "no lock" and both emit +# `action=advance issue=N` (a TOCTOU double-spawn — atomic temp+mv alone only +# prevents a torn READ, not two processes interleaving read-then-write). # # Repo derived from the git remote; override with $1. Bot login via # $BOT_LOGIN (passed through to the step scripts). Honors $GATES_FILE exactly @@ -91,10 +113,32 @@ in_flight_issues="$(printf '%s\n' "$census_out" | sed -n 's/^in_flight=//p')" feedback_pr="$(printf '%s\n' "$feedback_out" | awk -F'\t' 'NF>=1 && $1 ~ /^[0-9]+$/ {print $1}' | sort -n | head -1)" # --- Spawn lock: read + self-heal against the FRESH census above ----------- +# TTL rationale: this lock is written the instant a tick emits +# `action=advance issue=N`, before the orchestrator that will push +# `feat/issue-N-*` even exists yet. A real orchestrator reaches that push +# within at most a few minutes of being spawned. 15 minutes is comfortably +# above that, so a lock that is STILL "no branch, still advance_ready" past +# this TTL can only mean the spawn crashed (or was never launched) before +# creating a branch — self-heal by clearing it rather than wedging the issue +# forever (see INVARIANT (c) in the header comment above). +LOCK_TTL_SECONDS=900 + state_dir="$root/.claude/state" lock_file="$state_dir/loop-advance.lock" +flock_file="$state_dir/loop-advance.flock" mkdir -p "$state_dir" +# Concurrent-tick guard (issue #81 re-review, TOCTOU): two overlapping ticks +# must not both observe "no lock held for N" and both emit +# `action=advance issue=N` — exactly the double-spawn bug #81 exists to kill. +# Atomic temp+mv (below) only prevents a torn READ of the lock file; it does +# not make "read lock -> self-heal -> decide -> write lock" atomic ACROSS two +# processes. Serialize that whole critical section with a real file lock so +# only one tick at a time can be inside it (released automatically when this +# script exits and fd 9 closes). +exec 9>"$flock_file" +flock -x 9 + lock_issue="" if [ -f "$lock_file" ]; then lock_issue="$(sed -n 's/^issue=\([0-9][0-9]*\).*/\1/p' "$lock_file" | head -1)" @@ -102,10 +146,30 @@ fi if [ -n "$lock_issue" ]; then still_qualifies=0 - [ "$lock_issue" = "$advance_ready" ] && still_qualifies=1 - printf '%s\n' "$in_flight_issues" | grep -qx "$lock_issue" && still_qualifies=1 + reason="" + if printf '%s\n' "$in_flight_issues" | grep -qx "$lock_issue"; then + # (b): a branch now exists — the pre-branch window this lock guards is + # closed (a second tick would refuse to advance N anyway once in_flight). + reason="branch now exists (in_flight) — lock's purpose is served" + elif [ "$lock_issue" = "$advance_ready" ]; then + # Still no branch. Either the spawn just started (keep the lock) or it + # crashed before ever pushing a branch (clear it) — (c): use the + # recorded ts as a bounded TTL to tell the two apart. + lock_ts="$(sed -n 's/^issue=[0-9][0-9]* ts=\(.*\)$/\1/p' "$lock_file" | head -1)" + lock_epoch="$(date -u -d "$lock_ts" +%s 2>/dev/null || echo 0)" + now_epoch="$(date -u +%s)" + age=$(( now_epoch - lock_epoch )) + if [ "$lock_epoch" -eq 0 ] || [ "$age" -gt "$LOCK_TTL_SECONDS" ]; then + reason="lock is older than ${LOCK_TTL_SECONDS}s with still no branch — treating as a crashed spawn" + else + still_qualifies=1 + fi + else + # (a): no longer advance_ready and no branch -> an open PR must exist now. + reason="no longer advance_ready/in_flight — open PR exists or branch is gone" + fi if [ "$still_qualifies" -eq 0 ]; then - echo "# lock self-heal: cleared stale spawn lock for issue=$lock_issue (no longer advance_ready/in_flight — open PR exists or branch is gone)" + echo "# lock self-heal: cleared stale spawn lock for issue=$lock_issue ($reason)" rm -f "$lock_file" lock_issue="" fi diff --git a/.claude/scripts/loop-tick.test.sh b/.claude/scripts/loop-tick.test.sh index 8b7b6ad..3dde40e 100644 --- a/.claude/scripts/loop-tick.test.sh +++ b/.claude/scripts/loop-tick.test.sh @@ -178,6 +178,67 @@ check "all four labeled step headers appear in the tick's output" bash -c ' ' _ "$out1" check "notify-poll.sh full output line passed through, not swallowed" bash -c 'printf "%s\n" "$1" | grep -qF "fake notify-poll output"' _ "$out1" check "merge-ready.sh full output line passed through, not swallowed" bash -c 'printf "%s\n" "$1" | grep -qF "merge-ready: merged=0 skipped=0"' _ "$out1" +# census_out and feedback_out are captured into shell variables and re-printed +# via `printf '%s\n' "$census_out"` / `"$feedback_out"` (loop-tick.sh) — assert +# a BODY line from each fake fixture (not just the "N/4 ..." header banner +# above it) survives verbatim, so silently deleting either printf (which +# would swallow exactly the output a human needs to debug a wrong verdict) +# fails this test loudly. Mutation-checked: removing either printf line from +# loop-tick.sh makes the corresponding check below fail while all the header +# checks above stay green. +check "loop-census.sh full BODY line passed through, not swallowed" bash -c 'printf "%s\n" "$1" | grep -qF "cadence=IDLE cron=*/15 * * * *"' _ "$out1" +check "pr-feedback.sh full BODY line passed through, not swallowed" bash -c 'printf "%s\n" "$1" | grep -qF "9 feat/issue-9-x owner 2026-01-01T00:00:00Z"' _ "$out4" + +# --------------------------------------------------------------------------- +# 8. TTL self-heal: a lock for the SAME issue that's older than LOCK_TTL_SECONDS +# and STILL advance_ready (no branch ever showed up) must be treated as a +# crashed spawn — cleared and re-advanced — not kept forever the way a +# fresh same-issue lock correctly is (scenario 3). +# --------------------------------------------------------------------------- +dir8="$(new_fixture scenario8 'open_prs=0 +feedback_prs=0 +planned_issues=1 +issue=9 branch=none title=Fresh issue +advance_ready=9 +cadence=FAST cron=* * * * *' '')" +lock8="$dir8/../state/loop-advance.lock" +mkdir -p "$(dirname "$lock8")" +printf 'issue=9 ts=2020-01-01T00:00:00Z\n' > "$lock8" +out8="$(run_tick "$dir8")" +check "scenario 8 (TTL self-heal): stale same-issue lock past TTL is cleared and re-advanced" bash -c '[ "$(printf "%s\n" "$1" | tail -1)" = "action=advance issue=9" ]' _ "$out8" +check "scenario 8: diagnostic cites a crashed spawn (TTL expiry), not just self-heal" bash -c 'printf "%s\n" "$1" | grep -q "crashed spawn"' _ "$out8" +check "scenario 8: lock file now has a FRESH ts, not the stale 2020 one" bash -c '! grep -q "2020-01-01" "$1"' _ "$lock8" + +# --------------------------------------------------------------------------- +# 9. Concurrent-tick TOCTOU (issue #81 re-review): two ticks fired back to +# back, before either has written the lock, must not BOTH pass the +# check-then-write and both emit action=advance for the same issue — the +# exact double-spawn bug #81 exists to kill. Fire them as real overlapping +# background processes against the SAME fixture/state dir; `flock` must +# serialize the read-check-write so exactly one advances and the other +# backs off having observed the first tick's lock. +# --------------------------------------------------------------------------- +dir9="$(new_fixture scenario9 'open_prs=0 +feedback_prs=0 +planned_issues=1 +issue=42 branch=none title=Concurrent thing +advance_ready=42 +cadence=FAST cron=* * * * *' '')" +outA_file="$work/scenario9.a.out" +outB_file="$work/scenario9.b.out" +run_tick "$dir9" > "$outA_file" & +pidA=$! +run_tick "$dir9" > "$outB_file" & +pidB=$! +wait "$pidA" +wait "$pidB" +verdictA="$(tail -1 "$outA_file")" +verdictB="$(tail -1 "$outB_file")" +advances=0 +[ "$verdictA" = "action=advance issue=42" ] && advances=$((advances + 1)) +[ "$verdictB" = "action=advance issue=42" ] && advances=$((advances + 1)) +check "scenario 9 (concurrent ticks): exactly ONE of two overlapping ticks advances issue=42" bash -c '[ "$1" -eq 1 ]' _ "$advances" +check "scenario 9: the other tick backs off with action=none instead of double-advancing" bash -c '[ "$1" = "action=none" ] || [ "$2" = "action=none" ]' _ "$verdictA" "$verdictB" echo "" if [ "$fail" -eq 0 ]; then diff --git a/docs/USAGE.md b/docs/USAGE.md index f10e7e4..32b8d00 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -125,7 +125,12 @@ scripts above, IN ORDER, with their full output preserved, and prints exactly on at the end — `action=none`, `action=advance issue=N`, or `action=feedback pr=N` — collapsing the whole tick into a single pre-approvable command. It also owns a self-healing spawn lock (`.claude/state/loop-advance.lock`) so a second tick fired before the first ADVANCE has even reached PR stage -never double-spawns an orchestrator for the same issue; see the script's header comment for the full contract. +never double-spawns an orchestrator for the same issue: the lock is released once the issue's branch exists +(work has reached PR-race stage) or an open PR exists, OR — if neither ever happens because the spawn +crashed before pushing a branch — once the lock is older than its 15-minute TTL, so a crashed spawn cannot +wedge the issue forever. The read-check-write around the lock is additionally serialized with `flock` so two +overlapping ticks can't both pass the check and double-spawn; see the script's header comment for the full +contract. With all three wired, the loop runs hands-off: **add issues → review → approve → it merges and advances**. A natural step 4 is to start the next `module:*` issue only when **no PRs are open**, so work stays