Skip to content

fix(releasecheck): default a 30s HTTP client instead of DefaultClient - #92

Merged
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/releasecheck-http-timeout
Aug 16, 2026
Merged

fix(releasecheck): default a 30s HTTP client instead of DefaultClient#92
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/releasecheck-http-timeout

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

releasecheck.normalizeOptions used http.DefaultClient when the caller did not pass a client. Public check-update CLIs (discrawl, gitcrawl, slacrawl, graincrawl, notcrawl) call it with context.Background(), so a black-holed api.github.com hangs forever.

Evidence

Red:  TestNormalizeOptionsDefaultsBoundedHTTPClient
      default Client is http.DefaultClient, which has no timeout
Green: GOWORK=off go test -count=1 ./...

Default client is now &http.Client{Timeout: 30s}, matching remote.NewClient. Caller-supplied clients are unchanged.

Real behavior proof

Behavior addressed: Release-check HTTP now has a 30s default timeout instead of DefaultClient.
Real environment tested: macOS, Go from the worktree, crawlkit /tmp/oc-impl-crawlkit head fbaa0dc.
Exact steps or command run after this patch: GOWORK=off go test -count=1 ./...
Evidence after fix: red/green recorded above.
Observed result after fix: default client Timeout is 30s and is not DefaultClient.
What was not tested: A live GitHub API outage.

Default a 30s http.Client instead of http.DefaultClient so a
black-holed api.github.com cannot hang public check-update CLIs.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. 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. labels Aug 15, 2026
@clawsweeper

clawsweeper Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 15, 2026, 10:42 PM ET / August 16, 2026, 02:42 UTC.

ClawSweeper review

What this changes

The PR replaces the release-check fallback HTTP client with a 30-second client and adds coverage for the default and caller-supplied client paths.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

This PR remains necessary because current main still uses the unbounded default HTTP client. The narrow replacement has sufficient exact-head runtime proof and no actionable review findings.

Priority: P2
Reviewed head: 983d7271ea73d12c857ebc94c84c6dd579e6a29c

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) A focused implementation, regression coverage, and exact-head runtime evidence make this a strong merge candidate.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (logs): A maintainer exercised the real Check boundary with a nil client against a controlled server that withheld headers and recorded the 30-second timeout on the exact PR head.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (logs): A maintainer exercised the real Check boundary with a nil client against a controlled server that withheld headers and recorded the 30-second timeout on the exact PR head.
Evidence reviewed 6 items Current main still has the defect: Current main defaults a nil client to http.DefaultClient, whose timeout remains unset.
PR patch is narrowly scoped: The branch changes only the nil-client fallback to a 30-second client and adds focused tests for the default and supplied-client paths.
Real behavior proof: A maintainer exercised Check with a nil client against a controlled server that withheld response headers and recorded the 30-second timeout on the exact head.
Findings None None.
Security None None.

How this fits together

Downstream crawl CLIs use the shared release-check package to read cached release data or query GitHub Releases and report update status. The fallback client determines how long a request can block when callers provide no client.

flowchart LR
  A[Downstream crawl CLI] --> B[Release check]
  B --> C{Fresh cache?}
  C -->|Yes| D[Cached update result]
  C -->|No| E[Fallback HTTP client]
  E --> F[GitHub Releases API]
  F --> G[Update status or suppressed error]
Loading

Before merge

  • Resolve merge risk (P1) - Existing callers that omit a client will now abandon a slow or stalled update check after 30 seconds; supplied clients remain unchanged and Notify converts the request error into a skipped check.
  • Complete next step (P2) - No mechanical repair remains; this PR has sufficient proof and awaits ordinary maintainer merge review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +1/-1; tests +22 The behavioral change is confined to the nil-client fallback and is paired with focused default and supplied-client coverage.

Merge-risk options

Maintainer options:

  1. Accept the bounded default (recommended)
    Merge the 30-second fallback because it confines only callers that omit a client, while callers with an explicit transport keep their current behavior.

Technical review

Best possible solution:

Adopt the bounded default while retaining caller-supplied client behavior, so optional update notices cannot indefinitely stall a CLI.

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

Yes. Current main routes a nil client to http.DefaultClient and then Client.Do; the maintainer’s exact-head stalled-server run demonstrates the after-fix 30-second boundary.

Is this the best way to solve the issue?

Yes. Replacing only the nil fallback preserves explicit caller transports and avoids new configuration or public API changes.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. A maintainer exercised the real Check boundary with a nil client against a controlled server that withheld headers and recorded the 30-second timeout on the exact PR head.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): A maintainer exercised the real Check boundary with a nil client against a controlled server that withheld headers and recorded the 30-second timeout on the exact PR head.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This fixes a bounded but real CLI availability problem in optional release checks.
  • merge-risk: 🚨 compatibility: The fallback client now times out after 30 seconds, changing behavior for existing callers that do not supply a client.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): A maintainer exercised the real Check boundary with a nil client against a controlled server that withheld headers and recorded the 30-second timeout on the exact PR head.
  • proof: sufficient: Contributor real behavior proof is sufficient. A maintainer exercised the real Check boundary with a nil client against a controlled server that withheld headers and recorded the 30-second timeout on the exact PR head.

Evidence

What I checked:

  • Current main still has the defect: Current main defaults a nil client to http.DefaultClient, whose timeout remains unset. (releasecheck/releasecheck.go:217, 00a94648f6f2)
  • PR patch is narrowly scoped: The branch changes only the nil-client fallback to a 30-second client and adds focused tests for the default and supplied-client paths. (releasecheck/releasecheck.go:217, 983d7271ea73)
  • Real behavior proof: A maintainer exercised Check with a nil client against a controlled server that withheld response headers and recorded the 30-second timeout on the exact head. (983d7271ea73)
  • Feature provenance: The original releasecheck implementation, including its prior DefaultClient fallback, was introduced in this commit. (releasecheck/releasecheck.go:212, 48b8ab6917b7)
  • Release check: v0.14.7 points at current main and therefore does not yet contain this branch change. (00a94648f6f2)
  • Advisory helper limitation: The structured autoreview helper could not load the first PR commit from this partial clone because its promisor remote was unreachable; the verdict relies on the locally available combined diff, source, history, and maintainer proof. (fbaa0dc06004)

Likely related people:

  • Vincent Koc: Introduced the releasecheck package and its original nil-client fallback. (role: feature introducer; confidence: high; commits: 48b8ab6917b7; files: releasecheck/releasecheck.go, releasecheck/releasecheck_test.go)
  • steipete: Provided exact-head real request-path validation and recently carried the v0.14.7 release commit. (role: reviewer; confidence: high; commits: 00a94648f6f2; files: releasecheck/releasecheck.go)

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 (2 earlier review cycles)
  • reviewed 2026-08-15T20:47:11.755Z sha fbaa0dc :: needs real behavior proof before merge. :: [P3] Remove the release-owned changelog edit
  • reviewed 2026-08-16T02:30:58.611Z sha 983d727 :: needs real behavior proof before merge. :: none

@steipete

Copy link
Copy Markdown
Contributor

Maintainer verification on exact head 983d7271ea73d12c857ebc94c84c6dd579e6a29c:

  • Ran make check: module tidy, formatting, vet, dead-code scan, govulncheck, all package tests, and race tests passed.
  • Ran the public releasecheck.Check boundary with a nil client against a controlled HTTP server that accepted the request and deliberately withheld response headers.
stalled server received release request
check returned after 30.0s: check latest openclaw/crawlkit release: Get "http://127.0.0.1:<redacted>/repos/openclaw/crawlkit/releases/latest": context deadline exceeded (Client.Timeout exceeded while awaiting headers)

This proves the default client bounds the real request path at 30 seconds; the supplied-client preservation case remains covered by the focused regression test.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Aug 16, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
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 proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed 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. labels Aug 16, 2026
@steipete
steipete merged commit 1ae588d into openclaw:main Aug 16, 2026
4 checks passed
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. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants