One review panel per branch, and 200 turns for the fixer - #649
Merged
harrykim8672 merged 2 commits intoAug 3, 2026
Conversation
wafflebase#648 came out agent:blocked with a page saying "the fixer agent failed, so the requested changes were not applied". Both halves were wrong. agent-review-panel.yml had NO concurrency guard, so two CI completions close together started two full pipelines against the same branch. Two FIXERS then ran for 21 overlapping minutes on the same nine findings. The later one converged in 76 turns and pushed 102e0fa; the earlier one crossed its 80-turn ceiling and its job failed, which fired `stalled`. The changes HAD been applied -- by the other run, three minutes earlier, CI-green. The paged latch then froze the PR, so the commit that did land was never reviewed. $22.30 spent, about a third of it on a duplicate that could only lose. The same absent guard had already broken wafflebase#605 differently: two panels completing in the SAME SECOND with identical external_id and contradictory verdicts -- correctness 1 major vs 2 major, docs success vs failure, twelve check runs for six lenses, last writer wins. CANCEL, not queue: a superseded panel is reviewing a sha that is no longer the head, so its verdict is stale before it is written, and queueing pays for it in full and then discards it. Keyed on the head BRANCH, which is what the workflow_run payload carries -- the PR number is not resolved until `gate`. No run_id in the key; that is the mistake that makes a group match only itself and guard nothing. THE TRAP THIS CHANGE INTRODUCED, found before pushing: `stalled` ran under always(), and a panel cancelled before it starts reports `skipped` -- one of its own trigger conditions. It would have paged and latched the PR to agent:blocked at the exact moment a fresher round was starting, and the latch would then have stopped that round. Now `!cancelled()`, which still fires on every genuine failure since a failed dependency is not a cancellation. A spurious page is this job's worst outcome because the page is STICKY -- every other part of the pipeline reads it as "a human owns this now". Fixer --max-turns 80 -> 200, above agent-implement's 150 on purpose: the implementer writes a change it planned itself, while the fixer must first READ code it did not write at locations the panel chose, then fix every finding in one pass because the prompt tells it to converge in one round. At 80 it died at exactly 81 while the duplicate landed the same work in 76 -- that is a coin flip, not a margin. The ceiling is a runaway backstop, not a budget: MAX_REVIEW_ROUNDS and the 45-minute job timeout both bind well before 200 turns of useful work does. A page must not guess, so the fixer-failure reason now distinguishes THREE states: the branch advanced (applied, unreviewed, with the sha), it did not (nothing pushed), or the head could not be read (say that). The flat old sentence sent this investigation after an unresponsive fixer instead of an unreviewed commit. Also corrects a harness-engineering.md line claiming a paged PR still re-reviews on every CI-green push -- the gate's paged latch ended that. verify:self green 11/11, workflow parses. The three-state reason logic was extracted from the YAML and executed against wafflebase#648's real shape, a genuine no-push, and an unreadable head, since inline github-script JS can hold neither a test nor a linter. The remaining always() uses in this file are step-level, and the one that writes state (Post per-lens check runs) is self-healing because check runs resolve latest-per-name. NOT here: don't latch on a page whose premise the branch contradicts. That needs a decision about who may clear a latch. wafflebase#648 itself still needs unblocking by hand so one round can review 102e0fa. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 37 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe review panel workflow now cancels superseded runs for each PR head branch, increases the fixer turn limit, skips paging for cancelled runs, and classifies fixer failures by current branch state. Design and task documentation record these changes and deferred follow-up work. ChangesReview panel reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PRHead
participant PanelWorkflow
participant FixerClaude
participant FailurePager
PRHead->>PanelWorkflow: trigger review panel
PanelWorkflow->>PanelWorkflow: cancel superseded branch run
PanelWorkflow->>FixerClaude: run fixer with 200-turn limit
FixerClaude-->>PanelWorkflow: fixer result
PanelWorkflow->>FailurePager: compare reviewed SHA with current PR head
FailurePager-->>PRHead: report branch-state-specific outcome
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/agent-review-panel.yml:
- Around line 1434-1453: Wrap the current-head PR lookup and head SHA extraction
used before advanced classification in try/catch, setting advanced to null when
pulls.list or related map access fails so the “could not read the branch head”
message is emitted. Reuse the PR number already emitted by gate for both the
returned PR and head lookup, avoiding a second PR resolution.
- Around line 1454-1459: Update the stalled-reporting logic that builds
fixReason so it does not claim the fixer pushed changes solely because the
branch advanced. Use the fixer-step result and recorded pre-fix SHA when
available to establish provenance; otherwise describe the branch movement as
unknown and avoid attributing it to the fixer. Preserve distinct messaging for
confirmed fixer failure with no push and for unreadable branch state.
- Around line 1381-1394: Move the stuck-check cleanup out of the `stalled` job
into a separate cancellation-safe cleanup job that runs for superseded or
cancelled panel runs and closes any `agent-review-*` checks left in progress for
the old SHA. Keep the existing `stalled` paging condition under `!cancelled()`
and preserve its genuine-failure behavior; update dependencies and finalization
references as needed so cleanup still executes when `stalled` is skipped.
- Around line 53-55: Update the workflow-level concurrency group to include
github.event.workflow_run.head_repository.full_name alongside head_branch,
ensuring runs from different source repositories cannot share a cancellation key
while preserving cancel-in-progress behavior.
In `@docs/tasks/active/20260804-panel-concurrency-todo.md`:
- Line 82: Update the line beginning with “#648” in the task document so the
issue reference is written as normal text rather than a Markdown heading
trigger, while preserving the existing wording and reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a35e2fc9-7b86-485c-a8d8-43774741b978
📒 Files selected for processing (3)
.github/workflows/agent-review-panel.ymldocs/design/harness-engineering.mddocs/tasks/active/20260804-panel-concurrency-todo.md
Comment on lines
+1434
to
+1453
| // Did the branch move? `workflow_run.head_sha` is the sha this panel | ||
| // reviewed; `pr.head.sha` is the branch NOW. A fix job can fail with | ||
| // the changes already pushed — its own commit landed and a later step | ||
| // died, or (before the concurrency guard above) a duplicate run pushed | ||
| // while this one burned its turn ceiling. | ||
| // | ||
| // On #648 the flat "the requested changes were not applied" was simply | ||
| // FALSE: `102e0fa73` was on the branch and CI-green three minutes | ||
| // before this comment was written. That sentence then sent a human | ||
| // looking for an unresponsive fixer instead of an unreviewed commit, | ||
| // and the paged latch meant nothing ever reviewed it. A page is the | ||
| // one message a human is guaranteed to read, so it is the last place | ||
| // that should guess. | ||
| // Three states, not two. "We could not read the head" must not print | ||
| // as "pushed nothing" — that is the same false certainty in a | ||
| // different place, and it would be wrong exactly when a human most | ||
| // needs to know where to look. | ||
| const reviewedSha = context.payload.workflow_run.head_sha; | ||
| const headSha = typeof pr.head?.sha === 'string' ? pr.head.sha : ''; | ||
| const advanced = headSha === '' ? null : headSha !== reviewedSha; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)agent-review-panel\.yml$|agent-review-panel'
echo
echo "Relevant section:"
sed -n '1380,1485p' .github/workflows/agent-review-panel.yml
echo
echo "Search for reads of pr.head.sha and pulls/list usages:"
rg -n "context\.payload\.workflow_run\.head_sha|pulls\.list|pr\.head|advanced|workflow_run|gate" .github/workflows/agent-review-panel.ymlRepository: wafflebase/wafflebase
Length of output: 11904
Handle current-head read failures before the three-state classification.
pulls.list runs before advanced is computed, so a transient read error exits the step without leaving advanced === null. Add a try/catch around the current head lookup and map lookup failures to advanced === nullso the “could not read the branch head” message is emitted for failed reads. Reuse the PR number emitted bygate` for the returned PR and head lookup to avoid resolving the same PR twice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/agent-review-panel.yml around lines 1434 - 1453, Wrap the
current-head PR lookup and head SHA extraction used before advanced
classification in try/catch, setting advanced to null when pulls.list or related
map access fails so the “could not read the branch head” message is emitted.
Reuse the PR number already emitted by gate for both the returned PR and head
lookup, avoiding a second PR resolution.
Comment on lines
+1454
to
+1459
| const fixReason = | ||
| advanced === null | ||
| ? "the fixer agent's job failed, and this run could not read the branch head to tell whether anything was pushed — check the branch before assuming the changes are missing" | ||
| : advanced | ||
| ? `the fixer agent's job failed, but the branch DID advance to \`${headSha.slice(0, 9)}\` — changes were applied and have NOT been reviewed (a common cause is the fixer exhausting its turn budget after pushing)` | ||
| : 'the fixer agent failed and pushed nothing, so the requested changes were not applied'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists =="
git ls-files | grep -F '.github/workflows/agent-review-panel.yml' || true
echo "== relevant lines =="
sed -n '1380,1485p' .github/workflows/agent-review-panel.yml | nl -ba -v1380
echo "== searches for workflow_run.head_sha, r.fix, advanced, headSha, pre-fix =="
rg -n "workflow_run\.head_sha|r\.fix|advanced|headSha|pre-fix|reviewed|stalled|stalled reporting|runId|workflow_dispatch|Agent Review" .github/workflows/agent-review-panel.ymlRepository: wafflebase/wafflebase
Length of output: 279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant lines =="
sed -n '1380,1485p' .github/workflows/agent-review-panel.yml
echo "== searches for workflow_run.head_sha, r.fix, advanced, headSha, pre-fix, stalled =="
rg -n "workflow_run\.head_sha|r\.fix|advanced|headSha|pre-fix|reviewed|stalled|runId|workflow_dispatch|Agent Review" .github/workflows/agent-review-panel.ymlRepository: wafflebase/wafflebase
Length of output: 10161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow jobs outline around fix/stalled =="
rg -n "name: (check|deps|review-panel|promote|fix|Reconcile agent state|Page a human|Reconcile|stalled)" .github/workflows/agent-review-panel.yml
echo "== job names/results and fix/job dependencies =="
sed -n '1,120p' .github/workflows/agent-review-panel.yml
rlimit=`wc -l < .github/workflows/agent-review-panel.yml`; echo "total lines: $rlimit"
sed -n '800,1340p' .github/workflows/agent-review-panel.yml | sed -n '/name: fix|name: promote|name: review-panel|needs:/,/_fixer\|checkout\|npx\|dependency\|install/p'Repository: wafflebase/wafflebase
Length of output: 6895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== jobs block =="
sed -n '125,260p' .github/workflows/agent-review-panel.yml
echo "== fix job block and relevant step names =="
sed -n '1130,1280p' .github/workflows/agent-review-panel.ymlRepository: wafflebase/wafflebase
Length of output: 15867
Do not attribute every branch move to the fixer.
r.fix === 'failure' also covers failures before Address panel findings, including Review-round guard, token creation, checkout, dependency installation, and state setup. In addition, advanced compares the current PR head with workflow_run.head_sha, not with the recorded pre-fix SHA. A human push or another run can move the branch from the reviewed SHA without the fixer running or pushing. Pass the fixer-step result and pre-fix SHA into stalled reporting, or use wording that leaves push provenance unknown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/agent-review-panel.yml around lines 1454 - 1459, Update
the stalled-reporting logic that builds fixReason so it does not claim the fixer
pushed changes solely because the branch advanced. Use the fixer-step result and
recorded pre-fix SHA when available to establish provenance; otherwise describe
the branch movement as unknown and avoid attributing it to the fixer. Preserve
distinct messaging for confirmed fixer failure with no push and for unreadable
branch state.
| worth one more panel round. It needs a decision about who may clear a latch, which | ||
| is a trust question rather than a bug fix. | ||
|
|
||
| #648 itself still needs unblocking by hand so one round can review `102e0fa73`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Markdownlint warning.
#648 at the start of the line triggers MD018. Write the issue reference as normal text.
Suggested change
-#648 itself still needs unblocking by hand so one round can review `102e0fa73`.
+Issue `#648` still needs unblocking by hand so one round can review `102e0fa73`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #648 itself still needs unblocking by hand so one round can review `102e0fa73`. | |
| Issue `#648` still needs unblocking by hand so one round can review `102e0fa73`. |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 82-82: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tasks/active/20260804-panel-concurrency-todo.md` at line 82, Update the
line beginning with “#648” in the task document so the issue reference is
written as normal text rather than a Markdown heading trigger, while preserving
the existing wording and reference.
Source: Linters/SAST tools
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Review response on wafflebase#649. Three fixed, two skipped with reasons. A FORK COULD CANCEL A LEGITIMATE PANEL. `concurrency` is evaluated at the workflow level, BEFORE `gate` applies its head_repository check, so a key of head_branch alone let anyone fork this PUBLIC repo, open a PR from a branch named to match an in-flight agent branch, and have their CI completion cancel the real panel -- denial of review from an unprivileged position, and the cancelling run is then skipped by the gate, so nothing records the review that should have happened. The group now includes head_repository.full_name. Same threat model the paged latch in `gate` already reasons about out loud. MOVING `stalled` TO !cancelled() STRANDED THE STUCK-CHECK CLEANUP. That step exists for the "panel KILLED mid-run (timeout/cancel)" case, in its own words, and rode on `stalled`'s always(). Taking `stalled` off always() removed it at exactly the moment the concurrency guard made cancellation the COMMON outcome, so every superseded round would have left six agent-review-* checks spinning forever. Split into a close-stuck-checks job that runs on cancellation, never pages, never labels, and holds checks:write alone. A separate job rather than another condition because the two need OPPOSITE cancellation behaviour -- paging must not happen on a cancelled run and cleanup must -- which is precisely why bundling them regressed. MY OWN FIX STILL ATTRIBUTED THE PUSH. "the branch DID advance ... changes were applied" claims the FIXER pushed the REQUESTED fix, and this run can observe neither: a human push and a concurrent run's fixer are indistinguishable from here. That is the same unproven claim the old sentence made, pointing the other way. It now states both shas and says a commit landed unreviewed, without saying who or what. SKIPPED: wrapping the pulls.list lookup in try/catch and reusing gate's PR number. The `advanced` computation cannot throw (typeof pr.head?.sha === 'string'), so the new path is covered; a pulls.list throw failing the step is pre-existing behaviour, and gate emits the PR number but not the head sha this needs, so reusing it would still cost a second call. SKIPPED: `wafflebase#648` at line start rendering as a heading. CommonMark requires whitespace after the # run, so it is not an ATX heading -- GitHub follows CommonMark and auto-links it, which is the intent. verify:self green 11/11, workflow parses, 7 jobs resolve. The reason logic was re-executed against all three shapes and asserted to carry no unproven attribution. Both docs re-synced: four lines went stale with these fixes and are corrected rather than left describing the previous shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
harrykim8672
approved these changes
Aug 3, 2026
harrykim8672
left a comment
harrykim8672
left a comment
Contributor
There was a problem hiding this comment.
Thanks for your contribution
6 tasks
6 tasks
hwisoo-kim
added a commit
to hwisoo-kim/wafflebase
that referenced
this pull request
Aug 4, 2026
`@claude rerun` on wafflebase#632 and wafflebase#648 said "Re-running CI now; the review panel will run again". CI genuinely re-ran -- run_started_at 06:01, success at 06:15 -- and NO panel run was created. Both PRs sat with stale verdicts and no fixer, twice, with nothing reporting a failure. A RE-RUN DOES NOT EMIT workflow_run: requested. That fires when a run is CREATED, and a re-run reuses the run id, so only `completed` fires. The panel's trigger became `requested`-only in wafflebase#651 -- correctly, for the latency win -- and `@claude rerun`'s entire mechanism is reRunWorkflow on the PR's CI run. The command was uncoupled from the trigger it depends on without either being touched. Confirmed rather than inferred: at 06:15, agent-iterate-ci.yml -- which listens to `completed` -- fired for both PRs, and no Agent Review Panel run exists anywhere in the window (checked by creation time and by completion time, paginated). The panel now subscribes to [requested, completed], and the gate admits `completed` ONLY when run_attempt > 1: requested / attempt 1 -> admit fresh CI, the parallel-start path completed / attempt 1 -> refuse requested already started this round completed / attempt 2+ -> admit a re-run, where requested never fires So one panel per CI run, and subscribing to both does not double the ~$12 round. Two existing properties make that safe rather than merely intended: the concurrency group from wafflebase#649 cancels a superseded run, so even if `requested` did fire on a re-run the pair collapses to one; and the `ci` job's decide() returns 'proceed' immediately for an already-completed successful run, so the completed-triggered path does not park waiting for a conclusion it already has. wafflebase#651's own guard asserted `types: [requested]` as a literal, with the reasoning that subscribing to both doubles every round. That reasoning held for a fresh run and missed re-runs entirely. The test now pins the stronger contract -- requested present, completed present, and the gate clause that prevents the doubling -- so what stops the cost is asserted where it actually lives. Mutation-tested: removing the gate clause fails it. The four trigger cases were evaluated by extracting the gate expression from the YAML and running it, since a workflow `if:` holds no test. verify:self green 11/11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hwisoo-kim
added a commit
to hwisoo-kim/wafflebase
that referenced
this pull request
Aug 4, 2026
`@claude rerun` on wafflebase#632 and wafflebase#648 said "Re-running CI now; the review panel will run again". CI genuinely re-ran -- run_started_at 06:01, success at 06:15 -- and NO panel run was created. Both PRs sat with stale verdicts and no fixer, twice, with nothing reporting a failure. A RE-RUN DOES NOT EMIT workflow_run: requested. That fires when a run is CREATED, and a re-run reuses the run id, so only `completed` fires. The panel's trigger became `requested`-only in wafflebase#651 -- correctly, for the latency win -- and `@claude rerun`'s entire mechanism is reRunWorkflow on the PR's CI run. The command was uncoupled from the trigger it depends on without either being touched. Confirmed rather than inferred: at 06:15, agent-iterate-ci.yml -- which listens to `completed` -- fired for both PRs, and no Agent Review Panel run exists anywhere in the window (checked by creation time and by completion time, paginated). The panel now subscribes to [requested, completed], and the gate admits `completed` ONLY when run_attempt > 1: requested / attempt 1 -> admit fresh CI, the parallel-start path completed / attempt 1 -> refuse requested already started this round completed / attempt 2+ -> admit a re-run, where requested never fires So one panel per CI run, and subscribing to both does not double the ~$12 round. Two existing properties make that safe rather than merely intended: the concurrency group from wafflebase#649 cancels a superseded run, so even if `requested` did fire on a re-run the pair collapses to one; and the `ci` job's decide() returns 'proceed' immediately for an already-completed successful run, so the completed-triggered path does not park waiting for a conclusion it already has. reasoning that subscribing to both doubles every round. That reasoning held for a fresh run and missed re-runs entirely. The test now pins the stronger contract -- requested present, completed present, and the gate clause that prevents the doubling -- so what stops the cost is asserted where it actually lives. Mutation-tested: removing the gate clause fails it. The four trigger cases were evaluated by extracting the gate expression from the YAML and running it, since a workflow `if:` holds no test. verify:self green 11/11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
harrykim8672
pushed a commit
that referenced
this pull request
Aug 4, 2026
harvest.mjs keyed "when did the panel let go of this PR" off the <!-- agent-handoff --> comment, with the ready_for_review timeline event as a fallback, and then read "what had the panel concluded" THROUGH that cutoff. Four problems, and the fourth is the one that cost data. - No marker, no harvest. mark-ready.mjs posts the marker inside a try/catch AFTER flipping the PR ready, so a genuinely promoted PR can carry none. Measured: 18 of 33 agent PRs have neither a marker nor a ready_for_review event, having been opened ready rather than promoted. - ready_for_review answers a different question. It fires whether or not the panel ever spoke, so on a manually-readied PR every later human commit became a candidate. It over-fired on the PRs it covered and covered none of the PRs it was reached for. - A rebase erased it. The cutoff was a comment timestamp compared against COMMITTER DATES, so a force-push after promotion pushed every commit past it, emptied beforeHandoff, and left headAtHandoff undefined. - Signature 2 read the panel through signature 1's cutoff. The comparison set came from whichever commit was head at hand-off, so no hand-off meant no set and every CodeRabbit candidate withheld on a PR whose check runs were readable the whole time -- and one set was applied to every finding regardless of which commit it was about. Signature 2 never needed a hand-off: "CodeRabbit flagged something our panel reviewed and did not raise" is exactly as true on a human PR reviewed via @claude review, where nothing was handed off at all. The panel's own check runs are the only signal that is present whenever the panel ran, carries a server timestamp, and means "the panel approved". completed_at is stamped by GitHub, so the rebase that rewrites every committer date on the PR cannot move it. panelRounds walks the commit list and yields one round per commit the panel CONCLUDED on; panelApprovedAt takes the cutoff from those; roundsUpTo resolves a comparison set PER CodeRabbit COMMENT, from the rounds at or before the commit that comment is about. ready_for_review is removed outright rather than demoted. Keeping it as a last resort would preserve the over-fire in exactly the cases nothing else can check. Its timeline request is gone with it. The marker stays as a fallback for a PR with no readable round, because it does mean the panel approved -- it is only unreliably present. An @claude review is a round too (commentRounds), with conclusion "" -- not a missing value but the fact that it reached no gate conclusion, which is also what keeps it out of panelApprovedAt. It used to be a whole-PR fallback reached only when check runs were absent; as a round it gets the same per-commit treatment the gating arm does, and gating and advisory rounds are now UNIONED rather than ranked. The record's claim is "the panel did not raise this", and both arms run the same lens panel, so a finding either raised is a finding the panel raised. Ranking made sense when the question was which rendering of ONE round to trust; across rounds it discards real findings and files them as misses. panelSaw still prefers the gating round, which is where "check runs are the structured record" is actually observable. Two phases, because establishing the history is cheap and reading it is not. panelRoundAt splits out the half answerable from the check-runs LIST response (conclusion, reviewedSha, completedAt); the per-run withFullOutput refetch that carries output.text runs only for rounds a candidate is actually compared against, cached by sha. panelVerdictAt now calls panelRoundAt, so the aggregate conclusion rule ("one failing lens means the panel did not let it through") stays in one place instead of growing a second copy for the cheap path. Corrected while building, and only real data caught it: the first implementation took the NEWEST all-success round. Every human fix opens a new round, and that round approves too -- so on #548 the cutoff landed on 2026-07-28, four days after the three human fixes of 2026-07-25, and lost all three, including rows a human has already curated into misses.jsonl. It is the FIRST approval, matching markerHandoffAt's own "FIRST marker, not the last": a later re-approval does not un-say the first one. An unreadable commit yields an `unreadable: true` round rather than nothing, which keeps the property the previous code deliberately had -- "we know which commit the panel was looking at and we could not read what it concluded" -- and makes the comparison set refuse rather than resolve to [], which would file every CodeRabbit finding on the PR as a miss against a verdict nobody has seen. One rule covers both unplaceable directions: what cannot be placed cannot be excluded. No schema change. wafflebase/miss@1, same fields, same order; handoffAt keeps its name and now means "when the panel approved", which is what every consumer already assumed. Ids are unchanged, so dedupeById's first-wins rule still protects every curated verifiedBy. 676 agent tests green, 662 on main; eslint scripts clean under the lockfile's pinned 9.24.0. Real PRs before and after, read-only: candidate sets IDENTICAL across #548, #559, #578, #582, #591, #649 and the no-marker population #581, #590, #605, #652, with #548 re-harvesting to the same four ids. API cost measured at 13 to 24 calls on a 9-commit PR. Stated plainly because the plan predicted otherwise: there is NO measurable gain on today's data. #581, #590 and #605 have no marker, but the panel never approved them either, and they carry no CodeRabbit blocking findings -- so neither signature was unlocked. The gains are demonstrated synthetically. What this buys is a mechanism that is correct for the next PR; the rebase case and the per-comment anchoring are latent bugs rather than observed losses. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
hwisoo-kim
added a commit
to hwisoo-kim/wafflebase
that referenced
this pull request
Aug 4, 2026
The review panel caught this and it is the worst thing in this branch: as pushed, subscribing to `completed` DISABLED THE REVIEW PANEL on every PR. `concurrency` is claimed at the WORKFLOW level -- at run creation, before any job's `if:` is evaluated. With one undifferentiated group, a fresh CI run's `completed` event creates a second run that lands in the same `cancel-in-progress` group and KILLS the panel the `requested` event started ~13 minutes earlier, mid-review. The second run then refuses the event and skips every job, so no verdicts are recorded -- and `stalled` is `!cancelled()`, added in wafflebase#649, so nothing pages either. The panel takes ~14 min against CI's ~13, so the collision is near-certain rather than occasional. A job-level `if:` cannot protect a group already claimed on its behalf. My commit message cited that same concurrency guard as the reason the change was SAFE. It was the mechanism of the failure. I reasoned only about a re-run emitting both events and never about the fresh-run case the timing makes routine. The group now carries a suffix. Runs that will be refused (`completed` on attempt 1) land in a `noop` lane where cancelling each other costs nothing; every run that will actually review shares `active`, which keeps what the guard exists for -- wafflebase#605's two same-second contradictory verdicts, wafflebase#648's two fixers on one branch for 21 minutes. That puts the admission rule in TWO places, so checks.test.mjs now extracts both the gate `if:` and the group suffix from the YAML, evaluates them across all four event/attempt combinations, and fails if they disagree. It also asserts the partition is not degenerate: a group that always answered `active` would pass a same-answer check while restoring the bug. Mutation-tested by removing the suffix. The design doc's claim that concurrency made this safe is corrected rather than deleted -- it was wrong in a way worth recording. Also fixes three violations the wafflebase#658 lint caught in this branch's own new test code: a literal double-space in a regex, and two eslint-disable directives for a rule that is not enabled. verify:self green 11/11, eslint scripts clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6 tasks
harrykim8672
pushed a commit
to hwisoo-kim/wafflebase
that referenced
this pull request
Aug 4, 2026
`@claude rerun` on wafflebase#632 and wafflebase#648 said "Re-running CI now; the review panel will run again". CI genuinely re-ran -- run_started_at 06:01, success at 06:15 -- and NO panel run was created. Both PRs sat with stale verdicts and no fixer, twice, with nothing reporting a failure. A RE-RUN DOES NOT EMIT workflow_run: requested. That fires when a run is CREATED, and a re-run reuses the run id, so only `completed` fires. The panel's trigger became `requested`-only in wafflebase#651 -- correctly, for the latency win -- and `@claude rerun`'s entire mechanism is reRunWorkflow on the PR's CI run. The command was uncoupled from the trigger it depends on without either being touched. Confirmed rather than inferred: at 06:15, agent-iterate-ci.yml -- which listens to `completed` -- fired for both PRs, and no Agent Review Panel run exists anywhere in the window (checked by creation time and by completion time, paginated). The panel now subscribes to [requested, completed], and the gate admits `completed` ONLY when run_attempt > 1: requested / attempt 1 -> admit fresh CI, the parallel-start path completed / attempt 1 -> refuse requested already started this round completed / attempt 2+ -> admit a re-run, where requested never fires So one panel per CI run, and subscribing to both does not double the ~$12 round. Two existing properties make that safe rather than merely intended: the concurrency group from wafflebase#649 cancels a superseded run, so even if `requested` did fire on a re-run the pair collapses to one; and the `ci` job's decide() returns 'proceed' immediately for an already-completed successful run, so the completed-triggered path does not park waiting for a conclusion it already has. reasoning that subscribing to both doubles every round. That reasoning held for a fresh run and missed re-runs entirely. The test now pins the stronger contract -- requested present, completed present, and the gate clause that prevents the doubling -- so what stops the cost is asserted where it actually lives. Mutation-tested: removing the gate clause fails it. The four trigger cases were evaluated by extracting the gate expression from the YAML and running it, since a workflow `if:` holds no test. verify:self green 11/11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
harrykim8672
pushed a commit
to hwisoo-kim/wafflebase
that referenced
this pull request
Aug 4, 2026
The review panel caught this and it is the worst thing in this branch: as pushed, subscribing to `completed` DISABLED THE REVIEW PANEL on every PR. `concurrency` is claimed at the WORKFLOW level -- at run creation, before any job's `if:` is evaluated. With one undifferentiated group, a fresh CI run's `completed` event creates a second run that lands in the same `cancel-in-progress` group and KILLS the panel the `requested` event started ~13 minutes earlier, mid-review. The second run then refuses the event and skips every job, so no verdicts are recorded -- and `stalled` is `!cancelled()`, added in wafflebase#649, so nothing pages either. The panel takes ~14 min against CI's ~13, so the collision is near-certain rather than occasional. A job-level `if:` cannot protect a group already claimed on its behalf. My commit message cited that same concurrency guard as the reason the change was SAFE. It was the mechanism of the failure. I reasoned only about a re-run emitting both events and never about the fresh-run case the timing makes routine. The group now carries a suffix. Runs that will be refused (`completed` on attempt 1) land in a `noop` lane where cancelling each other costs nothing; every run that will actually review shares `active`, which keeps what the guard exists for -- wafflebase#605's two same-second contradictory verdicts, wafflebase#648's two fixers on one branch for 21 minutes. That puts the admission rule in TWO places, so checks.test.mjs now extracts both the gate `if:` and the group suffix from the YAML, evaluates them across all four event/attempt combinations, and fails if they disagree. It also asserts the partition is not degenerate: a group that always answered `active` would pass a same-answer check while restoring the bug. Mutation-tested by removing the suffix. The design doc's claim that concurrency made this safe is corrected rather than deleted -- it was wrong in a way worth recording. Also fixes three violations the wafflebase#658 lint caught in this branch's own new test code: a literal double-space in a regex, and two eslint-disable directives for a rule that is not enabled. verify:self green 11/11, eslint scripts clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
harrykim8672
pushed a commit
that referenced
this pull request
Aug 4, 2026
* Grant pull-requests:write where labels are actually written `@claude rerun` on #632 and #648 announced that it had dropped agent:blocked. Both stayed blocked. The run logs say why: ##[warning]Could not remove agent:blocked (Resource not accessible by integration). LABELS ON A PULL REQUEST NEED pull-requests:write. A PR is an issue for most of the API and its COMMENTS are reachable with issues:write -- which is why the paged-comment deletion in the same step succeeded, reporting "cleared 1 paged marker(s)" -- but its LABELS are not. The job held pull-requests:read. AND THE SUMMARY CLAIMED THE DROP REGARDLESS. The old code warned on failure and then built the message unconditionally, so a human read "dropped agent:blocked" with the label sitting right next to it. It now reports which of dropped / was not set / could not drop actually happened; the three branches were extracted from the YAML and executed, since inline github-script JS holds no test. THE SAME GRANT WAS MISSING FROM review-panel, and there it has been silently dead since it was written: agent:reviewing has NEVER appeared on a PR. set-state.mjs is fail-safe -- any API error logs and exits 0, which is right for a label that gates nothing -- so the step reported success every time. Confirmed by timeline rather than assumed: #648, #632, #605 and #633 all go implementing -> fixing -> blocked with no reviewing state between them. The comment above that grant claimed "labels use the issues API", which is the misconception itself, and is now corrected. Widening review-panel is safe on the argument its issues:write already rested on: the SDK step receives no GitHub token (its env is CLAUDE_CODE_OAUTH_TOKEN only), so neither grant widens what prompt-injected branch code can reach. Enumerated rather than guessed. Jobs that write labels with the default GITHUB_TOKEN: `iterate` inherits the grant workflow-level, `fix` and `stalled` declare it, and these two were the only gaps. An earlier scan also flagged agent-implement, wrongly -- its label write happens inside the agent's PROMPT using the App token, whose installation permissions the workflow block does not govern, and #648's timeline shows agent:implementing applied successfully by yorkie-agent[bot]. The stale labels on #632 and #648 were removed by hand; both PRs' paged markers were already gone, so the label was the only thing left wrong. verify:self green 11/11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Re-engage the panel on a CI re-run, not just a fresh one `@claude rerun` on #632 and #648 said "Re-running CI now; the review panel will run again". CI genuinely re-ran -- run_started_at 06:01, success at 06:15 -- and NO panel run was created. Both PRs sat with stale verdicts and no fixer, twice, with nothing reporting a failure. A RE-RUN DOES NOT EMIT workflow_run: requested. That fires when a run is CREATED, and a re-run reuses the run id, so only `completed` fires. The panel's trigger became `requested`-only in #651 -- correctly, for the latency win -- and `@claude rerun`'s entire mechanism is reRunWorkflow on the PR's CI run. The command was uncoupled from the trigger it depends on without either being touched. Confirmed rather than inferred: at 06:15, agent-iterate-ci.yml -- which listens to `completed` -- fired for both PRs, and no Agent Review Panel run exists anywhere in the window (checked by creation time and by completion time, paginated). The panel now subscribes to [requested, completed], and the gate admits `completed` ONLY when run_attempt > 1: requested / attempt 1 -> admit fresh CI, the parallel-start path completed / attempt 1 -> refuse requested already started this round completed / attempt 2+ -> admit a re-run, where requested never fires So one panel per CI run, and subscribing to both does not double the ~$12 round. Two existing properties make that safe rather than merely intended: the concurrency group from #649 cancels a superseded run, so even if `requested` did fire on a re-run the pair collapses to one; and the `ci` job's decide() returns 'proceed' immediately for an already-completed successful run, so the completed-triggered path does not park waiting for a conclusion it already has. reasoning that subscribing to both doubles every round. That reasoning held for a fresh run and missed re-runs entirely. The test now pins the stronger contract -- requested present, completed present, and the gate clause that prevents the doubling -- so what stops the cost is asserted where it actually lives. Mutation-tested: removing the gate clause fails it. The four trigger cases were evaluated by extracting the gate expression from the YAML and running it, since a workflow `if:` holds no test. verify:self green 11/11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Partition the concurrency group, or `completed` kills the panel The review panel caught this and it is the worst thing in this branch: as pushed, subscribing to `completed` DISABLED THE REVIEW PANEL on every PR. `concurrency` is claimed at the WORKFLOW level -- at run creation, before any job's `if:` is evaluated. With one undifferentiated group, a fresh CI run's `completed` event creates a second run that lands in the same `cancel-in-progress` group and KILLS the panel the `requested` event started ~13 minutes earlier, mid-review. The second run then refuses the event and skips every job, so no verdicts are recorded -- and `stalled` is `!cancelled()`, added in #649, so nothing pages either. The panel takes ~14 min against CI's ~13, so the collision is near-certain rather than occasional. A job-level `if:` cannot protect a group already claimed on its behalf. My commit message cited that same concurrency guard as the reason the change was SAFE. It was the mechanism of the failure. I reasoned only about a re-run emitting both events and never about the fresh-run case the timing makes routine. The group now carries a suffix. Runs that will be refused (`completed` on attempt 1) land in a `noop` lane where cancelling each other costs nothing; every run that will actually review shares `active`, which keeps what the guard exists for -- #605's two same-second contradictory verdicts, #648's two fixers on one branch for 21 minutes. That puts the admission rule in TWO places, so checks.test.mjs now extracts both the gate `if:` and the group suffix from the YAML, evaluates them across all four event/attempt combinations, and fails if they disagree. It also asserts the partition is not degenerate: a group that always answered `active` would pass a same-answer check while restoring the bug. Mutation-tested by removing the suffix. The design doc's claim that concurrency made this safe is corrected rather than deleted -- it was wrong in a way worth recording. Also fixes three violations the #658 lint caught in this branch's own new test code: a literal double-space in a regex, and two eslint-disable directives for a rule that is not enabled. verify:self green 11/11, eslint scripts clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Harry Kim <309633041+hwisoo-kim@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two fixes from investigating why #648 came out
agent:blocked. Neither thelabel nor the page described what actually happened.
What broke on #648
agent-review-panel.ymlhad noconcurrencyguard, so two CI completionsclose together started two full pipelines against
agent/586-cli-system-exit-code:30803409664Reached maximum number of turns (80)30803718950102e0fa73Two fixer agents, 21 overlapping minutes, the same nine findings. B converged
and pushed. A crossed its ceiling and its job failed, which fired
stalled:That sentence was false.
102e0fa73was on the branch and CI-green three minutesearlier. The paged latch then correctly froze the PR — so the commit that did
land was never reviewed. $22.30 spent, roughly a third of it on a duplicate that
could only lose.
The same absent guard had already broken #605 with a different symptom: two
panels completing in the same second with identical
external_idandcontradictory verdicts — correctness 1 major vs 2 major, docs success vs failure,
twelve check runs for six lenses, last writer wins.
The changes
1.
concurrencyon the panel,cancel-in-progress: true.Cancel rather than queue: a superseded panel is reviewing a sha that is no longer
the head, so its verdict is stale before it is written, and queueing pays for it in
full and then discards it. Keyed on the head branch — the PR number isn't
resolved until the
gatejob. Norun_idin the key; that's the mistake that makesa group match only itself and guard nothing.
2. Fixer
--max-turns80 → 200.Above
agent-implement's 150 on purpose. The implementer writes a change it planneditself; the fixer must first read code it did not write, at locations the panel
chose, then fix every finding in one pass because the prompt tells it to converge in
one round. At 80 it died at exactly 81 while the duplicate landed the same work in
76 — a coin flip, not a margin. The ceiling is a runaway backstop, not a budget:
MAX_REVIEW_ROUNDSand the 45-minute job timeout both bind well before 200 turns ofuseful work does.
3. The page no longer guesses. On a fixer failure it distinguishes three states:
the branch advanced (applied, unreviewed, naming the sha), it did not (nothing
pushed), or the head couldn't be read (says so).
The trap this change introduced
Caught before pushing, and it's the part most worth reviewing.
stalledran underalways(). Withcancel-in-progressnow cancellingsuperseded runs, a panel cancelled before it starts reports
skipped— which isone of
stalled's own trigger conditions. It would have paged and latched the PR toagent:blockedat the exact moment a fresher round was starting, and the latchwould then have stopped that round from running.
Now
!cancelled(), which still fires on every genuine failure — a failed dependencyis not a cancellation. A spurious page is this job's worst outcome because the page
is sticky: every other part of the pipeline reads it as "a human owns this now".
I also checked the remaining
always()uses in the file. All step-level, and the onethat writes state (
Post per-lens check runs) is self-healing — a superseded run'slens checks get overwritten, since check runs resolve latest-per-name.
Verification
pnpm verify:selfgreen (11/11) · workflow parses.github-scriptJS can hold neither a test nor a linter, so it was run againstCLI: classify failures so system errors exit 2 #648's real shape (head moved → "applied and NOT reviewed"), a genuine no-push, and
an unreadable head. The unknown case says it's unknown rather than falling back to
"pushed nothing", which would be the same false certainty in a new place.
What I'd push back on as a reviewer
200 is a judgement, not a measurement. The only data points are 76 (converged)
and 81 (died); nothing says where the real ceiling should be. It's deliberately far
above both so the ceiling stops being the binding constraint — the round cap and job
timeout are the intended bounds — but if fixer runs start costing noticeably more per
round, this is the number to revisit.
cancel-in-progresscan kill a fixer mid-work. That's intended (its base isstale), and safe because the fixer only ever appends a commit. But a cancelled fixer
that had already pushed leaves a commit whose round never completed — the push
re-triggers CI and a fresh panel, so it converges, just not for free.
Also in here
One stale line in
harness-engineering.mdclaiming a paged PR still re-reviews onevery CI-green push. The gate's paged latch ended that, and the sentence sat directly
above what I was editing.
Deliberately NOT in this PR
The third finding from the investigation: don't latch on a page whose premise the
branch contradicts. If the head advanced and CI is green after a page, that's worth
one more panel round. It needs a decision about who may clear a latch — a trust
question, not a bug fix.
#648 itself still needs unblocking by hand so one round can review
102e0fa73.Nothing here does that.
Linked Issues
None — from the #648 post-mortem.
Author checklist
Verification checklist
Risk Assessment
only reduces what runs; the turn ceiling and the page wording change no scope.
concurrencyblock restores the previous (racy) behaviour exactly.
Notes for Reviewers
interactive session and reviewed by me.
The failures looked unrelated — contradictory verdicts vs a mislabelled PR — which
is why it survived the first one.
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation