Skip to content

feat(sdk)!: add options.headers, fetchOptions, and fetch - #456

Merged
EricAndrechek merged 19 commits into
mainfrom
sdk-fetch-override
Aug 12, 2026
Merged

feat(sdk)!: add options.headers, fetchOptions, and fetch#456
EricAndrechek merged 19 commits into
mainfrom
sdk-fetch-override

Conversation

@EricAndrechek

@EricAndrechek EricAndrechek commented Aug 12, 2026

Copy link
Copy Markdown
Member

Closes #269. Closes #464. Adds the SDK's HTTP-customization surface — options.headers, options.fetchOptions, and options.fetch — so a WaveHouse behind a gate is reachable from the client.

Why

ClientConfig exposed only baseURL, auth, and options.maxRetries. A WaveHouse fronted by a header-gated proxy (Cloudflare Access, an mTLS sidecar, an auth gateway) simply couldn't be talked to — a defense-in-depth gate forced consumers off the SDK entirely. Found via WaveHouse-Stats dogfooding.

The second driver is a runtime bug consumers can't fix from inside the SDK: undici 8.8.0–8.9.0 stalls a request before it goes out when a keep-alive socket is reused on an idle event loop (nodejs/undici#5600, fixed in 8.10.0), and Node 26 bundles 8.9.0. Severity varies with runtime and idle gap — upstream measured ~450–465ms; on Node 26.7.0 with 6s gaps against a stub server answering instantly:

SDK  max=23990ms  >500ms: 4/5  [12, 23990, 2203, 2100, 2099]

What landed

Option Purpose
options.headers Static headers on every REST request — the CF-Access case
options.fetchOptions Extra RequestInit merged in — cache, keepalive, credentials, Next.js next: { tags }
options.fetch Replace the HTTP implementation outright

Shaped after Supabase (global.fetch/global.headers) and OpenAI/Anthropic (fetch/fetchOptions/defaultHeaders) rather than invented here.

Header precedence, lowest to highest: options.headersauth → SDK-computed (Content-Type, Accept). Names match case-insensitively, and a collision drops the configured value rather than joining it. Both rules come from bugs other SDKs shipped: a global Content-Type joined with an upload's own produced application/json, image/png and 415s, and a case-sensitive Authorization check let a lowercase spelling ride alongside the canonical one. Two configured spellings of one header collapse to the last, so Headers can't comma-join them on the wire.

fetchOptions can't corrupt the requestmethod, headers, body, and signal are applied after the spread. fetchOptions.headers is ignored rather than merged; options.headers is the header channel, and merging both would give one concept two precedence stories and a side door around auth.

The undici workaround took three tries to get right

Worth reading before you follow it elsewhere, because the obvious forms don't work. Measured on undici 8.9.0, 10ms server, 1.5s idle gaps (per-request ms):

default Agent()                    22 1513 1493  584
Agent({ keepAliveTimeout: 1_000 }) 13 1500 1496 1513   ← inert
Agent({ pipelining: 0 })           13   13   12   12
same script on undici 8.10.0       22   15   13   13

Lowering keepAliveTimeout does nothing — the retirement timer is starved by the same idle event loop that causes the bug.

Worse, importing a fixed undici isn't enough on its own. undici keeps its connection pool on a shared globalThis symbol claimed by whichever copy loads first — the bundled one, on an affected runtime. With 8.9.0 loaded first and 8.10.0's fetch doing the work:

no explicit dispatcher    21 1514 1495  583   ← still stalling
explicit new Agent()      17   14   12   13

So the documented snippet passes dispatcher explicitly. Verified end-to-end through the SDK with 8.9.0 loaded first: 24, 15, 13, 14ms.

Breaking changes

PipeRef.fetch no longer accepts limitcloses #464, raised by CodeRabbit here. It took the shared per-call options type, which carries limit, but forwarded only signal, so wh.pipe('x').fetch({ limit: 10 }) compiled and silently did nothing. Nothing to forward: the endpoint binds the body as the pipe's parameters (internal/api/pipes.gopipes.BindParams), so a row cap belongs in the pipe's SQL as {{limit}}, passed via wh.pipe(name, { limit }) — which is what the docs already showed. Now a dedicated exported PipeRequestOptionssignal?: AbortSignal; limit?: never.

The never is load-bearing. The first attempt used Pick<RequestOptions, "signal">, which CodeRabbit correctly flagged as only half a fix: TypeScript's excess-property check is a freshness heuristic, so it rejects a fresh literal but not a variable. const opts: RequestOptions = { signal, limit: 10 }; wh.pipe('x').fetch(opts) still compiled and still dropped the limit — the original defect, in the shape real code is more likely to take. Both forms are now pinned by @ts-expect-error tests.

Collateral effect, and the half you'll actually hit: a value declared RequestOptions no longer assigns to a pipe .fetch() at all, even carrying no limit at runtime, since the declared type permits one. Type a shared options object as PipeRequestOptions — the table and query-builder .fetch() accept it too, so it works everywhere — or inline { signal }. Structural wrappers are unaffected: method parameters compare bivariantly, so interface Fetchable { fetch(opts?: RequestOptions): … } is still satisfied by PipeRef (verified).

Pre-existing on main; folded in here because this PR renames that exact type and gives it a JSDoc describing it as the options for .fetch(), which made the false advertisement more prominent rather than less.

FetchOptionsRequestOptions (the per-call type accepted by .fetch()), no deprecated alias. The old name collided conceptually with the new options.fetchOptions, which — per the ecosystem — means "extra RequestInit", not "options for our .fetch() method". Nothing consumes it pre-1.0; renaming the import is the whole migration for this one — the PipeRef.fetch narrowing above is a separate, behavioural break in the same file. The module-private RequestOptions in http.ts became RequestSpec to free the name.

Design notes

fetch stays optional all the way to the internal HttpContext rather than being resolved at construction, so the default path calls the global directly. That keeps it late-bound (replacing globalThis.fetch after a client exists still works — what vi.stubGlobal does) and avoids invoking a detached fetch reference, which throws "Illegal invocation" on browsers, workerd, and Bun. Both are pinned by tests. Supabase's resolveFetch independently converges on the same closure-per-call shape; the SDKs that capture at construction are the ones with stale-fetch bug reports.

FetchLike is the standard fetch signature, written out rather than as typeof fetch because that resolves differently depending on whether the consumer's lib includes DOM.

Scope

REST only. .stream() and .liveQuery()'s live connection go through EventSource, which accepts neither headers nor a fetch — so a header-gated deployment can query but not stream until #203 changes that transport. Called out in a :::caution on the docs page rather than a footnote: the equivalent gap in Supabase's realtime client was found by a user whose RLS policies silently stopped matching. .liveQuery()'s initial backfill is an ordinary REST call and is covered.

Browser callers should know custom headers must also pass CORS preflight, and WaveHouse allow-lists a fixed set with no config knob — documented.

Tests

158 pass, 1 skipped across the package (30 in client.test.ts), covering: routing through a supplied fetch with the full RequestInit asserted; global fallback; late-binding after construction; retries using the override; headers applied to every request; Content-Type not displaceable; auth beating a lowercase authorization; two casings collapsing to one; fetchOptions merged; fetchOptions unable to touch method/body/headers; and two @ts-expect-error pins that PipeRef.fetch rejects limit, as a literal and via a named RequestOptions value.

Beyond unit tests, every documented snippet was compiled against the built package under DOM-inclusive and Node-only lib configs, and the header behaviour was verified on the wire against a live server — the CF-Access header sent, auth beating an impostor, Content-Type intact. That last step matters here: compile-plus-happy-path is exactly the verification that let the implied-dispatcher bug through twice.

Related

EricAndrechek and others added 8 commits August 12, 2026 00:24
The SDK called the global `fetch` directly, so consumers had no way to
influence how requests are made — ClientOptions exposed only maxRetries.
`options.fetch` accepts any fetch-compatible function and is used for every
request, retries included.

The motivating case: undici 8.8.0-8.9.0 stalls for seconds before writing a
request onto a socket idle for a few seconds (nodejs/undici#5600, fixed in
8.10.0), and Node 26 bundles 8.9.0. A consumer polling every few seconds
sees multi-second latency on calls that should take milliseconds, with no
recourse inside the SDK. With this they can supply an undici dispatcher
with a tuned keepAliveTimeout and carry on. The same hook covers the
ordinary reasons SDKs grow one: proxies, client certificates, tracing or
circuit-breaker middleware, and mocking HTTP in a consumer's own tests
without monkey-patching a global.

`fetch` stays optional through to HttpContext rather than being defaulted at
construction, so the default path still calls the global directly. That
keeps it late-bound — replacing globalThis.fetch after a client exists still
works, which is what vi.stubGlobal does — and avoids invoking a detached
`fetch` reference, which is not universally safe. Both properties are pinned
by tests.

Streaming is unaffected: .stream()/.liveQuery() go through EventSource,
which this does not replace. Documented as such.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014EJvon2pyU4zaSHTGJj8fE
Documentation sync for the preceding commit — a new public ClientOptions
field on a published package, which AGENTS.md §Documentation Sync requires
under [Unreleased].

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014EJvon2pyU4zaSHTGJj8fE
Pre-push review found the advertised type surface didn't match what
shipped, in two ways.

FetchLike was declared and referenced by ClientOptions but never listed
in index.ts's export block, so `import type { FetchLike } from
"@wavehouse/sdk"` did not resolve -- while the CHANGELOG claimed it was
exported. It's now in the export list; verified in dist/index.d.ts.

The documented undici example did not compile. Reproduced with the
repo's own tsc against undici 7.28: under the default lib (which
includes DOM, and is what this package's own tsconfig uses) the wrapper
fails with TS2322 on Response -- undici declares its own Request and
Response, separate from the ones behind the global fetch -- plus TS2345
on the input arg. Assigning undici's fetch export directly fails under
Node-only libs too. So the "satisfies it without casting" claim was
wrong in general, not just at the edges. The example now carries the
casts that make it typecheck (verified clean under both lib configs
against the built package), and the JSDoc says plainly that such
implementations need a cast, and why.

Also corrected: liveQuery's initial backfill is an ordinary request and
does go through the supplied fetch -- only the SSE connection is exempt.
The docs, JSDoc and CHANGELOG all said otherwise, and streaming.md
already described the backfill correctly, so the pages disagreed.

The docs section had been inserted between the ClientConfig table and
that section's trailing content, orphaning the auth note and the
baseURL path-prefix subsection under an h3 about fetch; it now sits
after them. Adds the install note for the undici import, states the
contract an implementer must satisfy (string URL, plain RequestInit,
only .ok/.status/.headers/.text() read back, rejection -> retried
NETWORK_ERROR, AbortError -> ABORTED), and leads with a middleware
example that compiles anywhere.

The override test asserted only that init.method was some string; it
now pins the full init -- string URL, POST, Content-Type, Authorization,
serialized body -- which is what proxy and middleware consumers rely on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
Second pre-push review round. The contract sentence added in the last
commit was itself wrong, and the type didn't accept the shape it
described.

The contract claimed the SDK reads "only .ok/.status/.headers/.text()"
off the response. The error path also calls res.json() and reads
res.statusText (errors.ts:7,17). A response object built to the
documented spec doesn't crash -- parseErrorResponse swallows the
TypeError -- it silently degrades, yielding a WaveHouseError whose
message is typed string but is undefined at runtime, on every error
response. Contract now lists all six members and says which are read
when.

FetchLike was typeof fetch, which rejects the very middleware signature
the docs tell you to write: (url: string, init?: RequestInit) =>
Promise<Response> is not assignable to a parameter accepting
RequestInfo | URL, so it fails on contravariance with an error that has
nothing to do with the cause the docs give. Narrowed to the signature
the SDK actually calls -- a string URL is all it ever passes. That's
strictly more permissive: the global fetch still assigns, and declared
middleware now assigns with no cast. Verified against the built package
under both a DOM-inclusive and a Node-only lib config.

Abort handling is now stated: ABORTED requires a DOMException named
AbortError, so an implementation that signals abort another way --
node-fetch throws its own AbortError class -- is retried as
NETWORK_ERROR instead. That was worth saying out loud given the JSDoc
names node-fetch as a supported implementation.

Corrected the severity attributed to nodejs/undici#5600. It was
described as stalling "for seconds" on a socket idle for seconds; the
issue is titled "stalls up to ~500ms on an idle event loop" and the
reported measurements are ~450-465ms against a 10ms server, with the
repro making back-to-back requests. The trigger is an idle event loop,
not a seconds-idle socket. Also says plainly that 8.10.0 fixes it and
upgrading is the real answer -- the page previously steered readers
toward a permanent code change for a bug already fixed upstream.

The undici snippet used FetchLike and createClient with no SDK import,
so it didn't compile as pasted; it now imports what it uses. Config
table notes SSE streams are excluded, since that carve-out was three
sections below where most readers stop.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
…inert

Third pre-push review round. The docs prescribed a workaround for
nodejs/undici#5600 that does not work, which I confirmed by measuring
it rather than reasoning about it.

Against a 10ms local server on undici 8.9.0 (Node 22.23.2), with 1.5s
idle gaps, per-request latency:

  default Agent()                    22 1513 1493  584
  Agent({ keepAliveTimeout: 1_000 }) 13 1500 1496 1513
  Agent({ keepAliveTimeout: 1 })     15 1484 1511 1500
  Agent({ pipelining: 0 })           13   13   12   12
  same script on undici 8.10.0       22   15   13   13

So the tuning the page recommended changes nothing: the socket
retirement timer is starved by the same idle event loop that triggers
the bug, so the socket is still pooled and still reused. What actually
made the documented snippet fast is that `npm install undici` resolves
to 8.10.0 -- a fixed implementation -- not the dispatcher argument.

Reframed the remedy accordingly: options.fetch is how you route through
an undici you install yourself, so the copy bundled with your Node
never handles the request. Dropped the keepAliveTimeout argument and
its "retire idle sockets sooner" comment, and said plainly that the
knob does not help, since it is the obvious thing for a reader to try.
For anyone genuinely pinned to an affected undici, pipelining: 0 is
documented instead -- measured effective, at a connection per request.
The setGlobalDispatcher fallback is now attributed correctly: the
mechanism works, but only helps when the installed undici is 8.10.0+.

Also corrected the severity in the other direction from last commit.
The upstream issue title says ~500ms and I took it at face value; the
stall is actually erratic and idle-gap dependent, measuring ~100ms to
~2.5s across gaps. The docs now give the upstream number and our own
range rather than a single figure.

Contract precision: .status is only read on the failure path
(http.ts:80, errors.ts:19-22) -- the success branch touches only
.text() and .headers. Listing it as always-read was conservative but
wrong for a paragraph that is an explicit spec.

Verified the rewritten snippet by running it, not just typechecking
it: against a live server it returns data on the success path and, on
a 404, a WaveHouseError whose message is "Not Found" -- i.e. the
.statusText fallback the contract documents.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
Fourth pre-push review round.

The setGlobalDispatcher caveat ended "since it's the bundled
implementation doing the stalling", which argues for the opposite
conclusion: if the bundled copy is what stalls, a reader infers that
swapping the global dispatcher cannot help -- which would also undercut
the options.fetch remedy above it. The mechanism is that
setGlobalDispatcher hands connection handling to the installed copy, so
that copy is the one that has to carry the fix. Reworded to say so.

Also adds stubbing HTTP in your own tests to the list of reasons to
reach for the option. It is probably the most common one, and the
CHANGELOG entry already called it out while the page did not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
The text claimed our measurements ranged "~100 ms to ~2.5 s". That
ceiling is an artifact of one setup: npm-installed undici 8.9.0 on Node
22, where gaps of 4s+ show no stall at all because the default 4s
keepAliveTimeout has already retired the socket, so there is nothing to
reuse.

Measuring against Node 26's bundled 8.9.0 through the SDK tells a
different story -- 6s gaps against a stub server answering instantly
produced [12, 23990, 2203, 2100, 2099] ms. I can't reproduce that from
Node 22 with an npm-installed undici, so rather than assert a bound I
can't defend in both directions, the docs now say severity varies with
runtime and idle gap, quote the upstream ~450-465ms figure, and give
our own range as ~100ms to tens of seconds.

This also keeps the entry consistent with #455's account of the same
bug, which reported multi-second stalls from the e2e harness's
multi-second lingers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
@github-actions github-actions Bot added documentation Improvements or additions to documentation area/sdk TypeScript SDK (clients/ts/) area/docs Documentation, site/, README labels Aug 12, 2026
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@EricAndrechek, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: be1c21b0-047f-47bb-933a-4cc550241110

📥 Commits

Reviewing files that changed from the base of the PR and between a83de0c and f68c2ba.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • clients/ts/src/client.test.ts
  • clients/ts/src/index.ts
  • clients/ts/src/pipes.ts
  • clients/ts/src/types.ts
  • docs/src/content/docs/sdk/pipes.md
  • docs/src/content/docs/sdk/queries.md
📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added configurable REST request headers and RequestInit options.
    • Added custom fetch support for requests and retries, with global fetch fallback.
    • Exported the FetchLike type for TypeScript integrations.
    • Preserved SDK-controlled request fields and normalized header handling.
  • Breaking Changes

    • Renamed the per-request options type from FetchOptions to RequestOptions.
    • Pipe requests now accept only abort signals; row limits must be defined in pipe SQL.
  • Documentation

    • Added configuration guidance, retry behavior, error handling, limitations, and runtime integration examples.

Walkthrough

The TypeScript SDK adds configurable REST fetch behavior through options.fetch, options.headers, and options.fetchOptions. It exports FetchLike, renames per-call options to RequestOptions, narrows pipe fetch options to signal, and documents request, retry, SSE, abort, and undici behavior.

Changes

Configurable REST fetch transport

Layer / File(s) Summary
Fetch and request contracts
clients/ts/src/types.ts, clients/ts/src/index.ts, clients/ts/src/http.ts
The SDK defines FetchLike, adds client-level fetch and request configuration, exports RequestOptions, and renames the internal request descriptor to RequestSpec.
Request configuration and routing
clients/ts/src/client.ts, clients/ts/src/http.ts, clients/ts/src/client.test.ts
The client passes fetch settings into HTTP context. Requests merge headers case-insensitively and apply fetchOptions while preserving SDK-controlled fields. Tests cover custom fetch, fallback, retries, headers, and protected request fields.
Per-call request API update
clients/ts/src/pipes.ts, clients/ts/src/query-builder.ts, clients/ts/src/table.ts, clients/ts/src/client.test.ts
Fetch methods now use RequestOptions. PipeRef.fetch accepts only signal, and tests reject per-call limit.
SDK behavior documentation
docs/src/content/docs/sdk/index.mdx, docs/src/content/docs/sdk/pipes.md, docs/src/content/docs/sdk/reference.md, CHANGELOG.md
The documentation describes REST customization, RequestInit merging, custom fetch behavior, retry and abort handling, SSE scope, pipe options, and undici integration.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SDKClient
  participant HTTPRequest
  participant ConfiguredFetch
  participant GlobalFetch
  SDKClient->>HTTPRequest: Create request with client options
  HTTPRequest->>HTTPRequest: Merge headers and fetchOptions
  alt Custom fetch configured
    HTTPRequest->>ConfiguredFetch: Send URL and protected RequestInit
    HTTPRequest->>ConfiguredFetch: Retry request when required
  else No custom fetch configured
    HTTPRequest->>GlobalFetch: Resolve and send request at invocation time
  end
Loading

Possibly related issues

Suggested reviewers: taitelee

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely identifies the main SDK HTTP customization changes through headers, fetchOptions, and fetch.
Description check ✅ Passed The description directly explains the SDK HTTP customization, breaking changes, scope, implementation details, documentation, and tests.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sdk-fetch-override
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch sdk-fetch-override

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

📚 Docs preview is livehttps://f1ea3dcb-wavehouse-docs.wave-rf.workers.dev

  • Commitf68c2ba: docs(sdk): state the half of the pipes break consumers will actually hit
  • Author@EricAndrechek, Claude Opus 5 (1M context)
  • Committed — 2026-08-12 17:31 (UTC-04:00)
  • Deployed — 2026-08-12 17:47 EDT

@github-code-quality

github-code-quality Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: Go

Go

The overall coverage in commit f68c2ba in the sdk-fetch-override branch remains at 90%, unchanged from commit 2dd2ab6 in the main branch.


Updated August 12, 2026 21:48 UTC

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b6d6d5e4-2aa4-4ac6-8d94-42c47e15434a

📥 Commits

Reviewing files that changed from the base of the PR and between 2dd2ab6 and 2f1b161.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • clients/ts/src/client.test.ts
  • clients/ts/src/client.ts
  • clients/ts/src/http.ts
  • clients/ts/src/index.ts
  • clients/ts/src/types.ts
  • docs/src/content/docs/sdk/index.mdx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
clients/ts/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

The TypeScript SDK in clients/ts/ is the canonical client; when backend API changes affect users, update the SDK surface, auth handling, query builder, streaming helpers, pipes/policy helpers, or regenerated types as needed.

Files:

  • clients/ts/src/index.ts
  • clients/ts/src/client.test.ts
  • clients/ts/src/http.ts
  • clients/ts/src/types.ts
  • clients/ts/src/client.ts
docs/src/content/docs/**/*.{md,mdx}

📄 CodeRabbit inference engine (AGENTS.md)

Documentation prose under the Starlight docs site must stay accurate against code, include runnable examples where relevant, and reflect code↔docs sync for changed behavior.

Files:

  • docs/src/content/docs/sdk/index.mdx
🧠 Learnings (10)
📓 Common learnings
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: clients/ts/src/types.ts:61-71
Timestamp: 2026-08-11T15:22:23.813Z
Learning: In the TypeScript SDK, `ClientConfig.baseURL` must be an absolute URL with a scheme and host. A relative `baseURL` causes `resolveURL` to throw a `TypeError` on the first request. REST requests reject because `resolveURL` runs outside `request()`'s retry `try` block. In `clients/ts/src/stream/sse.ts`, `SSETransport` catches this failure and reports `SSE_CONNECT_ERROR` through the optional `StreamSubscriber.error` callback, so a subscriber without that callback can observe no error.
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/**/*.{ts,tsx} : The TypeScript SDK in `clients/ts/` is the canonical client; when backend API changes affect users, update the SDK surface, auth handling, query builder, streaming helpers, pipes/policy helpers, or regenerated types as needed.
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/README.md : Keep the TypeScript SDK README in sync with SDK-facing changes and public client behavior.
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/README.md : Keep the TypeScript SDK README in sync with SDK-facing changes and public client behavior.

Applied to files:

  • clients/ts/src/index.ts
  • clients/ts/src/client.test.ts
  • CHANGELOG.md
  • clients/ts/src/types.ts
  • docs/src/content/docs/sdk/index.mdx
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/**/*.{ts,tsx} : The TypeScript SDK in `clients/ts/` is the canonical client; when backend API changes affect users, update the SDK surface, auth handling, query builder, streaming helpers, pipes/policy helpers, or regenerated types as needed.

Applied to files:

  • clients/ts/src/index.ts
  • clients/ts/src/client.test.ts
  • clients/ts/src/types.ts
  • docs/src/content/docs/sdk/index.mdx
📚 Learning: 2026-08-11T15:22:20.507Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: clients/ts/src/types.ts:61-71
Timestamp: 2026-08-11T15:22:20.507Z
Learning: In the TypeScript SDK, ensure `ClientConfig.baseURL` is validated or documented as requiring an absolute URL with both a scheme and host. Relative URLs cause `resolveURL` to throw a `TypeError` before REST retry handling runs. Ensure SSE connection failures are surfaced through `StreamSubscriber.error` and are not silently lost when that callback is absent.

Applied to files:

  • clients/ts/src/index.ts
  • clients/ts/src/client.test.ts
  • clients/ts/src/http.ts
  • clients/ts/src/types.ts
  • clients/ts/src/client.ts
📚 Learning: 2026-08-12T05:38:52.277Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 455
File: tests/e2e/sdk/helpers.ts:214-221
Timestamp: 2026-08-12T05:38:52.277Z
Learning: In `tests/e2e/sdk/helpers.ts`, `chQuery` must reclassify a caught error as a request timeout or caller abort only when the error is an abort error. On Node 22, `AbortSignal.timeout()` produces an error named `TimeoutError`, `AbortController.abort()` and caller cancellation through `AbortSignal.any()` produce `AbortError`, and `JSON.parse()` failures produce `SyntaxError`.

Applied to files:

  • clients/ts/src/client.test.ts
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/stream/**/*.{go} : Streaming/SSE code must preserve the hub’s per-role projection model, subscriber queues, bucket fan-out, heartbeating, and metrics semantics.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/observability/**/*.{go} : Observability code must preserve the repo’s logging and telemetry invariants, including stdout fan-out, OTLP behavior, lazy dialing, and the private Prometheus registry.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to docs/src/content/docs/configuration.md : Document every new or changed configuration option in the configuration docs.

Applied to files:

  • docs/src/content/docs/sdk/index.mdx
📚 Learning: 2026-08-11T15:22:23.813Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: clients/ts/src/types.ts:61-71
Timestamp: 2026-08-11T15:22:23.813Z
Learning: In the TypeScript SDK, `ClientConfig.baseURL` must be an absolute URL with a scheme and host. A relative `baseURL` causes `resolveURL` to throw a `TypeError` on the first request. REST requests reject because `resolveURL` runs outside `request()`'s retry `try` block. In `clients/ts/src/stream/sse.ts`, `SSETransport` catches this failure and reports `SSE_CONNECT_ERROR` through the optional `StreamSubscriber.error` callback, so a subscriber without that callback can observe no error.

Applied to files:

  • docs/src/content/docs/sdk/index.mdx
🪛 LanguageTool
docs/src/content/docs/sdk/index.mdx

[style] ~355-~355: Since ownership is already implied, this phrasing may be redundant.
Context: ...h client certificates, wrap requests in your own middleware (logging, tracing, circuit b...

(PRP_OWN)


[style] ~356-~356: Since ownership is already implied, this phrasing may be redundant.
Context: ...racing, circuit breaking), stub HTTP in your own tests without monkey-patching a global,...

(PRP_OWN)

🔇 Additional comments (7)
clients/ts/src/types.ts (1)

83-134: LGTM!

Also applies to: 328-329

clients/ts/src/index.ts (1)

31-32: LGTM!

clients/ts/src/client.ts (1)

40-40: LGTM!

clients/ts/src/http.ts (1)

59-69: LGTM!

clients/ts/src/client.test.ts (1)

9-9: LGTM!

Also applies to: 147-149, 155-191, 201-208

CHANGELOG.md (1)

14-15: LGTM!

docs/src/content/docs/sdk/index.mdx (1)

330-330: LGTM!

Also applies to: 352-429, 436-442

Comment thread clients/ts/src/client.test.ts Outdated
Comment thread docs/src/content/docs/sdk/index.mdx Outdated
@github-project-automation github-project-automation Bot moved this from Backlog to In review in WaveHouse Task Board Aug 12, 2026
Both from CodeRabbit on #456.

The override tests still carried `as unknown as FetchLike` from when
FetchLike was typeof fetch and a bare vi.fn() couldn't satisfy it. Since
the narrowing that is no longer true, so the casts were vestigial --
and a cast in the test undercuts the claim that a consumer needs none.
Now `vi.fn<FetchLike>(...)`, which also types the recorded call, so the
init assertions are checked against RequestInit instead of any.

The pinned-to-an-affected-undici escape was described in prose but not
shown, and it is the one case where you do have to merge a dispatcher
into the init the SDK hands you -- the main example no longer does,
since the remedy there is to bring your own fixed undici. Added the
snippet; typechecked under both lib configs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 12, 2026
@EricAndrechek

EricAndrechek commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

Scope expanded — this PR took the rest of #269's HTTP-customization surface rather than shipping the fetch override alone. All three are now implemented, reviewed, and green:

  • options.fetch — supply the HTTP implementation
  • options.headers — static headers on every REST request, case-insensitive, merged underneath the SDK's own
  • options.fetchOptions — extra RequestInit; SDK-controlled fields always win

Rationale: all three answer the same question ("how do I talk to a WaveHouse behind a gate?"), and shipping them separately would have meant three passes over the same config surface and docs section, with the first two each half-answering the case that motivated the issue.

Also landed here, beyond the original scope:

  • BREAKING: FetchOptionsRequestOptions (no deprecated alias; nothing consumes it pre-1.0). The old name collided with the new options.fetchOptions, which per the ecosystem means "extra RequestInit", not "options for our .fetch() method". The module-private RequestOptions in http.ts became RequestSpec.
  • FetchLike widened to the standard fetch signature. It was briefly narrowed mid-branch on the theory that it accepted strictly more; that held in one direction only — a value typed as the narrow form is rejected by every other SDK's wide option, so a shared wrapper couldn't be handed to both.
  • PipeRef.fetch no longer accepts a limit it silently ignored (closes fix(sdk): PipeRef.fetch accepts a limit option it silently ignores #464).

Split out and not in this PR: per-call overrides and dynamic header callbacks (#459), the auth() per-request contract (#458), and streaming/SSE coverage (#203 — nothing here can reach EventSource, which takes neither headers nor a fetch).

(Edited 2026-08-12 — the original text listed headers and fetchOptions as still to do, and predated the FetchLike widening.)

EricAndrechek and others added 5 commits August 12, 2026 14:30
Completes #269's REST-side scope. Three knobs now, shaped after the
conventions in Supabase's, OpenAI's and Anthropic's clients rather than
invented here.

options.headers adds static headers to every REST request -- the
motivating case being a Cloudflare Access service token in front of
WaveHouse. Names match case-insensitively, as HTTP requires, and the
merge is deliberately asymmetric: configured headers sit underneath the
SDK's own, and a collision drops the configured one rather than joining
the two. auth keeps Authorization; a request's Content-Type and Accept
can't be displaced by a global. Both rules are lifted from bugs other
SDKs shipped -- a global Content-Type joined with an upload's own
produced `application/json, image/png` and 415s, and a case-sensitive
Authorization check let a lowercase spelling ride alongside the
canonical one.

options.fetchOptions merges extra RequestInit fields, for what isn't a
header and doesn't warrant replacing the transport: credentials:
"include" for a cookie-authenticated origin, plus mode/cache and
runtime extensions like Next.js's next: { tags }. The fields the SDK
controls -- method, headers, body, signal -- are applied after the
spread, so this can't corrupt the request. fetchOptions.headers is
ignored rather than merged; options.headers is the header channel, and
merging both would give two precedence stories for one concept.

FetchLike widens to the standard fetch signature. It was narrowed
earlier in this branch on the theory that it accepted strictly more,
which was only true in one direction: a value typed as the narrow form
is rejected by every other SDK's wide option, so a shared wrapper
couldn't be handed to both. Every client surveyed uses the wide shape
and none carry complaints about it. Still written out rather than
`typeof fetch`, which resolves differently with and without DOM in lib.

REST-only, stated plainly in the docs rather than left implicit: SSE
takes neither headers nor a fetch, so a header-gated deployment can
query but not stream until #203. The same gap in Supabase's realtime
client was found by a user whose RLS policies silently stopped
matching, which is the failure mode worth pre-empting.

Verified on the wire against a live server, not just against a mocked
fetch: the CF-Access header is sent, auth beats a lowercase
`authorization` impostor, and Content-Type stays application/json.

Per-call overrides and dynamic header callbacks are deferred to #459.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
…widening

Review round on the expanded scope. Three real defects, two of them
caused by the FetchLike widening in the previous commit.

mergeHeaders matched case-insensitively against the SDK's headers but
not within the configured set, so `{ "x-tenant": "a", "X-Tenant": "b" }`
survived as two keys and the Headers constructor comma-joined them at
fetch time -- `x-tenant: a, b`. Verified on Node. That is the exact
corruption the function's own docblock cites as its reason to exist, so
it was worth closing rather than documenting. Configured names now
replace each other, last spelling wins, with a test.

Widening FetchLike broke both undici snippets and I did not re-check
them -- I verified the new headers/fetchOptions examples and assumed the
existing ones were unaffected. They were not: `url` is now
`string | URL | Request`, which is not assignable to undici's
RequestInfo whenever DOM is in lib, i.e. the TypeScript default.
Reproduced as TS2345 against undici 7.28 and undici-types 8.3. Both
snippets and the matching JSDoc now cast the URL, and the
"casts are load-bearing" paragraph explains why there are two.

The CHANGELOG entry still carried the pre-widening rationale and
contradicted itself a few sentences apart -- claiming FetchLike both
matches the standard signature and is deliberately narrower than it.
The narrow claim is now false against the shipped type. CHANGELOG.md
sits outside scripts/docs-prose.sh, so no gate would have caught it.

Also corrected in the docs:

- credentials: "include" was the headline fetchOptions example, and it
  cannot work against a stock WaveHouse. corsMiddleware deliberately
  never emits Access-Control-Allow-Credentials (it is a Bearer-token
  API, asserted by a test in router_test.go), and the default
  cors_allowed_origins "*" makes include a hard browser failure anyway.
  Now led by cache/keepalive, with credentials kept as a qualified note
  pointing at the proxy-owns-CORS case.
- Custom headers from a browser must pass CORS preflight, and
  Access-Control-Allow-Headers is a fixed list with no config knob, so
  a CF-Access header only works server-side or behind a proxy that owns
  CORS. Said outright rather than left for someone to discover.
- "Values replace, never append" described nothing observable, since a
  collision drops the configured value rather than replacing the SDK's.
  Reworded to what actually happens.
- mode/cache/keepalive were lumped in with "not declared on the
  standard RequestInit". They are standard; only the runtime
  extensions need a cast.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
Docs review found that the documented workaround does not do what it
claims, and measurement confirms it. Third time the undici guidance in
this branch has been wrong in a way only measurement caught.

The section said: install undici 8.10.0 yourself and route through it,
"so the version bundled with your Node never handles them". That is not
what happens. undici keeps its connection pool on a shared globalThis
symbol -- Symbol(undici.globalDispatcher.2) -- and the first copy loaded
claims it, which on an affected runtime is the bundled one. fetch()
resolves that global when no dispatcher is passed, so an installed
8.10.0's fetch dispatches through the bundled 8.9.0's pool.

Reproduced with both copies loaded side by side. new.getGlobalDispatcher()
returns an instance of old.Agent, and the timings follow, 1.5s idle gaps
against a 10ms server:

  no explicit dispatcher    21  1514  1495   583
  explicit new Agent()      17    14    12    13

So the snippet now passes the dispatcher explicitly, which never
consults the shared symbol, and a caution block explains why with those
numbers. Verified end to end through the SDK with 8.9.0 loaded first:
24, 15, 13, 14 ms.

This also collapses the section, since the pinned-to-an-affected-version
case was already passing an explicit dispatcher -- it is now the same
snippet with different Agent options rather than a second one. The
setGlobalDispatcher note gains the reason it works: an explicit write
wins the same race.

Also from the same review:

- "Both casts are load-bearing" undercounted the snippet, which has
  three. init as never was left unexplained, which is the one that looks
  most alarming. All three are now named, with what each bridges.
- Next.js next: { tags } was described as per-deployment cache control.
  It tags a cache entry for on-demand revalidation via revalidateTag().

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
Review round on the previous fix.

The caution said an explicit dispatcher and an explicit
setGlobalDispatcher both win "because it never consults the shared
symbol". That is only true of the dispatcher. setGlobalDispatcher wins
the opposite way -- it overwrites the symbol after the first copy
claimed it -- which is what the paragraph further down already said, so
the page contradicted itself and left no way to understand why a call
on the installed copy affects the bundled fetch at all. Reasons are now
split.

The casts paragraph still spelled the init cast `init as never`, which
was the pre-dispatcher snippet's text. The snippet now casts the merged
object. Beyond not matching, copying the cast as the prose spelled it
would drop `dispatcher` and land back on the stalling global pool that
the caution three lines above exists to prevent.

Also took the reviewer's suggestion on the pinned-version paragraph: it
reuses a snippet whose comment reads "8.10.0+", which is the opposite
of that reader's situation, so it now says to use your pinned version
with pipelining: 0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
The caution said the shared globalThis symbol is claimed by "the first
copy loaded -- which on an affected runtime is the bundled one". The
second half is wrong, and wrong in the ordering the snippet above it
creates.

Node does not claim the symbol at startup; it loads its bundled undici
on the first call to the built-in fetch. Verified locally: no
undici symbol exists on globalThis at startup, and
Symbol(undici.globalDispatcher.1) appears only after a fetch() call.
Since ESM imports evaluate before any user code runs, `import ... from
"undici"` normally makes the INSTALLED copy the claimant.

My earlier measurement forced the bundled-first ordering with an
explicit `import "old"`, which is why it looked deterministic. The
measurement sentence already says "with 8.9.0 loaded first", so it was
honest; the surrounding claim was not.

Reworded to what holds: whichever copy loads first claims it, Node
claims for the bundled copy on the first built-in fetch rather than at
startup, so ownership comes down to a load order you don't really
control -- which is a better argument for passing the dispatcher
explicitly than the deterministic version was, since it removes the
dependency instead of betting on it. Same softening in the CHANGELOG;
the JSDoc already hedged correctly.

Also labels the measurement block's units.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
Two leftovers from the previous round.

The caution named a built-in fetch() call as what makes Node claim the
shared dispatcher symbol. Too narrow. Verified on Node 22: new
Headers(), new Response(), new Request(), new FormData() and even
reading globalThis.WebSocket all claim it; only reading globalThis.fetch
without calling does not. That matters because the caution exists to
stop a reader reasoning about the ordering -- someone who audits "I
never call the built-in fetch, everything goes through undiciFetch"
gets a false all-clear. Worth saying outright that this SDK is one such
claimant: http.ts constructs a Headers on the abort and
retry-exhausted paths.

The CHANGELOG still said undici needs casts on "both the URL argument
and the return value". There are three -- the init cast is unavoidable
regardless of the DOM-lib split, since dispatcher isn't a RequestInit
field at all. Same undercount already fixed in the docs and already
correct in the JSDoc, so the PR was contradicting itself across its own
artifacts. CHANGELOG.md is excluded from scripts/docs-prose.sh by name,
which is why this is the second factual drift to survive there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
@EricAndrechek EricAndrechek changed the title feat(sdk): allow overriding the HTTP implementation via options.fetch feat(sdk)!: add options.headers, fetchOptions, and fetch Aug 12, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6c5b3cc9-35af-4bb6-9d93-2c65619151da

📥 Commits

Reviewing files that changed from the base of the PR and between 2f1b161 and d113264.

📒 Files selected for processing (10)
  • CHANGELOG.md
  • clients/ts/src/client.test.ts
  • clients/ts/src/client.ts
  • clients/ts/src/http.ts
  • clients/ts/src/index.ts
  • clients/ts/src/pipes.ts
  • clients/ts/src/query-builder.ts
  • clients/ts/src/table.ts
  • clients/ts/src/types.ts
  • docs/src/content/docs/sdk/index.mdx
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
  • GitHub Check: Unit tests
  • GitHub Check: Coverage
  • GitHub Check: Integration tests
  • GitHub Check: Docs build
  • GitHub Check: E2E tests
  • GitHub Check: Lint
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (2)
clients/ts/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

The TypeScript SDK in clients/ts/ is the canonical client; when backend API changes affect users, update the SDK surface, auth handling, query builder, streaming helpers, pipes/policy helpers, or regenerated types as needed.

Files:

  • clients/ts/src/index.ts
  • clients/ts/src/client.ts
  • clients/ts/src/table.ts
  • clients/ts/src/query-builder.ts
  • clients/ts/src/client.test.ts
  • clients/ts/src/types.ts
  • clients/ts/src/http.ts
  • clients/ts/src/pipes.ts
docs/src/content/docs/**/*.{md,mdx}

📄 CodeRabbit inference engine (AGENTS.md)

Documentation prose under the Starlight docs site must stay accurate against code, include runnable examples where relevant, and reflect code↔docs sync for changed behavior.

Files:

  • docs/src/content/docs/sdk/index.mdx
🧠 Learnings (24)
📓 Common learnings
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: docs/src/content/docs/sdk/index.mdx:0-0
Timestamp: 2026-08-12T15:28:20.891Z
Learning: For `docs/src/content/docs/sdk/index.mdx`, the documented workaround for the undici idle-event-loop keep-alive stall is to upgrade to undici 8.10.0 or later. If a consumer is pinned to an affected version, `new Agent({ pipelining: 0 })` must be merged as `dispatcher` into the SDK-provided `RequestInit`; this disables keep-alive reuse. Configuring `keepAliveTimeout` does not mitigate this stall because the socket retirement timer is starved by the same idle event loop.
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/**/*.{ts,tsx} : The TypeScript SDK in `clients/ts/` is the canonical client; when backend API changes affect users, update the SDK surface, auth handling, query builder, streaming helpers, pipes/policy helpers, or regenerated types as needed.
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: clients/ts/src/types.ts:61-71
Timestamp: 2026-08-11T15:22:23.813Z
Learning: In the TypeScript SDK, `ClientConfig.baseURL` must be an absolute URL with a scheme and host. A relative `baseURL` causes `resolveURL` to throw a `TypeError` on the first request. REST requests reject because `resolveURL` runs outside `request()`'s retry `try` block. In `clients/ts/src/stream/sse.ts`, `SSETransport` catches this failure and reports `SSE_CONNECT_ERROR` through the optional `StreamSubscriber.error` callback, so a subscriber without that callback can observe no error.
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/README.md : Keep the TypeScript SDK README in sync with SDK-facing changes and public client behavior.
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/README.md : Keep the TypeScript SDK README in sync with SDK-facing changes and public client behavior.

Applied to files:

  • clients/ts/src/index.ts
  • clients/ts/src/client.ts
  • clients/ts/src/table.ts
  • clients/ts/src/query-builder.ts
  • clients/ts/src/client.test.ts
  • CHANGELOG.md
  • clients/ts/src/types.ts
  • clients/ts/src/http.ts
  • docs/src/content/docs/sdk/index.mdx
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/**/*.{ts,tsx} : The TypeScript SDK in `clients/ts/` is the canonical client; when backend API changes affect users, update the SDK surface, auth handling, query builder, streaming helpers, pipes/policy helpers, or regenerated types as needed.

Applied to files:

  • clients/ts/src/index.ts
  • clients/ts/src/client.ts
  • clients/ts/src/query-builder.ts
  • clients/ts/src/client.test.ts
  • CHANGELOG.md
  • clients/ts/src/types.ts
  • clients/ts/src/http.ts
  • docs/src/content/docs/sdk/index.mdx
📚 Learning: 2026-08-11T15:22:20.507Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: clients/ts/src/types.ts:61-71
Timestamp: 2026-08-11T15:22:20.507Z
Learning: In the TypeScript SDK, ensure `ClientConfig.baseURL` is validated or documented as requiring an absolute URL with both a scheme and host. Relative URLs cause `resolveURL` to throw a `TypeError` before REST retry handling runs. Ensure SSE connection failures are surfaced through `StreamSubscriber.error` and are not silently lost when that callback is absent.

Applied to files:

  • clients/ts/src/index.ts
  • clients/ts/src/client.ts
  • clients/ts/src/table.ts
  • clients/ts/src/query-builder.ts
  • clients/ts/src/client.test.ts
  • clients/ts/src/types.ts
  • clients/ts/src/http.ts
  • clients/ts/src/pipes.ts
📚 Learning: 2026-08-11T21:56:03.206Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/query_builder.go:278-291
Timestamp: 2026-08-11T21:56:03.206Z
Learning: In `clients/go/query_builder.go`, `fetchNextTyped` intentionally treats a failed JSON decode of a non-object typed `Row` as normal end-of-pagination. This behavior matches the existing “cursor column was not in the projection” path and TypeScript SDK parity. The broader behavior change is tracked in GitHub issue `#452`.

Applied to files:

  • clients/ts/src/table.ts
  • clients/ts/src/query-builder.ts
📚 Learning: 2026-08-11T21:56:06.521Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: docs/src/content/docs/sdk/go/queries.md:356-362
Timestamp: 2026-08-11T21:56:06.521Z
Learning: In Wave-RF/WaveHouse Go SDK cursor pagination, `fetchNextTyped` uses only the first `QueryBuilder.OrderBy` column and a strict `gt` or `lt` filter. Duplicate values at a page boundary can skip rows. The Go SDK documentation must require a unique ordering column until the shared Go and TypeScript composite-cursor or tie-breaker implementation tracked in GitHub issue `#452` is available.

Applied to files:

  • clients/ts/src/query-builder.ts
📚 Learning: 2026-08-12T05:38:52.277Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 455
File: tests/e2e/sdk/helpers.ts:214-221
Timestamp: 2026-08-12T05:38:52.277Z
Learning: In `tests/e2e/sdk/helpers.ts`, `chQuery` must reclassify a caught error as a request timeout or caller abort only when the error is an abort error. On Node 22, `AbortSignal.timeout()` produces an error named `TimeoutError`, `AbortController.abort()` and caller cancellation through `AbortSignal.any()` produce `AbortError`, and `JSON.parse()` failures produce `SyntaxError`.

Applied to files:

  • clients/ts/src/client.test.ts
  • CHANGELOG.md
📚 Learning: 2026-05-19T14:41:38.228Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 142
File: docs/scripts/screenshot.mjs:31-31
Timestamp: 2026-05-19T14:41:38.228Z
Learning: `docs/scripts/screenshot.mjs` in the Wave-RF/WaveHouse repo is an intentionally manual dev-iteration tool (not wired into CI). Response-status validation and retry logic are deliberately deferred until the script is promoted to a CI visual-regression workflow. Do not flag the absence of `response.ok()` checks as an issue in this file.

Applied to files:

  • clients/ts/src/client.test.ts
📚 Learning: 2026-08-12T05:35:22.804Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 455
File: clients/ts/README.md:11-11
Timestamp: 2026-08-12T05:35:22.804Z
Learning: For Wave-RF/WaveHouse, CI currently tests Node 22 only through `.nvmrc` and `.github/actions/setup-env` using `node-version-file`. Local `make ci` verification on Node 26 is not CI coverage and must not be documented as continuously tested runtime support.

Applied to files:

  • clients/ts/src/client.test.ts
📚 Learning: 2026-08-12T15:28:20.891Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: docs/src/content/docs/sdk/index.mdx:0-0
Timestamp: 2026-08-12T15:28:20.891Z
Learning: For `docs/src/content/docs/sdk/index.mdx`, the documented workaround for the undici idle-event-loop keep-alive stall is to upgrade to undici 8.10.0 or later. If a consumer is pinned to an affected version, `new Agent({ pipelining: 0 })` must be merged as `dispatcher` into the SDK-provided `RequestInit`; this disables keep-alive reuse. Configuring `keepAliveTimeout` does not mitigate this stall because the socket retirement timer is starved by the same idle event loop.

Applied to files:

  • CHANGELOG.md
  • clients/ts/src/types.ts
  • docs/src/content/docs/sdk/index.mdx
📚 Learning: 2026-07-07T12:38:15.328Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:15.328Z
Learning: Repo: Wave-RF/WaveHouse. WaveHouse deliberately does not log or trace any client IP address anywhere in the codebase. `middleware.RealIP` was removed in PR `#332` due to IP-spoofing GHSAs, and trusted-proxy-aware client-IP extraction for logs/traces is tracked as a future cross-cutting effort in issue `#333`. Do not suggest adding `r.RemoteAddr` or naive `X-Forwarded-For`-derived IPs to logs (e.g., audit logs in internal/auth/auth.go for the operator-key path) until `#333` lands with proper trusted-proxy handling.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-08-11T15:22:47.380Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: docs/src/content/docs/sdk/index.mdx:330-334
Timestamp: 2026-08-11T15:22:47.380Z
Learning: In WaveHouse Go server authentication, `internal/auth/auth.go` `bearerToken` returns from the `Authorization` header path before modifying `r.URL`. It removes the `token` query parameter only when authentication uses the query parameter without an `Authorization` header. Documentation must state that this protects WaveHouse's own logs only; reverse proxies, CDNs, load balancers, and other upstream intermediaries require query-string redaction.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/auth/**/*.{go} : JWT auth middleware must always run, verify with either HMAC or JWKS (not both), pin accepted `alg` to the active verifier, and keep authN/authZ decoupled except for the sanctioned operator key.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-05-13T20:41:09.256Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/api/health_test.go:100-163
Timestamp: 2026-05-13T20:41:09.256Z
Learning: In `internal/api/health_test.go` (WaveHouse), every handler test explicitly asserts `Content-Type: application/json` and `X-Content-Type-Options: nosniff` headers, including on 503 responses. This is deliberate regression coverage: the comment in `TestHealth_Readiness_PingFails` explains that without the 503-path header test, a future refactor moving header setup into the success branch would silently drop headers on error responses. New boot-degraded tests should follow the same pattern.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/observability/**/*.{go} : Observability code must preserve the repo’s logging and telemetry invariants, including stdout fan-out, OTLP behavior, lazy dialing, and the private Prometheus registry.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/stream/**/*.{go} : Streaming/SSE code must preserve the hub’s per-role projection model, subscriber queues, bucket fan-out, heartbeating, and metrics semantics.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-05-25T11:24:24.022Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 180
File: internal/cache/local.go:0-0
Timestamp: 2026-05-25T11:24:24.022Z
Learning: In the WaveHouse codebase (`internal/cache/local.go` and related packages), package-level `var` declarations of immutable `metric.MeasurementOption` / OTel attribute sets (e.g., `cacheL1Attrs = metric.WithAttributes(attribute.String("tier", "L1"))`) are intentional and acceptable. These are pre-allocated constants analogous to `regexp.MustCompile(...)`, not mutable global state. The AGENTS.md "no global state / constructor injection" rule applies to application dependencies (Cache, Publisher, Deduplicator), not to stateless OTel metric attribute options. Do not flag these as violations of the constructor-injection guideline.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-05-20T01:02:03.228Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:03.228Z
Learning: In the WaveHouse project (`internal/api/**/*_test.go`), the convention for testing `RequireRole` middleware is to inject `ContextKeyRole` directly into the request context rather than using `testutil.MakeJWT`. JWT token parsing is covered separately in `middleware_test.go` (17 dedicated tests). Do not suggest switching role-gate tests to JWT-driven tests — the separation of concerns is intentional to keep failure surfaces isolated.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-05-25T11:25:11.992Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 180
File: internal/observability/instruments.go:40-117
Timestamp: 2026-05-25T11:25:11.992Z
Learning: In the WaveHouse project (Go), package-level `var` declarations of OTel metric instruments (e.g., `metric.Float64Histogram`, `metric.Int64Counter`) created via `Meter().Float64Histogram(...)` / `Meter().Int64Counter(...)` are idiomatic and intentional — they follow the OTel Go SDK global proxy pattern and are NOT considered "global state" violations under the AGENTS.md constructor-injection rule. That rule targets swappable application-level interface dependencies (Cache, Publisher, Subscriber, Deduplicator), not OTel proxy instruments. Do not suggest wrapping these into an `Instruments` struct for injection.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-05-20T20:30:22.556Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-06-26T15:07:28.749Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 0
File: :0-0
Timestamp: 2026-06-26T15:07:28.749Z
Learning: In the Go SSE implementation in `internal/api/stream.go`, keepalive frames from `internal/stream.Heartbeater` are only written from the post-replay select loop. The replay/gap-fill step is synchronous before entering that loop, so registering the `internal/stream.Subscriber` before replay does not materially improve idle-time coverage during replay; it can at most buffer one heartbeat in the subscriber's capacity-1 queue. Covering a genuinely long replay would require interleaving replay with the select loop and is tied to the broader delivery-path rework tracked by Issue `#294`.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.

Applied to files:

  • CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to docs/src/content/docs/configuration.md : Document every new or changed configuration option in the configuration docs.

Applied to files:

  • docs/src/content/docs/sdk/index.mdx
📚 Learning: 2026-08-11T15:22:23.813Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: clients/ts/src/types.ts:61-71
Timestamp: 2026-08-11T15:22:23.813Z
Learning: In the TypeScript SDK, `ClientConfig.baseURL` must be an absolute URL with a scheme and host. A relative `baseURL` causes `resolveURL` to throw a `TypeError` on the first request. REST requests reject because `resolveURL` runs outside `request()`'s retry `try` block. In `clients/ts/src/stream/sse.ts`, `SSETransport` catches this failure and reports `SSE_CONNECT_ERROR` through the optional `StreamSubscriber.error` callback, so a subscriber without that callback can observe no error.

Applied to files:

  • docs/src/content/docs/sdk/index.mdx
🪛 LanguageTool
docs/src/content/docs/sdk/index.mdx

[style] ~389-~389: Since ownership is already implied, this phrasing may be redundant.
Context: ...he SDK's value stands alone, and two of your own entries differing only in case collapse...

(PRP_OWN)


[style] ~518-~518: Since ownership is already implied, this phrasing may be redundant.
Context: ... you don't really control, and auditing your own code for fetch calls won't tell you: ...

(PRP_OWN)


[style] ~539-~539: Since ownership is already implied, this phrasing may be redundant.
Context: ... one underlying reason: undici declares its own request/response types, separate from t...

(PRP_OWN)


