fix(state): retry transient invalid 2xx bodies in worker record requests

[steipete]

Problem

Run 30569173886 failed hydration with WorkerRecordRequestError: Worker record request failed (200): invalid_json_body — the edge transiently served a blank 200. #888 made this fail loudly (correct — better than crashing callers far from the request), but a single transient blank body still kills the whole review run.

Fix

signedPost now retries invalid 2xx bodies within the existing SIGNED_REQUEST_MAX_ATTEMPTS budget (3 attempts, exponential backoff), throwing the same invalid_json_body error with body snippet if the condition persists. 4xx behavior unchanged (deterministic, no retry); 5xx/network retry behavior unchanged.

Proof

• Updated tests: persistent blank/HTML/null 2xx bodies now assert 3 attempts before the error; new recovery test asserts a blank 200 followed by a valid 200 returns the value on attempt 2.
• Suite: worker-records-request 13/13.
• Autoreview (codex, xhigh): clean, "patch is correct (0.99)".

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed July 30, 2026, 2:29 PM ET / 18:29 UTC.

ClawSweeper review

What this changes

Retries Worker record POST requests after a transient invalid 2xx response body and expands the request tests for persistent and recovering malformed-success responses.

Merge readiness

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

Keep this PR open: it addresses a real worker-record hydration failure, but the new outer retry loop can combine with signedRequest's existing 5xx/network retries and exceed the stated bounded request budget. It also needs after-fix real behavior proof before merge.

Priority: P1
Reviewed head: 0c32279cf5e3b9981251e1350b273e86dbf30534

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The recovery idea is focused and test-backed, but the retry-budget defect and absence of real behavior proof keep it below merge-ready quality.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: The PR describes 13 mocked request tests, but it does not include an after-fix live Worker-record request, integration transcript, or redacted runtime log demonstrating recovery from a malformed 2xx response. 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	🦐 gold shrimp (3/6)	2 actionable review findings remain.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: The PR describes 13 mocked request tests, but it does not include an after-fix live Worker-record request, integration transcript, or redacted runtime log demonstrating recovery from a malformed 2xx response. 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	Prior invalid-body behavior: The related merged PR established that invalid 2xx JSON bodies must surface as invalid_json_body rather than propagate a null value; this PR is a narrower follow-up to tolerate a transient edge response. Nested retry budgets: The proposed signedPost loop calls signedRequest again for each invalid successful response. The related implementation documents that signedRequest already retries 5xx and network failures, so an invalid 2xx followed by a retryable transport failure can consume more than the claimed three total HTTP attempts. Focused test coverage: The branch adds persistent blank/HTML/null-body cases and a recovery case, but those mocked tests do not exercise the mixed invalid-body plus 5xx/network path that exposes the combined retry budget.
Findings	2 actionable findings	[P2] Preserve one total retry budget [P3] Remove the release-owned changelog entry
Security	None	None.

How this fits together

ClawSweeper's Worker-record client sends signed requests from review and apply automation to the canonical record store. signedPost interprets the Worker response and returns record data to hydration and state-management callers, so transport retry behavior affects review-run availability.

flowchart LR
  A[Review and apply jobs] --> B[Worker record client]
  B --> C[Signed POST request]
  C --> D[Canonical Worker store]
  D --> E[HTTP response body]
  E --> F{Valid JSON object?}
  F -->|Yes| G[Hydration and state operations]
  F -->|No| H[Bounded retry or request error]

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: The PR describes 13 mocked request tests, but it does not include an after-fix live Worker-record request, integration transcript, or redacted runtime log demonstrating recovery from a malformed 2xx response. 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 one total retry budget (P2) - signedRequest already owns retry handling for 5xx and network failures, so calling it from this new invalid-body loop can exceed SIGNED_REQUEST_MAX_ATTEMPTS when failure modes mix. Share the counter/backoff state or fold invalid-body validation into the existing retry loop so the documented three-attempt bound remains true.
• Remove the release-owned changelog entry (P3) - Repository policy reserves CHANGELOG.md for release ownership during normal PR work. Keep the user-visible release context in the PR body or commit message and leave this entry for the release process.
• Resolve merge risk (P1) - A malformed 2xx can enter the new outer loop, while later 5xx/network failures still use signedRequest's internal retry loop; that can exceed the configured request and backoff budget, delaying or amplifying failed automation runs.
• Resolve merge risk (P1) - The PR body provides mocked test results but no after-fix run against the Worker-record request path, so the observed edge-recovery claim is not yet demonstrated in a real setup.
• Complete next step (P2) - This PR needs contributor-provided real behavior proof and a corrected shared retry-budget design; those are not safe repair-lane handoffs.

