fix(exact-review): handle terminal admissions and tuple failures#840
Conversation
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 24, 2026, 6:38 PM ET / 22:38 UTC. ClawSweeper reviewWhat this changesThis PR checks exact-review targets before dispatch so closed or conclusively gone items are completed without work, and preserves permanent publisher failures in the batch result so they are not retried as unknown failures. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 4 items remain Keep this PR open for human review: the patch is a focused repair for two credible exact-review failure modes and I found no discrete correctness defect in the supplied diff, but it lacks contributor-provided after-fix runtime proof for behavior that can suppress queue dispatch or change terminal handling. Priority: P2 Review scores
Verification
How this fits togetherThe exact-review system accepts GitHub events into a Durable Object queue, dispatches bounded review work, and then publishes durable review results. This change sits at queue admission and batch-result publication, affecting whether queued work is dispatched, retried, parked, or completed. flowchart LR
A[GitHub review event] --> B[Exact-review durable queue]
B --> C[Admission target check]
C -->|Open target| D[Review workflow dispatch]
C -->|Closed or conclusively gone| E[Complete queue item]
D --> F[Batch result publisher]
F --> G[Durable state and GitHub result]
F --> H[Permanent or retryable outcome]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Add redacted after-fix runtime proof that drives the real queue owner through open, closed, visible-404, inaccessible-target, and tuple-failure cases, showing both the resulting queue state and the persisted batch disposition before merge. Do we have a high-confidence way to reproduce the issue? No high-confidence independently executed reproduction is available in this review. The PR describes clear current-main paths and adds focused source-level regressions, so the failures are source-reproducible but not independently demonstrated here. Is this the best way to solve the issue? Unclear. The patch follows the existing bounded-retry and dispatch-classification design, but runtime proof is needed to show the new terminal admission and batch-output boundaries behave safely with real request handling. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 3cb93c6a16bf. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(exact-review): handle terminal admissions and tuple failures This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
1 participant
Summary
tuple_protocol_invalid, in the batch outcome so the coordinator cannot relabel them as generic retryable failures.Problem
Two independent paths could keep work moving when it should stop:
404is not enough proof: it can also mean the bot cannot see the repository.retryable_failure/unknown_failure, causing the terminal failure to retry.The initial terminal-admission implementation also needed two operational safeguards: preserve the visible completion path for command-requested reviews, and rate-bound real GitHub reads without allowing a rate-limited
403to park valid items.Implementation
closed, genuinely missing (404with the repository still visible to the same token), or gone (410) is removed as completed work without dispatch. A liveopentarget follows the normal dispatch path.403is item-specific only when it is not identified as a GitHub rate-limit response (429, retry/rate headers, or standard rate-limit/abuse text).Causal links and related work
tuple_protocol_invalidresult.main: #836 fixes publication-head latching; #837 changes recovery ordering; #838 supplies the bounded review-retry model used here; and #839 supplies the dispatch-failure classification extended here. They are related infrastructure, not duplicate fixes.Validation
node --test test/dashboard-worker.test.ts— 203 passing.node --test test/exact-review-publication-batches.test.ts— 44 passing.pnpm run build:dashboard— passed.pnpm exec oxfmt --check dashboard/exact-review-queue.ts src/repair/publish-event-result.ts test/dashboard-worker.test.ts test/exact-review-publication-batches.test.ts— passed for changed surfaces.git diff --check— passed.cbx_4ce91d0f8380, Playwright Noble / Node 24.15.0) ran the same queue suite 203/203 and publication-batch suite 44/44 against this final code; it used only local storage and fake GitHub responses. The lease stopped automatically.Isolated before/after runtime probes for the two original failures also passed:
origin/maindispatched one already-closed item; this PR’s terminal path dispatched zero and completed the queue item locally.origin/mainproduced no batch mutation output after an impossible tuple; this PR producedpermanent_failurewithtuple_protocol_invalidand an error fingerprint.pnpm run format:checkremains baseline noise across unrelated repository files; the changed files pass the focused formatter check above.Review closeout
codex review --uncommitted— final dirty-patch review clean after accepted command-status, rate-budget, partial-pass, and rate-limited-403 findings were fixed and focused proof rerun.codex review --base origin/main— final branch review clean: no actionable regressions relative to the merge base.Risk and rollout
The admission path adds read-only GitHub checks and a small durable five-second throttle only when a full admission burst is reached or terminal completion reveals more ready review work. It intentionally does not bypass command-status completion. No production queue dispatch, item comment, workflow dispatch, gate change, or state mutation was performed as validation. No changelog or workflow change is included.