claude-code-review: gha#392 background-agent stub review recurred despite gha#393's run_in_background:false mitigation

[d-morrison]

gha#393's run_in_background: false prompt mitigation for gha#392 (reviewer spawns background Agent calls, then ends its turn waiting on completion notifications that never arrive in a headless CI run) is live in run-claude-review-attempt/action.yml and has been for a while (v2 slid to 70c7fa8 carrying it, per gha#392's own measurement comments). It has now recurred with the identical signature.

Evidence

Run 32347489886, job review / claude-review, on Morrison-Lab/ai-config#1744 at head 185054b, is_error: false, subtype: success, num_turns: 13, total_cost_usd: $4.21.

The final result message:

Stopping Point: Not a clean stopping point / work remains queued: four background review agents (2 CLAUDE.md compliance passes, 2 Opus bug-scan passes) on PR Morrison-Lab/ai-config#1744 are still running. Once they complete, I still need to validate any flagged issues, then produce the final review output ending in a ### Verdict line, per this review-only run's requirements.

Despite claude_args's --append-system-prompt explicitly stating (verbatim, still present in run-claude-review-attempt/action.yml at the time of this run):

This run is a single, synchronous CI job with a fixed timeout and no one to resume it afterward. Do NOT spawn a background agent or sub-task to continue the review in a future turn or after this job ends... every one of those calls MUST be synchronous: pass run_in_background: false on each Agent/Task call.

the reviewer spawned four background agents anyway and ended its turn waiting on them -- exactly the gha#392 failure shape, just with the review's own multi-pass structure (2 CLAUDE.md compliance + 2 Opus bug-scan) instead of #392's eight-way fan-out.

Why this is likely worth a second look

A prompt instruction is not a hard constraint -- the model can and (per this run) does deviate from it. gha#392's own "Suggested direction" section named the mechanical alternative (constrain the tool rather than instruct around it) and its follow-up comment on gha#392 walked that back specifically for denying Agent outright (breaks the plugin's legitimate synchronous fan-out). Nothing in that thread evaluated a narrower mechanical constraint -- e.g. whether Claude Code's tool-permission syntax supports a parameter-scoped grant (something like an Agent(run_in_background:false)-shaped allow rule, if the permission engine supports matching on tool input rather than only tool name) that would make the synchronous-only constraint enforced rather than requested. Worth checking whether that's possible before concluding a prompt-only fix is the ceiling here.

Also unaddressed from gha#392 itself

Its closing "Suggested direction" paragraph asked: "Worth checking whether the guard can also treat 'final message names background agents' as a distinct outcome from a generic stub, since the retry is unlikely to help and the spend is large." That's still open -- check-review-execution.sh treats this the same as any other no-verdict stub (retryable when denial count is low, per gha#185's threshold), even though a same-prompt retry of a run that just demonstrated it ignores the synchronous-only instruction is not obviously more likely to recover than #198's high-denial pattern is.

[d-morrison]

Additional occurrence on #536, worth recording as fresh evidence for this pattern.