Findings

• [P2] Preserve one total retry budget — scripts/worker-records.ts:735
• [P3] Remove the release-owned changelog entry — CHANGELOG.md:21

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch scope	3 files affected; 58 added, 27 removed	The implementation is focused on one request helper, its tests, and a release-owned changelog entry.
Request-test suite	13 asserted cases	The PR expands malformed-2xx coverage, but does not cover interaction with the existing transport retry loop.

Merge-risk options

Maintainer options:

1. Unify the retry budget (recommended)
   Move invalid-2xx handling into the existing retry accounting, or pass shared attempt state, and add a mixed invalid-body plus retryable-failure test before merge.
2. Pause the recovery change
   Keep the current fail-loud behavior if a single bounded retry policy cannot be preserved without a broader Worker request-contract decision.

Technical review

Best possible solution:

Keep one shared retry counter and backoff budget across malformed-2xx, 5xx, and network failures, remove the release-owned changelog edit, then attach a redacted live or integration-style recovery transcript showing the request succeeds after one invalid 2xx body.

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

No high-confidence live reproduction is included for the after-fix behavior. The mocked request test path is clear from source, and the PR body reports a real blank-200 failure, but it does not show a real recovery run on the current change.

Is this the best way to solve the issue?

No. Retrying invalid bodies is a reasonable direction, but the retry must share the existing signedRequest budget so a mixed malformed-body and transport-failure sequence remains bounded.

Full review comments:

• [P2] Preserve one total retry budget — scripts/worker-records.ts:735
signedRequest already owns retry handling for 5xx and network failures, so calling it from this new invalid-body loop can exceed SIGNED_REQUEST_MAX_ATTEMPTS when failure modes mix. Share the counter/backoff state or fold invalid-body validation into the existing retry loop so the documented three-attempt bound remains true.
  Confidence: 0.94
• [P3] Remove the release-owned changelog entry — CHANGELOG.md:21
  Repository policy reserves CHANGELOG.md for release ownership during normal PR work. Keep the user-visible release context in the PR body or commit message and leave this entry for the release process.
  Confidence: 0.98

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 b4d1edea05cc.

Labels

Label changes:

• add P1: A transient Worker response currently aborts review hydration, and the proposed correction affects the availability of active review automation.
• add merge-risk: 🚨 availability: Nested retry paths can exceed the intended bounded attempt/backoff budget and delay or overload failing operational runs.
• add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• 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 describes 13 mocked request tests, but it does not include an after-fix live Worker-record request, integration transcript, or redacted runtime log demonstrating recovery from a malformed 2xx response. 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: A transient Worker response currently aborts review hydration, and the proposed correction affects the availability of active review automation.
• merge-risk: 🚨 availability: Nested retry paths can exceed the intended bounded attempt/backoff budget and delay or overload failing operational runs.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR describes 13 mocked request tests, but it does not include an after-fix live Worker-record request, integration transcript, or redacted runtime log demonstrating recovery from a malformed 2xx response. 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:

• Prior invalid-body behavior: The related merged PR established that invalid 2xx JSON bodies must surface as invalid_json_body rather than propagate a null value; this PR is a narrower follow-up to tolerate a transient edge response. (scripts/worker-records.ts:734, 0533a8dc00a8)
• Nested retry budgets: The proposed signedPost loop calls signedRequest again for each invalid successful response. The related implementation documents that signedRequest already retries 5xx and network failures, so an invalid 2xx followed by a retryable transport failure can consume more than the claimed three total HTTP attempts. (scripts/worker-records.ts:735, 0c32279cf5e3)
• Focused test coverage: The branch adds persistent blank/HTML/null-body cases and a recovery case, but those mocked tests do not exercise the mixed invalid-body plus 5xx/network path that exposes the combined retry budget. (test/worker-records-request.test.ts:60, 0c32279cf5e3)
• Repository policy: The target repository policy says normal PRs should not edit release-owned CHANGELOG.md; release context belongs in the PR body and commit message instead. (AGENTS.md:39, b4d1edea05cc)

Likely related people:

• steipete: The merged predecessor for the same Worker-record response-validation path is attributed to this contributor and establishes the current invalid-body contract that this PR extends. (role: recent area contributor; confidence: high; commits: 0533a8dc00a8; files: scripts/worker-records.ts, test/worker-records-request.test.ts)

Rank-up moves

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

• Use shared retry accounting and add coverage for an invalid 2xx followed by a retryable transport failure.
• Add redacted real behavior proof such as a Worker-record integration run or recovery log; after updating the PR body, request a fresh review if one does not start automatically.
• Remove the release-owned CHANGELOG.md modification.

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.