Skip to content

revert: undo #892 legacy terminal reconciliation - #893

Closed
brokemac79 wants to merge 1 commit into
mainfrom
codex/revert-csw-065-legacy-reconciliation
Closed

revert: undo #892 legacy terminal reconciliation#893
brokemac79 wants to merge 1 commit into
mainfrom
codex/revert-csw-065-legacy-reconciliation

Conversation

@brokemac79

@brokemac79 brokemac79 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Disposition

Do not merge — this revert is withdrawn and will be closed.

The accepted P1 review finding is correct: the production dry run only proves that the cited stranded row is protocol-v2. It does not invalidate the separately tested bounded protocol-v1 reconciliation that this branch removes. Retaining that current-main safeguard is safer than globally reverting it while the protocol-v2 row remains unresolved.

Accepted review finding

  • P1: preserve genuine protocol-v1 reconciliation. This branch would restore retention of ordinary protocol-v1 rows that are terminal, ownerless, and otherwise safe to reconcile. The current exact-review queue branch review reproduced that regression and accepted the finding.

Evidence

  • Maintenance dry run 30270961772 used apply: false and reported legacyTerminalCandidates: 0, legacyTerminalEligible: 0, and legacyTerminalChanged: 0 for the reported root; no queue mutation occurred.
  • The root item remains a pending publication with an open retry_exhausted dead letter. Its originating run 30091560737 recorded protocol version 2 and LIVE_PROCEEDED: true.
  • Focused closeout proof on this branch: pnpm run build:all; dashboard suite (240 passing); repair-client suite (6 passing); publication-batch suite (55 passing).
  • codex review --uncommitted confirmed no local patch. git fetch origin main --prune && codex review --base origin/main accepted the P1: this exact revert removes the only bounded terminal cleanup for genuine protocol-v1 rows without a replacement.

Correct successor scope

The follow-up must preserve current-main protocol-v1 behavior and target only the stranded protocol-v2 state-batch lifecycle. Before any apply capability is proposed, it must demonstrate the producer-run completion, acknowledgement/delivery terminal condition, absence of active lease/batch ownership, and absence of newer authority for the exact row. This PR does not change queue state or authorize an apply.

Original proposal

