webgrip/unfold
5
0
Fork 0
Code Issues 1 Pull requests 2 Projects Releases 54 Packages 8 Wiki Activity Actions

fix(ploeg): retry a failed reviewer and close review_failed when no review came #195

Closed
ryangr0 wants to merge 3 commits from ryangr0/ploeg-failed-reader-review-failed into development AGit
pull from: ryangr0/ploeg-failed-reader-review-failed
merge into: webgrip:development
Conversation 0 Commits 3 Files changed 19 +675 -126
ryangr0 commented 2026-10-03 12:38:21 +00:00
Owner
Copy link

Implements the accepted ADR-0043 (VIK-1304, handoff EXEC-05).

Problem

When a reading Run failed, retryFailedWriter logged "a reading Run failed; its findings are missing" and the plan moved on. When that reader was the reviewer of a writer's pull request, the Shift closed plan_exhausted and readyForReview settled the item awaiting_review. The tracker comment then said "plan complete" about a review that never happened. This happened to Shift 90 (OOMKilled reviewer) and Shift 118 (ACP idle watchdog). TestFailedReader_StillAdvancesTheRound pinned the old behaviour.

Invariant

  • A reading Role whose Runs in a Round all failed is retried in that Round, under the writer's two budgets, before the plan moves on.
  • A Shift whose last reading Round after the last writer still has such a Role once the budgets are spent closes review_failed, never plan_exhausted.
  • No surface reports that Shift as reviewed, approved or "plan complete".

Change (Ploeg)

  • retryFailedRuns handles the writer first, as before (ADR-0019). Otherwise retryFailedReaders reopens every failed reading Role that still has attempts left. It does this in one store.ReopenRound call in the current Round, so the counter does not advance and readers that reported are not re-run.
  • Attempts are limited by MaxRunAttempts (agent failures: agent_error, idle, timeout, so an ACP watchdog kill stays an agent failure) and by MaxInfraFailures (infrastructure failures).
  • The pool is checked when the retry is claimed. An empty pool opens no Run, and the sweep parks the Shift with the spend named, as for any Run nobody can pay for.
  • When the budgets run out, the plan advances. If it then ends plan_exhausted and the last reading Round after the last writer has a Role with no non-failed Outcome, the close reason is review_failed. With a writer's pull request, the item still settles awaiting_review.
  • Close message and tracker comment: "Ploeg finished this item, but an agent review of its pull request is missing" / "not reviewed by an agent: reviewer Run failed (agent_error) after 3 attempt(s). Agent review unavailable; a person is asked to review and merge". When another reader of that Round did report, the comment says "not reviewed by every agent … Agent review incomplete".
  • Unchanged: a reader that reports without a verdict still closes plan_exhausted, a stuck reader still freezes the plan, and an approval still closes review_approved.
  • store.RunReport.FailureReason is new (read from the existing column; no migration).
  • The shift-orchestration spec rewords the swept-reader scenario as the ADR asks and adds a requirement for the reader retry. docs/how-to/review-an-agent-pr.md lists review_failed.

Change (Vloer)

In the Ready for review lane, the row chip reads "Agent review unavailable" (attention). The Work Item page says no agent reviewed the pull request. closeReasonLabel('review_failed') reads "No agent reviewed it: the reviewer kept failing". The demo replay is re-recorded because public/ changed.

Tests

New in pkg/shiftengine/failedreader_test.go, against real embedded Postgres. All of them except the stuck and wording tests failed on development before the change:

  • TestFailedReader_ReopensItsOwnRound (the ADR's named replacement)
  • agent budget ends after 3 attempts for agent_error, idle and timeout
  • infrastructure kills use their own budget of 10
  • the Shift 118 replay: writer pr_opened and three failed reviewer attempts give review_failed, awaiting_review, and a tracker comment without "plan complete" or "approved"
  • a retry that approves (review_approved) or reports without a verdict (plan_exhausted)
  • one of two reviewers fails and is retried alone
  • a failed analyst before the writer is not a missing review
  • a stuck reviewer still freezes the plan
  • a retry with an empty pool opens no Run (ErrBudgetExhausted) and is parked
  • 8 concurrent evaluators, the sweep and a restarted engine create exactly one retry
  • the wording test

TestFailedReader_StillAdvancesTheRound is removed, as the ADR says. TestExpiredReaderDoesNotBlockTheRound becomes TestSweptReaderIsRetriedByTheSweepAndNeverBlocksTheRound. Vloer adds assertions in test/ploeg-view.test.mjs and test/reasons.test.mjs.

Commands (at c2ced737, Go 1.27.1, Node 24.21.0)

  • mise exec -- go test ./pkg/shiftengine/ ./pkg/store/ -count=1 -v: 273 top-level tests passed, 0 failed
  • mise exec -- openspec validate --all --strict: 17 passed
  • mise run demo-record: re-recorded (391 requests)
  • mise run verify: all gates passed (vloer 7 incl. npm test 700/700, demo-replay 1, vloer-extension 4, ploeg 7, brand 1, site 5, site-demo 1, helm 15, release 3, integration 1, docs 1)

Not in this PR

  • The reviewed SHA surviving retries depends on EXEC-04 (VIK-1738).
  • Durable delivery of the tracker comment is AUTH-02's outbox (VIK-1728).
  • After deploy, check that the next failed reviewer shows close_reason = review_failed.

Refs VIK-1304

🤖 Generated with Claude Code

Implements the accepted [ADR-0043](apps/ploeg/docs/adrs/0043-a-failed-reading-run-is-retried-and-a-missing-review-closes-review-failed.md) (VIK-1304, handoff EXEC-05). ## Problem When a reading Run failed, `retryFailedWriter` logged "a reading Run failed; its findings are missing" and the plan moved on. When that reader was the reviewer of a writer's pull request, the Shift closed `plan_exhausted` and `readyForReview` settled the item `awaiting_review`. The tracker comment then said "plan complete" about a review that never happened. This happened to Shift 90 (OOMKilled reviewer) and Shift 118 (ACP idle watchdog). `TestFailedReader_StillAdvancesTheRound` pinned the old behaviour. ## Invariant - A reading Role whose Runs in a Round all failed is retried in that Round, under the writer's two budgets, before the plan moves on. - A Shift whose last reading Round after the last writer still has such a Role once the budgets are spent closes `review_failed`, never `plan_exhausted`. - No surface reports that Shift as reviewed, approved or "plan complete". ## Change (Ploeg) - `retryFailedRuns` handles the writer first, as before (ADR-0019). Otherwise `retryFailedReaders` reopens every failed reading Role that still has attempts left. It does this in one `store.ReopenRound` call in the current Round, so the counter does not advance and readers that reported are not re-run. - Attempts are limited by `MaxRunAttempts` (agent failures: `agent_error`, `idle`, `timeout`, so an ACP watchdog kill stays an agent failure) and by `MaxInfraFailures` (infrastructure failures). - The pool is checked when the retry is claimed. An empty pool opens no Run, and the sweep parks the Shift with the spend named, as for any Run nobody can pay for. - When the budgets run out, the plan advances. If it then ends `plan_exhausted` and the last reading Round after the last writer has a Role with no non-failed Outcome, the close reason is `review_failed`. With a writer's pull request, the item still settles `awaiting_review`. - Close message and tracker comment: "Ploeg finished this item, but an agent review of its pull request is missing" / "not reviewed by an agent: reviewer Run failed (agent_error) after 3 attempt(s). Agent review unavailable; a person is asked to review and merge". When another reader of that Round did report, the comment says "not reviewed by every agent … Agent review incomplete". - Unchanged: a reader that reports without a verdict still closes `plan_exhausted`, a `stuck` reader still freezes the plan, and an approval still closes `review_approved`. - `store.RunReport.FailureReason` is new (read from the existing column; no migration). - The `shift-orchestration` spec rewords the swept-reader scenario as the ADR asks and adds a requirement for the reader retry. `docs/how-to/review-an-agent-pr.md` lists `review_failed`. ## Change (Vloer) In the Ready for review lane, the row chip reads "Agent review unavailable" (attention). The Work Item page says no agent reviewed the pull request. `closeReasonLabel('review_failed')` reads "No agent reviewed it: the reviewer kept failing". The demo replay is re-recorded because `public/` changed. ## Tests New in `pkg/shiftengine/failedreader_test.go`, against real embedded Postgres. All of them except the stuck and wording tests failed on `development` before the change: - `TestFailedReader_ReopensItsOwnRound` (the ADR's named replacement) - agent budget ends after 3 attempts for `agent_error`, `idle` and `timeout` - infrastructure kills use their own budget of 10 - the Shift 118 replay: writer `pr_opened` and three failed reviewer attempts give `review_failed`, `awaiting_review`, and a tracker comment without "plan complete" or "approved" - a retry that approves (`review_approved`) or reports without a verdict (`plan_exhausted`) - one of two reviewers fails and is retried alone - a failed analyst before the writer is not a missing review - a stuck reviewer still freezes the plan - a retry with an empty pool opens no Run (`ErrBudgetExhausted`) and is parked - 8 concurrent evaluators, the sweep and a restarted engine create exactly one retry - the wording test `TestFailedReader_StillAdvancesTheRound` is removed, as the ADR says. `TestExpiredReaderDoesNotBlockTheRound` becomes `TestSweptReaderIsRetriedByTheSweepAndNeverBlocksTheRound`. Vloer adds assertions in `test/ploeg-view.test.mjs` and `test/reasons.test.mjs`. ## Commands (at c2ced737, Go 1.27.1, Node 24.21.0) - `mise exec -- go test ./pkg/shiftengine/ ./pkg/store/ -count=1 -v`: 273 top-level tests passed, 0 failed - `mise exec -- openspec validate --all --strict`: 17 passed - `mise run demo-record`: re-recorded (391 requests) - `mise run verify`: all gates passed (vloer 7 incl. `npm test` 700/700, demo-replay 1, vloer-extension 4, ploeg 7, brand 1, site 5, site-demo 1, helm 15, release 3, integration 1, docs 1) ## Not in this PR - The reviewed SHA surviving retries depends on EXEC-04 (VIK-1738). - Durable delivery of the tracker comment is AUTH-02's outbox (VIK-1728). - After deploy, check that the next failed reviewer shows `close_reason = review_failed`. Refs VIK-1304 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
A reading Run that failed was logged and stepped over. When it was the
reviewer of a writer's pull request, the Shift closed plan_exhausted,
the item settled awaiting_review and the tracker said "plan complete"
for a review that never happened (Shift 90, Shift 118).

ADR-0043 (accepted) decides the fix, implemented here:

- A Round whose reading Role's Runs all failed re-opens in place for
  that Role only, through store.ReopenRound, without advancing the round
  counter. Readers that reported are not re-run. A failed writer keeps
  precedence.
- The retry uses the writer's two budgets: MaxRunAttempts for agent
  failures (agent_error, idle, timeout: an ACP watchdog kill stays an
  agent failure) and MaxInfraFailures for infrastructure ones. The pool
  is checked when the retry is claimed, so an empty pool opens no Run
  and the sweep parks the Shift naming the spend.
- When the budgets are spent the Round completes and the plan advances.
  If the plan then ends plan_exhausted and the last reading Round after
  the last writer has a Role with no non-failed Outcome, the Shift
  closes review_failed. With a writer's pull request the item still
  settles awaiting_review. The close message and the tracker comment say
  "not reviewed by an agent: <role> Run failed (<failure_reason>)" and
  "Agent review unavailable", never "plan complete" or an approval.
- A reader that reported without a verdict still closes plan_exhausted,
  and a stuck reader still freezes the plan.

store.RunReport now carries the Run's failure reason. The
shift-orchestration spec's swept-reader scenario is reworded and a
requirement for the reader retry is added. The review how-to lists
review_failed.

VIK-1304

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ploeg now closes a Shift review_failed when its reviewer Run failed and
its retries ran out. The pull request still waits for review, so the
item sits in Ready for review, where its row read "No agent verdict".

The review lane chip now reads "Agent review unavailable" with the
reason in its title, the Work Item page says no agent reviewed the pull
request, and the close reason reads "No agent reviewed it: the reviewer
kept failing" wherever Shift close reasons are listed.

VIK-1304

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
chore(site): re-record the demo replay with the review_failed wording
Some checks failed
[Workflow] On Pull Request / warnings (pull_request) Has been cancelled
Details
[Workflow] On Pull Request / release-policy (pull_request) Has been cancelled
Details
[Workflow] On Pull Request / checks (pull_request) Has been cancelled
Details
c2ced7378a
 
VIK-1304

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ryangr0 closed this pull request 2026-10-04 08:41:33 +00:00
Some checks failed
[Workflow] On Pull Request / warnings (pull_request) Has been cancelled
Details
[Workflow] On Pull Request / release-policy (pull_request) Has been cancelled
Details
[Workflow] On Pull Request / checks (pull_request) Has been cancelled
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
webgrip/unfold!195
No description provided.