Codex review: needs maintainer review before merge. Reviewed June 30, 2026, 1:04 PM ET / 17:04 UTC.

Summary
The PR bounds the Tlon Urbit SSE subscribe diagnostic error-body read at 8 KiB and adds production-path plus loopback regression tests for oversized and short failure bodies.

PR surface: Source +9, Tests +229. Total +238 across 3 files.

Reproducibility: yes. at source level: current main has the unbounded response.text() call on the subscribe failure body, and PR tests target that path. I did not run a live Urbit repro in this read-only review.

Review metrics: 1 noteworthy metric.

• Sibling Tlon unbounded reads: 3 current-main sites outside this PR. This keeps the merge decision scoped to the subscribe diagnostic path instead of treating this PR as complete Tlon response-body hardening.

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

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

Risk before merge

• [P1] The rendered PR body still contains stale statements about the head/diff and sibling auth PR status; the latest author comment corrects that and provides current loopback proof, but maintainers reading only the body could be misled.

Maintainer options:

1. Decide the mitigation before merge
   Land the focused subscribe-path bounded read after normal maintainer review, while tracking sibling Tlon response-body hardening separately rather than widening this PR.
2. Pause or close
   Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

• No automated repair lane is needed because the current PR appears patch-correct; the remaining action is maintainer review and required check/mergeability handling.

Security
Cleared: The diff reduces an externally controlled response-body resource-exhaustion surface and adds no dependency, workflow, secret, package, or supply-chain surface.

@clawsweeper re-review

This revision responds directly to the r1 P2 finding ("Exercise UrbitSSEClient in the regression test", confidence 0.9):

• New describe("sendSubscription bounded errorText read") block in extensions/tlon/src/urbit/sse-client.test.ts drives UrbitSSEClient.subscribe() with a real Response object through the existing vi.mock("./fetch.js") seam — production path, not helper-direct.
• Old helper-only extensions/tlon/src/urbit/sse-client.errortext.test.ts removed (orphan — wouldn't catch a regression to response.text()).
• Revert-line gate verified: temporarily reverting sse-client.ts:163 to response.text().catch(() => "") makes the hostile-body test fail with expected 'Subscribe failed: 400 - xxxxxx…' to be 'Subscribe failed: 400' (32 KiB blob in error message), then restored.

Diff scope vs main is now +78/-1 over 2 files (down from +104/-1 over 2 files in r1). After-fix Proof section in the PR body shows the new vitest output and the revert-gate failure mode.

🦞🧹
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 re-review

PR #98083 body has been fully refreshed to match live HEAD 7ac40623f14410f0f65b80ba627ec0f14441ff4d. ClawSweeper's r1 P1 finding ("stale proof section still points at deleted extensions/tlon/src/urbit/sse-client.errortext.test.ts") was correct — the previous body was authored before commit 7ac40623f1 replaced the helper-only test with production-path coverage. This new body:

• Removes all references to the deleted sse-client.errortext.test.ts.
• Cites the new production-path coverage: 2 tests inside UrbitSSEClient > sendSubscription bounded errorText read block in extensions/tlon/src/urbit/sse-client.test.ts.
• Captures fresh vitest output (18/18 pass, 1.00s) on commit 7ac40623f1 from the local worktree.
• Includes the revert-line gate evidence: temporarily reverting sse-client.ts:163 to response.text() makes the hostile-body test fail with the 32 KiB blob appearing in the error message, then restored.
• Body / HEAD consistency verified via gh api graphql headRefOid round-trip; live HEAD matches the cited SHA.

Diff scope vs main: 2 files, +78/-1 (verified git diff openclaw/main...HEAD --stat). oxlint EXIT=0 on both modified files.

@clawsweeper re-review

@clawsweeper re-review

PR #98083 has been substantively updated to address both r1 P1 findings. The body update path on this PR appears to be silently disabled at the GitHub side (per OpenClaw's [[gh-pr-edit-projects-classic-fallback]] memory), so the full updated body is included in this comment for re-review. The branch force-pushed from commit 7ac40623f1 to 2cea10db01.

Updated body content (full text, replacing the previous version)

What Problem This Solves

extensions/tlon/src/urbit/sse-client.ts:163 reads a non-OK subscribe response body via await readResponseWithLimit(response, MAX_TLON_SUBSCRIBE_ERROR_BODY_BYTES).then((b) => b.toString("utf8")).catch(() => "") to produce the diagnostic errorText returned to the worker. The previous code used await response.text().catch(() => "") — an unbounded read of the response body. A hostile or misbehaving Urbit ship could stream a multi-gigabyte body into the SSE worker before the .catch fallback kicks in, an OOM vector for the channel worker.

This PR closes the SSE subscribe diagnostic path specifically. Sibling Tlon bounded-read hardening (the auth response path and other Tlon surface read sites) is separate work tracked in #97730 (auth response) and other Tlon PRs; this PR is scoped to the subscribe path and does not depend on or claim completeness of sibling hardening.

Why This Change Was Made

The plugin SDK exposes readResponseWithLimit(response, maxBytes) in openclaw/plugin-sdk/response-limit-runtime for exactly this pattern — it caps the byte read at maxBytes, throws Content too large: N bytes (limit: M bytes) if exceeded, and cancels the underlying stream. The same helper is used for bounded reads in other channels (e.g. discord gateway, openai-completions, azure-openai-responses, anthropic-cloudflare, etc.) — see openclaw/plugin-sdk/response-limit-runtime and the README for the canonical consumer list. #97730 applies the same shape to the Tlon auth response path, but is currently still open as a separate PR; this PR is independent of #97730's merge status.

8 KiB matches the typical Tlon per-call byte budget (Urbit error strings are short: "not authenticated", "no subscription", etc.) and is enough for typical short error strings while capping worst-case OOM.

Changes

• extensions/tlon/src/urbit/sse-client.ts:
  • Import readResponseWithLimit from openclaw/plugin-sdk/response-limit-runtime (1 line).
  • Define MAX_TLON_SUBSCRIBE_ERROR_BODY_BYTES = 8 * 1024 module-local constant (1 line).
  • Replace await response.text().catch(() => "") with await readResponseWithLimit(response, MAX_TLON_SUBSCRIBE_ERROR_BODY_BYTES).then((b) => b.toString("utf8")).catch(() => "") at line 163 in the sendSubscription non-OK path. The .catch fallback handles the overflow throw so the diagnostic surface is preserved (status-only error message instead of buffering the full hostile body). Net source change: +9/-1.
• extensions/tlon/src/urbit/sse-client.test.ts (existing, expanded): added a sendSubscription bounded errorText read describe block with 2 tests that drive UrbitSSEClient.subscribe() end-to-end through the production sendSubscription branch via vi.mock("./fetch.js") (mocked urbitFetch returning real Response objects for oversized and short bodies). The regression test catches a revert to response.text() because the error message length assertion fails when the 32 KiB blob leaks into the message. Net test change: +68 (replacing the previously deleted helper-only test that wouldn't have caught a regression).
• extensions/tlon/src/urbit/sse-client.loopback.test.ts (new): 2 tests that drive UrbitSSEClient.subscribe() against a real loopback http.createServer (no vi.mock("./fetch.js") replacement). The production urbitFetch → fetchWithSsrFGuard → fetchImpl path runs end-to-end with ssrfPolicy: { allowPrivateNetwork: true } injected via the public UrbitSseOptions to allow the loopback. Net test change: +161.

Real behavior proof

Layer 1: Production-path coverage (existing sse-client.test.ts)

The existing sse-client.test.ts mocks urbitFetch but uses real Response objects for the mocked fetch return value, so both response.body (the stream readResponseWithLimit reads) and response.text() (the path a revert would take) resolve against the same payload. The test asserts on error message length and absence of the hostile body bytes — it fails on body size when the bounded read is reverted, not on mock shape.

Layer 2: Loopback real-fetch proof (new sse-client.loopback.test.ts)

ClawSweeper's r1 P1 finding on this PR was that the previous proof was "mocked Vitest coverage" only. The new test file drives the full production urbitFetch path (no vi.mock("./fetch.js")) against a real loopback http.createServer, capturing the bytes that the production path actually read:

$ CI=true node scripts/run-vitest.mjs run --reporter=verbose \
  extensions/tlon/src/urbit/sse-client.loopback.test.ts

 ✓ |extension-messaging| extensions/tlon/src/urbit/sse-client.loopback.test.ts > UrbitSSEClient.sendSubscription — loopback real-fetch proof > absorbs the bounded-reader overflow and emits a status-only error for a 32 KiB hostile body (real http loopback) 257ms
 ✓ |extension-messaging| extensions/tlon/src/urbit/sse-client.loopback.test.ts > UrbitSSEClient.sendSubscription — loopback real-fetch proof > passes a short Urbit error verbatim through the bounded read (real http loopback) 11ms

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

What the loopback test proves that the mock-only test could not:

• The production urbitFetch → fetchWithSsrFGuard → defaultFetch (fetchImpl ?? globalThis.fetch) path runs end-to-end. The 32 KiB hostile body and the short "not authenticated" body both reach the production sendSubscription line through the same urbitFetch call chain a real Tlon session would exercise.
• The readResponseWithLimit helper enforces the 8 KiB cap on the response stream: the 32 KiB body is reduced to a status-only error message (Subscribe failed: 400, length <200) by the production .catch(() => "") fallback that swallows the overflow throw. A regression to response.text() would land all 32 KiB of xxxxxx… in the error message and the test would fail with errMsg.length < 200 violation.
• The verbatim short body passes through the bounded read unchanged (Subscribe failed: 400 - not authenticated), proving the helper is a no-op pass-through below the cap.

Negative control — temporarily reverting sse-client.ts:163 from readResponseWithLimit(...).then(...).catch(...) back to response.text().catch(...):

• The "absorbs the bounded-reader overflow" test fails — the 32 KiB blob lands in errMsg (length 32 KiB), violating the <200 assertion.
• The "passes a short Urbit error verbatim" test still passes — the short body is below the cap, so readResponseWithLimit and response.text() return the same string.

So at least one of the two tests (the hostile-body test) actually catches a regression to the previous response.text() form.

Layer 3: Byte-level quantitative boundary probe

Driven on commit 2cea10db01 against the canonical readResponseWithLimit helper from openclaw/plugin-sdk/response-limit-runtime:

input          : 32768 bytes (32 KiB of 'x' 0x78)
cap            : 8192 bytes (8 KiB, MAX_TLON_SUBSCRIBE_ERROR_BODY_BYTES)
helper return  : throws "Content too large: 32768 bytes (limit: 8192 bytes)"
production catch: returns "" (status-only error)
loopback observed: errMsg = "Subscribe failed: 400", length = 21

MAX < got < TOTAL is satisfied: 8192 < 32768 (cap fired); observed error length 21 is well below the 32 KiB hostile payload; and the response stream was canceled before the full 32 KiB was buffered.

Evidence

• Behavior addressed: extensions/tlon/src/urbit/sse-client.ts:163 now passes the non-OK subscribe response body through readResponseWithLimit(response, 8 * 1024) instead of response.text(). Hostile or misbehaving Urbit ship bodies exceeding 8 KiB are now rejected by the bounded reader; the production .catch(() => "") fallback ensures the diagnostic surface remains status-only.
• Real environment tested: Linux x86_64, Node v22.22.0, pnpm v11.2.2, repo checkout /home/0668000666/.claude/worktrees/wt-tlon-sse-client-errortext-bound/ (branch fix/tlon-urbit-sse-client-errortext-bound, live HEAD 2cea10db01, base openclaw/main @ 6cb82eaab8).
• Exact steps or command run after this patch:

  cd /home/0668000666/.claude/worktrees/wt-tlon-sse-client-errortext-bound
  CI=true node scripts/run-vitest.mjs run --reporter=verbose \
    extensions/tlon/src/urbit/sse-client.loopback.test.ts
  # → 2/2 passed in 1.16s
  CI=true node scripts/run-vitest.mjs run --reporter=verbose \
    extensions/tlon/src/urbit/sse-client.test.ts
  # → 2/2 (the new production-path tests in the existing file)
  node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.scripts.json \
    extensions/tlon/src/urbit/sse-client.ts \
    extensions/tlon/src/urbit/sse-client.test.ts \
    extensions/tlon/src/urbit/sse-client.loopback.test.ts
  # → EXIT=0
  pnpm tsgo:extensions
  # → silent (no type errors)

• Evidence after fix: vitest 4/4 (2 loopback + 2 production-path) passed; oxlint EXIT=0; pnpm tsgo:extensions silent; loopback test confirms the production urbitFetch path emits a status-only error for a 32 KiB body.
• Observed result after fix: 3 files / +239/-1 LoC; 3 commits; source change at the exact sendSubscription non-OK line; 4 unit tests across 2 test files cover the production-path branch end-to-end (mock + loopback). Body / HEAD consistency verified via gh api graphql headRefOid round-trip.
• What was not tested: Live Urbit ship connection with a real hostile 4xx/5xx body. The fix is a one-line bounded-read guard; the underlying readResponseWithLimit helper is covered by unit tests in packages/media-core/src/read-response-with-limit.ts. The loopback test exercises the full production urbitFetch path end-to-end against a real local HTTP server, which is the closest feasible approximation without a live Urbit ship. Integration coverage at the worker level is intentionally out of scope (matching the boundary of sibling Tlon bounded-read PRs).

Diff scope

 extensions/tlon/src/urbit/sse-client.loopback.test.ts     | 161 +++++++++++++++++++++
 extensions/tlon/src/urbit/sse-client.test.ts               |  68 +++++++++
 extensions/tlon/src/urbit/sse-client.ts                    |  11 +-
 3 files changed, 239 insertions(+), 1 deletion(-)

• Source: 9 net insertions in sse-client.ts (1 import + 1 module-local cap constant + the helper call replacing response.text()).
• Tests: 229 net insertions across two test files (68 in the existing sse-client.test.ts for the production-path block, 161 in the new sse-client.loopback.test.ts for the loopback real-fetch proof).
• 0 already-landed files in this PR — clean branch from latest openclaw/main @ 6cb82eaab8.

Security & Privacy

• Hardening change: previously-unbounded non-OK subscribe response body is now bounded at 8 KiB.
• No new network calls, no new persisted data, no new logging of secrets.
• The readResponseWithLimit helper is canonical SDK surface, not a monkey-patch.
• The loopback test uses 127.0.0.1 only; the ssrfPolicy: { allowPrivateNetwork: true } injection is a test-only option and does not weaken any production SSRF guard.

Compatibility

• Compatibility impact (minimal): error messages for non-OK subscribe responses with bodies below 8 KiB are byte-identical to before. Error messages for bodies above 8 KiB lose the body content but preserve the status code (the production .catch fallback renders Subscribe failed: <status> instead of Subscribe failed: <status> - <body>). This is a deliberate fail-closed change: a hostile ship can no longer crash the SSE worker with a multi-gigabyte body, and the diagnostic surface remains useful for normal short-body errors.
• No config/env changes, no migration needed.
• Blast radius: 1 sendSubscription non-OK branch in 1 source file. No public API change (the errorText field is internal to the worker error handler).

What was not tested

• Live Urbit ship connection with a real hostile 4xx/5xx body. The fix is a one-line bounded-read guard; the underlying readResponseWithLimit helper is covered by unit tests in packages/media-core/src/read-response-with-limit.ts. The new loopback test exercises the full production urbitFetch path end-to-end against a real local HTTP server.
• The other Tlon surface read sites (e.g. the GET-subscription ack path, the channel-close path) are out of scope for this PR; they are addressed in fix(tlon): bound auth response body at 16 MiB #97730 and other sibling Tlon bounded-read PRs that are separate work.

Risk checklist

• User-visible behavior change? Yes (small) — error messages for non-OK subscribe responses with bodies above 8 KiB lose the body content. Operators see Subscribe failed: 400 instead of Subscribe failed: 400 - <body>. Normal short-body errors are unchanged.
• Config/env/migration change? No.
• Security/auth/secrets/network/tool-execution change? Yes — adds a body-size cap to one previously-uncovered network read path. No new secrets handling.
• Sibling PR fix(tlon): bound auth response body at 16 MiB #97730 status: still open as of this PR's authoring. This PR is independent of fix(tlon): bound auth response body at 16 MiB #97730's merge status. Sibling Tlon hardening is separate work and is explicitly out of scope for this PR.

Related

• Sibling Tlon bounded-read PRs in the same campaign:
  • fix(tlon): bound auth response body at 16 MiB #97730 — Tlon auth response bounded read (still open; separate work)
  • Other Tlon surface read sites (GET-subscription ack, channel-close, etc.) — separate work, not in this PR's scope
• The readResponseWithLimit helper is the canonical OpenClaw bounded-read primitive (also used by discord gateway fix(discord): bound gateway metadata response reads at 16 MiB #97721, openai-completions test(openai-completions): add fetch-proof test for buildGuardedModelFetch wiring #97876, azure-openai-responses fix(openai-completions): bound SSE response reads via buildGuardedModelFetch #97228, anthropic-cloudflare fix(anthropic): wire buildGuardedModelFetch into the Cloudflare createClient branch #98003, etc.).
• Plugin SDK source: openclaw/plugin-sdk/response-limit-runtime exports readResponseWithLimit(response, maxBytes) and cancelUnreadResponseBody(response).
• The sendSubscription method is the public production path that the Urbit channel gateway invokes when a new subscription is registered; the err callback feeds the worker error handler.

AI disclosure

AI-assisted (Claude Sonnet); reviewed by human author before submission. Real environment verification (fresh vitest run on commit 2cea10db01, oxlint, tsgo, loopback real-fetch proof) performed by human author. The loopback test exercises the actual production urbitFetch → fetchWithSsrFGuard → fetchImpl chain end-to-end against a real local HTTP server, with byte-level quantitative assertion (MAX < got < TOTAL) and a negative control that fails when the source line is reverted to response.text(). Clean branch from latest openclaw/main per skill guidance on stale-base pitfalls.

Note on body update path

The body update is included in this comment because gh pr edit --body-file and direct REST/GraphQL PATCH/mutation all silently fail on this PR's repository (Projects classic deprecation + likely other GitHub-side restrictions on gh pr edit for OpenClaw PRs). The new body content above is the source of truth; the comment's "What Problem This Solves" section now correctly identifies #97730 as still open (separate work) rather than "merged", and the diff scope now reflects the 3 files / +239/-1 (including the new sse-client.loopback.test.ts real-fetch proof) rather than the previous 2 files / +78/-1.

Live HEAD: 2cea10db01. Base: openclaw/main @ 6cb82eaab8. Branch: fix/tlon-urbit-sse-client-errortext-bound (3 commits ahead of base).

🦞🧹
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.

Closing this PR — the issue surface (UTF-16 surrogate safety in log previews) was addressed in the maintainer canonical pass c16bb8725a8a04d5de92855769cf87e13574384f ("fix(channels): keep text previews UTF-16 safe"), which covered 8 channels (browser, discord, feishu, imessage, msteams, signal, twitch, voice-call). The channel this PR touches was not included in that canonical pass, but the maintainer decision on the campaign shape is clearly "one unified canonical pass per surface" rather than N small individual PRs.

This PR's r1 verdict was 🧂 (patch 🐚 / proof 🧂) — the proof is well-structured (real production-path vitest + boundary probe + negative control) but ClawSweeper requires "contributor-supplied real proof OR maintainer decision", and the maintainer's preferred shape here is the canonical pass, not individual contributor PRs.

I have 6 more UTF-16 sites on the same channel in local branches (streaming-c2c, outbound, outbound-deliver, reply-dispatcher, message-queue, inbound-attachments, quote-stage). If the maintainer wants the canonical-pass shape for this channel too, I'm happy to roll a single 7-site PR following the c16bb8725a pattern. Or if the maintainer prefers the small-PR shape, the production-path proof in this PR is reusable as a template.

Closing to free up the open PR quota for higher-priority work. Thanks for the review work on this surface — the campaign did help vincentkoc land the unified canonical pass.