This branch exactly reverts merge commit 2345e9fb14dc302404125038270996ed53e59858 (#892). It is retained only as review evidence for the withdrawn revert; it must not merge.

Links

This reverts commit 2345e9f.

The merged protocol-v1 path does not select the stranded protocol-v2 root row.
@brokemac79

Copy link
Copy Markdown
Contributor Author

@clawsweeper review

@clawsweeper

clawsweeper Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jul 27, 2026
@clawsweeper

clawsweeper Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 27, 2026, 9:58 AM ET / 13:58 UTC.

ClawSweeper review

What this changes

This PR exactly reverts the recently merged bounded reconciliation path, client counters, documentation, and tests for terminal protocol-v1 exact-review publication rows.

Merge readiness

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

Keep this PR open for revision rather than merge: the production dry run establishes that the cited stranded row is protocol-v2 and therefore not repaired by the new protocol-v1 path, but it does not establish that removing the bounded protocol-v1 reconciliation is safe. The unchanged branch still removes a recently merged, separately validated cleanup path for genuine protocol-v1 rows.

Priority: P1
Reviewed head: 8523b3e87221693acdea032f8e787deba5cc1ea6

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The PR has a clear incident observation but currently removes a separately validated safety path without branch-based proof that the regression is acceptable.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The linked production dry run is useful evidence that the reported row is not protocol-v1, but it ran against the merged behavior and does not show the reverted branch safely handling a genuine protocol-v1 row or the protocol-v2 lifecycle; add redacted branch-based queue output or live logs before merge. 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 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The linked production dry run is useful evidence that the reported row is not protocol-v1, but it ran against the merged behavior and does not show the reverted branch safely handling a genuine protocol-v1 row or the protocol-v2 lifecycle; add redacted branch-based queue output or live logs before merge. 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: The merged protocol-v1 reconciliation path remains on current main and includes terminal candidate selection plus safety checks for ordinary publication rows; this PR deletes that path rather than narrowing it to the reported protocol-v2 incident.
Focused behavior coverage removed: The branch deletes the tests that proved terminal protocol-v1 removal, dry-run non-mutation, command-context retention, active-batch protection, and newer-authority protection.
Production evidence has a narrower conclusion: The linked dry run reported zero legacy-terminal candidates, which supports the claim that the cited protocol-v2 row was not selected; it does not demonstrate that genuine protocol-v1 terminal rows should no longer be reconciled.
Findings 1 actionable finding [P1] Preserve reconciliation for genuine protocol-v1 rows
Security None None.

How this fits together

The exact-review publication queue retains work items until their GitHub targets and durable acknowledgement state establish completion. Its maintenance reconciliation endpoint reads queue state and bounded target status, then removes only safe stale publication rows while reporting the results to repair tooling and operators.

flowchart LR
  A[Queued publication rows] --> B[Maintenance reconciliation request]
  B --> C[Ownership and lineage safety checks]
  C --> D[Bounded GitHub target-state read]
  D --> E[Safe terminal-row removal]
  E --> F[Queue state and reconciliation counters]
  F --> G[Operator dry-run or apply review]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The linked production dry run is useful evidence that the reported row is not protocol-v1, but it ran against the merged behavior and does not show the reverted branch safely handling a genuine protocol-v1 row or the protocol-v2 lifecycle; add redacted branch-based queue output or live logs before merge. 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 reconciliation for genuine protocol-v1 rows (P1) - This unchanged branch removes the bounded terminal-reconciliation path and all of its safety coverage. The linked dry run only shows that the cited stranded row is protocol-v2 and was not selected; it does not invalidate cleanup for genuine protocol-v1 rows, which current main deliberately supports. Retain that path and address the protocol-v2 lifecycle separately.
  • Resolve merge risk (P1) - Merging removes the only current-main bounded cleanup path for genuine protocol-v1 terminal publication rows even though the cited dry run only disproves its applicability to one protocol-v2 incident.
  • Resolve merge risk (P1) - The branch provides no after-revert queue evidence showing that legitimate protocol-v1 rows remain safe while the actual protocol-v2 completion and acknowledgement lifecycle is addressed elsewhere.
  • Complete next step (P2) - The remaining action is to resolve the regression in this revert and prove the protocol-v2 lifecycle boundary; that is not a safe mechanical repair of the submitted branch.
  • Improve patch quality - Preserve the genuine protocol-v1 reconciliation behavior or demonstrate a narrower replacement.
  • Improve patch quality - Add redacted after-fix queue output from the branch covering both the cited protocol-v2 row and a genuine protocol-v1 terminal row; updating the PR body triggers re-review, or a maintainer can comment @clawsweeper re-review.

Findings

  • [P1] Preserve reconciliation for genuine protocol-v1 rows — dashboard/exact-review-queue.ts:2873
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Reverted surface 5 files affected; 33 added, 694 removed The PR removes the implementation, typed response counters, operational plan text, and 457 focused test lines for a recently merged maintenance behavior.

Merge-risk options

Maintainer options:

  1. Preserve protocol-v1 reconciliation (recommended)
    Revise this work so the bounded protocol-v1 cleanup remains intact and pursue the protocol-v2 row as a separate, lifecycle-proven repair.
  2. Explicitly accept the cleanup regression
    Merge the revert only if maintainers intentionally retire protocol-v1 terminal reconciliation and accept that eligible legacy rows will remain queued until another supported path exists.

Technical review

Best possible solution:

Keep the proven protocol-v1 reconciliation path, then implement a separate narrowly scoped repair for the protocol-v2 stranded-row lifecycle only after its durable completion and acknowledgement conditions are demonstrated.

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

No high-confidence after-fix reproduction is present. The diff and removed tests make the regression source-reproducible, but the submitted evidence does not run the reverted branch against both a genuine protocol-v1 terminal row and the cited protocol-v2 row.

Is this the best way to solve the issue?

No. The dry run correctly shows that the cited protocol-v2 row is outside the protocol-v1 selector, but removing the independently safe protocol-v1 path is broader than necessary; preserve it and repair protocol-v2 separately.

Full review comments:

  • [P1] Preserve reconciliation for genuine protocol-v1 rows — dashboard/exact-review-queue.ts:2873
    This unchanged branch removes the bounded terminal-reconciliation path and all of its safety coverage. The linked dry run only shows that the cited stranded row is protocol-v2 and was not selected; it does not invalidate cleanup for genuine protocol-v1 rows, which current main deliberately supports. Retain that path and address the protocol-v2 lifecycle separately.
    Confidence: 0.94

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P1: This PR would remove a currently supported queue-cleanup behavior for legacy publication rows, with an existing user-impacting maintenance workflow left unaddressed.
  • merge-risk: 🚨 compatibility: Existing operators and legacy protocol-v1 queue rows would lose the bounded reconciliation behavior and result counters introduced on current main.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The linked production dry run is useful evidence that the reported row is not protocol-v1, but it ran against the merged behavior and does not show the reverted branch safely handling a genuine protocol-v1 row or the protocol-v2 lifecycle; add redacted branch-based queue output or live logs before merge. 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:

  • Current-main behavior: The merged protocol-v1 reconciliation path remains on current main and includes terminal candidate selection plus safety checks for ordinary publication rows; this PR deletes that path rather than narrowing it to the reported protocol-v2 incident. (dashboard/exact-review-queue.ts:3492, 2345e9fb14dc)
  • Focused behavior coverage removed: The branch deletes the tests that proved terminal protocol-v1 removal, dry-run non-mutation, command-context retention, active-batch protection, and newer-authority protection. (test/dashboard-worker.test.ts:5543, 8523b3e87221)
  • Production evidence has a narrower conclusion: The linked dry run reported zero legacy-terminal candidates, which supports the claim that the cited protocol-v2 row was not selected; it does not demonstrate that genuine protocol-v1 terminal rows should no longer be reconciled. (src/repair/exact-review-batch-queue-client.ts:35, 2345e9fb14dc)
  • Prior review remains unresolved: The latest completed ClawSweeper review at the unchanged head SHA raised the P1 requirement to preserve reconciliation for genuine protocol-v1 rows and requested branch-based evidence covering both the cited protocol-v2 row and a genuine protocol-v1 terminal row. (dashboard/exact-review-queue.ts:2873, 8523b3e87221)

Likely related people:

  • brokemac79: The same contributor authored the merged protocol-v1 reconciliation implementation and its focused queue/client tests, so they have direct history on the path this PR removes. (role: introduced current reconciliation behavior; confidence: high; commits: 00cecde3858c, 2345e9fb14dc; files: dashboard/exact-review-queue.ts, src/repair/exact-review-batch-queue-client.ts, test/dashboard-worker.test.ts)

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.

History

Review history (1 earlier review cycle)
  • reviewed 2026-07-27T13:52:42.470Z sha 8523b3e :: needs real behavior proof before merge. :: [P1] Preserve reconciliation for genuine protocol-v1 rows

@brokemac79

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@brokemac79

Copy link
Copy Markdown
Contributor Author

Closing as documented in the PR body: preserve #892's protocol-v1 safeguard; the correct follow-up is a separately proven protocol-v2 lifecycle repair.

@brokemac79 brokemac79 closed this Jul 27, 2026
@clawsweeper

clawsweeper Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

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

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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.

1 participant