Skip to content

revert: rollback exact-review admission and tuple fixes (#840)#841

Draft
brokemac79 wants to merge 1 commit into
mainfrom
codex/rollback-pr-840
Draft

revert: rollback exact-review admission and tuple fixes (#840)#841
brokemac79 wants to merge 1 commit into
mainfrom
codex/rollback-pr-840

Conversation

@brokemac79

Copy link
Copy Markdown
Contributor

Summary

Draft emergency rollback for #840, merged as 8006cf0bcf7ff810761c11a3cf0609b33b03475b.

Purpose and use

This is deliberately a draft. Do not merge it unless the post-merge monitor establishes that #840 caused a material live regression and a rollback is the right containment action. If the monitor remains healthy, close this draft without merging it.

The commit reverts #840 only and restores the exact pre-#840 tree. Consequently it also restores the prior behaviour for:

  • durable tuple/permanent-failure publication outcomes; and
  • header-signalled GitHub 403 rate-limit classification.

Those are intentional rollback trade-offs, not new design changes in this PR.

Validation

  • node --test test/dashboard-worker.test.ts — 188 passed
  • node --test test/exact-review-publication-batches.test.ts — 44 passed
  • pnpm run build:dashboard — passed
  • git diff --cached --check — passed before commit
  • codex review --uncommitted — reviewed the two intentional restored behaviours above; no unrelated change accepted
  • git fetch origin main --prune && codex review --base origin/main — clean: exact rollback to the pre-fix(exact-review): handle terminal admissions and tuple failures #840 tree, no unintended deviations; whitespace-clean

The current formatter would reformat the restored pre-#840 files. That churn is intentionally excluded so this remains a mechanical, reversible rollback.

Risk / rollout

Merging this draft would remove the safeguards introduced by #840. Keep it ready as a rapid containment option only; use the live monitor and maintainer judgment before taking that action.

@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. P2 Normal priority bug or improvement with limited blast radius. labels Jul 25, 2026
@snowzlmbot

snowzlmbot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Regarding #840 Proof of what was fixed before, openclaw/openclaw#111368This PR is exactly what shows the fixes to the previous bugs. 111368, there's a small issue with Clawsweeper right now - PRs that are in the backlog period can't be accurately reviewed or have their review conclusions issued a reflection of that

No impact on newly submitted issues/PRs

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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 24, 2026, 8:37 PM ET / July 25, 2026, 00:37 UTC.

ClawSweeper review

What this changes

This draft mechanically reverts the exact-review queue’s terminal-item admission safeguards and the publisher’s structured batch-outcome reporting added by the prior merged PR.

Merge readiness

Blocked until stronger real behavior proof is added - 5 items remain

Keep this draft PR open as a maintainer-controlled containment option, not a merge candidate: it intentionally restores the failure modes fixed by #840, and no material post-merge regression evidence is present here to justify that trade-off. Likely related people: brokemac, with high confidence from the merged implementation and this rollback proposal.

Priority: P2
Reviewed head: 65b5a13058e5ffa9578e51b2eac1af3fbb2451b0
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The rollback appears mechanically focused, but it is not quality-ready to merge without real operational evidence that the rollback condition exists.
Proof confidence 🧂 unranked krab (1/6) Needs stronger real behavior proof before merge: The PR body summarizes focused tests and a source-level exact-rollback review, but it does not provide live monitor evidence of the regression that would justify deploying this rollback or demonstrate containment after it. Any posted evidence should redact private endpoints, IP addresses, tokens, and other sensitive details. 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 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The PR body summarizes focused tests and a source-level exact-rollback review, but it does not provide live monitor evidence of the regression that would justify deploying this rollback or demonstrate containment after it. Any posted evidence should redact private endpoints, IP addresses, tokens, and other sensitive details. 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 Current-main behavior being reverted: The PR base is current main commit 8006cf0, and this branch deletes the admission interval plus the bounded live target checks from the exact-review queue.
Terminal publisher result is intentionally removed: The rollback removes the batch-mutation write that preserves a permanent tuple failure for the batch coordinator before the publisher rethrows.
Merged implementation provenance: #840 merged at 2026-07-25T00:22:42Z as current main commit 8006cf0; its documented before/after probes cover the two behaviors this draft restores.
Findings None None.
Security None None.

How this fits together

GitHub events enter the exact-review queue, which admits work, dispatches bounded review batches, and records durable completion state. The publisher reports each batch item’s outcome back to the coordinator so terminal failures are not retried as generic transient failures.

flowchart LR
  A[GitHub review event] --> B[Exact-review queue]
  B --> C[Admission status check]
  C --> D[Bounded batch dispatch]
  D --> E[Publisher result]
  E --> F[Durable queue and dashboard state]
  C --> G[Terminal completion without dispatch]
Loading

Decision needed

Question Recommendation
Has post-merge monitoring established a material regression from #840 that warrants restoring its known queue-admission and tuple-failure behaviors? Keep the guarded draft: Leave this rollback unmerged while the post-merge monitor window completes and retain the current safeguards on main.

Why: The branch is deliberately a faithful rollback rather than a corrective patch, so source review cannot determine whether the operational containment trade-off is justified.

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The PR body summarizes focused tests and a source-level exact-rollback review, but it does not provide live monitor evidence of the regression that would justify deploying this rollback or demonstrate containment after it. Any posted evidence should redact private endpoints, IP addresses, tokens, and other sensitive details. 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.
  • Resolve merge risk (P1) - Merging restores dispatch of ordinary closed targets and removes the durable admission throttle, so unnecessary review work may again enter the worker path.
  • Resolve merge risk (P1) - Merging also restores the path where a permanent tuple failure can lack a batch mutation result and be retried as a generic failure.
  • Resolve merge risk (P1) - The PR contains no live monitor evidence that the newly merged safeguards caused a material regression requiring emergency containment.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Rollback surface 6 files; 60 added, 1,088 removed The change is a broad but mechanically scoped reversal of the immediately preceding exact-review fix.

Root-cause cluster

Relationship: partial_overlap
Canonical: #840
Summary: This draft reverses the merged exact-review and publisher protections from the canonical implementation PR; whether that reversal is warranted depends on a separate live operational signal.

Members:

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

Merge-risk options

Maintainer options:

  1. Hold as a verified-incident rollback (recommended)
    Keep the branch in draft until a maintainer has live evidence that the current queue safeguards are causing a material regression.
  2. Accept the rollback trade-off
    If a live incident is verified, merge with an explicit decision to restore closed-target dispatch and generic retry fallback behavior as temporary containment.
  3. Close after healthy monitoring
    Close the draft if the monitor remains healthy because retaining the merged safeguards is safer than proactively restoring the prior failure modes.

Technical review

Best possible solution:

Keep the protections from #840 on main; use this branch only if a documented monitor signal establishes a material regression and verifies that the rollback is the safest containment action.

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

Not applicable as a reported bug reproduction: this is a contingency rollback, and the supplied context does not establish a current material regression from the merged behavior.

Is this the best way to solve the issue?

No for immediate merge: the rollback is mechanically coherent, but it is only the best solution after monitor evidence proves that its known regressions are preferable to the live incident it would contain.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: This is a bounded operational rollback proposal for a recent queue and publication reliability change, with no confirmed live emergency in the supplied evidence.
  • add merge-risk: 🚨 availability: The rollback removes safeguards that prevent unnecessary dispatch of terminal review items and preserve durable dispatcher pacing.
  • add merge-risk: 🚨 automation: The affected queue and publisher paths determine whether review batches dispatch and whether terminal publication failures are retried.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body summarizes focused tests and a source-level exact-rollback review, but it does not provide live monitor evidence of the regression that would justify deploying this rollback or demonstrate containment after it. Any posted evidence should redact private endpoints, IP addresses, tokens, and other sensitive details. 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:

  • P2: This is a bounded operational rollback proposal for a recent queue and publication reliability change, with no confirmed live emergency in the supplied evidence.
  • merge-risk: 🚨 availability: The rollback removes safeguards that prevent unnecessary dispatch of terminal review items and preserve durable dispatcher pacing.
  • merge-risk: 🚨 automation: The affected queue and publisher paths determine whether review batches dispatch and whether terminal publication failures are retried.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body summarizes focused tests and a source-level exact-rollback review, but it does not provide live monitor evidence of the regression that would justify deploying this rollback or demonstrate containment after it. Any posted evidence should redact private endpoints, IP addresses, tokens, and other sensitive details. 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:

Likely related people:

Rank-up moves

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

  • Attach redacted monitor or live-run evidence showing the material regression attributed to the merged behavior.
  • Show that the rollback contains the observed regression in the relevant environment, or close the draft after a healthy monitoring window.

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. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. 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