4 consecutive claude-review stubs across 2 commits, each with a high permission_denials_count (the gha#198 pattern this issue's own body distinguishes from the background-agent-specific gha#392 pattern):

Attempt	Trigger	Denials	Run
1	automatic (push)	17	32393563593
2	automatic (push)	9	32394217032
2 retry	manual re-run of #2	-- (succeeded, real verdict: Needs more work)	same run, later attempt
3	automatic (push, addressing round-2 findings)	36	32396678856
3 retry	manual re-run of #3	19	same run, later attempt

Notably a manual re-run of the identical commit succeeded once (attempt 2's retry), producing a genuine "Needs more work" verdict with real findings -- then the very next commit (addressing those findings) stubbed twice more, including on its own manual re-run. That's consistent with this issue's framing: the denial count is highly variable run-to-run on materially the same diff/context, which points at the code-review plugin's 4-way parallel sub-agent fan-out non-deterministically hitting denied tool calls, not at anything specific to this PR's content.

Per self-review-fallback.md, gave up on re-triggering after 2 automatic + 2 manual attempts on the final commit and posted a self-review instead: #536 (comment)

No new investigation attempted here -- just logging a clean, quantified reproduction for whoever picks this up, since the pattern is otherwise easy to dismiss as a one-off.

[d-morrison]

One more data point: gha#539 stubbed twice more on its final commit (13, then 17 denials) before a self-review was needed. Running tally this session across 3 PRs (#536, #537, #539): 8 stub attempts total, only 2 self-resolved via a manual re-run, the rest needed a self-review fallback. This is now a very frequent, not-occasional failure mode -- worth prioritizing the tool-permission-scoping investigation this issue's body proposes.

[d-morrison]

Claude Code (remote session, gia sweep) is working on this — paws off until I'm done.

The prioritized question is answered: yes, parameter-scoped rules exist

The body asks:

whether Claude Code's tool-permission syntax supports a parameter-scoped grant (something like an Agent(run_in_background:false)-shaped allow rule, if the permission engine supports matching on tool input rather than only tool name)

It does. From code.claude.com/docs/en/permissions, §"Match by input parameter", verbatim:

Deny and ask rules can match a top-level input parameter on any tool with Tool(param:value). The rule matches when Claude calls the tool with that parameter set to that exact value. An allow rule for one parameter value wouldn't establish that the call is safe overall, so allow rules continue to use each tool's own specifier syntax.

Its own example table names this tool and this parameter:

Rule	Matches
Agent(model:opus)	Agent calls that request the Opus model tier
Agent(isolation:worktree)	Agent calls that request a git worktree
Bash(run_in_background:true)	Bash calls that run in the background

So Agent(run_in_background:true) as a deny rule is well-formed, and it is the narrower constraint the issue proposes: it leaves the plugin's legitimate synchronous fan-out untouched while making the background spawn unavailable rather than merely discouraged. That is precisely what gha#392's follow-up wanted and could not get by denying Agent outright.

Note the direction. The issue guesses at an Agent(run_in_background:false) allow rule; the docs rule that shape out ("allow rules continue to use each tool's own specifier syntax"). The workable form is the deny on true.

The catch, and it is a real one

The same section states:

A parameter the model omits is never matched, so Agent(model:*) doesn't match a call that leaves model unset

Agent's run_in_background defaults to true — this repo's own CLAUDE.md records that, from gha#392's measurement. So a deny on Agent(run_in_background:true) closes the path where the reviewer passes true explicitly, and does not close the path where it simply omits the parameter and gets the background default.

That matters for how the fix should be described. It is a genuine mechanical constraint on one of two routes to the same failure, not a complete one — and I would rather say so now than ship it as if it closed the issue.

Which route the observed runs took is not recoverable from the evidence in this issue: neither the run-32347489886 excerpt nor the gha#536/#539 tallies record whether run_in_background was passed. The execution artifact would show it (--verbose is what the docs point at for exact parameter names and values).

What I am picking up

1. The deny rule, as a partial-but-mechanical constraint, with its omission gap documented rather than glossed.
2. The second, still-open ask from gha#392 that this issue's last section carries — treating "final message names background agents" as its own outcome rather than a generic retryable stub. That one slots directly into the failure_kind vocabulary #548 just added, so the resulting PR comment can say "a same-prompt retry is unlikely to recover this" instead of silently retrying and spending again.

Two further constraints I will respect, both from this repo's own CLAUDE.md: a PR touching run-claude-review-attempt cannot self-verify before merge, and a new @v2 reference does not resolve until the tag slides.

---

Generated by Claude Code

[d-morrison]

Reopened — #550 shipped one of this issue's two halves, not both

#550 merged as 7f77056 and GitHub auto-closed this issue. That is wrong, and I've reopened it.

What actually landed:

• Agent(run_in_background:true) and its Task alias are denied in --disallowedTools, so the synchronous-only constraint is mechanical rather than a request. This closes the explicit-true route only — an omitted parameter is never matched and run_in_background defaults to true, so the prompt instruction stays and is not redundant.
• Those denials are excluded from the stub-retry gate, because a denial we engineer is not evidence the reviewer was starved.

What did not land — this issue's "Also unaddressed from gha#392 itself" section, quoted here since it is the reason the issue stays open:

Worth checking whether the guard can also treat "final message names background agents" as a distinct outcome from a generic stub, since the retry is unlikely to help and the spend is large.

That is #551, and the retry-gate exclusion is not it. The exclusion stops intended denials from mis-classifying a run; it does not give background-agent stubs their own failure_kind, so such a run is still retried into a second charge exactly as before. Deferred because the detection must key on a structured field rather than prose (this repo's own rule, from classify-gemini-failure.sh's header) and this session's network policy blocked the artifact that would settle which field.

Also still open: the second of the two routes above. A call that simply omits run_in_background still backgrounds, and no rule can match it.

Why this needed catching

issue-first.md names this exact failure — a PR closing an issue whose scope is wider than its diff, leaving the residual items "not deferred and not reopened — they are silently gone", and worse than an ordinary lost to-do because a closed issue is evidence the work was handled.

I used Refs #532 rather than Closes #532 in both the PR body and the squash commit message specifically to avoid this, and it happened anyway — the link was established earlier in the PR's life and survived the wording. So the lesson is narrower than the rule as written: choosing Refs is necessary and not sufficient, because GitHub's issue linkage is state on the PR rather than a function of the current text. Check the issue's actual state after the merge, not just the words you used before it.

---

Generated by Claude Code