[style] ~543-~543: Consider using the typographical ellipsis character here instead.
Context: ...RequestInfonames its *own*Request; { ...init, dispatcher } as never` covers the ...

(ELLIPSIS)


[style] ~545-~545: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...ither spelling, so one snippet compiles whether or not your lib includes DOM); and the retur...

(WHETHER)

🔇 Additional comments (17)
clients/ts/src/types.ts (1)

86-160: LGTM!

Also applies to: 334-342, 360-366

clients/ts/src/index.ts (1)

32-46: LGTM!

clients/ts/src/http.ts (1)

5-5: LGTM!

Also applies to: 28-56, 62-62, 84-106

CHANGELOG.md (1)

14-14: LGTM!

Also applies to: 28-28

clients/ts/src/client.ts (1)

41-42: LGTM!

clients/ts/src/client.test.ts (1)

150-152: LGTM!

Also applies to: 167-173, 197-199, 212-289, 291-331

docs/src/content/docs/sdk/index.mdx (5)

330-336: LGTM!


358-401: LGTM!


403-435: LGTM!


470-474: LGTM!


493-562: LGTM!

clients/ts/src/pipes.ts (1)

4-4: LGTM!

clients/ts/src/query-builder.ts (3)

12-12: LGTM!


140-140: LGTM!


242-242: LGTM!

clients/ts/src/table.ts (2)

9-9: LGTM!


61-61: LGTM!

Comment thread clients/ts/src/pipes.ts Outdated
Closes #464. From CodeRabbit on #456.

PipeRef.fetch took the same RequestOptions as the query builder, which
carries `limit`, but forwarded only `signal` -- so
wh.pipe('x').fetch({ limit: 10 }) compiled, ran, and quietly did
nothing. QueryBuilder.fetch and TableRef.fetch both honour it, so the
inconsistency sat inside one shared type.

There is nothing to forward: internal/api/pipes.go binds the request
body as the pipe's parameters via pipes.BindParams, so a row cap only
exists if the pipe's own SQL declares one. The fix is the surface, not
the plumbing -- fetch now takes Pick<RequestOptions, "signal">, and the
JSDoc points at wh.pipe(name, { limit }) as the real route. A
@ts-expect-error test pins the rejection so it can't quietly widen back.

Pre-existing rather than introduced here, and I first filed it to keep
this diff focused. That was the wrong call: this PR renames that exact
type and gives it a JSDoc describing it as the options for `.fetch()`,
which makes the false advertisement more prominent, not less.

The new test also exposed a flaw in the suite's own fixture. fetchSpy
resolved a single shared Response, and a Response body can only be read
once -- so any test making two requests had the second fail and retry,
and an assertion on mock.calls[1] was reading that retry rather than a
second request. "adds configured headers to every request" was passing
for exactly that wrong reason. The mock now builds a fresh Response per
call, and that test asserts a call count of 2 so the distinction is
pinned rather than assumed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
coderabbitai[bot]
coderabbitai Bot previously approved these changes Aug 12, 2026
Both reviewers caught the same miss on the previous commit: it changed
a public method's signature and documented that only in the JSDoc.

CHANGELOG had no entry at all, which is worse than an omission here --
the existing BREAKING entry for the FetchOptions rename lists pipes.ts
in its file set and says "renaming the import is the whole migration",
so a consumer reading [Unreleased] end to end would conclude the only
pipes.ts change was cosmetic and their .fetch({ limit }) still
compiles. Added its own BREAKING entry with the migration, and
qualified the rename entry so the two don't contradict.

sdk/pipes.md documented the method as a bare `.fetch(opts?)` with opts
undefined, while sdk/queries.md documents an identically-titled
`.fetch(opts?)` with an options table including limit. Someone learning
opts on the queries page and carrying it to pipes got a type error with
nothing in the docs explaining it. The pipes page now states the
narrower type and redirects to the pipe-parameter route, and the API
tree in reference.md annotates the entry, which previously rendered
byte-identically to the two that do take limit.

Also fixed "bound a pipe with a {{limit}} parameter" in the JSDoc,
which read as the noun rather than an imperative.

No prose anywhere showed `.fetch({ limit })` on a pipe, so no example
was broken -- both docs and README already passed limit as a pipe
parameter, which is the route the narrowing pushes people toward.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
@EricAndrechek
EricAndrechek marked this pull request as ready for review August 12, 2026 21:09
@EricAndrechek
EricAndrechek requested review from a team and taitelee August 12, 2026 21:09

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 170f75a9-7b41-4e0c-81b7-4ce77900c8bf

📥 Commits

Reviewing files that changed from the base of the PR and between d113264 and a83de0c.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • clients/ts/src/client.test.ts
  • clients/ts/src/pipes.ts
  • docs/src/content/docs/sdk/pipes.md
  • docs/src/content/docs/sdk/reference.md
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
  • GitHub Check: Integration tests
  • GitHub Check: E2E tests
  • GitHub Check: Unit tests
  • GitHub Check: Coverage
  • GitHub Check: Docs build
  • GitHub Check: Lint
🧰 Additional context used
📓 Path-based instructions (2)
docs/src/content/docs/**/*.{md,mdx}

📄 CodeRabbit inference engine (AGENTS.md)

Documentation prose under the Starlight docs site must stay accurate against code, include runnable examples where relevant, and reflect code↔docs sync for changed behavior.

Files:

  • docs/src/content/docs/sdk/pipes.md
  • docs/src/content/docs/sdk/reference.md
clients/ts/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

The TypeScript SDK in clients/ts/ is the canonical client; when backend API changes affect users, update the SDK surface, auth handling, query builder, streaming helpers, pipes/policy helpers, or regenerated types as needed.

Files:

  • clients/ts/src/pipes.ts
  • clients/ts/src/client.test.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: clients/ts/src/client.test.ts:0-0
Timestamp: 2026-08-12T20:34:04.763Z
Learning: In the TypeScript SDK, `clients/ts/src/types.ts` defines `FetchLike` as `(input: string | URL | Request, init?: RequestInit) => Promise<Response>`. This explicit standard fetch-compatible signature avoids `typeof fetch` differences when DOM library types are absent and allows fetch wrappers to be shared with SDKs that use the standard wide input type.
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: clients/ts/src/pipes.ts:28-28
Timestamp: 2026-08-12T20:33:30.744Z
Learning: In the TypeScript SDK, `PipeRef.fetch` in `clients/ts/src/pipes.ts` accepts only a signal option. Pipe row limits are not generic request options. A pipe SQL definition can declare a `{{limit}}` parameter, and callers provide that parameter through `wh.pipe(name, { limit })`. The API binds the pipe request body as pipe parameters through `pipes.BindParams` in `internal/api/pipes.go`.
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/**/*.{ts,tsx} : The TypeScript SDK in `clients/ts/` is the canonical client; when backend API changes affect users, update the SDK surface, auth handling, query builder, streaming helpers, pipes/policy helpers, or regenerated types as needed.
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: clients/ts/src/types.ts:61-71
Timestamp: 2026-08-11T15:22:23.813Z
Learning: In the TypeScript SDK, `ClientConfig.baseURL` must be an absolute URL with a scheme and host. A relative `baseURL` causes `resolveURL` to throw a `TypeError` on the first request. REST requests reject because `resolveURL` runs outside `request()`'s retry `try` block. In `clients/ts/src/stream/sse.ts`, `SSETransport` catches this failure and reports `SSE_CONNECT_ERROR` through the optional `StreamSubscriber.error` callback, so a subscriber without that callback can observe no error.
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/README.md : Keep the TypeScript SDK README in sync with SDK-facing changes and public client behavior.
📚 Learning: 2026-08-12T20:33:30.744Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: clients/ts/src/pipes.ts:28-28
Timestamp: 2026-08-12T20:33:30.744Z
Learning: In the TypeScript SDK, `PipeRef.fetch` in `clients/ts/src/pipes.ts` accepts only a signal option. Pipe row limits are not generic request options. A pipe SQL definition can declare a `{{limit}}` parameter, and callers provide that parameter through `wh.pipe(name, { limit })`. The API binds the pipe request body as pipe parameters through `pipes.BindParams` in `internal/api/pipes.go`.

Applied to files:

  • docs/src/content/docs/sdk/pipes.md
  • docs/src/content/docs/sdk/reference.md
  • clients/ts/src/pipes.ts
  • clients/ts/src/client.test.ts
  • CHANGELOG.md
📚 Learning: 2026-06-29T14:21:45.067Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 343
File: internal/api/pipe_deps.go:0-0
Timestamp: 2026-06-29T14:21:45.067Z
Learning: In `internal/api/pipes.go`, direct table-function reads and direct cross-database table reads are intentionally omitted from the pipe dependency set and continue using the normal query-derived TTL; only resolved-but-unmaintainable dependencies (such as unknown or unfoldable view-derived names) trigger the unresolved-dependency TTL cap.

Applied to files:

  • docs/src/content/docs/sdk/pipes.md
  • clients/ts/src/pipes.ts
  • clients/ts/src/client.test.ts
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.

Applied to files:

  • docs/src/content/docs/sdk/pipes.md
  • docs/src/content/docs/sdk/reference.md
  • CHANGELOG.md
📚 Learning: 2026-08-12T20:34:04.763Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: clients/ts/src/client.test.ts:0-0
Timestamp: 2026-08-12T20:34:04.763Z
Learning: In the TypeScript SDK, `clients/ts/src/types.ts` defines `FetchLike` as `(input: string | URL | Request, init?: RequestInit) => Promise<Response>`. This explicit standard fetch-compatible signature avoids `typeof fetch` differences when DOM library types are absent and allows fetch wrappers to be shared with SDKs that use the standard wide input type.

Applied to files:

  • clients/ts/src/pipes.ts
  • clients/ts/src/client.test.ts
  • CHANGELOG.md
📚 Learning: 2026-08-11T21:56:06.521Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: docs/src/content/docs/sdk/go/queries.md:356-362
Timestamp: 2026-08-11T21:56:06.521Z
Learning: In Wave-RF/WaveHouse Go SDK cursor pagination, `fetchNextTyped` uses only the first `QueryBuilder.OrderBy` column and a strict `gt` or `lt` filter. Duplicate values at a page boundary can skip rows. The Go SDK documentation must require a unique ordering column until the shared Go and TypeScript composite-cursor or tie-breaker implementation tracked in GitHub issue `#452` is available.

Applied to files:

  • clients/ts/src/pipes.ts
  • CHANGELOG.md
📚 Learning: 2026-08-11T21:56:03.206Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/query_builder.go:278-291
Timestamp: 2026-08-11T21:56:03.206Z
Learning: In `clients/go/query_builder.go`, `fetchNextTyped` intentionally treats a failed JSON decode of a non-object typed `Row` as normal end-of-pagination. This behavior matches the existing “cursor column was not in the projection” path and TypeScript SDK parity. The broader behavior change is tracked in GitHub issue `#452`.

Applied to files:

  • clients/ts/src/pipes.ts
  • clients/ts/src/client.test.ts
  • CHANGELOG.md
📚 Learning: 2026-05-20T20:30:22.556Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.

Applied to files:

  • clients/ts/src/pipes.ts
📚 Learning: 2026-08-11T15:22:20.507Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: clients/ts/src/types.ts:61-71
Timestamp: 2026-08-11T15:22:20.507Z
Learning: In the TypeScript SDK, ensure `ClientConfig.baseURL` is validated or documented as requiring an absolute URL with both a scheme and host. Relative URLs cause `resolveURL` to throw a `TypeError` before REST retry handling runs. Ensure SSE connection failures are surfaced through `StreamSubscriber.error` and are not silently lost when that callback is absent.

Applied to files:

  • clients/ts/src/pipes.ts
  • clients/ts/src/client.test.ts
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/**/*.{ts,tsx} : The TypeScript SDK in `clients/ts/` is the canonical client; when backend API changes affect users, update the SDK surface, auth handling, query builder, streaming helpers, pipes/policy helpers, or regenerated types as needed.

Applied to files:

  • clients/ts/src/client.test.ts
  • CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/README.md : Keep the TypeScript SDK README in sync with SDK-facing changes and public client behavior.

Applied to files:

  • clients/ts/src/client.test.ts
  • CHANGELOG.md
📚 Learning: 2026-08-12T05:38:52.277Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 455
File: tests/e2e/sdk/helpers.ts:214-221
Timestamp: 2026-08-12T05:38:52.277Z
Learning: In `tests/e2e/sdk/helpers.ts`, `chQuery` must reclassify a caught error as a request timeout or caller abort only when the error is an abort error. On Node 22, `AbortSignal.timeout()` produces an error named `TimeoutError`, `AbortController.abort()` and caller cancellation through `AbortSignal.any()` produce `AbortError`, and `JSON.parse()` failures produce `SyntaxError`.

Applied to files:

  • clients/ts/src/client.test.ts
  • CHANGELOG.md
📚 Learning: 2026-05-19T14:41:38.228Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 142
File: docs/scripts/screenshot.mjs:31-31
Timestamp: 2026-05-19T14:41:38.228Z
Learning: `docs/scripts/screenshot.mjs` in the Wave-RF/WaveHouse repo is an intentionally manual dev-iteration tool (not wired into CI). Response-status validation and retry logic are deliberately deferred until the script is promoted to a CI visual-regression workflow. Do not flag the absence of `response.ok()` checks as an issue in this file.

Applied to files:

  • clients/ts/src/client.test.ts
📚 Learning: 2026-08-12T05:35:22.804Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 455
File: clients/ts/README.md:11-11
Timestamp: 2026-08-12T05:35:22.804Z
Learning: For Wave-RF/WaveHouse, CI currently tests Node 22 only through `.nvmrc` and `.github/actions/setup-env` using `node-version-file`. Local `make ci` verification on Node 26 is not CI coverage and must not be documented as continuously tested runtime support.

Applied to files:

  • clients/ts/src/client.test.ts
📚 Learning: 2026-08-12T15:28:20.891Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: docs/src/content/docs/sdk/index.mdx:0-0
Timestamp: 2026-08-12T15:28:20.891Z
Learning: For `docs/src/content/docs/sdk/index.mdx`, the documented workaround for the undici idle-event-loop keep-alive stall is to upgrade to undici 8.10.0 or later. If a consumer is pinned to an affected version, `new Agent({ pipelining: 0 })` must be merged as `dispatcher` into the SDK-provided `RequestInit`; this disables keep-alive reuse. Configuring `keepAliveTimeout` does not mitigate this stall because the socket retirement timer is starved by the same idle event loop.

Applied to files:

  • CHANGELOG.md
🔇 Additional comments (9)
CHANGELOG.md (1)

28-30: LGTM!

docs/src/content/docs/sdk/pipes.md (1)

24-30: LGTM!

docs/src/content/docs/sdk/reference.md (1)

66-66: LGTM!

clients/ts/src/client.test.ts (5)

9-19: LGTM!


122-132: LGTM!


163-226: LGTM!


228-307: LGTM!


309-349: LGTM!

clients/ts/src/pipes.ts (1)

4-4: LGTM!

Comment thread clients/ts/src/pipes.ts Outdated
EricAndrechek and others added 2 commits August 12, 2026 17:20
From CodeRabbit on #456. The previous fix was half a fix.

Pick<RequestOptions, "signal"> only rejects a fresh object literal.
TypeScript's excess-property check does not apply to a variable, so:

  const opts: RequestOptions = { signal, limit: 10 };
  wh.pipe('x').fetch(opts);   // compiled, limit silently dropped

Verified with tsc: the literal form errored while the named form
passed. That leaves the original defect intact for anyone who builds
options once and shares them, which is the more realistic shape in real
code than an inline literal.

Replaced with an exported PipeRequestOptions declaring signal and
limit?: never. `never` rather than omitting the property, because
omission is exactly what leaves the named case open. Both forms are now
pinned by @ts-expect-error tests.

The tradeoff, measured rather than assumed: a RequestOptions value now
fails to assign even when it carries no limit at runtime, since the
declared type still permits one. That is correct -- the compiler cannot
know the value is limit-free, and the two option sets genuinely differ
-- but it is a real ergonomic cost for a caller sharing one options
object across query and pipe fetches, so it is worth stating plainly.

Note this reverses a judgement from an earlier review round, which read
Pick<>'s permissiveness as a compatibility feature for the wrapper
case. It is the same behaviour; the disagreement is whether admitting a
silently-ignored limit is forgiving or defective. Given the whole point
of #464 is that the type advertised something it ignored, defective.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
Both reviewers caught the same under-description. The docs and CHANGELOG
framed the new compile error as conditional on passing a limit. It is
not: limit?: never makes any value *declared* RequestOptions
unassignable, limit or no limit, because assignability is decided on the
declared type. Verified -- a limit-free `const o: RequestOptions =
{ signal }` fails with "Type 'number | undefined' is not assignable to
type 'undefined'", which reads like an SDK bug when the value plainly
has no limit.

That is the error consumers will meet, so it now leads with the way
out. Verified both halves of the advice rather than asserting them:
a PipeRequestOptions value is accepted by the pipe, table AND
query-builder .fetch(), so it works as a single shared type; and an
un-annotated `const o = { signal }` passes by inference.

Also documented a reassuring property I had not checked, from the code
reviewer: method parameters compare bivariantly, so PipeRef still
satisfies a structural `interface Fetchable { fetch(opts?:
RequestOptions) }`. Verified. Only direct argument passing breaks, which
shrinks the blast radius considerably and is worth saying, since a
reader otherwise has to guess.

Separately: RequestOptions had become a term of art in the docs without
being named anywhere it is defined -- queries.md documented the query
builder's options as an untitled table -- so the pipes page contrasted
against a name the linked page never used. queries.md now names the
type at its home and points at the pipes variant.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G2TCZsU46Qeu3efG1cjaXM
@EricAndrechek
EricAndrechek merged commit ea4fbf6 into main Aug 12, 2026
20 checks passed
@EricAndrechek
EricAndrechek deleted the sdk-fetch-override branch August 12, 2026 21:53
@github-project-automation github-project-automation Bot moved this from In review to Done in WaveHouse Task Board Aug 12, 2026
EricAndrechek added a commit that referenced this pull request Aug 18, 2026
Completes the previous commit, which fixed the escaping DOMException but
left the classification keyed off the error object — so the answer
depended on maxRetries. An implementation throwing something other than a
DOMException named AbortError (AbortSignal.timeout raises a TimeoutError,
node-fetch its own class) fell through to NETWORK_ERROR at the fetch
site, and was only reclassified as ABORTED if a retry remained for the
backoff sleep to notice the signal. Measured: same abort, NETWORK_ERROR
at maxRetries=0 and ABORTED at 2.

The outer check now also consults opts.signal?.aborted, which collapses
every case to ABORTED and makes the rule simpler to state: the SDK reads
the signal, not your error type. Pinned at both retry settings.

Three public docs asserted the opposite, all written or edited by this
branch — the FetchLike TSDoc that ships in dist/index.d.ts, the
supplying-your-own-fetch section, and the abort clause of the #456
CHANGELOG entry. All corrected.

Adds the ### Fixed CHANGELOG entry the abort fix needed: it is a
user-visible break of the SDK's never-throws contract on the REST path,
reachable with the default maxRetries and any AbortSignal, so it clears
the "any notable change" bar rather than riding along unmentioned.

Docs: anchors the closing SSE_AUTH_ERROR paragraph to the rule it
actually excepts — caller-side failures are terminal — instead of
claiming exception status without naming one.
EricAndrechek added a commit that referenced this pull request Aug 18, 2026
…cs say

The previous commit added `opts.signal?.aborted` but kept the
error-type check beside it, so the docs' new rule — "decided from the
signal, not the rejection's type" — was only half implemented.

The surviving clause was reachable in exactly one situation: an
`options.fetch` enforcing its own per-attempt deadline, which aborts an
internal controller and rejects with a genuine AbortError while the
caller's signal is untouched. That returned a terminal ABORTED where the
docs promise a retried NETWORK_ERROR — and the docs are right on the
merits. Nobody asked to cancel; a middleware timeout is transient. So the
clause is gone rather than documented, which also makes the sentence true
as written.

The inner catch around the backoff sleep now uses the same predicate, so
the two classifications fifteen lines apart cannot drift. No existing
test covered the dropped path — every abort test passes an
already-aborted signal — so a new one pins it: a middleware AbortError
with no caller signal retries and succeeds. Mutation-verified.

CHANGELOG: the #456 entry still claimed `FetchLike` "matches the standard
`fetch` signature", which this branch's own BREAKING entry three items
below negates — the URL parameter narrows to `string`, and the standard
first parameter is `RequestInfo | URL`. Third stale clause reconciled in
that paragraph; the other two were the REST-only carve-out and the abort
rule.
EricAndrechek added a commit that referenced this pull request Aug 18, 2026
## Summary

Moves `.stream()` / `.liveQuery()` off `EventSource` and onto `fetch`,
so the JWT travels as `Authorization: Bearer` instead of `?token=` in
the request URI — where every proxy, CDN, and load balancer in front of
WaveHouse is free to log it.

**Server untouched.** `bearerToken()` has always preferred the header,
and the CORS preflight has allow-listed `Authorization` +
`Last-Event-ID` since #215 — `internal/api/router_test.go:352` is an
existing test naming #203 as its reason. `?token=` stays accepted for
clients that genuinely can't set headers.

**Advances #203; does not close it.** Tasks 1 and 2 of 3 are done
(header auth, cURL flow). The third — retiring `?token=` server-side —
is #468, and closing that closes this.

**This does not make streams authenticated, and (regarding #203) does
not close it.** `/v1/stream` stays ungated: an expired or missing token
still resolves to `default_role` and gets a filtered `200`, never a
`401`. What changes is that the JWT stops appearing in request URIs, and
therefore in proxy, CDN, and load-balancer logs. Enforcing expiry on a
live stream is #239 and is deliberately not delivered here.

Part of #194.

## Decisions worth your eyes

**1. Redirects are refused only when the request carries a credential.**

Platforms strip `Authorization` on a cross-origin redirect
([whatwg/fetch#1544](whatwg/fetch#1544) — the
mitigation for the class behind
[CVE-2022-1650](GHSA-6h5x-7c5m-7cr7),
which was this exact bug in the `eventsource` package) while forwarding
other headers intact. A credentialed hop therefore either silently
downgrades the stream to `default_role` — this endpoint answers an
unauthenticated caller rather than rejecting them — or hands a
configured proxy secret to whatever the redirect names. Refusing costs
nothing, because following would never have produced an *authenticated*
stream anyway.

With no credential there's nothing to protect, so redirects are followed
and CDN canonicalization, geo/LB indirection, and http→https upgrades
all work. `options.fetch` overrides `redirect` if you need the
credentialed case followed regardless.

`manual` rather than `error`: `error` rejects with a bare `TypeError`
indistinguishable from a connection failure, which the reconnect loop
would retry forever against a redirect that will never stop happening.

**2. `FetchLike`'s URL parameter narrows to `string`.** The design note
on #269 said the type was narrow so hand-written `(url: string, init?)
=> Promise<Response>` middleware would assign. What shipped was `string
| URL | Request`, which — parameters being contravariant — rejects
exactly that. Purely additive for implementers; only code that
*imported* `FetchLike` and called through it with a `URL` breaks.

**3. The SDK gains its first runtime dependency:
`eventsource-parser@^3.1.0`.** MIT, **zero transitive deps**, npm
provenance-attested (SLSA v1), no install scripts, 61.7M downloads/week,
same maintainer as the canonical `eventsource`. Dual CJS/ESM, so it
composes with our `dist/index.cjs`. Measured cost in the CDN IIFE
bundle: 3402 B minified, 1430 B gzipped.

A caret rather than an exact pin because pinning in a *published
library* duplicates the package in any consumer tree already resolving a
3.x and freezes them out of patch/security releases until we cut one —
our lockfile still governs CI. The range stops below the ESM-only 4.0.0.

Why rent rather than hand-roll: the correct parser is ~440 lines, and
the parts that matter — reassembling a frame split across chunk
boundaries without quadratic recopying, disambiguating a trailing `\r`
at a chunk boundary between a bare-CR terminator and half a split
`\r\n`, and capping buffered input against a hostile stream — are what a
naive version gets wrong, and are unavoidable even though we control the
server, since HTTP/2 and intermediaries re-chunk freely. The buffer cap
is set explicitly to 16 MiB; the parser defaults to unbounded.

**This retires the "zero-dependency" claim** everywhere it appeared —
both READMEs, AGENTS.md invariant 14, four site pages, and an older
`Unreleased` CHANGELOG entry that would otherwise have shipped in the
same release notes as the entry introducing the dependency. The only
surviving mentions are in released CHANGELOG history, which is a record
rather than a claim.

## What this fixes beyond the headline

- **Expired-token silent downgrade.** `auth()` was called once and baked
into the URL; `EventSource` then reconnected forever with that token.
Once expired the stream stayed open serving a reduced view. This is why
#203 is a **prerequisite for #239**.
- **Blank `id:` clearing resumption.** The hub emits `id: ` for
passthrough payloads; per spec an empty `id` *clears* the last-event-id.
The transport retains the last **non-empty** id.
- **Real errors.** A rejection carries its actual status and message
instead of `EventSource`'s status-free `onerror` — which, in the old
transport, meant a gateway `401` surfaced as a silent `closed` with no
`error` callback at all. Note the limit: a browser stream going
cross-origin only sees the status if the rejection passes CORS and the
gateway answered the `Authorization` preflight; otherwise it degrades to
a retryable network error.
- **Non-SSE `200`s refused** (`SSE_BAD_CONTENT_TYPE`). An auth gateway's
login page would otherwise feed HTML to the parser, which per the SSE
grammar parses to *nothing* — leaving the stream live and permanently
silent.
- **No Node polyfill.** `polyfills.ts` and the `eventsource`
devDependency are deleted.
- **`options.fetch` / `headers` / `fetchOptions` reach streams**,
closing the carve-out documented in #456.

## Behavior changes to be aware of

- **A credentialed cross-origin browser stream now preflights on the
initial connect.** `EventSource` never preflighted at all (its request
isn't a `fetch()`, so Fetch's unsafe-request flag is never set). A proxy
that answers CORS itself must allow `Authorization` on `OPTIONS
/v1/stream` or the stream never opens. Documented in
`reverse-proxy.mdx`.
- **A proxy that strips `Authorization` on `/v1/stream`, or redirects it
while credentialed, now breaks.** Both previously "worked" by accident.
- **Resumption is at-least-once and time-bounded** — this was always
true; the docs now say so. The last event you saw is *certainly*
redelivered (the id is a `received_timestamp` and replay is inclusive),
the SDK does not dedupe live frames, and replay is capped by
`mq.gap_window_minutes` (15 default).

## Testing

`sse.test.ts` is rewritten against an injected fetch returning a
scripted `ReadableStream`. The old harness stubbed a global
`FakeEventSource` and could only assert on URL strings — framing,
reconnect, and resumption had **no coverage at all**. 204 tests now,
with 14 behavioral fixes mutation-verified: reverting each one makes a
test fail, and — after review caught a case where it didn't — fail on
the assertion that names it.

The e2e auth test was rewritten to be discriminating: `anon` is denied
`payload` on the events table, so dropping the `Authorization` header
flips the assertion. The previous version could not fail.

## Beyond the nominal scope

Two REST-path fixes ride along, both surfaced by review of the streaming
work and both in `clients/ts/src/http.ts`. Flagging them because an SSE
PR is not where you would look for them, and either can be split out on
request.

- **A cancelled request could throw instead of returning `ABORTED`.**
The network-error backoff is the one `sleep` inside `request()`'s catch,
so its rejection had no handler and escaped as a raw `DOMException`.
Nothing wraps `request()`, so it reached callers as an unhandled
rejection — and the `AbortController` example in our own reference
demonstrated a branch that could not be taken against an unreachable
server.
- **Abort is now classified from the signal, not the rejection's type.**
Keying off the error made the outcome depend on `maxRetries`:
`AbortSignal.timeout()` raises a `TimeoutError`, so it reported
`NETWORK_ERROR` at `maxRetries: 0` and `ABORTED` at `2`. It also
mis-handled middleware — an `options.fetch` enforcing its own
per-attempt deadline aborts an internal controller while the caller
never cancelled, which is transient and should be retried, not reported
as a terminal `ABORTED`.

The same rule then had to be applied to the stream transport, where the
old error-type check was worse: an `AbortError` from `auth()` or a
custom `fetch` ended the stream terminally and emitted **nothing**.

## Review

Fifteen pre-push rounds against both gating reviewers, who verify by
executing the code rather than reading it. Worth knowing what they
caught, since none of it was reachable by CI:

| | |
|---|---|
| Behavior bugs | `SSE_CONNECT_ERROR` never reaching a subscriber; a
closed stream stuck reporting `live`; a consumed-body guard that didn't
guard; an unhandled rejection that killed the host process; a stranded
reconnect timer |
| Regression vs `EventSource` | `close()` from inside a handler no
longer stopped delivery |
| Coverage holes | deleting the bearer half of the credentialed-redirect
rule left the suite green |
| False claims in docs | `EventSource` "preflighted on reconnect" (it
never preflights); "WaveHouse does not reject a stream" (it 400s on a
missing table); "the SDK isolates a throwing handler" (true only inside
the transport — several paths outside it are not, now enumerated in the
SDK reference and filed as #473) |

Two recurring shapes, both worth knowing before you read the diff.

**In the code: a guard or cleanup applied to N−1 of N call sites.** Six
defects shared it. `if (this._closed) return` now appears eight times in
`sse.ts`, several added a round apart. Assume any new early-exit path in
this transport is the one that got missed.

**In the prose: a true mechanism attached to a wider case set than it
holds for.** This accounts for essentially every documentation defect
found here, and it recurred for eight consecutive rounds — three times
*inside the sentence written to fix the previous instance*. The
reviewers' diagnosis is the useful part: none of these were factual
errors about the system, they were missing quantifiers. Nearly every
claim in this area is a function of a variable the docs cannot name —
the reader's token-provider latency, which origin, which credentials
mode — so the domain lives only in the author's head at the moment of
writing, and the next revision reaches for the deepest true mechanism
and silently re-attaches it to the whole case set. The empirical tell
was sharp: the rule-shaped sentences never needed correcting; the
value-shaped ones were corrected every round.

The Live Queries failure section is written to that conclusion — it
states rules and gives the reader a test, rather than reporting which
outcome is typical. Two amplifiers were also removed: sentences that
counted table rows (a ninth row would have silently falsified five of
them) and facts restated independently in four or five files. **If you
are reviewing prose here, the question that finds bugs is "for which
cases is this true?", not "is this true?"** Four axes account for
essentially every defect found on this branch, and a claim that is
silent about which side it means is the shape to distrust:

1. **Who rejected it** — WaveHouse (a `400` on the stream route; a
`404`/`405` off it) versus something in front. Never "the server said
401": `/v1/stream` is ungated.
2. **Where the caller runs** — server-side or same-origin (statuses
visible) versus browser cross-origin, where CORS can make any rejection,
including a rejected preflight, indistinguishable from a network drop.
3. **Whether the request carries a credential** — decides `redirect:
"manual"` versus `"follow"`, and the test is
`Authorization`-or-`headers`, so cookies are *not* credentials by it
(#478).
4. **Whether the failure is semantically transient** — the
4xx-is-terminal rule is a transport mechanism, not a claim about the
world (#469).

One structural note so it isn't rediscovered: `CHANGELOG.md` is
denylisted from the docs-prose gate (`scripts/docs-prose.sh:38`), so
that entry has never been read by the automated docs reviewer. It is
worth reading at docs scrutiny rather than skimming as boilerplate — a
false claim survived three rounds there for exactly that reason.

**The Go diff is one comment, so this looks deployment-free. It isn't.**
A credentialed cross-origin browser stream now preflights where
`EventSource` never did, so a proxy that answers CORS itself must allow
`Authorization` on `OPTIONS /v1/stream` or streams stop opening —
silently, in a retry loop, not with a visible error. That break is
documented in `reverse-proxy.mdx`; a reviewer reading only `internal/`
will conclude nothing operational changed.

## Follow-ups filed

Design work deferred out of this PR:

- **#465** — gzip on `/v1/stream` (per-frame flush; measure before
adopting)
- **#466** — normalize a schemeless `baseURL`, with a loopback exception
- **#467** — binary framing negotiated via `Accept`, sequenced behind
#465
- **#468** — retire `?token=` server-side; closes #203
- **#204** — commented with the POST-body analysis that unblocks
multiplexing

Defects found by review of this branch and left unfixed here, each
because the
fix lands outside the transport or carries a design question I didn't
want to
answer unilaterally in a PR about auth:

- **#469** — a stream `429` is terminal; should honor `Retry-After`
- **#471** — a rejected resumption preflight leaves a stream re-dialing
forever
- **#473** — a throwing subscriber silently stops delivery to the
others. Widened during review to cover every path outside the
transport's guard — `.subscribe()`'s initial unguarded `status` call
(worse via `liveQuery()`, which returns no handle at all), the fan-out
dropping the event for a concurrent `for await` and leaving it
un-terminated on a terminal close, a throwing `status` handler making
`.connected()` time out against a live stream, and `liveQuery()`'s
backfill flush discarding its buffer. The docs and CHANGELOG now
describe the real contract rather than the one I first wrote.
- **#476** — `http.ts`'s `sleep()` leaks an abort listener when the
timer wins
- **#484** — transport hardening against a non-conforming peer, raised
as "what I'd watch" in the final review: a parser-buffer overflow
re-dials at a flat rate forever because the backoff reset counts the
overflowing connection as healthy, and `FetchLike`'s contract never
states that `init.signal` must be honored. Neither is reachable with a
conforming peer; both are cheap now.
- **#477** — `StreamController` retains every event when nothing
iterates. The buffer's only drain is the async iterator's `next()`, so a
`.subscribe()`-only consumer — the pattern the docs lead with — holds
every event it has ever received, unbounded. Doubled on a filtered or
live stream, since both controller layers buffer.
- **#478** — a cookie-authenticated stream bypasses the redirect guard.
`credentialed` tests for a bearer token or configured `headers`; cookies
are neither, so the request follows a redirect and can arrive
unauthenticated.
- **#449** — pre-existing liveQuery dedup boundary, re-confirmed by
review
- **#445** — not touched here, but surfaced again while reviewing the
streaming docs and worth a person's eye: `wh.pipe(name).stream()` is
documented as working in three places while `pipes.ts` streams
`?table=<pipeName>`, so it subscribes to a topic nothing publishes and
silently yields nothing. The DLQ variant carries an inline caveat for
the same class of gap; the pipe one doesn't.

## Reviewer notes

The interesting file is `clients/ts/src/stream/sse.ts`. `controller.ts`
is untouched — the transport sits behind the same `StreamTransport`
interface — so the diff is scoped to the transport, its tests, and the
docs the change invalidated.

`clients/ts/src/stream/live-query.test.ts` is new and is the first test
coverage `LiveQuery` has had. It pins one thing worth knowing about: the
backfill, not the stream, spends the first `auth()` call, and that
ordering is emergent from four independent details rather than declared
anywhere. One added `await` on the REST path silently swaps the two
failure modes the docs describe, so the test exists to make that a red
build rather than a documentation drift.

Merged with `main` at `1064a4fe`, which brought #381 (per-subscriber SSE
row filtering) and #457. Both conflicted textually with this branch and
both were resolved keeping each side; #381 adds no new status code, so
this PR's claim that `/v1/stream` raises exactly one 4xx itself still
holds.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
jfwoods added a commit that referenced this pull request Aug 21, 2026
Three catch-up changes, all client-side. The server is untouched.

Routes: main merged every admin-gated endpoint under /v1/ops (#479) with
no aliases, so thirteen call sites were 404ing against a current server —
schema list/refresh, DLQ stats, raw SQL, policy get/put/validate, and
pipes CRUD, plus the codegen CLI's schema fetch. Rewrote them along with
the tests, the shared wire_cases.json fixture, and the Go SDK docs. The
fixture is replayed by both conformance runners, so the stale paths broke
the TypeScript half too; `make test-conformance-ts` is back to 45/45.

ClientOptions.Headers: the TypeScript SDK gained options.headers in #456
and Go had no equivalent. Headers now apply to every request the client
makes, REST and SSE alike — which is also how an operator sends the
server's non-JWT X-Operator-Key. The SDK's own headers are set afterwards
and win a collision; net/http canonicalizes names, so matching is
case-insensitive; the map is copied at construction so later mutation
can't reach into requests.

SSE robustness, mirroring main's fetch-based rewrite (#470). The Go SDK
already authenticated by header, so that part was never stale, but three
gaps were:

- A credentialed stream followed redirects. net/http drops Authorization
  on a cross-host hop while forwarding custom headers verbatim, so a
  redirect either downgraded the stream to default_role in silence or
  handed configured secrets to wherever it pointed. Now refused with a
  terminal SSE_REDIRECT. Uncredentialed streams still follow.
- A 200 with any content type was treated as an event stream, so an auth
  gateway's login page left the stream sitting in StatusLive delivering
  nothing. Now a terminal SSE_BAD_CONTENT_TYPE.
- Every failure collapsed into one retryable SSE_ERROR, and malformed
  frames came back as a bare fmt.Errorf, so errors.As and IsRetryable
  didn't work on them. Replaced with the taxonomy the TypeScript SDK
  uses — SSE_AUTH_ERROR, SSE_NETWORK_ERROR, SSE_CONNECT_ERROR,
  SSE_REDIRECT, SSE_BAD_CONTENT_TYPE, SSE_PARSE_ERROR, SSE_READ_ERROR —
  each with its own retryable flag, all delivered as *Error.

Also documents what main changed underneath the Go SDK without changing
its code: DateTime values arrive canonicalized to RFC 3339 UTC (#402),
SSE applies policy row-filters per subscriber and fails closed (#381,
#457), /v1/stream is ungated so WaveHouse never 401s a stream, and
/v1/ops/dlq/stats is absent (404) when the DLQ is disabled rather than
returning empty stats.

Tests: terminal-failure table (bad content type, missing content type,
credentialed redirect, non-HTTP scheme), redirect-followed-when-
uncredentialed, typed retryable parse errors, and header precedence and
copying on both transports.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs Documentation, site/, README area/sdk TypeScript SDK (clients/ts/) documentation Improvements or additions to documentation

Projects

Status: Done

1 participant