Skip to content

draft: revert #843 dispatch 422 classification#845

Draft
brokemac79 wants to merge 1 commit into
mainfrom
codex/revert-pr-843
Draft

draft: revert #843 dispatch 422 classification#845
brokemac79 wants to merge 1 commit into
mainfrom
codex/revert-pr-843

Conversation

@brokemac79

Copy link
Copy Markdown
Contributor

Draft rollback — do not merge pre-emptively

This draft exactly reverts squash commit df5ec544d6d6220e84cc6886ee999dd0885b09ab from #843. It exists only as a ready rollback if production evidence shows #843 is harmful. It must remain draft until Martin explicitly approves merging it after a demonstrated regression.

What this restores

The revert restores the pre-#843 exact-review dispatch behavior, including:

  • top-level source_head_sha dispatch compatibility;
  • the previous HTTP 422 classification; and
  • no persisted redacted validation detail.

Intentionally rejected Codex-review findings

Both dirty and branch review correctly identify that this exact revert would reintroduce the #843 failure modes:

  1. A synchronize event carrying both a source head and review options can exceed GitHub's 10-key client_payload limit and receive a 422.
  2. Secondary-rate-limit, opaque, and shared-validation 422s can again be parked as permanent item failures instead of receiving bounded global backoff.

Those findings are intentionally not fixed because this is an exact emergency rollback of #843, requested in advance. They are the reason this PR must remain draft and must not be merged without an observed #843 regression and explicit maintainer approval.

Validation

pnpm run build:all
node --test --test-name-pattern='(bounds item-specific terminal-state check failures|parks an item after repeated item-specific target-state failures|parks permanent dispatch rejection|globally backs off GitHub outages)' test/dashboard-worker.test.ts
node --test --test-name-pattern='exact event item' test/sweep-workflow.test.ts
git diff --check

All commands passed. actionlint is unavailable on this host.

Merge condition and rollout

Do not merge merely because this draft exists. First capture concrete evidence that #843 causes a production regression, identify why the safer forward fix cannot be used, and obtain Martin's explicit merge approval. No automerge, gates, or ClawSweeper command is requested for this draft.

Links

@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: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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, 12:01 AM ET / 04:01 UTC.

ClawSweeper review

What this changes

This draft exactly reverts the merged dispatch-422 repair by restoring top-level source-head dispatch data, older 422 classification, and the prior durable-state shape.

Merge readiness

Blocked until real behavior proof from a real setup is added - 10 items remain

Keep this draft open as an incident-only contingency, not a merge candidate. Its body and diff explicitly restore the two dispatch failure modes that current main fixes, and no production regression from the current implementation is supplied.

Priority: P1
Reviewed head: 5ea35475eff5e19b7026c7521934c6282c0a43b1
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The branch is a coherent rollback artifact, but it intentionally restores known P1 dispatch failures and lacks real-world evidence that current main needs that rollback.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The body reports builds and focused mocked tests, but it provides no after-fix real dispatch or production evidence; because this is an incident rollback, concrete redacted runtime proof is required 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 🧂 unranked krab (1/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The body reports builds and focused mocked tests, but it provides no after-fix real dispatch or production evidence; because this is an incident rollback, concrete redacted runtime proof is required 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 retains the forward-safe payload reader: Current main reads the source revision from either the nested queue claim or the legacy top-level field, while this draft removes the nested location from the workflow run name and payload parser.
Current main preserves scoped 422 handling and safe diagnostic state: The merged implementation includes durable dispatch-failure detail and a distinct validation-unknown classification; this draft removes those fields and associated behavior.
Merged provenance: The supplied PR history identifies commit df5ec54 as the merged implementation of the dispatch-422 repair, and this branch contains one commit explicitly reverting it.
Findings 2 actionable findings [P1] Preserve nested source-head compatibility
[P1] Do not make opaque 422 responses terminal again
Security None None.

How this fits together

The exact-review queue accepts repository events and explicit review commands, creates a GitHub workflow dispatch payload, and records dispatch results in durable queue state. The sweep workflow consumes that payload to start a targeted Codex review, so payload compatibility and retry classification determine whether legitimate review work runs or is parked.

flowchart LR
  Event[Repository event or command] --> Queue[Exact-review queue]
  Queue --> Payload[GitHub dispatch payload]
  Payload --> Workflow[Sweep workflow]
  Workflow --> Review[Targeted Codex review]
  Queue --> Outcome[Retry or parked outcome]
  Outcome --> State[Durable queue state]
Loading

Decision needed

Question Recommendation
Has a production regression from the merged dispatch-422 repair been demonstrated strongly enough that restoring its known failure modes is the safer incident response? Keep the draft as contingency: Leave the rollback unmerged until redacted production evidence identifies a current-main regression and a safer forward fix is ruled out.

Why: The branch intentionally reintroduces known availability and payload-compatibility failures, so code review cannot decide whether an unobserved operational regression justifies that tradeoff.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The body reports builds and focused mocked tests, but it provides no after-fix real dispatch or production evidence; because this is an incident rollback, concrete redacted runtime proof is required 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 nested source-head compatibility (P1) - This rollback restores the top-level source-head path that can push a command-plus-synchronize dispatch over GitHub's 10-key client-payload limit. Keep the nested queue_claim source-head reader/writer unless an observed regression proves that compatibility is unsafe.
  • Do not make opaque 422 responses terminal again (P1) - This rollback removes the distinction that keeps opaque or shared validation 422s retryable through bounded global backoff. That can park a valid exact-review update permanently without creating a worker run, so retain the current scoped classification or replace it with a proven forward fix.
  • Resolve merge risk (P1) - Merging this branch would restore the command-plus-synchronize payload path that can exceed GitHub's 10-key client-payload limit and prevent an exact review from starting.
  • Resolve merge risk (P1) - Merging this branch would again treat opaque or shared 422 responses as item-scoped terminal failures, potentially parking valid review work instead of applying bounded global backoff.
  • Resolve merge risk (P1) - No observed production regression from the current main implementation is included, so an emergency rollback would lack an evidence-backed trigger.
  • Improve patch quality - Attach redacted evidence of a regression caused by current main, such as a controlled dispatch trace or runtime logs.
  • Improve patch quality - Explain why a narrow forward fix cannot preserve the current payload and 422 safeguards.
  • Improve patch quality - Obtain Martin's explicit approval before marking the draft ready for review.

Findings

  • [P1] Preserve nested source-head compatibility — .github/workflows/sweep.yml:20
  • [P1] Do not make opaque 422 responses terminal again — dashboard/exact-review-queue.ts:2497
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Rollback scope 3 files affected; 25 additions and 296 deletions The branch removes the current dispatch safeguards and their focused regression coverage across workflow, queue, and tests.

Merge-risk options

Maintainer options:

  1. Keep the rollback draft unmerged (recommended)
    Retain it as an incident contingency until production evidence and explicit approval justify restoring the known dispatch failure modes.
  2. Use a narrow forward fix
    If current main regresses, repair the demonstrated condition without removing the nested payload compatibility or global handling for opaque shared 422s.

Technical review

Best possible solution:

Retain the draft only as a documented emergency option; if a current-main regression is observed, capture redacted runtime evidence and prefer the narrowest forward repair unless Martin explicitly approves this rollback.

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

Yes. Current source and the merged regression coverage define the command-plus-synchronize payload and opaque-422 cases that this branch intentionally restores, but no fresh production regression caused by current main is provided.

Is this the best way to solve the issue?

No. An exact revert is not the best default because it knowingly restores the documented failure paths; a demonstrated regression should first receive the narrowest forward-compatible repair.

Full review comments:

  • [P1] Preserve nested source-head compatibility — .github/workflows/sweep.yml:20
    This rollback restores the top-level source-head path that can push a command-plus-synchronize dispatch over GitHub's 10-key client-payload limit. Keep the nested queue_claim source-head reader/writer unless an observed regression proves that compatibility is unsafe.
    Confidence: 0.99
  • [P1] Do not make opaque 422 responses terminal again — dashboard/exact-review-queue.ts:2497
    This rollback removes the distinction that keeps opaque or shared validation 422s retryable through bounded global backoff. That can park a valid exact-review update permanently without creating a worker run, so retain the current scoped classification or replace it with a proven forward fix.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P1: This rollback can suppress real exact-review dispatches and affect active review automation if merged.
  • add merge-risk: 🚨 compatibility: The branch restores a top-level payload layout that conflicts with the nested source-head compatibility path on current main.
  • add merge-risk: 🚨 availability: The branch restores terminal parking for 422 cases that current main retains for bounded shared retry.
  • 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 body reports builds and focused mocked tests, but it provides no after-fix real dispatch or production evidence; because this is an incident rollback, concrete redacted runtime proof is required 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.

Label justifications:

  • P1: This rollback can suppress real exact-review dispatches and affect active review automation if merged.
  • merge-risk: 🚨 compatibility: The branch restores a top-level payload layout that conflicts with the nested source-head compatibility path on current main.
  • merge-risk: 🚨 availability: The branch restores terminal parking for 422 cases that current main retains for bounded shared retry.
  • 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 body reports builds and focused mocked tests, but it provides no after-fix real dispatch or production evidence; because this is an incident rollback, concrete redacted runtime proof is required 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 retains the forward-safe payload reader: Current main reads the source revision from either the nested queue claim or the legacy top-level field, while this draft removes the nested location from the workflow run name and payload parser. (.github/workflows/sweep.yml:20, df5ec544d6d6)
  • Current main preserves scoped 422 handling and safe diagnostic state: The merged implementation includes durable dispatch-failure detail and a distinct validation-unknown classification; this draft removes those fields and associated behavior. (dashboard/exact-review-queue.ts:109, df5ec544d6d6)
  • Merged provenance: The supplied PR history identifies commit df5ec54 as the merged implementation of the dispatch-422 repair, and this branch contains one commit explicitly reverting it. (dashboard/exact-review-queue.ts:2497, df5ec544d6d6)
  • Release position: The supplied release baseline is v0.3.0 at dc82491, which predates the merged repair; the forward-safe behavior is currently main-only rather than proven released. (df5ec544d6d6)

Likely related people:

  • brokemac79: The supplied history shows this contributor authored the merged stale-head and dispatch-422 work that defines the current exact-review behavior, as well as this rollback contingency. (role: recent area contributor; confidence: high; commits: 4bfea9218576, df5ec544d6d6, 5ea35475eff5; files: dashboard/exact-review-queue.ts, .github/workflows/sweep.yml, test/dashboard-worker.test.ts)
  • Martin: The PR body names Martin as the person whose explicit approval is required before this incident rollback may be merged. (role: explicit approval authority; confidence: medium; files: .github/workflows/sweep.yml, dashboard/exact-review-queue.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.

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

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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: 🧂 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.

1 participant