You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
check-review-execution.sh deliberately separates two no-verdict shapes by denial count, and the separation did not hold on a real run: a retry whose own result logged permission_denials_count: 32 was still treated as gha#185's low-denial (retryable) case.
Distinct from the neighbouring issues, so filing separately rather than commenting:
max_denials="${STUB_RETRY_MAX_DENIALS:-5}"if [[ "$denials"-le"$max_denials" ]];thenecho"stub_review=true">>"$GITHUB_OUTPUT"elseecho"::warning::permission_denials_count=$denials exceeds the stub-retry threshold ($max_denials) — this looks like gha#198's pattern, not gha#185's; not marking as retryable."fi
32 -le 5 is false, so that ::warning:: should have been emitted and stub_review should not have been set. The warning does not appear anywhere in the job log. The only lines at that point are:
23:37:42.6742659Z ##[error]Claude review states no verdict (no '### Verdict' heading or 'Verdict:' line anywhere in its output) ...
23:37:42.6744853Z ##[error]Process completed with exit code 1.
And the "Resolve final review outcome" step then took its low-denial branch:
##[error]Attempt 1 produced a stub review (gha#185) and the retry ALSO ended
without a verdict with a low denial count — treating as a failed review.
Attempt 1 cost $4.3908, the retry $4.9769, total $9.3677 — for two reviews that produced no verdict.
Two candidate mechanisms, not yet separated
I could not settle which, because the execution artifact
(claude-review-execution-30499595673-1-attempt2, ID 8743143462) sits on productionresultssa5.blob.core.windows.net, which this session's egress proxy
refuses with CONNECT tunnel failed, response 403. Stating both rather than
guessing:
denials resolved to 0 rather than 32. It comes from jq -r '.permission_denials_count // 0' <<< "$result", so anything that
leaves $result empty or unparseable silently yields 0, which passes the -le 5 test and produces exactly the observed output: no warning, stub_review=true. The // 0 fallback makes a extraction failure
indistinguishable from a genuinely clean run — a fail-fast
smell independent of whichever mechanism turns out to be at fault here.
The resolve step's message is misattributed. It may be reporting
attempt 1's stub_review flag while describing the retry, in which case the
classification was right and only the sentence is wrong. fix(claude-code-review): retry once when a review completes with no verdict #201's review
already fixed one instance of exactly this misattribution class, so it is a
live possibility rather than a stretch.
Deciding between them needs one look at that artifact's last result event, or
a set -x on the denials assignment.
Why it matters beyond one run
The threshold exists specifically to avoid re-spending $2-4 on a pattern #198 documented as not recovering. On this run it re-spent $4.98 and did not
recover, which is the exact outcome the check was written to prevent. If
mechanism 1 is right, the threshold is inert whenever extraction fails, so the
guard has been weaker than its comment claims for as long as that fallback has
been there.
Suggested fix, pending the diagnosis
Fail loudly when permission_denials_count cannot be read, rather than
defaulting to 0 — a missing field and a zero count should not be the same
input to a spend decision.
Echo the resolved denials value (and max_denials) unconditionally, so the
classification is visible in the log without an artifact download.
If mechanism 2, key the resolve step's message on the attempt whose flag it is
actually reading.
Sidebar: the 32 denials themselves
Worth a look independently of the classification bug. 32 denials in an 18-turn
run means the reviewer spent much of its budget being refused tools. The
allowlist for this run had no Bash(gh api:*) and no Bash(git log:*)/Bash(git show:*), which a review of a workflow-heavy diff would plausibly reach for. That
may be the upstream cause of the missing verdict in the first place, and it is
the same escalating-denial shape #198 records.
check-review-execution.shdeliberately separates two no-verdict shapes by denial count, and the separation did not hold on a real run: a retry whose own result loggedpermission_denials_count: 32was still treated as gha#185's low-denial (retryable) case.Distinct from the neighbouring issues, so filing separately rather than commenting:
Install Bunskipped and$0.0000. Here both attempts ran to completion and billed real money.Evidence
Run 30499595673, job
review / claude-review, on #357 at head716ae36.The retry's own result object, logged immediately before
Log saved to .../claude-execution-output.json:{"type":"result","subtype":"success","is_error":false,"duration_ms":345210, "num_turns":18,"total_cost_usd":4.976871200000001,"permission_denials_count":32}The guard's threshold logic:
32 -le 5is false, so that::warning::should have been emitted andstub_reviewshould not have been set. The warning does not appear anywhere in the job log. The only lines at that point are:And the "Resolve final review outcome" step then took its low-denial branch:
Attempt 1 cost
$4.3908, the retry$4.9769, total$9.3677— for two reviews that produced no verdict.Two candidate mechanisms, not yet separated
I could not settle which, because the execution artifact
(
claude-review-execution-30499595673-1-attempt2, ID 8743143462) sits onproductionresultssa5.blob.core.windows.net, which this session's egress proxyrefuses with
CONNECT tunnel failed, response 403. Stating both rather thanguessing:
denialsresolved to 0 rather than 32. It comes fromjq -r '.permission_denials_count // 0' <<< "$result", so anything thatleaves
$resultempty or unparseable silently yields0, which passes the-le 5test and produces exactly the observed output: no warning,stub_review=true. The// 0fallback makes a extraction failureindistinguishable from a genuinely clean run — a
fail-fastsmell independent of whichever mechanism turns out to be at fault here.
attempt 1's
stub_reviewflag while describing the retry, in which case theclassification was right and only the sentence is wrong. fix(claude-code-review): retry once when a review completes with no verdict #201's review
already fixed one instance of exactly this misattribution class, so it is a
live possibility rather than a stretch.
Deciding between them needs one look at that artifact's last
resultevent, ora
set -xon thedenialsassignment.Why it matters beyond one run
The threshold exists specifically to avoid re-spending
$2-4on a pattern#198 documented as not recovering. On this run it re-spent
$4.98and did notrecover, which is the exact outcome the check was written to prevent. If
mechanism 1 is right, the threshold is inert whenever extraction fails, so the
guard has been weaker than its comment claims for as long as that fallback has
been there.
Suggested fix, pending the diagnosis
permission_denials_countcannot be read, rather thandefaulting to
0— a missing field and a zero count should not be the sameinput to a spend decision.
denialsvalue (andmax_denials) unconditionally, so theclassification is visible in the log without an artifact download.
actually reading.
Sidebar: the 32 denials themselves
Worth a look independently of the classification bug. 32 denials in an 18-turn
run means the reviewer spent much of its budget being refused tools. The
allowlist for this run had no
Bash(gh api:*)and noBash(git log:*)/Bash(git show:*), which a review of a workflow-heavy diff would plausibly reach for. Thatmay be the upstream cause of the missing verdict in the first place, and it is
the same escalating-denial shape #198 records.