ai-code-review: failover only covers dispatch failure, not a dispatched agent's runtime failure

[dem-extra1]

Deferred from review of #357, where ai-code-review.yml was added.

The gap

ai-code-review.yml picks an agent at random and dispatches that agent's review workflow:

if gh workflow run "$agent_wf" --repo "$REPO" "${REF_ARGS[@]}" -f pr_number="$PR_NUMBER"; then
  echo "Dispatched the review to $agent ($agent_wf)."
  exit 0
fi

gh workflow run returns as soon as the run is queued, so a zero exit status
proves only that the dispatch was accepted --- not that the review ran, and not
that it produced a verdict. An agent that dispatches fine and then dies partway
through (quota exhaustion, an SDK error, the stub-review signature gha#185
covers for Claude) is never failed over to the next candidate: the loop has
already exited 0.

So the fallback currently covers exactly the dispatch-time cases:

• no API key/token secret configured for that agent
• the agent's review workflow file missing or disabled in the caller repo

Why it was not fixed in #357

Waiting synchronously is not a one-line change, and the obvious form does not
work. gh workflow run reports no run id, so gh run watch has nothing to
attach to; locating the dispatched run means polling
gh run list --workflow "$agent_wf" and matching on a heuristic (recent
created_at, the head SHA, the dispatch inputs), each of which can pick the
wrong run when two dispatches land close together --- which is the normal case
on a PR that gets several pushes.

There is also a real design question underneath, worth settling before
implementing: a synchronous wait turns this from a ~10 second dispatcher into a
job that occupies a runner for the whole length of a review, and the timeout it
would need is the review's timeout plus a margin.

Suggested approach, if picked up

• Have each per-agent review workflow report its own outcome somewhere
  attributable (a check-run name, or a PR comment marker), and have this
  workflow poll for that rather than for the run itself --- the outcome is what
  the failover actually cares about, and it is not ambiguous between concurrent
  runs the way a run-list heuristic is.
• Bound the wait explicitly and treat a timeout as "this agent did not deliver",
  falling through to the next candidate.
• Decide whether a failed-over review should say so on the PR, so a reader can
  tell a second agent's review from a first agent's.

Meanwhile

#357 documents the current scope rather than leaving it implied --- in the
workflow's own header comment, in website/workflows.qmd, and in
website/reference/ai-code-review.qmd --- so the README's summary no longer
reads as though a runtime failure is covered.

[d-morrison]

Recurrence data point (Morrison-Lab/ai-config#1194, 2026-08-06): a dispatched claude-review hung twice in a row on the same PR -- runs 31062828893 and 31063429910 both sat review / claude-review: in_progress with no status update for 8+ minutes and never posted a verdict or failed. Because a hung run posts nothing to the PR (only the success path posts Claude finished review -- View run), there was no PR-side breadcrumb; the stall was only visible by polling the run object directly. This is exactly the "dispatched agent's runtime failure" this issue is about (and the no-comment-on-stall gap #424 proposes to close). The self-review fallback had to stand in.

[d-morrison]

Correction to my note above: only the FIRST dispatched run (31062828893) hung. The SECOND run (31063429910) did NOT stall -- I misread its stale updated_at (frozen at 01:41:25Z) as a hang, but it was actively working and posted a full, substantive verdict ~13 minutes in ($28.31, three parallel verify sub-agents). So this PR (#1194) is evidence of a claude-review run whose run-object updated_at goes stale for many minutes mid-run while it is genuinely working -- which is its own trap (reading updated_at as a liveness signal produces a false 'stalled' conclusion), on top of the no-PR-breadcrumb gap. The stall-detection half of my earlier comment stands only for run 31062828893.

[d-morrison]

The suggested approach's precondition is now partly met — #548 ships an attributable outcome marker

Noting this before the issue is picked up, since the suggested approach opens by asking for something that did not exist when this was filed and now does for two agents.

Have each per-agent review workflow report its own outcome somewhere attributable (a check-run name, or a PR comment marker), and have this workflow poll for that rather than for the run itself

#548 (merged today as d23fd48) is that marker for claude-code-review.yml. Before it, every posting step in that workflow was gated on Resolve final review outcome having succeeded, so a review that produced no verdict posted nothing — the "no PR-side breadcrumb" the comments above describe. It now posts a failure comment on that path via report-review-failure, carrying a failure_kind (short-circuit, hard-error, no-output, stub, high-denial, deferred), the denied tools, the cost, and the run URL.

gemini-code-review.yml already had its own equivalent through report-gemini-failure (#379). So two of the N candidate agents now emit a PR-side outcome marker; the others do not, which makes "have each per-agent workflow report its own outcome" a concrete, enumerable piece of work rather than an open-ended one.

The important qualification: this covers one of the two runtime-failure classes, not both

The comments above describe two distinct failures, and only one of them reaches a marker:

Class	Example	Marker?
Job completes, no usable verdict	quota mid-run, stub review, denial-count over threshold	Yes, since #548
Job never completes (hangs)	run 31062828893	No — still #424's territory

The distinction is structural rather than incidental: report-review-failure fires off resolve-final's outcome, and a run that never reaches that step never posts anything. So a poller keyed purely on the marker would wait out its full timeout on a hang. That is not fatal — this issue's second bullet already says to bound the wait and treat a timeout as "did not deliver" — but it does mean the marker replaces the run-list heuristic for one class while the timeout remains load-bearing for the other. Worth stating explicitly so the timeout is not later mistaken for belt-and-braces.

Two practical notes for whoever implements this

The marker is not at @v2 yet. v2 is at d738e66 and d23fd48 is not in it, so consumers do not have the marker today, and neither does this repo's own dogfood caller. I hit this live an hour ago: gha#550's review produced no verdict and left the PR thread completely silent, because the workflow that ran predates the fix. A poller written against the marker cannot be released ahead of the tag slide.

A live example of the denial class is on #543. gha#543 now carries a reproduction: six Bash denials on compound commands with redirection, one over the stub-retry threshold, $3.42 spent and no verdict. That is the high-denial failure_kind — a useful concrete case to test a failover against, since it is reproducible rather than intermittent.

Not claiming this issue; #550 is in flight on this session's branch.

---

Generated by Claude Code