Skip to content

revert: draft contingency rollback for #846#847

Closed
brokemac79 wants to merge 1 commit into
mainfrom
codex/revert-pr-846-durable-tuple-snapshot
Closed

revert: draft contingency rollback for #846#847
brokemac79 wants to merge 1 commit into
mainfrom
codex/revert-pr-846-durable-tuple-snapshot

Conversation

@brokemac79

Copy link
Copy Markdown
Contributor

Draft contingency rollback — do not merge without explicit incident approval

This draft is a contingency rollback for #846, merged as 4707e591f3a7ab04b52d13768dba57b5059a0d99 on 2026-07-25.

It is deliberately a draft: #846 fixes the proven cause of the missing #93584 re-review completion. Do not merge this PR to address #93584; its existing DLQ item does not auto-replay. Use it only if a new, post-deploy incident proves that #846 itself must be rolled back, and obtain explicit maintainer approval first.

Scope

This selectively restores the pre-#846 production behavior:

It intentionally retains the unrelated verified-pull fixture from #843's follow-up CI repair. A complete textual revert would reintroduce the known deterministic pnpm check failure because it removes the /repos/openclaw/clawsweeper/pulls/842 mock. This draft therefore contains no change to that fixture and no other production behavior.

Risk

Merging this draft reintroduces the #93584 root cause in the isolated exact-review batch path: when a durable tuple already exists, an empty worker-root base is compared with the durable-state candidate and can terminalise as tuple_protocol_invalid with missing comparable state-mutation timestamp. It does not replay, repair, or otherwise mutate #93584.

The separate stale-publication revision conflict observed for #111368 is unaffected.

Validation

  • pnpm run build:all
  • node --test test/dashboard-worker.test.ts — 208 passed
  • pnpm run lint:repair
  • pnpm run lint:scripts
  • pnpm exec oxfmt --check src/repair/event-record-store.ts src/repair/publish-event-result.ts test/repair/event-record-store.test.ts
  • git diff --cached --check

Codex review closeout

  • codex review --uncommitted reported the expected P1: the rollback restores the durable-tuple failure.
  • codex review --base origin/main reported the same expected P1 at src/repair/publish-event-result.ts:156.

That finding is intentionally rejected only because this PR is an explicit, user-requested contingency rollback. It is not a clean safety fix and must remain a draft unless a separate incident decision authorizes this exact risk.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jul 25, 2026
@clawsweeper

clawsweeper Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 25, 2026, 1:25 AM ET / 05:25 UTC.

ClawSweeper review

What this changes

This draft selectively reverts the durable-state tuple-baseline capture introduced by the merged exact-review publication fix and removes its regression test.

Merge readiness

Blocked until real behavior proof is added - 6 items remain

Keep this draft open for an explicit incident decision, but it is not a safe normal merge candidate: its stated effect is to restore the exact-review publication failure fixed on current main. Likely related people: brokemac79 is the recent exact-review publication contributor (medium confidence).

