diff --git a/apps/arbiter/lib/arbiter/worker/review_gate.ex b/apps/arbiter/lib/arbiter/worker/review_gate.ex index d97a7eea6..a0ca7511e 100644 --- a/apps/arbiter/lib/arbiter/worker/review_gate.ex +++ b/apps/arbiter/lib/arbiter/worker/review_gate.ex @@ -1225,9 +1225,16 @@ defmodule Arbiter.Worker.ReviewGate do # A stale exit from an worker we've moved on from. def handle_info({:worker_exited, _other, _status}, state), do: {:noreply, state} - # Timeouts are tagged with the attempt that scheduled them so a stale timer - # from a prior pass can't escalate a pass that has already advanced. - def handle_info({:timeout, _attempt}, %{reported?: true} = state), do: {:noreply, state} + # Timeouts are tagged with the {round, attempt} pair that scheduled them so + # a stale timer from a prior pass can't escalate a pass that has already + # advanced. `attempt` alone is not enough (bd-28u8v4): it resets to 0 at the + # start of every round (bd-bgeo6i, so reprompt budgets start fresh), so + # round N's implementer and round N+1's implementer are both launched as the + # same attempt number and a timer armed for the former would otherwise be + # accepted as belonging to the latter. `round` never repeats within a gate's + # lifetime, so the pair is unique for as long as the gate runs. + def handle_info({:timeout, _round, _attempt}, %{reported?: true} = state), + do: {:noreply, state} # A reviewing pass hit the ceiling. Before escalating as timed-out, retry the # pass once with a fresh reviewer mind (bd-78vg4v): a hung / overloaded session @@ -1235,8 +1242,9 @@ defmodule Arbiter.Worker.ReviewGate do # attempt converges where the first stalled. Only the reviewing phase is # retried — a revising (implementer) pass still escalates on timeout below. def handle_info( - {:timeout, attempt}, - %{attempt: attempt, phase: :reviewing, timeout_retries_left: budget} = state + {:timeout, round, attempt}, + %{round: round, attempt: attempt, phase: :reviewing, timeout_retries_left: budget} = + state ) when budget > 0 and is_binary(state.current_prompt) do Logger.warning( @@ -1266,7 +1274,7 @@ defmodule Arbiter.Worker.ReviewGate do end end - def handle_info({:timeout, attempt}, %{attempt: attempt} = state) do + def handle_info({:timeout, round, attempt}, %{round: round, attempt: attempt} = state) do Logger.warning( "ReviewGate: #{state.phase} pass timed out for task=#{state.task_id} (round #{state.round})" ) @@ -1274,7 +1282,7 @@ defmodule Arbiter.Worker.ReviewGate do escalate_timeout(state) end - def handle_info({:timeout, _stale}, state), do: {:noreply, state} + def handle_info({:timeout, _stale_round, _stale_attempt}, state), do: {:noreply, state} # Author died before we could report — nothing to do. def handle_info({:DOWN, _ref, :process, pid, _reason}, %{author: pid} = state) do @@ -3878,7 +3886,7 @@ defmodule Arbiter.Worker.ReviewGate do case spawn_worker(state, id, role, prompt, command) do {:ok, pid} -> - Process.send_after(self(), {:timeout, attempt}, timeout_ms) + Process.send_after(self(), {:timeout, state.round, attempt}, timeout_ms) {:ok, %{ diff --git a/apps/arbiter/test/arbiter/worker/review_gate_test.exs b/apps/arbiter/test/arbiter/worker/review_gate_test.exs index 0f9637d27..2ee8b9bc4 100644 --- a/apps/arbiter/test/arbiter/worker/review_gate_test.exs +++ b/apps/arbiter/test/arbiter/worker/review_gate_test.exs @@ -57,6 +57,12 @@ defmodule Arbiter.Worker.ReviewGateTest do __DIR__ ) @timeout_retry Path.expand("../../fixtures/review_timeout_retry.sh", __DIR__) + @reject_twice Path.expand("../../fixtures/review_reject_twice.sh", __DIR__) + @revise_slow_then_fast Path.expand("../../fixtures/revise_slow_then_fast.sh", __DIR__) + @reject_slow_then_approve_fast Path.expand( + "../../fixtures/review_reject_slow_then_approve_fast.sh", + __DIR__ + ) @hang Path.expand("../../fixtures/review_hang.sh", __DIR__) @auth_expired Path.expand("../../fixtures/review_auth_expired.sh", __DIR__) @quota_exhausted Path.expand("../../fixtures/review_quota_exhausted.sh", __DIR__) @@ -3576,6 +3582,158 @@ defmodule Arbiter.Worker.ReviewGateTest do assert [%{round: 1, verdict: nil}] = impl_rounds end + # bd-28u8v4: `attempt` resets to 0 at the start of every round (bd-bgeo6i), + # so round 1's implementer and round 2's implementer are both launched as + # `attempt` 2 within their own round. The timer armed for round 1's + # implementer must not be able to escalate round 2's implementer just + # because the attempt numbers collide. + # + # Round 1's implementer (`revise_slow_then_fast.sh`) sleeps for most of the + # per-pass timeout before committing — long enough that its stale timer + # fires only AFTER round 2's implementer has already launched. Round 2's + # implementer then keeps running well past that stale-timer instant, but + # still comfortably within its own fresh timeout. With the bug, the stale + # round-1 timer escalates round 2's revising pass as timed-out; fixed, the + # gate ignores it and round 2's implementer is allowed its own full + # budget, converging normally when round 3 approves. + test "a round-1 implementer's timer does not escalate a round-2 implementer at the same attempt", + %{repo: repo, ws: ws} do + task = new_task(ws) + branch = "feature/rev" + :ok = seed_feature_branch(repo, branch) + + {:ok, pid} = + Worker.start( + task_id: task.id, + repo: "trib/repo", + workspace_id: ws.id, + meta: %{ + branch: branch, + repo_path: repo, + target_branch: "main", + merge_title: "Merge #{task.id}", + review_required: true, + review_rounds: 3, + worktree_path: repo, + review_command: [@reject_twice], + # pass 1 (round-1 implementer) sleeps 1.9s; pass 2 (round-2 + # implementer) sleeps 1.0s — both well under the 2.5s per-pass + # timeout on their own, but round 1's stale timer (armed at + # implementer-1-start + 2.5s) lands mid-flight through round 2's + # implementer run. + revise_command: [@revise_slow_then_fast, "19", "10"], + review_timeout_ms: 2_500 + } + ) + + on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid, :normal) end) + :ok = Worker.advance(pid, :claude) + send(pid, {:__claude_session_done__, "arb done"}) + + # Round 1 rejects → slow implementer revises → round 2 rejects → second + # implementer revises (surviving the stale round-1 timer) → round 3 + # approves → merge. No timeout escalation anywhere in between. + wait_until(fn -> match?(%{status: :completed}, Worker.state(pid)) end, 12_000) + assert merge_commit_count(repo) == 1 + refute Worker.state(pid).meta[:failure_reason] + + review_id = ReviewGate.reviewer_task_id(task.id) + runs = Ash.read!(Arbiter.Workers.Run) + + assert Enum.any?(runs, &(&1.task_id == review_id <> "#impl1")), + "expected a distinct implementer run row for round 1" + + assert Enum.any?(runs, &(&1.task_id == review_id <> "#impl2")), + "expected a distinct implementer run row for round 2 — it must not have been " <> + "killed by round 1's stale timer" + + require Ash.Query + + rounds = + Arbiter.ReviewGate.Round + |> Ash.Query.filter(task_id == ^task.id) + |> Ash.Query.sort(round: :asc, inserted_at: :asc) + |> Ash.read!() + + # No round anywhere recorded a timed-out verdict — the only source of + # `:timed_out` in this gate's vocabulary is the very bug under test. + refute Enum.any?(rounds, &(&1.verdict == :timed_out)) + end + + # bd-28u8v4: the reviewer-side sibling of the collision above. Round 1's + # reviewer (`attempt` 1) sleeps close to the per-pass timeout before + # REQUEST_CHANGES; the implementer commits immediately; round 2's + # reviewer is launched as `attempt` 1 again (bd-bgeo6i resets `attempt` + # per round) and is still running when round 1's reviewer timer fires. + # With the bug, that stale timer's `{:timeout, attempt}` matches round + # 2's reviewer and either retries or escalates it as timed-out; fixed, + # the gate ignores it because the timer is tagged with round 1, not + # round 2, and round 2's reviewer is left to approve normally. + test "a round-1 reviewer's timer does not time out a round-2 reviewer at the same attempt", + %{repo: repo, ws: ws} do + task = new_task(ws) + branch = "feature/rev" + :ok = seed_feature_branch(repo, branch) + + {:ok, pid} = + Worker.start( + task_id: task.id, + repo: "trib/repo", + workspace_id: ws.id, + meta: %{ + branch: branch, + repo_path: repo, + target_branch: "main", + merge_title: "Merge #{task.id}", + review_required: true, + review_rounds: 2, + worktree_path: repo, + # pass 1 (round-1 reviewer) sleeps 1.9s and rejects; every later + # pass (round-2 reviewer) sleeps 1.0s and approves — both well + # under the 2.5s per-pass timeout on their own, but round 1's + # stale timer (armed at reviewer-1-start + 2.5s) lands mid-flight + # through round 2's reviewer run. + review_command: [@reject_slow_then_approve_fast, "19", "10"], + revise_command: [@revise_commit], + review_timeout_ms: 2_500 + } + ) + + on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid, :normal) end) + :ok = Worker.advance(pid, :claude) + send(pid, {:__claude_session_done__, "arb done"}) + + # Round 1 rejects (slowly) → implementer revises (fast) → round 2 + # approves (surviving the stale round-1 reviewer timer) → merge. No + # timeout escalation or spurious timeout-retry run anywhere in between. + wait_until(fn -> match?(%{status: :completed}, Worker.state(pid)) end, 12_000) + assert merge_commit_count(repo) == 1 + refute Worker.state(pid).meta[:failure_reason] + + review_id = ReviewGate.reviewer_task_id(task.id) + runs = Ash.read!(Arbiter.Workers.Run) + + assert Enum.any?(runs, &(&1.task_id == review_id <> "#r2")), + "expected a distinct round-2 reviewer run row — it must not have been " <> + "killed by round 1's stale timer" + + refute Enum.any?(runs, &String.contains?(&1.task_id, "#r2#t")), + "round 1's stale timer must not have triggered a spurious round-2 " <> + "reviewer timeout-retry run" + + require Ash.Query + + rounds = + Arbiter.ReviewGate.Round + |> Ash.Query.filter(task_id == ^task.id) + |> Ash.Query.sort(round: :asc, inserted_at: :asc) + |> Ash.read!() + + # No round anywhere recorded a timed-out verdict — the only source of + # `:timed_out` in this gate's vocabulary is the very bug under test. + refute Enum.any?(rounds, &(&1.verdict == :timed_out)) + end + # bd-78vg4v: a large implementer transcript is CAPPED (head+tail) when # recorded into the durable thread, so the round-2 re-review prompt stays # bounded instead of ballooning past round-1's. The @revise_huge fixture diff --git a/apps/arbiter/test/fixtures/review_reject_slow_then_approve_fast.sh b/apps/arbiter/test/fixtures/review_reject_slow_then_approve_fast.sh new file mode 100755 index 000000000..18c2545d6 --- /dev/null +++ b/apps/arbiter/test/fixtures/review_reject_slow_then_approve_fast.sh @@ -0,0 +1,51 @@ +#!/bin/sh +# Fixture: a reviewer (ReviewGate) worker for bd-28u8v4 — the round-1 +# REVIEWER pass-timer collision (the sibling of `revise_slow_then_fast.sh` / +# `review_reject_twice.sh`, which cover the IMPLEMENTER side). Pass 1 +# (round-1 reviewer, `attempt` 1) sleeps close to the per-pass timeout before +# REQUEST_CHANGES; every later pass (round-2+ reviewer, `attempt` 1 again — +# `dispatch_next_review/2` resets `attempt` per round) sleeps briefly before +# APPROVE. Paired with an implementer that commits immediately, this drives a +# round-1 reviewer timer to fire WHILE the round-2 reviewer — at the same +# `attempt` number — is still mid-flight, without ever depending on the +# implementer pass to absorb the collision. +# +# `$1` is the sleep duration in tenths of a second for the FIRST (round-1) +# pass; `$2` is the sleep duration in tenths of a second for every LATER +# pass (default 0). A counter (kept in `.git`, so it never shows up in `git +# status --porcelain`) tells the passes apart. +sleep_tenths_1="${1:-19}" +sleep_tenths_later="${2:-0}" +git_dir="$(git rev-parse --git-dir)" +counter_file="$git_dir/review_reject_slow_then_approve_fast_pass" +pass=0 +[ -f "$counter_file" ] && pass="$(cat "$counter_file")" +pass=$((pass + 1)) +echo "$pass" > "$counter_file" + +if [ "$pass" -eq 1 ]; then + sleep_tenths="$sleep_tenths_1" +else + sleep_tenths="$sleep_tenths_later" +fi + +if [ "$sleep_tenths" -gt 0 ]; then + whole=$((sleep_tenths / 10)) + tenth=$((sleep_tenths % 10)) + sleep "${whole}.${tenth}" +fi + +if [ "$pass" -eq 1 ]; then + echo "reviewing pass $pass: rejecting" + echo "VERDICT: REQUEST_CHANGES" + echo "findings: [high] guard.txt:1 needs another pass" + echo "arb done" +else + echo "reviewing pass $pass: approving" + echo "VERDICT: APPROVE" + echo "DISPOSITIONS:" + echo "- [ADDRESSED] F1.1 — anchored in guard.txt:1 on the round-1 revise" + echo "findings: none" + echo "arb done" +fi +exit 0 diff --git a/apps/arbiter/test/fixtures/review_reject_twice.sh b/apps/arbiter/test/fixtures/review_reject_twice.sh new file mode 100755 index 000000000..d3d433a97 --- /dev/null +++ b/apps/arbiter/test/fixtures/review_reject_twice.sh @@ -0,0 +1,32 @@ +#!/bin/sh +# Fixture: a reviewer (ReviewGate) worker for bd-28u8v4 — the round-1 pass +# timer collision. REQUEST_CHANGES on its first TWO passes (round 1 and round +# 2), then APPROVE on every later pass. Paired with +# `revise_slow_then_fast.sh` (the implementer) to drive a gate through TWO +# revise rounds, so a round-1 implementer's stale timeout timer has a round-2 +# implementer pass — at the same `attempt` number — to collide with. +# +# A counter (kept in `.git`, so it never shows up in `git status +# --porcelain`) tracks how many reviewing passes have run. +git_dir="$(git rev-parse --git-dir)" +counter_file="$git_dir/review_reject_twice_pass" +pass=0 +[ -f "$counter_file" ] && pass="$(cat "$counter_file")" +pass=$((pass + 1)) +echo "$pass" > "$counter_file" + +if [ "$pass" -le 2 ]; then + echo "reviewing pass $pass: rejecting" + echo "VERDICT: REQUEST_CHANGES" + echo "findings: [high] guard.txt:1 needs another pass" + echo "arb done" +else + echo "reviewing pass $pass: approving" + echo "VERDICT: APPROVE" + echo "DISPOSITIONS:" + echo "- [ADDRESSED] F1.1 — anchored in guard.txt:1 on the round-1 revise" + echo "- [ADDRESSED] F2.1 — anchored in guard.txt:1 on the round-2 revise" + echo "findings: none" + echo "arb done" +fi +exit 0 diff --git a/apps/arbiter/test/fixtures/revise_slow_then_fast.sh b/apps/arbiter/test/fixtures/revise_slow_then_fast.sh new file mode 100755 index 000000000..59308b98a --- /dev/null +++ b/apps/arbiter/test/fixtures/revise_slow_then_fast.sh @@ -0,0 +1,47 @@ +#!/bin/sh +# Fixture: an IMPLEMENTER worker for bd-28u8v4 — the round-1 pass timer +# collision. The round-1 revise pass sleeps for most of the per-pass +# `review_timeout_ms` budget (but still comfortably within it) before +# committing; the round-2 revise pass commits immediately. Paired with +# `review_reject_twice.sh` (the reviewer, which rejects rounds 1 and 2 then +# approves), this reproduces the exact collision from bd-28u8v4: round 1's +# implementer and round 2's implementer are both the SECOND pass launched in +# their own round (`attempt` 2), so a timer armed for round 1's implementer +# and never cancelled/disambiguated would fire while round 2's implementer is +# still well within its own fresh budget. +# +# `$1` is the sleep duration in tenths of a second for the FIRST (round-1) +# pass; `$2` is the sleep duration in tenths of a second for the SECOND +# (round-2) pass (default 0). Every later pass commits immediately. A counter +# (kept in `.git`) tells the passes apart. +sleep_tenths_1="${1:-13}" +sleep_tenths_2="${2:-0}" +git_dir="$(git rev-parse --git-dir)" +counter_file="$git_dir/revise_slow_then_fast_pass" +pass=0 +[ -f "$counter_file" ] && pass="$(cat "$counter_file")" +pass=$((pass + 1)) +echo "$pass" > "$counter_file" + +sleep_tenths=0 +if [ "$pass" -eq 1 ]; then + sleep_tenths="$sleep_tenths_1" +elif [ "$pass" -eq 2 ]; then + sleep_tenths="$sleep_tenths_2" +fi + +if [ "$sleep_tenths" -gt 0 ]; then + # sleep supports fractional seconds via a tenths-to-decimal conversion. + whole=$((sleep_tenths / 10)) + tenth=$((sleep_tenths % 10)) + sleep "${whole}.${tenth}" +fi + +echo "implementer: addressing the reviewer's findings on this branch (pass $pass)" +echo "anchored guard (pass $pass)" >> guard.txt +git add guard.txt >/dev/null 2>&1 +git -c user.email=fixture@example.com -c user.name=Fixture \ + commit -q -m "address reviewer finding F1.1 (pass $pass)" >/dev/null 2>&1 +echo "FIXED: anchored the match in guard.txt:1 (pass $pass)" +echo "arb done" +exit 0 diff --git a/docs/review-coverage-and-guard-policy.md b/docs/review-coverage-and-guard-policy.md index 7d157105d..d83a5d768 100644 --- a/docs/review-coverage-and-guard-policy.md +++ b/docs/review-coverage-and-guard-policy.md @@ -134,12 +134,12 @@ inventory cannot silently rot. | # | Guard | Anchor | Protects against | Misfire mode | On failure | Patches | |---|---|---|---|---|---|---| -| G1 | Pre-spawn commit check — branch has commits ahead of target | `apps/arbiter/lib/arbiter/worker/review_gate.ex:3497` (`reviewer_commit_check`) | bd-1mksks: reviewing an empty branch, reviewer reports "no work" | Git hiccup reads as "no commits" | **Fails open** (git errors → proceed); genuine `{:ok, false}` → `escalate_pre_review` | 2 (bd-1mksks, bd-ofql8k) | -| G2 | Empty diff-range guard (`base_sha == head_sha`) | `apps/arbiter/lib/arbiter/worker/review_gate.ex:3541` (`empty_diff_guard`) | bd-31bh37: target already absorbed the commits; bogus REQUEST_CHANGES | A legitimately-absorbed branch escalates instead of completing | Escalates via `apps/arbiter/lib/arbiter/worker/review_gate.ex:3071` (`escalate_pre_review`) — since P9, **parks** `:empty_diff`, one escalation, run recorded `review_parked` | 1 | +| G1 | Pre-spawn commit check — branch has commits ahead of target | `apps/arbiter/lib/arbiter/worker/review_gate.ex:3563` (`reviewer_commit_check`) | bd-1mksks: reviewing an empty branch, reviewer reports "no work" | Git hiccup reads as "no commits" | **Fails open** (git errors → proceed); genuine `{:ok, false}` → `escalate_pre_review` | 2 (bd-1mksks, bd-ofql8k) | +| G2 | Empty diff-range guard (`base_sha == head_sha`) | `apps/arbiter/lib/arbiter/worker/review_gate.ex:3607` (`empty_diff_guard`) | bd-31bh37: target already absorbed the commits; bogus REQUEST_CHANGES | A legitimately-absorbed branch escalates instead of completing | Escalates via `apps/arbiter/lib/arbiter/worker/review_gate.ex:3137` (`escalate_pre_review`) — since P9, **parks** `:empty_diff`, one escalation, run recorded `review_parked` | 1 | | G3 | Reviewing-pass timeout, with one fresh-mind retry | `apps/arbiter/lib/arbiter/worker/review_gate.ex:1239` (`timeout_retries_left`), bound `apps/arbiter/lib/arbiter/worker/review_gate.ex:190` (`default_timeout_retries`) | bd-78vg4v: transient hung session | A slow-but-working review is killed and re-paid | Retry once, then `escalate_timeout` → since P9, **parks** `:reviewer_timeout` (was a **failed run** `:review_gate_inconclusive`) | 2 | -| G4 | Timeout → `:no_verdict`, not `:request_changes` | `apps/arbiter/lib/arbiter/worker/review_gate.ex:3105` (`escalate_timeout`) | bd-216r3e: synthetic single-finding REQUEST_CHANGES → self-sustaining re-dispatch loop | — (this one is a *fix* to a prior misfire) | Records `verdict: :timed_out`, reports `:no_verdict` | 1 | +| G4 | Timeout → `:no_verdict`, not `:request_changes` | `apps/arbiter/lib/arbiter/worker/review_gate.ex:3171` (`escalate_timeout`) | bd-216r3e: synthetic single-finding REQUEST_CHANGES → self-sustaining re-dispatch loop | — (this one is a *fix* to a prior misfire) | Records `verdict: :timed_out`, reports `:no_verdict` | 1 | | G5 | Verdict parse — `VERDICT:` line, memory then durable transcript | `apps/arbiter/lib/arbiter/worker/review_gate.ex:478` (`parse_verdict`), regex `apps/arbiter/lib/arbiter/worker/review_gate.ex:261` (`verdict_approve`) | bd-6dxit2: a dropped PubSub line reads as "reviewer said nothing" | A real verdict in an unrecognised shape → `:no_verdict` | `maybe_reprompt` | **Chain B: 4–5** | -| G6 | Verdict re-prompt budget | `apps/arbiter/lib/arbiter/worker/review_gate.ex:2534` (`maybe_reprompt`), bound `apps/arbiter/lib/arbiter/worker/review_gate.ex:181` (`default_verdict_retries`) | bd-8v8ays: malformed verdict wastes a whole review | Two passes paid, still no verdict | Since P9, **parks** `:inconclusive` (was a **failed run** `:review_gate_inconclusive`) | 3 | +| G6 | Verdict re-prompt budget | `apps/arbiter/lib/arbiter/worker/review_gate.ex:2600` (`maybe_reprompt`), bound `apps/arbiter/lib/arbiter/worker/review_gate.ex:181` (`default_verdict_retries`) | bd-8v8ays: malformed verdict wastes a whole review | Two passes paid, still no verdict | Since P9, **parks** `:inconclusive` (was a **failed run** `:review_gate_inconclusive`) | 3 | | G7 | Last-ditch transcript recovery before conceding | `apps/arbiter/lib/arbiter/worker/review_gate.ex:554` (`recover_verdict_from_scans`) | bd-869mmg/bd-atyrrq: a real review discarded as inconclusive | — | Re-dispatches the recovered verdict | 1 | | G8 | Empty-findings guard on REQUEST_CHANGES | `apps/arbiter/lib/arbiter/worker/review_gate.ex:1402` (`findings_present?`) | bd-3y2mda: revise loop entered with nothing to act on | A terse-but-real finding under 16 chars | Shares G6's budget; then, since P9, **parks** `:inconclusive` (was a **failed run**) | 2 | | G9 | Verdict guard: partial verification | `apps/arbiter/lib/arbiter/worker/review_gate.ex:2875` (`verdict_guard_spec`) | bd-4te55l: `VERIFICATION: PARTIAL` findings taken at face value | Honest disclosure is punished with an extra round | Re-prompt ×1, then fail-closed behind a banner; since P9 the terminal **parks** `:verdict_guard_exhausted` rather than failing the run (content stays closed — nothing merges) | 1 | @@ -149,8 +149,8 @@ inventory cannot silently rot. | G13 | Shared guard dispatcher + terminal handling | `apps/arbiter/lib/arbiter/worker/review_gate.ex:2791` (`run_verdict_guard`), `apps/arbiter/lib/arbiter/worker/review_gate.ex:2823` (`fail_closed`), registry `apps/arbiter/lib/arbiter/worker/review_gate.ex:2746` (`verdict_guards`) | Four guards drifting apart | — | — | — | | G14 | Round budget exhausted | `apps/arbiter/lib/arbiter/worker/review_gate.ex:1584` (`do_route_after_reject`) | Unbounded review↔revise ping-pong | A converging task one round short escalates | Escalate with transcript → **failed run**. P9 left this one deliberately: a reviewer that really said REQUEST_CHANGES every round is an honest rejection, so only the verdict-guard arm that routes here now parks | 2 | | G15 | Commit gate: HEAD unchanged after a fix round | `apps/arbiter/lib/arbiter/worker/review_gate.ex:1805` (`commit_gate_outcome`) | bd-2eyf9y: re-reviewing an identical diff; bd-cb7wpq: a finding legitimately fixed via a non-file channel (a PR title/description edit, a label, a comment) parked as if the worker had done nothing | A legitimate rebuttal-only round is treated as failure | Nudge ×1, then `escalate_commit_gate` → since P9, **parks** `:commit_gate_no_changes` / `:commit_gate_uncommitted`; since bd-cb7wpq, an explicit `NO-FILE-CHANGE:` disposition (`non_file_fix_declared?/1`) dispatches round 2 for a real re-review instead, and only escalates (`:commit_gate_no_changes_after_non_file_fix`) if it happens twice in a row with nothing new for the reviewer to check (was a **failed run**) | 3 (bd-2eyf9y, bd-c6tdbu, bd-cb7wpq) | -| G16 | Commit-gate escalations (4 shapes) | `apps/arbiter/lib/arbiter/worker/review_gate.ex:2081` (`escalate_commit_gate`), `apps/arbiter/lib/arbiter/worker/review_gate.ex:1793` (`escalate_no_changes`) | bd-c6tdbu: the "no changes" message was misleading after an approval-gap reject | — | Since P9, **parks** (`:commit_gate_*` / `:no_changes_after_approval_gap` / `:commit_gate_no_changes_after_non_file_fix`); was a **failed run** `:review_gate_inconclusive` | 2 | -| G17 | Reviewed-SHA stamp on APPROVE | `apps/arbiter/lib/arbiter/worker/review_gate.ex:3612` (`stamp_reviewed_head`) | bd-6bg54c cause B: guard could never learn a later round approved a newer head | Best-effort; a failed write silently leaves the *old, conservative* value — which is precisely the #1585 stall | Logs and continues | 1 | +| G16 | Commit-gate escalations (4 shapes) | `apps/arbiter/lib/arbiter/worker/review_gate.ex:2081` (`escalate_commit_gate`), `apps/arbiter/lib/arbiter/worker/review_gate.ex:1875` (`escalate_no_changes`) | bd-c6tdbu: the "no changes" message was misleading after an approval-gap reject | — | Since P9, **parks** (`:commit_gate_*` / `:no_changes_after_approval_gap` / `:commit_gate_no_changes_after_non_file_fix`); was a **failed run** `:review_gate_inconclusive` | 2 | +| G17 | Reviewed-SHA stamp on APPROVE | `apps/arbiter/lib/arbiter/worker/review_gate.ex:3678` (`stamp_reviewed_head`) | bd-6bg54c cause B: guard could never learn a later round approved a newer head | Best-effort; a failed write silently leaves the *old, conservative* value — which is precisely the #1585 stall | Logs and continues | 1 | | G18 | Pre-review push gate — the head under review must be on `origin/` | `apps/arbiter/lib/arbiter/worker/review_gate.ex:1001` (`push_gate`), `apps/arbiter/lib/arbiter/worker/review_gate.ex:1033` (`escalate_unpushed_head`), `apps/arbiter/lib/arbiter/worker/review_gate.ex:1094` (`pushed_head`), `apps/arbiter/lib/arbiter/worker/review_gate.ex:1598` (`remote_advance`, the pre-fix-round half), `apps/arbiter/lib/arbiter/reviews/push_state.ex` (the git primitive, incl. the worktree-must-be-on-the-branch precondition) | bd-2jkrqu: vs-5l45oz approved an UNPUSHED fix-round commit while MR !228 still held the unfixed head; the park escalation then claimed "the branch is pushed" and offered a hand merge | A transient push failure (offline, auth) parks a branch that was otherwise fine | Pushes once; **fails open** when push state is undeterminable (no `origin`, no worktree, or the worktree is not checked out on the branch — otherwise the gate would publish the checked-out branch's tip AS the PR branch); a diverged / rejected push **parks** `:head_not_pushed` with one escalation and never force-pushes. bd-bq8c8a adds the pre-fix-round half: one fetch before the implementer is dispatched, and a remote that **strictly advanced** skips the fix round and re-reviews the new head rather than producing an orphan commit that could only park here; it has no terminal of its own | 1 | | G19 | Reviewer print-timeout → rotate to the next `review_agent.type` provider | `apps/arbiter/lib/arbiter/worker/review_gate.ex:2356` (`handle_reviewer_print_timeout`), `apps/arbiter/lib/arbiter/worker/review_gate.ex:2364` (`rotate_reviewer`), bound `apps/arbiter/lib/arbiter/worker/review_gate.ex:2347` (`next_reviewer_provider`) | bd-3hb4ih: bd-1xss5z folded agy's own fixed `--print-timeout` into `@infra_failure_categories`, so a reviewer cut short by that CLI-internal wall parked the task outright — correct for expired credentials or a dead gateway, wrong for a wall that belongs to the CLI and that a different provider does not have | A pool entry that is merely slow costs a second provider's pass on the same diff | Rotates to the next pool entry with the IDENTICAL prompt (same diff, same round, no round and no verdict re-prompt consumed); at most one pass per pool entry per round; once every entry has timed out, **parks** `:reviewer_timeout` with one escalation naming each provider's timeout. A pool of one never rotates — G3/G4's terminal, unchanged | 1 | | G20 | Empty net-diff guard on APPROVE (`head_sha != base_sha` but the content nets to zero) | `apps/arbiter/lib/arbiter/worker/review_gate.ex:1423` (`finalize_approval`) | bd-aq81qz / PR #1957: a task redispatched onto a branch whose commits were already squashed onto main; the worker merged main in (a real commit, so G2's SHA-equality check does not fire) but `base_sha..HEAD` is empty. The reviewer APPROVEd and only a `review-coverage write failed: :no_net_diff` warning marked the miss | A branch whose target genuinely absorbed its commits via a different route than G2 expects escalates instead of completing | Reuses `apps/arbiter/lib/arbiter/worker/review_gate.ex:3802` (`coverage_net_diff_id`)'s fingerprint attempt; its `{:error, :no_net_diff}` **parks** `:empty_net_diff` instead of recording the APPROVE, one escalation, run recorded `review_parked` | 1 | @@ -366,7 +366,7 @@ below calls it and nothing writes coverage any other way: | Site | Today | Becomes | |---|---|---| -| ReviewGate clean approve | `apps/arbiter/lib/arbiter/worker/review_gate.ex:3612` (`stamp_reviewed_head`) writes `last_reviewed_sha` | `Coverage.record(kind: :reviewed, round: state.round, net_diff_id: …)` — **and the write is no longer best-effort**: a failed write must page, because a silently-missing row *is* the #1585 stall | +| ReviewGate clean approve | `apps/arbiter/lib/arbiter/worker/review_gate.ex:3678` (`stamp_reviewed_head`) writes `last_reviewed_sha` | `Coverage.record(kind: :reviewed, round: state.round, net_diff_id: …)` — **and the write is no longer best-effort**: a failed write must page, because a silently-missing row *is* the #1585 stall | | ReviewGate verdict guards | — | nothing (a `fail_closed` is a reject) | | ReviewPatrol post-review | `last_reviewed_sha: head` on the engagement | `Coverage.record(kind: :reviewed, source: :review_patrol)` on the **authoring task**, plus the engagement cursor as today | | ExternalReview baseline | `apps/arbiter/lib/arbiter/reviews/external_review.ex:1418` (`last_reviewed_sha`) | `Coverage.record(kind: :reviewed, source: :external_review)` when the external verdict is an approval; cursor only otherwise |