Skip to content

fix(tlon): bound error response body reads to prevent OOM#98496

Merged
vincentkoc merged 4 commits into
openclaw:mainfrom
Pandah97:fix/tlon-bound-error-response-reads
Jul 1, 2026
Merged

fix(tlon): bound error response body reads to prevent OOM#98496
vincentkoc merged 4 commits into
openclaw:mainfrom
Pandah97:fix/tlon-bound-error-response-reads

Conversation

@Pandah97

@Pandah97 Pandah97 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Replaces the 3 bare error-path response.text() calls in extensions/tlon/src/urbit/channel-ops.ts, extensions/tlon/src/channel.runtime.ts, and extensions/tlon/src/urbit/sse-client.ts with the shared bounded readResponseTextLimited capped at 16 KiB.

Covers pokeUrbitChannel error reads, generic channel error reads, and SSE connection failure error reads. All three paths use the identical readResponseTextLimited helper; the loopback test validates the helper with real OS TCP traffic, which covers all three call sites.

Linked context

No linked issue; this is a narrow follow-up to the bounded-response-read hardening series.

Related: PR #97587 (provider-http bounded layer), PR #98455 (browser-control bounded error reads).

Real behavior proof (required for external PRs)

Behavior or issue addressed: 3 unbounded Urbit API error-body reads in channel-ops.ts (pokeUrbitChannel), channel.runtime.ts (channel error), and sse-client.ts (SSE connection failure) — all using bare response.text() that buffers the entire payload before constructing the error message. All three call sites delegate to the same readResponseTextLimited(response, 16384) shared helper, so the loopback proof below validates the mechanism used by all three.

Real environment tested: Linux x86-64, Node 22, latest upstream/main.

Exact steps or command run after this patch:

pnpm test extensions/tlon/src/urbit/error-body-boundary.test.ts --reporter verbose

Evidence after fix:

 RUN  v4.1.8

 ✓ |extension-messaging| error-body-boundary.test.ts > bounds poke error body at 16 KiB 111ms
 ✓ |extension-messaging| error-body-boundary.test.ts > preserves short error body when under cap 8ms

 Test Files  1 passed (1)
      Tests  2 passed (2)
   Duration  1.51s

Bounded path (channel-ops.ts pokeUrbitChannel): 4 MiB streaming 500 response from real node:http loopback server → pokeUrbitChannel error message < 32 KiB containing X (proves streaming cancellation, not empty fallback).

Small body path: server returns session expired → error message contains session expired (proves under-cap preservation).

Coverage for channel.runtime.ts and sse-client.ts paths: Both call sites invoke the identical readResponseTextLimited(response, 16384) helper. The loopback test exercises this helper against a real node:http server serving oversized and normal-sized responses through Node's real fetch()/Response pipeline. The bounded read behavior is helper-invariant, not call-site-dependent, so the single loopback test validates all three runtime paths.

Full suite: pnpm test extensions/tlon/src/urbit/ — 7 files, 35 tests passed.

 Test Files  7 passed (7)
      Tests  35 passed (35)
   Duration  2.98s

What was not tested: Live Urbit ship API calls; no Urbit credentials are available in this environment.

Proof limitations or environment constraints: Loopback proof uses the real OS TCP stack and Node's real fetch/Response path, while the fetchWithSsrFGuard mock replaces the SSRF guard with a real-fetch pass-through inside the test seam.

Tests and validation

pnpm test extensions/tlon/src/urbit/error-body-boundary.test.ts — 2 tests passed
pnpm test extensions/tlon/src/urbit/ — 7 files, 35 tests passed

Risk checklist

Did user-visible behavior change? Only for oversized Urbit error responses over 16 KiB: they are now truncated at the cap instead of being fully materialized. Under-cap error responses are fully preserved.

Did config, environment, or migration behavior change? No.

Did security, auth, secrets, network, or tool execution behavior change? No — the same Urbit ship URLs and auth cookies are used.

What is the highest-risk area? sse-client.ts error path runs on every SSE stream connection failure; the bounded read replaces a bare response.text() that was already behind a .catch(() => ""), so behaviour is strictly additive (capped read instead of unbounded).

How is that risk mitigated? New dedicated error-boundary test file covers both oversized and normal-sized error bodies. All 35 existing urbit tests pass unchanged.

CI status

All CI checks pass: check-lint, check-test-types, all test shards, QA smoke. No pre-existing test failures remain.

Mergeability: clean (rebased onto latest upstream/main, no conflicts).

@clawsweeper

clawsweeper Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 1, 2026, 8:24 AM ET / 12:24 UTC.

Summary
This PR replaces three Tlon non-ok Urbit response.text() error-body reads with readResponseTextLimited(..., 16 KiB), adds loopback boundary coverage, and includes a one-line Anthropic test type assertion fix.

PR surface: Source +5, Tests +101. Total +106 across 5 files.

Reproducibility: yes. Current main has source-visible unbounded response.text() reads on the Tlon non-ok poke and subscription paths, and the PR body provides loopback oversized/short-body proof for the shared bounded-reader mechanism.

Review metrics: 2 noteworthy metrics.

  • Tlon Error Reads: 3 replaced. The PR intentionally changes three Tlon non-ok response-body diagnostics, so reviewers should judge proof and behavior across all three runtime paths.
  • Overlapping Tlon PRs: 1 open narrower PR. The open narrower Tlon poke PR affects the same channel-runtime path, so maintainers should choose the landing order before merging both branches.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Risk before merge

Maintainer options:

  1. Decide the mitigation before merge
    Land one canonical Tlon bounded error-body hardening change that covers all three non-ok Urbit paths, then retire any overlapping narrower PRs.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] Maintainers should choose the canonical relationship with the overlapping Tlon PR; there is no narrow automated repair needed on this branch.

Security
Cleared: The diff narrows Tlon HTTP error-body buffering and adds tests, with no new dependency, workflow, package, secret, credential, or auth-provider surface.

Review details

Best possible solution:

Land one canonical Tlon bounded error-body hardening change that covers all three non-ok Urbit paths, then retire any overlapping narrower PRs.

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

Yes. Current main has source-visible unbounded response.text() reads on the Tlon non-ok poke and subscription paths, and the PR body provides loopback oversized/short-body proof for the shared bounded-reader mechanism.

Is this the best way to solve the issue?

Yes, this is an acceptable narrow fix: the bounded read belongs at the Tlon HTTP error boundaries and uses an exported SDK helper. The main remaining maintainer choice is whether to prefer this truncated diagnostic behavior or the stricter discard-on-overflow shape used by the narrower overlapping PR.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal loopback proof over real node:http and Node fetch for oversized and short error bodies, plus the live GitHub Real behavior proof check passes for the PR head.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes terminal loopback proof over real node:http and Node fetch for oversized and short error bodies, plus the live GitHub Real behavior proof check passes for the PR head.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, 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 is normal-priority Tlon availability hardening with limited blast radius and no emergency production-regression signal.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes terminal loopback proof over real node:http and Node fetch for oversized and short error bodies, plus the live GitHub Real behavior proof check passes for the PR head.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes terminal loopback proof over real node:http and Node fetch for oversized and short error bodies, plus the live GitHub Real behavior proof check passes for the PR head.
Evidence reviewed

PR surface:

Source +5, Tests +101. Total +106 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 3 8 3 +5
Tests 2 102 1 +101
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 110 4 +106

What I checked:

Likely related people:

  • steipete: GitHub path history shows repeated recent work on Tlon channel runtime, channel ops, and SSE behavior, including shared Tlon request refactors. (role: feature owner history and recent Tlon area contributor; confidence: high; commits: 96e581242605, 827b0de0ce74, 65167c963733; files: extensions/tlon/src/channel.runtime.ts, extensions/tlon/src/urbit/channel-ops.ts, extensions/tlon/src/urbit/sse-client.ts)
  • vincentkoc: GitHub path history shows recent Tlon network/error handling work and provider error-body helper maintenance in the same bounded-read family. (role: recent Tlon and provider-helper contributor; confidence: high; commits: a79706820617, d77f428441f2, c59e9009039d; files: extensions/tlon/src/channel.runtime.ts, extensions/tlon/src/urbit/channel-ops.ts, extensions/tlon/src/urbit/sse-client.ts)
  • joshavant: GitHub path history shows the shared provider response-bound helper/export work reused by this PR. (role: shared helper contributor; confidence: medium; commits: 0a14444924e3; files: src/agents/provider-http-errors.ts, src/plugin-sdk/provider-http.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@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. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. labels Jul 1, 2026
@Pandah97
Pandah97 force-pushed the fix/tlon-bound-error-response-reads branch from 9153dea to 8dc152c Compare July 1, 2026 09:14
@Pandah97

Pandah97 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 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: the existing ClawSweeper review comment will be edited in place when the review finishes.

@openclaw-barnacle openclaw-barnacle Bot added the gateway Gateway runtime label Jul 1, 2026
@Pandah97 Pandah97 closed this Jul 1, 2026
@Pandah97 Pandah97 reopened this Jul 1, 2026
@Pandah97
Pandah97 force-pushed the fix/tlon-bound-error-response-reads branch 2 times, most recently from b26b71e to 9f672bf Compare July 1, 2026 09:59
@Pandah97

Pandah97 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 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: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. labels Jul 1, 2026
@Pandah97

Pandah97 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

1 similar comment
@Pandah97

Pandah97 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Pandah97 added 4 commits July 1, 2026 18:36
Replace bare response.text() on non-ok paths with readResponseTextLimited
capped at 16 KiB so a hostile or misconfigured Urbit ship cannot force the
gateway to buffer an arbitrary-size error body into process memory.

Affected paths:
- pokeUrbitChannel (channel-ops.ts)
- channel.runtime.ts poke path
- sendSubscription (sse-client.ts)
- Remove unused beforeEach import
- Wrap if/else bodies in braces (curly)
- Use block body for Promise executors (no-promise-executor-return)
- Fix TS2493 tuple type errors in server-cron-notifications and
  server-cron tests by adding explicit type annotations on mock.calls
- Fix TS2322 in anthropic.test.ts by adding as const to resource
  content block type
@Pandah97
Pandah97 force-pushed the fix/tlon-bound-error-response-reads branch from 9f672bf to de6b427 Compare July 1, 2026 10:36
@Pandah97

Pandah97 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 1, 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: the existing ClawSweeper review comment will be edited in place when the review finishes.

@openclaw-barnacle openclaw-barnacle Bot removed the gateway Gateway runtime label Jul 1, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. 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. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 1, 2026
@vincentkoc vincentkoc self-assigned this Jul 1, 2026

@vincentkoc vincentkoc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the Tlon error-body boundaries and the overlap with #98526. This is the canonical landing path because it bounds all three Tlon non-ok response reads, not just createHttpPokeApi; the 16 KiB bounded diagnostic behavior is acceptable here.

@vincentkoc
vincentkoc merged commit 8abd5d4 into openclaw:main Jul 1, 2026
121 of 131 checks passed
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 2, 2026
…8496)

* fix(tlon): bound error response body reads to prevent OOM

Replace bare response.text() on non-ok paths with readResponseTextLimited
capped at 16 KiB so a hostile or misconfigured Urbit ship cannot force the
gateway to buffer an arbitrary-size error body into process memory.

Affected paths:
- pokeUrbitChannel (channel-ops.ts)
- channel.runtime.ts poke path
- sendSubscription (sse-client.ts)

* fix(tlon): fix lint issues in error-body-boundary test

- Remove unused beforeEach import
- Wrap if/else bodies in braces (curly)
- Use block body for Promise executors (no-promise-executor-return)

* fix(types): resolve pre-existing TS test type errors

- Fix TS2493 tuple type errors in server-cron-notifications and
  server-cron tests by adding explicit type annotations on mock.calls
- Fix TS2322 in anthropic.test.ts by adding as const to resource
  content block type

* chore: trigger CI
Pandah97 added a commit to Pandah97/openclaw that referenced this pull request Jul 2, 2026
- Check content-length header before reading body, reject oversized
  responses early without allocating memory.
- Prefer resp.body() + subarray decode over resp.text() on the main
  path to limit string decoding to maxChars * 4 bytes.
- Three-layer defense: content-length pre-check → bounded Buffer
  decode → post-decode trimmed slice.
- Retain resp.text() fallback for older Playwright versions.

Fixes openclaw#98496
Pandah97 added a commit to Pandah97/openclaw that referenced this pull request Jul 2, 2026
- Remove resp.text() fallback that still loaded full body unbounded.
  resp.body() is universally available in Playwright — no environment
  needs the text() path.
- Remove local scratch docs/.local/ files from the PR branch; these
  are development notes, not intended for upstream.

Fixes openclaw#98496
Pandah97 added a commit to Pandah97/openclaw that referenced this pull request Jul 2, 2026
- Remove the content-length pre-reject early return that changed
  oversized response behavior from truncated prefix to empty body.
  body() + subarray bounded decode is backward compatible.
- Add 3 UT cases: small full decode, large with content-length,
  large without content-length (chunked).

Fixes openclaw#98496
Pandah97 added a commit to Pandah97/openclaw that referenced this pull request Jul 2, 2026
…ion test

- Add console.log proof lines showing body length and truncation
  status for each bounded-read scenario.
- Add negative control: proves old text() path buffers full body.
- Add mutation test: proves subarray() decode window is load-bearing.

Fixes openclaw#98496
Pandah97 added a commit to Pandah97/openclaw that referenced this pull request Jul 3, 2026
- Check content-length header before reading body, reject oversized
  responses early without allocating memory.
- Prefer resp.body() + subarray decode over resp.text() on the main
  path to limit string decoding to maxChars * 4 bytes.
- Three-layer defense: content-length pre-check → bounded Buffer
  decode → post-decode trimmed slice.
- Retain resp.text() fallback for older Playwright versions.

Fixes openclaw#98496
Pandah97 added a commit to Pandah97/openclaw that referenced this pull request Jul 3, 2026
- Remove resp.text() fallback that still loaded full body unbounded.
  resp.body() is universally available in Playwright — no environment
  needs the text() path.
- Remove local scratch docs/.local/ files from the PR branch; these
  are development notes, not intended for upstream.

Fixes openclaw#98496
Pandah97 added a commit to Pandah97/openclaw that referenced this pull request Jul 3, 2026
- Remove the content-length pre-reject early return that changed
  oversized response behavior from truncated prefix to empty body.
  body() + subarray bounded decode is backward compatible.
- Add 3 UT cases: small full decode, large with content-length,
  large without content-length (chunked).

Fixes openclaw#98496
Pandah97 added a commit to Pandah97/openclaw that referenced this pull request Jul 3, 2026
…ion test

- Add console.log proof lines showing body length and truncation
  status for each bounded-read scenario.
- Add negative control: proves old text() path buffers full body.
- Add mutation test: proves subarray() decode window is load-bearing.

Fixes openclaw#98496
chenyangjun-xy pushed a commit to chenyangjun-xy/openclaw that referenced this pull request Jul 3, 2026
…8496)

* fix(tlon): bound error response body reads to prevent OOM

Replace bare response.text() on non-ok paths with readResponseTextLimited
capped at 16 KiB so a hostile or misconfigured Urbit ship cannot force the
gateway to buffer an arbitrary-size error body into process memory.

Affected paths:
- pokeUrbitChannel (channel-ops.ts)
- channel.runtime.ts poke path
- sendSubscription (sse-client.ts)

* fix(tlon): fix lint issues in error-body-boundary test

- Remove unused beforeEach import
- Wrap if/else bodies in braces (curly)
- Use block body for Promise executors (no-promise-executor-return)

* fix(types): resolve pre-existing TS test type errors

- Fix TS2493 tuple type errors in server-cron-notifications and
  server-cron tests by adding explicit type annotations on mock.calls
- Fix TS2322 in anthropic.test.ts by adding as const to resource
  content block type

* chore: trigger CI
sheyanmin pushed a commit to sheyanmin/openclaw that referenced this pull request Jul 8, 2026
…8496)

* fix(tlon): bound error response body reads to prevent OOM

Replace bare response.text() on non-ok paths with readResponseTextLimited
capped at 16 KiB so a hostile or misconfigured Urbit ship cannot force the
gateway to buffer an arbitrary-size error body into process memory.

Affected paths:
- pokeUrbitChannel (channel-ops.ts)
- channel.runtime.ts poke path
- sendSubscription (sse-client.ts)

* fix(tlon): fix lint issues in error-body-boundary test

- Remove unused beforeEach import
- Wrap if/else bodies in braces (curly)
- Use block body for Promise executors (no-promise-executor-return)

* fix(types): resolve pre-existing TS test type errors

- Fix TS2493 tuple type errors in server-cron-notifications and
  server-cron tests by adding explicit type annotations on mock.calls
- Fix TS2322 in anthropic.test.ts by adding as const to resource
  content block type

* chore: trigger CI
wheakerd pushed a commit to wheakerd/clawdbot that referenced this pull request Jul 15, 2026
…8496)

* fix(tlon): bound error response body reads to prevent OOM

Replace bare response.text() on non-ok paths with readResponseTextLimited
capped at 16 KiB so a hostile or misconfigured Urbit ship cannot force the
gateway to buffer an arbitrary-size error body into process memory.

Affected paths:
- pokeUrbitChannel (channel-ops.ts)
- channel.runtime.ts poke path
- sendSubscription (sse-client.ts)

* fix(tlon): fix lint issues in error-body-boundary test

- Remove unused beforeEach import
- Wrap if/else bodies in braces (curly)
- Use block body for Promise executors (no-promise-executor-return)

* fix(types): resolve pre-existing TS test type errors

- Fix TS2493 tuple type errors in server-cron-notifications and
  server-cron tests by adding explicit type annotations on mock.calls
- Fix TS2322 in anthropic.test.ts by adding as const to resource
  content block type

* chore: trigger CI

(cherry picked from commit 8abd5d4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: tlon Channel integration: tlon P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S 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