fix(review): reconcile terminal legacy publications - #892
Conversation
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs maintainer review before merge. Reviewed July 27, 2026, 8:58 AM ET / 12:58 UTC. ClawSweeper reviewWhat this changesAdds bounded reconciliation for stranded protocol-v1 exact-review publication rows when a live target read is terminal, while retaining command-context, active-batch, leased, and newer-authority rows. Merge readinessThis PR remains necessary: the merged direct-publication transition retained the legacy queue as a bounded fallback, while protocol-v1 rows lack the revision metadata used by the existing reconciliation route. The updated PR body now provides representative real-runtime proof for the new bounded path; I found no discrete correctness defect in the supplied diff, so this is ready for normal maintainer merge review with the stated live dry-run rollout precaution. Priority: P2 Review scores
Verification
How this fits togetherExact-review results enter a Durable Object-backed publication queue before they are reflected in the public state surface. The maintenance reconciliation endpoint examines stored publication rows, checks ownership and target status, then removes only rows whose terminal state has no active or newer publication authority. flowchart LR
A[Legacy publication rows] --> B[Reconciliation endpoint]
B --> C[Batch and lease protection]
C --> D[Bounded target-status reads]
D --> E[Publication authority check]
E --> F[Terminal row removal]
F --> G[Maintenance counters]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Merge the bounded protocol-v1 reconciliation path, then have an operator inspect a live dry run and perform one Do we have a high-confidence way to reproduce the issue? Yes, with medium confidence: an isolated ExactReviewQueue can be seeded with a Is this the best way to solve the issue? Yes: extending the existing explicit reconciliation endpoint is narrower and safer than changing direct publication, dashboard projection, publisher workflow, or state-writer behavior; the authority, lease, batch, and command-context guards keep the repair constrained. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 1082c407028d. 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
History |
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
@clawsweeper re-review |
clawsweeper
Bot
added
proof: sufficient
1 participant
Summary
Reconcile stranded protocol-v1 exact-review publication rows when their target is terminal, without changing the direct-publication path, dashboard projection, publisher workflow, or state-writer behavior.
Problem
openclaw/openclaw#113313was merged while its legacy rowopenclaw/openclaw#113313@publish:30091560737:1remained ownerless in the public publication queue. The source workflow run30091560737completed successfully on 2026-07-24, before the later direct-publication change in #859. Protocol-v1 rows had no revision tuple, so the existing reconciliation route did not consider them.Implementation
/publications/reconcile.live_proceeded, a terminal target read, no active batch/lease, and no current or newer target authority.max_itemsbound without quadratic backlog scans.Validation
pnpm run build:allpnpm run lint:dashboardpnpm run lint:repairnode --test test/dashboard-worker.test.ts(246 passing)node --test test/repair/exact-review-batch-coordinator.test.ts(7 passing)codex review --uncommitted— final result: no actionable correctness issues.git fetch origin main --prune && codex review --base origin/main— final result: no actionable correctness issues.Focused coverage proves terminal removal, dry-run verification without mutation, expired-lease persistence, command acknowledgement retention, active batch/lease protection, and newer publication authority protection.
Representative Docker proof
Ran a Docker-backed Crabbox
local-containerscenario on the PR head (leasecbx_f9f31e55ac0f, auto-stopped, exit 0). It used the actualExactReviewQueueimplementation with an isolated local Durable Object and a live public GitHub read of the terminal target. No production queue, state, deployment, workflow event, replay, or apply path was touched.The fixture contains the stranded ordinary v1 row, an awaiting-command v1 row, and a v1 row owned by an active batch. The exact root target read was
openclaw/openclaw#113313, which GitHub returned as closed at2026-07-24T13:00:16Z.{ "target": { "number": 113313, "state": "closed", "closed_at": "2026-07-24T13:00:16Z" }, "dry_run": { "candidates": 1, "selected": 1, "eligible": 1, "changed": 0, "ordinary_row_retained": true }, "capped_apply": { "max_items": 1, "eligible": 1, "changed": 1, "ordinary_row_removed": true }, "protected_rows": { "command_context_retained": true, "active_batch_retained": true, "protected_batch_items": 1 } }This is representative non-production evidence, not an authorization to mutate the live queue. A production operator should still inspect a live dry run before any bounded maintenance apply.
Risks and rollout
This changes only the explicit reconciliation endpoint. Each pass remains capped at 100 selected rows and uses bounded target reads; it does not deploy, poll broadly, alter the public dashboard, or mutate live queue state as part of this PR. Operators should inspect a dry run first, then apply one bounded pass and observe normal queue health.
Links