Priority: P1
Reviewed head: 53bc7019e50c4e5a0345a8ff6be23470c09c2e9d
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) This is intentionally a known-regressing rollback draft with no real after-change proof for a normal merge decision.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR lists validation commands but provides no after-change real behavior proof; because the intended behavior is a known regression, it cannot become a normal merge candidate without incident-specific evidence. Any future evidence should redact private endpoints, tokens, IP addresses, and other sensitive details; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🧂 unranked krab (1/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR lists validation commands but provides no after-change real behavior proof; because the intended behavior is a known regression, it cannot become a normal merge candidate without incident-specific evidence. Any future evidence should redact private endpoints, tokens, IP addresses, and other sensitive details; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 4 items Known regression restored by the patch: The proposed change removes the durable-state source-root snapshot from the publisher, restoring worker-root capture before comparison; the PR body identifies this as the path that can terminalise an exact-review result with a missing comparable state-mutation timestamp.
Regression coverage is removed: The branch deletes the lifecycle test that models an empty isolated worker alongside an existing durable tuple and verifies that the durable-state baseline resolves the fresh review record.
Current-main provenance: The supplied related-item record identifies #846 as merged into current main at this commit; it introduced the durable-state snapshot source and its focused regression coverage after a real missing-review-completion incident.
Findings 1 actionable finding [P1] Preserve the durable-state baseline snapshot
Security None None.

How this fits together

Exact-review workers produce review artifacts in isolated directories, and the event publisher compares those artifacts with durable state before publishing the result. The baseline snapshot determines whether a new review record can be safely applied to the durable ClawSweeper state store.

flowchart LR
  A[Exact review worker] --> B[Isolated review artifact]
  C[Durable state checkout] --> D[Baseline tuple snapshot]
  B --> E[Event snapshot comparison]
  D --> E
  E --> F[Safe publication decision]
  F --> G[Durable review records]
Loading

Decision needed

Question Recommendation
Should this known-regressing contingency rollback remain as an open draft, or should it be closed and recreated only when a post-deploy incident supplies evidence that the current-main fix must be reverted? Close until an incident requires it: Close the draft now and recreate a narrow rollback from the recorded fix commit only if a new incident establishes that reverting current-main behavior is necessary.

Why: The branch intentionally reintroduces a production publication failure, so preserving it as an incident-only artifact versus closing it is an operational ownership decision rather than a code-correctness choice.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR lists validation commands but provides no after-change real behavior proof; because the intended behavior is a known regression, it cannot become a normal merge candidate without incident-specific evidence. Any future evidence should redact private endpoints, tokens, IP addresses, and other sensitive details; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Preserve the durable-state baseline snapshot (P1) - Do not replace the durable-state baseline with the isolated worker directory. When that directory starts empty while durable state already has a tuple, the later comparison can terminalise the review as tuple_protocol_invalid rather than publish its result.
  • Resolve merge risk (P1) - If merged outside a verified incident rollback, this restores a terminal exact-review publication failure for an empty worker directory with an existing durable tuple, and removes the regression test that detects that topology.
  • Improve patch quality - Obtain explicit incident-owner approval and fresh evidence that current-main behavior must be rolled back before considering merge.
  • Improve patch quality - Provide a redacted live publication trace showing the incident topology and the proposed rollback's expected recovery behavior.

Findings

  • [P1] Preserve the durable-state baseline snapshot — src/repair/publish-event-result.ts:156-159
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Regression-test coverage 1 focused lifecycle test removed The deleted test is the direct guard for the empty-worker and existing-durable-tuple topology that the merged fix addressed.
Patch scope 3 files affected; 7 additions, 61 deletions The small rollback removes both the production safeguard and its focused regression coverage.

Root-cause cluster

Relationship: superseded
Canonical: #846
Summary: This draft explicitly reverses the merged durable-tuple publication fix; current main retains the canonical behavior from the merged fix.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Pause the rollback branch (recommended)
    Do not merge this draft without a new incident demonstrating that current-main durable-state capture is the problem and an owner accepting the restored publication failure.
  2. Authorize a bounded incident rollback
    If rollback is required, record the incident evidence, affected exact-review items, recovery procedure, and explicit approval before merging this known regression.

Technical review

Best possible solution:

Keep the durable-state baseline behavior from current main; if a future incident truly requires rollback, use a separately approved, time-bounded rollback plan that documents the affected queue behavior and recovery path.

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

Yes, from source and the deleted regression test: an empty isolated worker plus an existing durable tuple and a fresh review tuple exercises the restored invalid-baseline comparison path. This review did not execute the reproduction because the target checkout is read-only.

Is this the best way to solve the issue?

No. Current main's durable-state baseline is the narrow fix for the documented publication failure; reverting it is appropriate only after a separately evidenced incident decision.

Full review comments:

  • [P1] Preserve the durable-state baseline snapshot — src/repair/publish-event-result.ts:156-159
    Do not replace the durable-state baseline with the isolated worker directory. When that directory starts empty while durable state already has a tuple, the later comparison can terminalise the review as tuple_protocol_invalid rather than publish its result.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P1: Merging the branch can again prevent exact-review publication for real review events in the known durable-tuple topology.
  • add merge-risk: 🚨 automation: The changed publisher path can terminalise an automated exact-review result instead of publishing it to durable state.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR lists validation commands but provides no after-change real behavior proof; because the intended behavior is a known regression, it cannot become a normal merge candidate without incident-specific evidence. Any future evidence should redact private endpoints, tokens, IP addresses, and other sensitive details; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P1: Merging the branch can again prevent exact-review publication for real review events in the known durable-tuple topology.
  • merge-risk: 🚨 automation: The changed publisher path can terminalise an automated exact-review result instead of publishing it to durable state.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR lists validation commands but provides no after-change real behavior proof; because the intended behavior is a known regression, it cannot become a normal merge candidate without incident-specific evidence. Any future evidence should redact private endpoints, tokens, IP addresses, and other sensitive details; updating the PR body should trigger a fresh review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Known regression restored by the patch: The proposed change removes the durable-state source-root snapshot from the publisher, restoring worker-root capture before comparison; the PR body identifies this as the path that can terminalise an exact-review result with a missing comparable state-mutation timestamp. (src/repair/publish-event-result.ts:156, 53bc7019e50c)
  • Regression coverage is removed: The branch deletes the lifecycle test that models an empty isolated worker alongside an existing durable tuple and verifies that the durable-state baseline resolves the fresh review record. (test/repair/event-record-store.test.ts:36, 53bc7019e50c)
  • Current-main provenance: The supplied related-item record identifies fix(exact-review): snapshot durable tuple before publication #846 as merged into current main at this commit; it introduced the durable-state snapshot source and its focused regression coverage after a real missing-review-completion incident. (src/repair/event-record-store.ts:99, 4707e591f3a7)
  • Review intent: The PR author explicitly states that this is a contingency rollback, must remain draft absent a new post-deploy incident, and intentionally accepts the same P1 regression identified by review. (53bc7019e50c)

Likely related people:

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.

@steipete

Copy link
Copy Markdown
Contributor

Thanks @brokemac79 for preparing this contingency. This rollback would recreate the durable-tuple baseline failure where an empty isolated worker is compared against existing durable state, which can strand an otherwise valid review publication. The current fix and its focused regression test remain healthy, so we are closing this draft. Please open a fresh revert PR if an actual production incident demonstrates that a rollback is needed.

@steipete steipete closed this Jul 25, 2026
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. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants