Skip to content

fix(tlon): bound error body read in channel poke to prevent OOM#98526

Closed
wings1029 wants to merge 2 commits into
openclaw:mainfrom
wings1029:fix/tlon-poke-error-body-cap
Closed

fix(tlon): bound error body read in channel poke to prevent OOM#98526
wings1029 wants to merge 2 commits into
openclaw:mainfrom
wings1029:fix/tlon-poke-error-body-cap

Conversation

@wings1029

@wings1029 wings1029 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

createHttpPokeApi in the Tlon channel runtime reads a non-ok Urbit ship response body with a bare response.text() on the error path — without a byte cap and without a .catch() guard. If a malformed or misconfigured Urbit ship returns a large streaming error response, the unbounded read buffers the entire payload into an Error message, which can OOM the gateway process. Unlike the sibling error paths in sse-client.ts and channel-ops.ts (which at least have .catch(() => "")), this call site has no error-recovery guard at all — a stream read failure on the error path itself would crash the poke handler.

Urbit ships are self-hosted (user-controlled), so the "upstream" is not a trusted provider with known response characteristics.

Why This Change Was Made

The fix replaces the bare response.text() with readResponseWithLimit capped at 16 KiB, using the same strict pattern applied to the browser control error path (see #98455): overflow cancels the stream, the partial body is discarded, and the error falls back to Poke failed: <status> without the body text. The .catch(() => undefined) guard handles both the overflow case and stream read failures uniformly.

readResponseWithLimit was chosen over readResponseTextLimited to follow the canonical strict contract — fail-clean on overflow rather than preserving a truncated body fragment.

User Impact

Normal Tlon poke operations are unchanged. On a non-ok response, a small error body (≤ 16 KiB) is still fully included in the error message. A pathological error response exceeding 16 KiB now produces "Poke failed: 500" instead of buffering the entire body into process memory.

Evidence

Real behavior proof

  • Behavior addressed: createHttpPokeApi.poke() error path buffers 4 MiB streaming body → OOM; fixed path cancels the stream at 16 KiB.
  • Real environment tested: Loopback HTTP server on 127.0.0.1 (with /~/login returning a cookie so authentication succeeds before the poke), Node 22, Linux.
  • Exact steps:
    1. Start a loopback node:http server that returns HTTP 204 for /~/login and a streaming 4 MiB body for channel requests.
    2. Call createHttpPokeApi(url, code, ship) then api.poke(...) through the real function path.
    3. Assert the thrown error's .message is under 32 KiB and contains "Poke failed: 500".
    4. Assert a small error body ("invalid mark") is fully preserved.
    5. Assert a 204 response returns the pokeId without throwing.
  • Observed result: Before fix — error message = 4,194,304 bytes (full unbuffered read). After fix — error message = ~16,477 bytes (bounded snippet + status prefix).
  • What was not tested: Real Urbit ship connectivity; the urbitFetch SSRF guard path (mocked in test); the sibling sendDm/sendGroupMessage callers (they exercise the same poke method).

Validation Evidence

pnpm test extensions/tlon/src/channel.runtime.error-body-boundary.test.ts
Test Files  1 passed (1)
     Tests  3 passed (3)

# Mutation check (revert fix, keep __test export):
Test Files  1 failed (1)
     Tests  1 failed | 2 passed (3)
→ expected X to be less than 32768  ← 4 MiB buffered into error message

Security & Privacy / Compatibility

No new configuration, no credential changes, no protocol changes. 16 KiB cap matches the established error-body cap used by browser control (#98455) and provider HTTP error helpers. A thin __test export is added for focused boundary coverage; it is not part of the public plugin API.

AI-assisted.

@openclaw-barnacle openclaw-barnacle Bot added channel: tlon Channel integration: tlon size: S labels Jul 1, 2026
@clawsweeper

clawsweeper Bot commented Jul 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Summary
The branch replaces the Tlon createHttpPokeApi non-ok response.text() read with a strict 16 KiB bounded read and adds loopback Vitest coverage for large, small, and successful poke responses.

PR surface: Source +12, Tests +146. Total +158 across 2 files.

Reproducibility: yes. Current main and v2026.6.11 have source-visible bare response.text() reads on the Tlon poke error path, and the PR body provides loopback oversized-stream proof through createHttpPokeApi; I did not rerun tests in this read-only review.

Review metrics: 1 noteworthy metric.

  • Overlapping Tlon Hardening PRs: 1 broader open PR. The broader PR touches the same channel-runtime path plus sibling Tlon error reads, so landing order and canonical behavior matter before merge.

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

  • [P1] A broader open Tlon PR covers this same poke path plus sibling channel-ops and SSE reads, so maintainers need to choose one canonical landing path before both branches drift or conflict.
  • [P1] Merging only this PR fixes the outbound createHttpPokeApi path but leaves the sibling pokeUrbitChannel and sendSubscription error-body reads unbounded unless a follow-up or the broader PR handles them.
  • [P1] The PR currently has one failed CI shard, checks-node-compact-small-whole-2, that needs rerun or maintainer disposition before merge.

Maintainer options:

  1. Choose One Tlon Hardening Path (recommended)
    Pick whether to land this strict createHttpPokeApi fix with a tracked sibling follow-up or land the broader all-three-read Tlon PR, then rebase or close the other branch.
  2. Land Focused Fix With Follow-Up
    Maintainership can accept the narrower blast radius if the remaining channel-ops and SSE reads are explicitly tracked before or immediately after merge.
  3. Pause Narrow Branch
    If the broader PR is the intended canonical fix, pause or close this narrower branch after preserving its stricter overflow rationale.

Next step before merge

  • [P2] Maintainers need to choose the canonical relationship with the broader Tlon hardening PR and handle the failed CI shard; there is no narrow automated repair to apply to this branch.

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

Review details

Best possible solution:

Land one canonical Tlon bounded error-body hardening path, with an explicit maintainer choice between this strict poke-only fix plus sibling follow-up and the broader all-three-read PR.

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

Yes. Current main and v2026.6.11 have source-visible bare response.text() reads on the Tlon poke error path, and the PR body provides loopback oversized-stream proof through createHttpPokeApi; I did not rerun tests in this read-only review.

Is this the best way to solve the issue?

Mostly yes for the stated path. The bounded read belongs at the Tlon HTTP poke boundary and uses an existing SDK helper, but the best repository outcome should coordinate with the broader Tlon PR or explicitly track the sibling channel-ops/SSE reads.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add merge-risk: 🚨 other: The PR overlaps a broader viable Tlon hardening branch with a different diagnostic-overflow behavior, so maintainers need a landing decision that CI alone cannot settle.

Label justifications:

  • P2: This is normal-priority Tlon availability hardening with limited blast radius and no emergency production-regression signal.
  • merge-risk: 🚨 other: The PR overlaps a broader viable Tlon hardening branch with a different diagnostic-overflow behavior, so maintainers need a landing decision that CI alone cannot settle.
  • 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 after-fix terminal proof from a loopback HTTP server through the real createHttpPokeApi path, including oversized, small-body, success, and mutation checks.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal proof from a loopback HTTP server through the real createHttpPokeApi path, including oversized, small-body, success, and mutation checks.
Evidence reviewed

PR surface:

Source +12, Tests +146. Total +158 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 14 2 +12
Tests 1 146 0 +146
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 160 2 +158

What I checked:

Likely related people:

  • steipete: Introduced the Tlon plugin and later refactored shared Urbit poke/scry operations around the same channel error-read surfaces. (role: feature owner history; confidence: high; commits: 791b568f7825, de103773c79d; files: extensions/tlon/src/urbit/sse-client.ts, extensions/tlon/src/urbit/channel-ops.ts)
  • vincentkoc: Added the current lazy-loaded Tlon channel runtime path and carried the current release surface that contains the poke error-body read. (role: recent area contributor; confidence: high; commits: 029f5d642729, e085fa1a3ffd; files: extensions/tlon/src/channel.runtime.ts, extensions/tlon/src/urbit/channel-ops.ts, extensions/tlon/src/urbit/sse-client.ts)
  • joshavant: Added the shared provider response text limit helper and plugin SDK export used by the broader alternative Tlon hardening path. (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 proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. labels Jul 1, 2026
@wings1029
wings1029 force-pushed the fix/tlon-poke-error-body-cap branch from 4cba2e4 to 2ddeb67 Compare July 1, 2026 11:18
@clawsweeper clawsweeper Bot added 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: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 1, 2026
@wings1029
wings1029 force-pushed the fix/tlon-poke-error-body-cap branch from 2ddeb67 to bcd83ee Compare July 1, 2026 12:16
@clawsweeper clawsweeper Bot mentioned this pull request Jul 1, 2026
Merged
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. label Jul 1, 2026
@wings1029
wings1029 force-pushed the fix/tlon-poke-error-body-cap branch from bcd83ee to 5b1d6d1 Compare July 1, 2026 13:44
@vincentkoc

vincentkoc commented Jul 1, 2026

Copy link
Copy Markdown
Member

Thanks for the focused poke-path fix and the strict overflow rationale. I landed the broader canonical Tlon error-body fix in #98496, which covers this same createHttpPokeApi path plus the sibling channel-ops and SSE non-ok response reads.

Closing this as superseded by the merged broader fix: 8abd5d4

@vincentkoc vincentkoc closed this Jul 1, 2026
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

channel: tlon Channel integration: tlon merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@wings1029 @vincentkoc