Skip to content

fix(publication): stop retrying reviews superseded by newer results - #1249

Draft
vincentkoc wants to merge 1 commit into
mainfrom
fix/publisher-superseded-terminal-20260825
Draft

fix(publication): stop retrying reviews superseded by newer results#1249
vincentkoc wants to merge 1 commit into
mainfrom
fix/publisher-superseded-terminal-20260825

Conversation

@vincentkoc

Copy link
Copy Markdown
Member

What Problem This Solves

Fixes an issue where exact-review publishers would retry an older review artifact forever after a newer durable review for the same revision had already become authoritative.

Why This Change Was Made

The apply lane now proves the complete durable review tuple before classifying an event as superseded: trusted non-placeholder lease owners, positive lease comment IDs, the same reviewed revision, and a strictly newer live lease comment ID. Legacy tuple-less artifacts still requeue the latest revision, while incomplete, mixed, or otherwise unverified evidence fails closed.

Verified supersession exits before state publication or any new GitHub review mutation. It emits completion_kind=superseded with reason_code=remote_newer_tuple; the existing workflow then releases the publisher-owned review lease.

User Impact

Operators should no longer see exact-review publication starvation when an older event loses a race to a newer durable review on the same revision. Ambiguous events remain retryable or fail closed rather than being silently discarded.

OpenClaw Bay Impact

OpenClaw Bay is unaffected. This change uses the existing superseded lifecycle outcome and remote_newer_tuple reason already understood by the queue and Bay projections; no observer schema, route, or display contract changes.

Documentation Impact

Reviewed docs/limits.md, especially the exact-review publication lifecycle. Its existing contract already states that newer remote tuples complete as terminal superseded outcomes without GitHub mutation, so no documentation update is required.

Evidence

  • Signed head: b44bb38ceefb785bcc817fc6c97964588956bd3d
  • Pre-commit Codex review: no actionable defects.
  • Committed-range Codex review against origin/main: no actionable regressions; focused typechecks, 66 tests, lint, formatting, and diff checks passed.
  • Static checks, three TypeScript builds, repository-wide formatting check, and all lint lanes passed.
  • Focused apply CAS proof passed and recorded no external mutation.
  • Event proof tests passed, including verified-only supersession plus legacy, mixed, and unverified fail-closed cases.
  • Publisher process proof passed with completion_kind=superseded, reason_code=remote_newer_tuple, no publication command, and { "kind": "superseded" } batch output.
  • Batch integration passed with no /publication-batch-results request and terminal superseded completion.
  • The full coverage gate reached the unrelated target-validation bootstrap cases, where six tests could not resolve registry.npmjs.org while installing pnpm@10.33.0; changed-surface proof and all non-network gates passed.

Real Behavior Proof

Claim: A publisher that observes one exact, verified newer durable review tuple terminates the stale event as superseded without publishing state or mutating GitHub.

Exercised surface: apply-decisions proof emission, publish-event-result, exact-review batch commit/completion, and existing lease-release routing.

Scenario: An issue report with durable lease tuple old-review-owner/9100 is overtaken after apply preflight by a trusted live review tuple new-review-owner/9200 on the same source revision.

Commands and environment:

node --test --test-name-pattern='issue apply CAS blocks a newer durable review tuple published after preflight' test/apply-label-sync.test.ts
node --test test/repair/event-apply-proof.test.ts
node --test test/repair/publish-event-result-superseded.test.ts
node --test --test-name-pattern='batch completion terminalizes a superseded member without a publication mutation' test/repair/exact-review-batch-cli.test.ts

The publisher proof runs the compiled dist/repair/publish-event-result.js with EXACT_EVENT_PUBLICATION=true, an isolated work root, a fake ClawSweeper CLI, and captured GitHub Actions outputs.

Observable result: The apply lane emits newerReviewTupleVerified: true with zero external mutation calls. The publisher exits successfully with completion_kind=superseded and reason_code=remote_newer_tuple, invokes only apply-artifacts and apply-decisions, and does not enter state publication. Batch completion records terminal_outcome=superseded without posting a publication mutation.

Artifact or trace: Assertions are durable in test/apply-label-sync.test.ts, test/repair/event-apply-proof.test.ts, test/repair/publish-event-result-superseded.test.ts, and test/repair/exact-review-batch-cli.test.ts.

Limits: GitHub and the queue are controlled local doubles; the proof does not dispatch a live publisher recovery, deploy a Worker, or mutate a real repository.

@clawsweeper

clawsweeper Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 26, 2026
@clawsweeper

clawsweeper Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 25, 2026, 8:32 PM ET / August 26, 2026, 00:32 UTC.

ClawSweeper review

What this changes

The branch classifies an exact-review event as terminally superseded only when a newer durable review tuple for the same revision is fully verified, preventing stale publication retries and GitHub mutations.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

Keep open. This member-authored draft adds a narrowly scoped, fail-closed terminal path that current main does not contain; no blocking correctness defect was found, but the automation-path proof should be recorded in the repository-required container format before landing.

Priority: P2
Reviewed head: b44bb38ceefb785bcc817fc6c97964588956bd3d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The patch has focused fail-closed checks and cross-stage tests, with container-backed automation evidence remaining the material confidence upgrade.
Proof confidence 🌊 off-meta tidepool Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply; the supplied controlled-double tests support patch validation but are not a live transport claim.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply; the supplied controlled-double tests support patch validation but are not a live transport claim.
Evidence reviewed 6 items Current-main gap: A direct two-tree comparison against the supplied main SHA shows the terminal superseded disposition and publisher exit are introduced only by this branch, so the requested behavior is not already on current main.
Fail-closed tuple predicate: The new predicate requires patchable durable-review provenance, non-placeholder owners, matching source revision, positive lease IDs, and a strictly newer live lease ID before terminalization.
Terminal publisher handling: A verified superseded disposition writes a terminal batch result and completion outputs, then returns before the ordinary publication path.
Findings None None.
Security None None.

Live Verification

Command: pnpm run build:repair && node --test test/repair/publish-event-result-superseded.test.ts

Result: FAIL (failed) — step 1 run pnpm run build:repair && node --test test/repair/publish-event-result-superseded.test.ts: previous terminal command was still running after 30 seconds: "pnpm run build:repair && node --test test/repair/publish-event-result-superseded.test.ts"

$ tsc -p tsconfig.repair.json
✔ publisher terminalizes a verified newer durable tuple without publication (113.046981ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 215.68506

Assertions:

  • FAIL expect_output: publisher terminalizes a verified newer durable tuple without publication

How this fits together

Exact-review publication applies a review artifact, compares it with the live durable review comment, and then either publishes state or completes the queue item. The changed guard turns one verified stale-comment case into a terminal queue outcome so the publisher releases its lease without publishing obsolete state.

flowchart LR
A[Exact review artifact] --> B[Apply-time review guard]
C[Live durable review comment] --> B
B --> D{Newer tuple fully verified?}
D -- Yes --> E[Superseded completion]
D -- No --> F[Existing apply or retry path]
E --> G[Batch lease release]
F --> H[Publication and state update]
Loading

Before merge

  • Resolve merge risk (P1) - A false-positive tuple verification would terminalize a valid publisher before state publication; the supplied evidence uses controlled doubles rather than the Docker-backed Crabbox production-path proof required by repository policy for this automation surface.
  • Complete next step (P2) - Obtain the repository-required container-backed terminal-path proof and let a maintainer review the current draft and in-progress checks; no narrow code repair was identified.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +114, tests +234 across 12 files The larger test delta exercises apply, publisher, and batch-completion boundaries for a terminal automation outcome.

Merge-risk options

Maintainer options:

  1. Record container-backed terminal-path proof (recommended)
    Run the compiled apply and publisher path in Crabbox local-container with verified and invalid tuple fixtures, preserving provider, image, lease, artifact, and limits in the PR body.

Technical review

Best possible solution:

Retain the strict same-revision, strictly newer lease-ID rule and land it only with a recorded Crabbox local-container trace showing both terminal supersession and fail-closed rejection of an incomplete or mixed tuple.

Do we have a high-confidence way to reproduce the issue?

No high-confidence live reproduction was established in this read-only review; source and focused fixtures define a controlled stale-versus-newer durable-tuple scenario with a clear expected terminal outcome.

Is this the best way to solve the issue?

Yes, conditionally: using the durable tuple rather than timestamps alone is the narrowest fail-closed solution, provided its production-path proof covers invalid and mixed tuple cases.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against afe976209aa5.

Labels

Label changes:

  • add P2: This is a bounded exact-review publication reliability fix with operator impact but no demonstrated user-facing outage.
  • add merge-risk: 🚨 automation: Merging changes whether a publisher retries or completes terminally, so a false-positive proof signal could suppress a valid publication.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply; the supplied controlled-double tests support patch validation but are not a live transport claim.

Label justifications:

  • P2: This is a bounded exact-review publication reliability fix with operator impact but no demonstrated user-facing outage.
  • merge-risk: 🚨 automation: Merging changes whether a publisher retries or completes terminally, so a false-positive proof signal could suppress a valid publication.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The author is a MEMBER, so the external-contributor proof gate does not apply; the supplied controlled-double tests support patch validation but are not a live transport claim.

Evidence

What I checked:

Likely related people:

  • Peter Steinberger: The available shortlog attributes four historical touches across the review-state, event-proof, and publisher paths to Peter Steinberger. (role: publication-area contributor; confidence: medium; commits: f32082eb7d3e, 32e1a069db2d; files: src/clawsweeper-review-comment-state.ts, src/repair/event-apply-proof.ts, src/repair/publish-event-result.ts)
  • Martin Cleary: Path history records Martin Cleary on the review-comment-state lineage preceding this change. (role: recent review-state contributor; confidence: low; commits: 023046ec0354, 115abf8548e0; files: src/clawsweeper-review-comment-state.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Record a Crabbox local-container trace through the compiled apply and publisher path for one verified newer tuple and one rejected invalid or mixed tuple.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant