fix(queue): keep the owed source-drift review when a completion callback is lost - #1251
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 automation
|
Codex review: needs maintainer review before merge. Reviewed August 30, 2026, 8:26 PM ET / August 31, 2026, 00:26 UTC. ClawSweeper reviewWhat this changesThe PR makes lost exact-review completion callbacks preserve an accepted source-drift requeue and labels queue-completion failures separately from review-generation failures. Regression provenancePossible regression — probable (reproduction; failure trace). No predecessor PR is attributed. Merge readinessKeep open for maintainer landing: current main still takes the bare terminal-run completion path, while this active branch preserves the owed source-drift review from the durable receipt. The current diff is narrowly covered and has strong before/after Worker proof; no blocking patch defect was found. Priority: P1 Review scores
Verification
How this fits togetherThe exact-review workflow publishes review results to a Durable Object queue, then completes its lease. If that callback is lost, terminal-run reconciliation now consults the durable publication receipt to decide whether a fresh source-drift review remains owed. flowchart LR
A[Exact review workflow] --> B[Publication receipt]
B --> C[Durable review queue]
D[Lost completion callback] --> E[Terminal-run reconciler]
C --> E
E --> F{Receipt requires requeue}
F -->|accepted or deduped| G[Fresh source-drift review]
F -->|otherwise| H[Complete leased item]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the receipt-aware reconciliation with its focused regression matrix, then monitor the first production deployment for unexpected requeue volume or completion-failure classifications. Do we have a high-confidence way to reproduce the issue? Yes. The supplied baseline run against the pinned current-main SHA records the lost-callback reconcile deleting the owed item, and the focused current-head matrix reproduces the same receipt states. Is this the best way to solve the issue? Yes. Reusing the already durable direct-publication receipt and existing requeue operation is narrower than changing retry policy or turning a successfully published review into a failed run. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0d07398d4fab. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
1995f4f to
18af960
Compare
|
Refreshed against the verdict at Add real behavior proof (Docker-backed runtime proof). Done. The in-memory driver is replaced by Resolve merge risk (P1, in-memory seams). Same change addresses it. The only remaining substitution is the initial leased item, which is seeded through the production state writer inside the Durable Object because a real lease requires a GitHub dispatch; every transition after that point is real. That limit is stated in the body and in the proof README. Resolve merge risk (P1, Bay impact). Stated and verified: no Bay change is needed and none is produced. Live Verification failure on the previous head. That was my proof artifact, not the patch. Gates re-run inside the same local-container image: Nothing here is an open maintainer decision from my side. |
clawsweeper
Bot
added
proof: sufficient
…ack is lost The Complete exact-review queue lease callback is the only carrier of direct_lifecycle_requeue, and both fallbacks that engage when it cannot reach the durable queue ignore that metadata. After an accepted direct publication the run stays green by design, so the workflow_run reconciler heals the still-leased item from the bare run conclusion and deletes it, discarding the source-drift follow-up review the callback would have created and removing the lease-expiry backstop with it. The accepted receipt already rewrote the leased item's decision to a publication decision carrying its directLifecycle plan, so /reconcile now recognises an owed requeue and heals it through the same requeueDirectLifecyclePublicationSync the /complete handler calls, recording the same requeue terminal fact on the fenced projection. In the durable-queue lane the same callback loss reddens the run, and the fail gate's single hardcoded annotation blamed classification=codex_or_content_failure even when its own fields reported review_outcome=success, and also for held reservations whose review never ran. The gate now derives the classification by reusing its own review-lane clause, so a completion failure reports queue_completion_failure while a genuine review failure keeps codex_or_content_failure. The gate's if-expression is unchanged, so exactly the same runs go red. Adds regression coverage for the healed requeue, for an unchanged green-run completion with no owed requeue, and for the classification expression staying in sync with the gate clause it mirrors. Replaces the in-memory drift driver with a Docker-backed Crabbox local-container harness that runs the real Wrangler Worker and its ExactReviewQueue Durable Object, drives both scenarios over the real signed routes against real SQLite storage, and captures the Bay telemetry tables and the public /api/durable-lifecycle-bay snapshot on either side of the recovery. Fixes openclaw#1245
18af960 to
138eacf
Compare
|
Refreshed against the verdict at [P1] Exclude superseded receipts from recovery requeues ( const directLifecycle = item.decision.publication?.directLifecycle;
const owedDirectLifecycleRequeue =
run.outcome === "success" &&
item.revision <= leaseRevision &&
directLifecycle?.plan.kind === "requeue" &&
(directLifecycle.receiptOutcome === "accepted" ||
directLifecycle.receiptOutcome === "deduped");Confirming your three evidence points independently, plus one more that makes the case stronger than stated. Resolve merge risk (P1) and Complete next step (P2): both closed by that same change. This is maintainer option 1, and the production delta is 3 lines. Rank-up move, both controls in the Durable Object proof: done. Before, on the previous head After, on The two existing scenarios are unchanged across both halves, so the accepted-receipt recovery this PR opened for still works and the delivered-callback control is unaffected. Regression coverage (your note that the added test was accepted-path only): Acceptance criteria: One environment note on the third acceptance command: crabbox's Good catch on this one. It would have turned a lost callback into duplicate review work on items a newer publisher already owned. |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
Preserve the owed source-drift review using the durable receipt, retain current-main queue failure metadata, and consolidate receipt regression coverage. Co-authored-by: yetval <yetvald@gmail.com>
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
* fix(queue): use saved lease authority for direct requeues Follow up on #1251 by reading direct-requeue authority from the saved lease decision. Preserve accepted and deduplicated receipt ownership, reject superseded completion authority, and keep newer command decisions intact. The original lost-completion fix is already upstream; retain its existing contributor credit. * test(queue): document the saved-lease authority repro
2 participants
Fixes #1245.
When a review publishes successfully but its queue-completion callback is lost, the workflow reconciler currently uses only the green run conclusion. It deletes the lease even when the accepted publication receipt says a source-drift follow-up is owed.
The queue now reads that durable receipt and reuses its existing requeue operation. Accepted or deduplicated requeue receipts preserve one fresh review; superseded receipts and newer queued revisions do not manufacture another review. The workflow also reports queue-completion failures separately from Codex/content failures, without changing which runs fail or introducing another retry loop.
This refresh incorporates current main, preserves the contributor's implementation and credit, consolidates repeated regression setup, and retains the one-off runtime drivers in their immutable original commit rather than adding them to the maintained tree. The final diff is about 300 lines including tests and documentation, versus over 1,100 in the original proposal.
Current behavior proof
7cbe4493cf05142ee3338c63e64bbb291e2e7da1; tree2e0888cdcc4280894627530b9b6fb6521890541b.0d07398d4fabc829c25e22c19a06a0656f49030b.cbx_7031f88109bf; imageami-0461d919be7deb53c; Nodev24.18.1, pnpm11.10.0, Wrangler4.107.0.docs/proof/exact-review-completion-metadata/run-proof.shandannotation-proof.mjsfrom commit138eacf9b30ced7465be928d9d6022f5647fa7d9, run separately against the baseline and integrated candidate. Reproduction commands are in the compact proof README retained by this PR.run_a106148f5b89; artifacts:proof-summary.json,transcript.md, and annotation output. The original contributor head was independently checked first inrun_400419e5dd76.requeued: 0, completed: 1; item deletedrequeued: 1, completed: 0; pending revision 5, cleared lease,source_drift_requeue, terminal dispositionrequeueThe real workflow annotation shell emitted
queue_completion_failurefor a successful review whose completion failed and for a held deferral whose completion failed. A genuine review failure remainedcodex_or_content_failure. The original failure-gate expression is unchanged.Limits: initial lease setup and state inspection use proof-only seams. All subsequent signed HTTP, storage, receipt, reconciliation, and lifecycle transitions use production code. The proof supplies the terminal-run payload rather than calling GitHub's run lookup. Workflow expressions are evaluated locally and the shell executes under bash. This proves local workerd behavior, not Cloudflare edge availability; it does not induce an outage in production. The baseline ran from an archive of the stated main SHA and the candidate proof ran before commit, with the reviewed source tree carried unchanged into the candidate head.
Validation and review disposition
pnpm run check: passed; 4,140 tests passed, 8 skipped (4,148 total), plus all 13 static test cases.OpenClaw Bay needs no code change: the real before/after proof confirms that a
requeueterminal fact leaves Bay event/pending tables empty and the public snapshot unchanged. Bay remains observer-only, with no new browser calls or mutation controls.