Visitar URL original
fix(ci): recover Security Review from HTTP 500 reads by joshavant · Pull Request #155196 · openclaw/openclaw · GitHub
Skip to content

fix(ci): recover Security Review from HTTP 500 reads - #155196

Merged
joshavant merged 1 commit into
mainfrom
fix/security-review-http500-retries
Sep 21, 2026
Merged

fix(ci): recover Security Review from HTTP 500 reads#155196
joshavant merged 1 commit into
mainfrom
fix/security-review-http500-retries

Conversation

@joshavant

@joshavant joshavant commented Sep 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Related: #154917

What Problem This Solves

Fixes Security Review failing immediately when a read-only GitHub API request returns a temporary HTTP 500.

User Impact

Security Review can recover automatically within the existing retry budget. Persistent failures still block approval, and write requests are not retried.

Why This Change Was Made

The shared API client now handles HTTP 500 alongside its existing transient-error retries: up to three GET/HEAD retries after one, two, and four seconds, within the original 30-second timeout. HTTP errors identify the request method and endpoint.

The reported failure on #154917 omitted the failing request's method and endpoint. This closes the confirmed read-retry gap and improves diagnostics; it does not establish which request failed in that historical run.

Evidence

  • Nine regression and diagnostic assertions failed on the original production code; all 253 focused and sibling tests pass with the fix.
  • Real CLI tests verify recovery after a temporary HTTP 500 and failure without approval after retries are exhausted. API tests cover GET/HEAD recovery and POST/PATCH/DELETE failures without retries.
  • Local test times: combined review 8.81 s; dependency guard 2.46 s. The four-file wrapper run completed in 12.74 s.
  • Hosted CI passed all 253 focused and sibling tests. Summed individual test times for the changed files: combined review 12.03 s; dependency guard 2.17 s.
  • Independent review found no actionable P0–P2 issues.
  • Changed-file checks passed: formatting, root test typecheck, lint, line-cap, dependency guards, and all dead-export scans. git diff --check passed.

@clawsweeper

clawsweeper Bot commented Sep 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@joshavant
joshavant marked this pull request as ready for review September 21, 2026 21:04
@joshavant
joshavant requested a review from a team as a code owner September 21, 2026 21:04
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation scripts Repository scripts size: S maintainer Maintainer-authored PR labels Sep 21, 2026
@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 21, 2026
@clawsweeper

clawsweeper Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 21, 2026, 5:17 PM ET / 21:17 UTC.

ClawSweeper review

What this changes

Security Review retries temporary HTTP 500 responses on read-only GitHub requests and includes the request method and endpoint in failure messages.

Merge readiness

✅ Ready for maintainer review

Keep open: this focused repair remains necessary on current main. The retry and diagnostic changes preserve existing safeguards, and no actionable patch defects were found.

Priority: P2
Reviewed head: 2a86b9f3330e4fcaa605a07a4e3fc7cb4ab007e6

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with meaningful regression coverage, matching documentation, and no blocking findings.
Proof confidence 🌊 off-meta tidepool Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its Security Review CLI tests cover recovery and exhaustion through mocked fetch; no real-transport recovery trace is claimed, and the patch does not materially change authority.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its Security Review CLI tests cover recovery and exhaustion through mocked fetch; no real-transport recovery trace is claimed, and the patch does not materially change authority.
Evidence reviewed 9 items Pinned introduced change: The merge-base-to-head patch adds HTTP 500 to the existing retry set and adds method/path diagnostics; the remaining changes document and test those behaviors.
Current-main necessity: Fetched main still retries only 502, 503, and 504. Its shared client is unchanged from the pinned merge base, so the HTTP 500 recovery gap remains.
Latest-release check: The v2026.9.5 source also excludes HTTP 500 from the retry set. After local release-blob retrieval failed, the same tagged file was successfully inspected through GitHub's contents API.
Findings None None.
Security None None.

How this fits together

Security Review reads GitHub pull-request, approval, and CI metadata before publishing merge-gate statuses. Its shared HTTP client supplies bounded reads to the review scripts and dependency guard.

