fix(sdk): keep a baseURL path prefix instead of discarding it - #448
Merged
Conversation
Pointing the SDK at a WaveHouse served under a path prefix — a BFF, an
app-server route, or a path-routed ingress — silently dropped the prefix
and sent every request to the origin root. Both transports resolved
absolute request paths against the base (`new URL('/v1/query', base)` in
http.ts, `new URL('/v1/stream', baseURL)` in stream/sse.ts), and per the
URL spec an absolute path replaces the base's path entirely. No error, a
client that looks correctly configured, and no workaround from outside
the SDK, since baseURL was the only path input.
Request paths are now joined onto the base by a single shared resolveURL
helper that both transports and the codegen CLI call — previously three
separate constructions that disagreed, since codegen's string concat
already handled prefixes. The helper normalizes the base to a directory
before resolving, so a bare last segment or a stray query/fragment can't
eat the prefix either; a root-hosted base resolves exactly as before.
The proxy in front must still strip the prefix before forwarding, since
WaveHouse has no configurable base path by design — the reverse-proxy
guide now covers that with nginx and Caddy snippets.
Closes #428.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe TypeScript SDK now preserves ChangesSDK URL resolution
Authentication query-token stripping
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK
participant resolveURL
participant WaveHouse
participant EventSource
Client->>SDK: Send query or schema request
SDK->>resolveURL: Resolve prefixed endpoint
resolveURL-->>SDK: Return request URL
SDK->>WaveHouse: Send HTTP request
Client->>SDK: Open stream
SDK->>resolveURL: Resolve /v1/stream
resolveURL-->>SDK: Return stream URL
SDK->>EventSource: Open SSE connection
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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. Comment |
github-actions
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d3b5a875-e64d-4e47-9136-472ccd7b0ecf
📒 Files selected for processing (12)
CHANGELOG.mdclients/ts/src/cli/codegen.tsclients/ts/src/client.test.tsclients/ts/src/http.test.tsclients/ts/src/http.tsclients/ts/src/stream/sse.test.tsclients/ts/src/stream/sse.tsclients/ts/src/types.tsclients/ts/src/url.test.tsclients/ts/src/url.tsdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/index.mdx
📜 Review details
⏰ Context from checks skipped due to timeout. (12)
- GitHub Check: E2E tests
- GitHub Check: Coverage
- GitHub Check: Docs build
- GitHub Check: Unit tests
- GitHub Check: Integration tests
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (go)
🧰 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/http.test.tsclients/ts/src/stream/sse.tsclients/ts/src/cli/codegen.tsclients/ts/src/client.test.tsclients/ts/src/url.test.tsclients/ts/src/types.tsclients/ts/src/stream/sse.test.tsclients/ts/src/http.tsclients/ts/src/url.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/reverse-proxy.mdxdocs/src/content/docs/sdk/index.mdx
🧠 Learnings (1)
📚 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
🪛 LanguageTool
docs/src/content/docs/reverse-proxy.mdx
[style] ~40-~40: Since ownership is already implied, this phrasing may be redundant.
Context: ... the proxy's job, and the server having its own opinion about it is a second place for ...
(PRP_OWN)
docs/src/content/docs/sdk/index.mdx
[style] ~331-~331: ‘in preference to’ might be wordy. Consider a shorter alternative.
Context: ...are present the server reads the header in preference to the query parameter, and strips the `?t...
(EN_WORDINESS_PREMIUM_IN_PREFERENCE_TO)
[style] ~336-~336: Consider using a more formal/concise alternative here.
Context: ...ix, for a WaveHouse reachable somewhere other than the root of an origin — behind a BFF, a...
(OTHER_THAN)
🔇 Additional comments (11)
clients/ts/src/url.ts (1)
1-28: LGTM!clients/ts/src/url.test.ts (1)
1-78: LGTM!clients/ts/src/http.ts (1)
3-3: LGTM!Also applies to: 33-33
clients/ts/src/http.test.ts (1)
183-206: LGTM!clients/ts/src/client.test.ts (1)
30-42: LGTM!clients/ts/src/stream/sse.ts (1)
2-2: LGTM!Also applies to: 57-57
clients/ts/src/stream/sse.test.ts (1)
1-98: LGTM!clients/ts/src/cli/codegen.ts (1)
14-15: LGTM!Also applies to: 168-168
docs/src/content/docs/reverse-proxy.mdx (1)
38-88: LGTM!docs/src/content/docs/sdk/index.mdx (1)
326-329: LGTM!Also applies to: 334-346
CHANGELOG.md (1)
53-53: LGTM!
github-project-automation
Bot
moved this from Backlog
to In review
in WaveHouse Task Board
There was a problem hiding this comment.
Pull request overview
Fixes an SDK URL-resolution bug where configuring baseURL with a path prefix (e.g. behind a BFF or path-routed ingress) would silently drop that prefix, causing requests to be sent to the origin root instead of under the intended prefix.
Changes:
- Introduces a shared
resolveURLhelper to join SDK request paths ontobaseURLwithout discarding any base path prefix. - Updates REST, SSE streaming, and the codegen CLI to use the shared resolver.
- Adds targeted tests and documentation updates (including reverse-proxy guidance) and records the fix in the changelog.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/src/content/docs/sdk/index.mdx | Documents baseURL path-prefix support and clarifies absolute-URL requirement. |
| docs/src/content/docs/reverse-proxy.mdx | Adds guidance for serving WaveHouse behind a path prefix (proxy must strip prefix). |
| clients/ts/src/url.ts | Adds resolveURL helper that preserves base path prefixes when building request URLs. |
| clients/ts/src/url.test.ts | Unit tests covering URL resolution edge cases (prefixes, slashes, params, base query/hash). |
| clients/ts/src/types.ts | Updates ClientConfig.baseURL JSDoc to mention path-prefix support. |
| clients/ts/src/stream/sse.ts | Switches SSE transport URL construction to resolveURL to preserve prefixes. |
| clients/ts/src/stream/sse.test.ts | Adds tests asserting SSE connections keep the prefix and query params (since/token). |
| clients/ts/src/http.ts | Switches REST transport URL construction to resolveURL and removes old helper. |
| clients/ts/src/http.test.ts | Adds tests ensuring REST requests keep the base path prefix (with/without appended params). |
| clients/ts/src/client.test.ts | Adds tests ensuring createClient preserves baseURL prefixes and requests are sent under them. |
| clients/ts/src/cli/codegen.ts | Switches schema-fetch URL construction to resolveURL for consistent prefix handling. |
| CHANGELOG.md | Notes the SDK baseURL path-prefix fix and references affected areas/tests/docs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
📚 Docs preview is live → https://f5b3941d-wavehouse-docs.wave-rf.workers.dev
|
Contributor
Both AI reviewers on #448 flagged that ClientConfig.baseURL's JSDoc describes path-prefix support without saying the value must be absolute, even though a relative base throws out of the first request rather than returning a Result. The docs page said so; the JSDoc did not. CodeRabbit separately caught that the pre-existing "strips the ?token= value ... so it can't leak into logs" note overclaims: the strip only keeps the token out of WaveHouse's own logs. On a streaming connection the token rides in the request URI, so every proxy, CDN, and load balancer in front records it in access logs unless query strings are redacted there. Scoped the claim and pointed at the operator-side fix — worth correcting here since this PR is what adds the prefix/proxy guidance that makes intermediaries more likely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
# Conflicts: # CHANGELOG.md
The docs review caught that the previous commit corrected the "stripped so it can't leak into logs" overclaim on the SDK page but left it standing in api.md and reverse-proxy.mdx — the two pages an operator actually reads while configuring the intermediary that does the logging. On reverse-proxy.mdx it sat three lines above the bullet telling you not to log the full URL, which contradicted itself. It also caught that the corrected sentence was still wrong about *when* the strip happens: bearerToken (internal/auth/auth.go) returns from the Authorization-header branch before touching r.URL, so with both present the query parameter is left intact. The strip only runs on the query-only path. Fixed the wording and the Go doc comment that seeded the claim in the first place. Also carves the newly-documented malformed-baseURL throw out of the "SDK never throws" absolutes (sdk/reference.md, sdk/index.mdx, clients/ts/README.md), which the same review flagged as newly contradictory, and expands BFF on first use per the site's convention. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
The docs review caught that the "never throws, except a malformed baseURL fails fast with a TypeError" carve-out is only half true. REST rejects, because resolveURL runs outside request()'s retry try — but the stream transport catches it (stream/sse.ts) and hands the subscriber a soft SSE_CONNECT_ERROR. The subscriber's `error` callback is optional, so a stream with a bad baseURL can fail silently: the same quiet misconfiguration this PR exists to remove, in the one paragraph that promises both transports behave alike. Scoped the claim per transport in sdk/index.mdx, sdk/reference.md, and clients/ts/README.md, and reworded the last echo of the old token/logs overclaim in an auth_test assertion message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
The carve-out I added claimed a malformed baseURL was *the* exception to "the SDK never throws". It isn't: client.ts also throws synchronously from _createStream when EventSource is undefined — which the same doc set already documents under Runtime support — and there's the client.sql migration guard besides. A Node reader trusting the Error Handling section would take an uncaught throw from a line the docs elsewhere say will throw. Reframed as "throws on caller and environment errors" with the two cases named, in sdk/reference.md, sdk/index.mdx, and the SDK README. Also drops the literal "TypeError: Invalid URL" message, which is Node's exact wording — Chrome, Firefox, and Safari all word it differently, and the section it sits in is browser-facing — and adds SSE_CONNECT_ERROR and SSE_ERROR to the error-code table, since the prose named a code the table never listed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
Both reviewers landed on the same site independently: http.ts awaits ctx.auth() outside any try, and no caller wraps request(), so a token provider that rejects propagates straight out of `await wh.from(...).fetch()`. That is the most likely of the throwing cases in practice — the Quick Start's own `auth: async () => getAccessToken()` is exactly the shape that hits it — and the enumeration a reader uses to decide whether they need try/catch was silent about it. Also adds the missing setup for the README's new caveat: it warned about a non-absolute baseURL without ever mentioning baseURL could carry a path, so the npm-page reader met the warning with no context. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
The SSE_CONNECT_ERROR / SSE_ERROR rows added last round were marked
retryable to match the payload, but the table defines that column as
"whether SDK would retry" and every other Yes row describes the SDK's
own retry ("retried per maxRetries", "auto-retries with Retry-After").
The SDK re-dials neither: connect() calls _doConnect().catch(...) once
and StreamController has no reconnect logic. Worst for the example the
row itself gives — a non-absolute baseURL fails identically forever.
Spelled out the real split: once the connection is open the native
EventSource reconnects on its own (SSE_ERROR), but a failure before it
is constructed (non-absolute baseURL, rejecting auth) is terminal
(SSE_CONNECT_ERROR) and needs a new stream.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
Both reviewers landed on this: the sentence attributed SSE_ERROR to the
post-open reconnect, but sse.ts's onerror maps readyState CONNECTING /
CLOSED / OPEN to the status callback, leaving SSE_ERROR as a trailing
else that a spec-compliant EventSource never reaches. So the reader most
at risk was the one wiring a "connection lost" handler to error: after
open, a drop — or a fatal server rejection — fires no error at all and
the signal arrives on status.
Reworded to say that, and untangled the README's pronouns ("which ...
its" had the callback as nearest antecedent).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
clients/ts/src/url.ts:21
resolveURLnormalizes missing trailing slashes, but it doesn’t collapse extra trailing slashes inbaseURL. That’s a behavioral regression for the codegen CLI, which previously didurl.replace(/\/+$/, "")and would turnhttp://localhost:8080////intohttp://localhost:8080/v1/schema. With the current helper, that input can produce URLs like////v1/schema(or similar), which many proxies/servers won’t route correctly.
const root = new URL(base);
// Normalize the base to a directory: a bare last segment (or a query/fragment
// shadowing one) makes relative resolution *replace* the prefix, not extend it.
root.search = "";
root.hash = "";
if (!root.pathname.endsWith("/")) root.pathname += "/";
const url = new URL(path.replace(/^\/+/, ""), root);
clients/ts/src/types.ts:71
resolveURLexplicitly clearssearchandhashonbaseURL, so any query string / fragment a caller includes will be silently ignored. Since this is now a deliberate behavior (and differs from what a user might assume a “URL” includes), it’s worth documenting here to prevent confusing misconfigurations (e.g. trying to pass proxy routing hints via?…).
/**
* Base URL of the WaveHouse server (e.g. "http://localhost:8080"). May include
* a path prefix ("https://app.example.com/api/warehouse") for a WaveHouse
* behind a backend-for-frontend (BFF), app-server route, or path-routed
* ingress; request paths are appended to it. The proxy in front must strip
* the prefix before forwarding.
*
* Must be **absolute** — scheme and host included. A relative value such as
* "/api/warehouse" throws a `TypeError` on the first request rather than
* returning a `Result`; use `` `${location.origin}/api/warehouse` ``.
*/
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da4df521-a62e-42d2-ad0a-6b0e1cbda7e8
📒 Files selected for processing (9)
CHANGELOG.mdclients/ts/README.mdclients/ts/src/types.tsdocs/src/content/docs/api.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/reference.mdinternal/auth/auth.gointernal/auth/auth_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Coverage
- GitHub Check: Integration tests
- GitHub Check: Docs build
- GitHub Check: E2E tests
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (4)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Write tests in table-driven form witht.Run(tt.name, ...)for multiple cases.
Use shared mocks frominternal/testutil/instead of ad-hoc mocks in tests.
Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (testutil.MakeJWT,testutil.MakeExpiredJWT,NewTestSchemaRegistry,policy.NewMemoryStore,pipes.NewMemoryStore,AssertJSONResponse,AssertJSONContains) where applicable.
Every new function should have corresponding test cases, and new code should aim for 80%+ coverage.
Files:
internal/auth/auth_test.go
clients/ts/README.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep the TypeScript SDK README in sync with SDK-facing changes and public client behavior.
Files:
clients/ts/README.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/types.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/reference.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/api.mddocs/src/content/docs/sdk/index.mdx
🧠 Learnings (4)
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/auth/auth_test.go
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/auth/auth_test.gointernal/auth/auth.go
📚 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:
clients/ts/README.mddocs/src/content/docs/sdk/reference.mdCHANGELOG.mddocs/src/content/docs/api.md
📚 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/types.ts
🪛 LanguageTool
docs/src/content/docs/sdk/reference.md
[style] ~40-~40: A comma is missing here.
Context: ...RROR| Yes | Stream failed to connect (e.g. a non-absolutebaseURL) | | 0 | SSE_...
(EG_NO_COMMA)
docs/src/content/docs/sdk/index.mdx
[style] ~338-~338: Consider using a more formal/concise alternative here.
Context: ...ix, for a WaveHouse reachable somewhere other than the root of an origin — behind a backen...
(OTHER_THAN)
🔇 Additional comments (10)
CHANGELOG.md (1)
54-54: LGTM!docs/src/content/docs/reverse-proxy.mdx (2)
38-88: LGTM!
137-137: LGTM!docs/src/content/docs/sdk/index.mdx (2)
326-349: LGTM!
371-371: LGTM!docs/src/content/docs/sdk/reference.md (2)
28-28: LGTM!
40-43: LGTM!docs/src/content/docs/api.md (1)
26-26: LGTM!internal/auth/auth_test.go (1)
182-182: LGTM!clients/ts/README.md (1)
58-59: LGTM!
CodeRabbit's Major finding on #448. bearerToken returned from the Authorization: Bearer branch before the strip, so a request presenting both credentials left the unused JWT sitting in r.URL for the rest of its life. Not an active leak today — WaveHouse's own request logging records only r.URL.Path, and the OTel HTTP instrumentation records no query attribute — so this is defense in depth, not a fix for an observed exposure. But it was an inconsistency in an invariant the code already asserted on the query-only path (auth_test.go: "must be stripped so it stays out of our own logs"), and any later handler or logging change would have turned it into a real one. Safe to close: nothing else in the codebase reads the token query param, and no test pinned the old behavior. The strip now runs once before either credential path returns. Header precedence is unchanged; unrelated query params survive; both cases are pinned by tests. The docs drop the "the header path leaves the query parameter untouched" caveat that described the old behavior — which my own earlier commit in this PR had just written down as intended. Also addresses the other two threads: the ClientConfig.baseURL JSDoc now splits the relative-baseURL failure per transport (REST rejects, streams report SSE_CONNECT_ERROR) rather than implying both behave alike, and the SDK README stops calling Result<T> a "tuple" and stops claiming "all operations" return one — .stream()/.liveQuery() return controllers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
The code reviewer caught that the previous commit left the same bug one frame up: an operator-key match returns from Middleware before bearerToken is ever called, so a break-glass request carrying ?token=<jwt> kept the unused JWT in r.URL — exempting the single most privileged credential from the strip, and making the "whichever credential wins" invariant I had just written down false. Resolving bearerToken above the operator branch closes it. The operator branch ignores the return value, so this is behavior-preserving for both JWT paths; the call now sits first with a comment saying why, so it doesn't drift back down. TestMiddleware_OperatorKey_StripsQueryToken pins it — verified it fails (raw JWT still in the URL) without the hoist. Also drops a false claim from the CHANGELOG entry: I wrote that the SDK presents both credentials on a stream reconnect, but it never does — EventSource takes no init dict, which is the whole reason ?token= exists. Docs: the SDK page no longer refers to `r.URL`, a Go symbol with no referent on a TypeScript page, and no longer opens two consecutive sentences with "Either way"; sdk/queries.md drops the flat "nothing throws", which contradicted the qualified wording this PR introduced on the pages next to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
Two review slips from the last commit. The new operator-strip test was inserted between TestMiddleware_OperatorKey_FailedAttemptLogged's doc comment and its function, so a paragraph about the WARN and the operator-key failure counter ended up heading a test that asserts neither, while FailedAttemptLogged was left with none. Moved the test up beside the other runOp operator cases, restoring both pairings. And Result<T>'s own JSDoc still said a flat "Never throws" — the canonical statement, the one that shows up in an editor tooltip, while this PR had qualified the same claim in five prose locations. Now matches them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012iXE6g6Bz7gJnfZFGCesrV
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
clients/ts/src/url.ts:21
resolveURLnormalizesbaseURLto end in a/, but it doesn’t collapse multiple trailing slashes. This regresses the codegen CLI’s previousurl.replace(/\/+$|/, "")behavior and can generate request paths like///v1/schema, which may 404 behind strict routers/proxies. Consider normalizing the base pathname to exactly one trailing slash before resolving.
root.search = "";
root.hash = "";
if (!root.pathname.endsWith("/")) root.pathname += "/";
const url = new URL(path.replace(/^\/+/, ""), root);
internal/auth/auth.go:127
- This comment says
tokenStr := bearerToken(r)is resolved “purely for its side effect”, buttokenStris also used later for JWT parsing. Tweaking the wording would avoid confusion for future readers about why the value is computed here (strip?tokenearly) and still used below (Bearer path).
// Resolved before the operator branch purely for its side effect: it
// strips ?token from r.URL, and an operator-key match returns without
// ever reaching the Bearer path. Leaving it below would exempt the
// most privileged credential from the strip — keep this first.
tokenStr := bearerToken(r)
EricAndrechek
marked this pull request as ready for review
August 11, 2026 18:51
github-project-automation
Bot
moved this from In review
to Done
in WaveHouse Task Board
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 79e2c266-7fb2-4dae-a61f-3fb02c2e6c12
📒 Files selected for processing (9)
CHANGELOG.mdclients/ts/README.mdclients/ts/src/types.tsdocs/src/content/docs/api.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/queries.mdinternal/auth/auth.gointernal/auth/auth_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Docs build
- GitHub Check: Coverage
- GitHub Check: E2E tests
- GitHub Check: Integration tests
🧰 Additional context used
📓 Path-based instructions (4)
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/queries.mddocs/src/content/docs/api.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/index.mdx
clients/ts/README.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep the TypeScript SDK README in sync with SDK-facing changes and public client behavior.
Files:
clients/ts/README.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/types.ts
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Write tests in table-driven form witht.Run(tt.name, ...)for multiple cases.
Use shared mocks frominternal/testutil/instead of ad-hoc mocks in tests.
Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (testutil.MakeJWT,testutil.MakeExpiredJWT,NewTestSchemaRegistry,policy.NewMemoryStore,pipes.NewMemoryStore,AssertJSONResponse,AssertJSONContains) where applicable.
Every new function should have corresponding test cases, and new code should aim for 80%+ coverage.
Files:
internal/auth/auth_test.go
🧠 Learnings (4)
📚 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/queries.mddocs/src/content/docs/api.mdCHANGELOG.mdclients/ts/README.md
📚 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/types.ts
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/auth/auth.gointernal/auth/auth_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/auth/auth_test.go
🔇 Additional comments (5)
clients/ts/src/types.ts (1)
70-74: LGTM!docs/src/content/docs/reverse-proxy.mdx (1)
137-137: LGTM!docs/src/content/docs/sdk/index.mdx (1)
331-333: LGTM!Also applies to: 346-346
docs/src/content/docs/api.md (1)
26-26: LGTM!docs/src/content/docs/sdk/queries.md (1)
9-11: LGTM!
Comment on lines
+130
to
+132
| Async request methods return a `Result<T>` object — destructure it as `{ data, error }` — and never throw for anything the server returns. `.stream()` and `.liveQuery()` return controllers instead, reporting failures through the subscriber's `error` callback. | ||
|
|
||
| The SDK does throw on caller and environment errors: a non-absolute `baseURL` (REST calls reject with a `TypeError`; streams report `SSE_CONNECT_ERROR`, and despite that error's `retryable: true` the SDK never re-dials the stream itself), `.stream()` / `.liveQuery()` in a runtime with no `EventSource`, and an `auth` callback that rejects — a token-refresh failure propagates out of the REST call. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate REST exceptions from stream error delivery.
Line [132] says that the SDK throws for the listed errors, but the stream case reports SSE_CONNECT_ERROR through a callback instead of throwing. The callback is optional, so the failure can be silent when no callback is supplied. Document the REST and stream behaviors in separate sentences.
As per coding guidelines, clients/ts/README.md must stay in sync with SDK-facing changes. Based on learnings, StreamSubscriber.error is optional and a failed stream connection can be silent when that callback is absent.
Proposed wording
- The SDK does throw on caller and environment errors: a non-absolute `baseURL` (REST calls reject with a `TypeError`; streams report `SSE_CONNECT_ERROR`, and despite that error's `retryable: true` the SDK never re-dials the stream itself), `.stream()` / `.liveQuery()` in a runtime with no `EventSource`, and an `auth` callback that rejects — a token-refresh failure propagates out of the REST call.
+ REST calls throw on caller and environment errors such as a non-absolute `baseURL` or a rejecting `auth` callback. `.stream()` / `.liveQuery()` throw when `EventSource` is unavailable. A stream with a non-absolute `baseURL` reports `SSE_CONNECT_ERROR` through the optional subscriber `error` callback and does not retry automatically.Sources: Coding guidelines, Learnings
Comment on lines
+13
to
+15
| * Discriminated union for all async SDK operations. Never throws for anything | ||
| * the server returns — caller and environment errors (a non-absolute `baseURL`, | ||
| * a missing `EventSource`, a rejecting `auth` callback) do throw. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the Result<T> JSDoc to request methods.
The JSDoc describes a discriminated union for “all async SDK operations.” .stream() and .liveQuery() return controllers, not Result<T>. Replace “all async SDK operations” with “async request operations” to keep this contract accurate.
Proposed wording
- * Discriminated union for all async SDK operations. Never throws for anything
+ * Discriminated union for async request operations. Never throws for anything
Comment on lines
+272
to
276
| if params := r.URL.Query(); params.Get("token") != "" { | ||
| queryToken = params.Get("token") | ||
| params.Del("token") | ||
| r.URL.RawQuery = params.Encode() | ||
| return q | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Go's documented duplicate-query behavior.
go doc net/url.Values.Get
# Inspect the stripping implementation and its focused tests.
rg -n -C 5 'params\.Get\("token"\)|params\.Del\("token"\)|RawQuery|tokenInURL' \
internal/auth/auth.go internal/auth/auth_test.goRepository: Wave-RF/WaveHouse
Length of output: 7282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '230,335p' internal/auth/auth.go
sed -n '1,90p' internal/auth/auth.go
rg -n -C 6 'operator|bearerToken|queryToken|URL\.Query|RawQuery|Authorization' internal/auth/auth.go internal/auth/auth_test.goRepository: Wave-RF/WaveHouse
Length of output: 47374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
go doc net/url.Values.Del
cat >/tmp/verify_query_values.go <<'EOF'
package main
import (
"fmt"
"net/url"
)
func main() {
params, err := url.ParseQuery("token=&token=JWT&table=clicks")
if err != nil {
panic(err)
}
fmt.Printf("values=%q get=%q\n", params["token"], params.Get("token"))
params.Del("token")
fmt.Printf("after_del=%q encoded=%q\n", params["token"], params.Encode())
}
EOF
go run /tmp/verify_query_values.go
rm -f /tmp/verify_query_values.goRepository: Wave-RF/WaveHouse
Length of output: 330
Strip all repeated token query values when the key exists.
url.Values.Get returns only the first value. Therefore, ?token=&token=<JWT> skips the strip branch and leaves the second JWT in r.URL.RawQuery. Detect the key with _, ok := params["token"], preserve first-value credential selection, and call params.Del("token") when ok is true. Add a regression test that checks all values or RawQuery, because Query().Get("token") alone cannot detect this case. Update the unconditional statement in CHANGELOG.md after the fix.
📍 Affects 3 files
internal/auth/auth.go#L272-L276(this comment)internal/auth/auth_test.go#L202-L213CHANGELOG.md#L38-L38
github-project-automation
Bot
moved this from Done
to In review
in WaveHouse Task Board
jfwoods
added a commit
that referenced
this pull request
Aug 11, 2026
The merge documented path-prefix support as a promise on the Go SDK's Config row and README, but nothing tested it — every test server in clients/go was root-hosted. The TS side shipped url.test.ts and friends pinning exactly this after #428. Adds a guard per transport, since the SSE URL is built in stream.go independently of buildURL: a mux serving only the prefixed path, so a dropped prefix 404s (REST) or never arrives (SSE). Both verified failing against a deliberately broken buildURL and stream URL before being kept. Qualifies the 401 row on both reference pages: "denied with 403" was too absolute. A missing token is evaluated as default_role, which may well succeed — the point is that it never yields 401. Also trims the changelog's claim that this branch added SSE_ERROR/SSE_CONNECT_ERROR to the TS error table; post-merge those rows come from #448 in the same Unreleased section.
This was referenced Aug 12, 2026
Open
Open
This was referenced Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #428.
The bug
Pointing the SDK at a WaveHouse served under a path prefix silently dropped the prefix:
Both transports resolved absolute request paths against the base —
new URL('/v1/query', base)inhttp.ts,new URL('/v1/stream', baseURL)instream/sse.ts— and per the URL spec an absolute path replaces the base's path entirely. No error, a client that looks correctly configured, and every request quietly going somewhere else. There was no workaround from outside the SDK:baseURLwas the only path input and it couldn't survive.That pushes an ordinary deployment shape — one origin fronting several services, a BFF adding auth ahead of the data API, a Next.js/Remix/Rails route proxying through, a path-routed ingress — off the SDK and onto hand-rolled
fetch, losing the query builder, typed errors, retry/backoff, and the streaming controller along with it.The fix
Took the issue's first option (relative resolution) over adding a
basePathconfig field — it's what the issue title asks for and it's zero new API surface.Request paths are now joined onto the base by one shared
resolveURLhelper (clients/ts/src/url.ts) that both transports and the codegen CLI call. Previously there were three separate URL constructions, and they disagreed: codegen's string concat already handled prefixes while the two transports didn't.The helper normalizes the base to a directory before resolving, so a bare last segment — or a stray query/fragment shadowing one — can't eat the prefix either. A root-hosted base (
http://localhost:8080, the overwhelmingly common case) resolves byte-identically to before; I diffed old vs new resolution across the edge cases to confirm.Tests
url.test.ts(new) — the helper: root and prefixed bases, multi-segment prefixes, trailing-slash equivalence, params merging with a path's existing query string, base query/fragment, non-default ports.stream/sse.test.ts(new) — the SSE transport against a stubbedEventSource, pinning the prefix alongside thesinceandtokenparams. This is the one the issue warns is easy to miss when fixinghttp.tsalone.http.test.ts,client.test.ts— prefix preserved through the REST transport and end-to-end fromcreateClient.Docs
ClientConfig.baseURLdocuments the prefix in both the table and its JSDoc. It also now states thatbaseURLmust be absolute — a same-origin relative/api/warehousethrows rather than returning aResult, which is pre-existing behavior that was never written down and bites hardest in exactly the BFF scenario this section is about.ingress-nginxdoes not strip by default and needsrewrite-target.Also in this PR: a
?token=strip fix ininternal/authFlagged by CodeRabbit while reviewing the docs change above, so it is worth explaining why a
fix(sdk):PR carries a server-side auth diff.bearerTokenreturned from theAuthorization: Bearerbranch before stripping?token=from the URL, so a request presenting both credentials left the unused JWT sitting inr.URLfor the rest of its life. The operator-key path had the same shape one frame up, returning beforebearerTokenran at all — exempting the single most privileged credential. The strip is now resolved once, ahead of every credential branch.This is defense in depth, not a fix for an observed exposure: WaveHouse's own logging records only
r.URL.Path, and the OTel HTTP instrumentation emits no query attribute, so nothing leaks today. It was worth closing because it was an inconsistency in an invariant the code already asserted on the query-only path, and because my own earlier commit here had just written the gap down as intended behavior — documenting it seemed strictly worse than fixing it.Header precedence is unchanged, unrelated query params survive, and four credential combinations are pinned by tests (query-only, header+query, non-Bearer header+query, operator key+query). I verified the operator-path test genuinely catches the bug: with the fix reverted it fails with the raw JWT still in the URL.
If you would rather this land separately, it is self-contained in
internal/auth/plus the three doc sentences that describe the behavior — say the word and I will split it out.Note for the reviewer
#269(custom headers /fetchoverride) is the sibling gap in the same config surface and is deliberately not in scope here.