flowchart TD
  A[Pull request or CI event] --> B[Security Review]
  B --> C[Shared GitHub client]
  C --> D{Read returns transient error?}
  D -->|Yes| E[Retry within existing timeout]
  E --> C
  D -->|Successful response| F[Evaluate approvals and CI]
  D -->|Exhausted or write error| G[Report failure]
  F --> H[Publish gate status]
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Tests +36, Docs +5, Other 0. Total +41 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 2 45 9 +36
Docs 1 5 0 +5
Config 0 0 0 0
Generated 0 0 0 0
Other 1 2 2 0
Total 4 52 11 +41

Review metrics

None.

Technical review

Best possible solution:

Keep transient-read recovery in the existing shared client while preserving bounded retries, non-replayed writes, and approval revalidation.

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

Yes, from source: an HTTP 500 on a Security Review GET currently bypasses retry handling and throws immediately. No runtime reproduction was executed here, and the historical failing request remains unidentified.

Is this the best way to solve the issue?

Yes. Extending the existing transient-read policy is the narrowest repair, and the diagnostic change preserves structured error handling.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against a3bcdf75ceec.

Labels

Label changes:

  • add P2: This is a bounded CI reliability repair for transient GitHub read failures, with no demonstrated core-runtime outage.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its Security Review CLI tests cover recovery and exhaustion through mocked fetch; no real-transport recovery trace is claimed, and the patch does not materially change authority.

Label justifications:

  • P2: This is a bounded CI reliability repair for transient GitHub read failures, with no demonstrated core-runtime outage.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The collaborator-authored PR is exempt from the external-contributor proof gate. Its Security Review CLI tests cover recovery and exhaustion through mocked fetch; no real-transport recovery trace is claimed, and the patch does not materially change authority.

Evidence

What I checked:

  • Pinned introduced change: The merge-base-to-head patch adds HTTP 500 to the existing retry set and adds method/path diagnostics; the remaining changes document and test those behaviors. (scripts/github/guard-shared.mjs:9, 2a86b9f3330e)
  • Current-main necessity: Fetched main still retries only 502, 503, and 504. Its shared client is unchanged from the pinned merge base, so the HTTP 500 recovery gap remains. (scripts/github/guard-shared.mjs:9, a3bcdf75ceec)
  • Latest-release check: The v2026.9.5 source also excludes HTTP 500 from the retry set. After local release-blob retrieval failed, the same tagged file was successfully inspected through GitHub's contents API. (scripts/github/guard-shared.mjs:8, ec9c1a13db89)
  • Retry and security boundaries: The existing GET/HEAD restriction, three retry delays, shared 30-second deadline, bounded response bodies, and numeric error status remain intact. Callers use status/type checks rather than depending on the old error-message prefix. No authority-bearing value, permission, dependency, or workflow execution source changes. (scripts/github/guard-shared.mjs:345, 2a86b9f3330e)
  • Regression coverage and proof limits: The added CLI cases exercise temporary-500 recovery and exhausted retries without approval writes. The API matrix covers GET/HEAD recovery and non-retried POST/PATCH/DELETE failures. The CLI fixture replaces fetch and the clock, so these are regression tests rather than real-transport proof. Tests were inspected but not executed during this read-only review. (test/scripts/security-review-script.test.ts:140, 2a86b9f3330e)
  • Hosted validation: GitHub confirms that the linked checks-node-compact-small-1 job succeeded for the exact reviewed head. The captured PR body reports 253 focused and sibling tests passing and records local and CI test costs; the job result alone does not establish live HTTP recovery. (2a86b9f3330e)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • openclaw/openclaw-secops: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

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.

@joshavant
joshavant merged commit 25b469e into main Sep 21, 2026
137 of 138 checks passed
@joshavant
joshavant deleted the fix/security-review-http500-retries branch September 21, 2026 21:27
@github-actions github-actions Bot mentioned this pull request Sep 21, 2026
Open
@MarMarLabs MarMarLabs mentioned this pull request Sep 21, 2026
Closed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 22, 2026
@joshavant
fix(ci): retry Security Review HTTP 500 reads (openclaw#155196)
b5b490c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

docs Improvements or additions to documentation maintainer Maintainer-authored PR P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. scripts Repository scripts size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@joshavant