Conversation
Complete Go SDK at clients/go/ with zero third-party runtime dependencies. Covers ingest, query (structured + SQL), streaming (SSE with reconnect), live queries, pipes, DLQ, schema, policy, and health endpoints. - Immutable QueryBuilder with generics (FetchTyped[Row], SQL[Row]) - SSE StreamController with reconnect/backoff and client-side filtering - LiveQuery drain-then-switch backfill (stream-first, dedup, go live) - Reflect-based Insert handling any []T, not just []map[string]any - wavehouse-codegen CLI for generating typed row structs from /v1/schema - 42 unit tests + 44 cross-language wire-format conformance cases - E2E test scaffolding (build tag e2e, 9 tests against live server) - Cross-language conformance runner for TS SDK (tests/conformance/) - Makefile targets: verify-go-sdk, test-go-sdk, test-go-sdk-e2e, lint-go-sdk
Six Starlight pages covering installation, queries, streaming, pipes, admin operations, and API reference. Sidebar nav group added. Cross-link from SDK index page. Architecture page updated with Go SDK. Root README updated with Go SDK install.
- Add test-go-sdk to CI unit job - AGENTS.md: Go SDK file structure + feature parity table - lint-go-sdk already wired via verify-parallel in Makefile
- context.Context as first param in doRequest (revive) - Checked all json Encode/Decode/Unmarshal returns (errcheck) - Wrapped defer Body.Close with error discard (errcheck) - if-else chain to switch in buildAST (gocritic) - Tagged switch on r.Method in test (staticcheck) - Renamed built-in shadow cap to capt (revive) - Removed wasted msg assignment (wastedassign) - WriteFile 0o644 to 0o600 (gosec) - nolint:gosec for cancel called in Close (gosec)
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds an official Go SDK, shared wire-format conformance coverage for Go and TypeScript, Go SDK CI and coverage handling, new SDK documentation structure, and repository updates that treat TypeScript and Go as parallel client SDKs. ChangesGo SDK
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to This PR adds a credential-bearing Go SDK with administration APIs, retries, streaming recovery, and code generation, but the current version still has concrete security and correctness risks that could leak credentials, generate unsafe or uncompilable code, execute the wrong pipe stream, duplicate writes, or lose events. It is not ready to merge until the high-impact issues are fixed or explicitly accepted by the appropriate owners. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Dev
participant Makefile
participant GoSDK as Go SDK tests
participant TSConf as TS conformance runner
participant Cov as scripts/cov
Dev->>Makefile: make test-sdk
Makefile->>GoSDK: run unit + conformance
Makefile->>TSConf: run TypeScript conformance
GoSDK-->>Cov: write go-sdk coverage
TSConf-->>Makefile: report fixture results
Makefile->>Cov: render and gate standalone go-sdk coverage
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 54.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 192 functions across 29 files. (37 skipped: 37 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
|
📚 Docs preview is live → https://4fd2c7e2-wavehouse-docs.wave-rf.workers.dev |
There was a problem hiding this comment.
Pull request overview
Adds a new, officially supported Go client SDK under clients/go/ as a nested Go module, aiming for wire-format/API parity with the existing TypeScript SDK. The PR also wires Go SDK lint/test into the repo Makefile + CI, and adds Go/TS conformance runners plus docs updates to publish Go SDK usage on the docs site.
Changes:
- Introduces the Go SDK module (
clients/go/) including query builder, ingest helpers, namespaces, streaming/live-query primitives, and a schema-based codegen CLI. - Adds cross-language wire-format conformance harnesses (Go test embedding shared fixtures + a Node runner for TS).
- Wires Go SDK lint/vet/tests into
make verify/make ciand GitHub Actions CI; updates docs/README/CHANGELOG/AGENTS to reflect the new canonical SDK.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/conformance/conformance_ts.mjs | Node-based TS conformance runner against shared wire fixture. |
| README.md | Updates project summary to mention both TypeScript + Go SDKs. |
| Makefile | Adds lint-go-sdk, verify-go-sdk, test-go-sdk, test-go-sdk-e2e; hooks into verify/ci. |
| docs/src/content/docs/sdk/index.mdx | Adds Go SDK callouts + navigation card. |
| docs/src/content/docs/sdk/go/index.md | New Go SDK landing page (install/quickstart/error model). |
| docs/src/content/docs/sdk/go/queries.md | Go SDK query + ingest documentation (builder, pagination, raw SQL). |
| docs/src/content/docs/sdk/go/streaming.md | Go SDK streaming + live query documentation. |
| docs/src/content/docs/sdk/go/pipes.md | Go SDK pipes usage + admin CRUD docs. |
| docs/src/content/docs/sdk/go/admin.md | Go SDK admin namespaces (schema/policy/dlq/sys) docs. |
| docs/src/content/docs/sdk/go/reference.md | Go SDK reference (API tree, error codes, codegen CLI). |
| docs/src/content/docs/architecture.md | Notes that both TS + Go SDKs exist. |
| docs/src/config/sidebar.ts | Adds a separate Go SDK sidebar tree. |
| clients/go/go.mod | Introduces nested Go module for the SDK. |
| clients/go/wavehouse.go | Core client wiring (config/options/namespaces) + helpers. |
| clients/go/http.go | HTTP transport: auth injection, retry/backoff, error parsing. |
| clients/go/errors.go | SDK error type + helpers for retryability and HTTP error parsing. |
| clients/go/types.go | Shared wire types (structured query AST, policy/pipes/schema, streaming, paging). |
| clients/go/query_builder.go | Immutable query builder + typed/untyped fetch, pagination cursoring, stream wrapper. |
| clients/go/table.go | Table ref: fetch/select/insert (JSON vs NDJSON batch) + schema + stream. |
| clients/go/sys.go | /v1/health namespace. |
| clients/go/schema.go | Admin schema namespace (list/refresh). |
| clients/go/policy.go | Admin policy namespace (get/set/validate). |
| clients/go/pipes.go | Pipe execution + pipes admin CRUD namespace. |
| clients/go/dlq.go | DLQ stats namespace + placeholder stream entrypoint. |
| clients/go/live_query.go | Live query orchestration (buffer, backfill, dedup, go-live). |
| clients/go/README.md | Standalone Go SDK README for module users. |
| clients/go/cmd/wavehouse-codegen/main.go | Codegen CLI to generate Go structs from /v1/schema. |
| clients/go/client_test.go | Unit tests for client defaults, From, SQL, token helper. |
| clients/go/http_test.go | Unit tests for transport behavior (auth, retry, abort, backoff). |
| clients/go/errors_test.go | Unit tests for error parsing + retryable classification. |
| clients/go/query_builder_test.go | Unit tests for builder immutability + AST emission + pagination. |
| clients/go/table_test.go | Unit tests for insert behavior (single, batch, typed slice, ndjson, empty batch). |
| clients/go/namespaces_test.go | Unit tests for Sys/Schema/Policy/DLQ/Pipes namespaces + PipeRef fetch. |
| clients/go/conformance_test.go | Go conformance test runner embedding the shared wire cases fixture. |
| clients/go/example_test.go | Go doc examples (non-asserting, requires running server). |
| clients/go/e2e_test.go | Optional //go:build e2e live-server E2E suite. |
| CHANGELOG.md | Announces the Go SDK addition under Unreleased. |
| AGENTS.md | Updates “SDK Sync” and repo structure to include Go SDK as canonical. |
| .github/workflows/ci.yml | Adds Go SDK tests to CI unit job invocation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 52
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7726491-84ac-4412-9a11-65f929f6d91a
📒 Files selected for processing (41)
.github/workflows/ci.ymlAGENTS.mdCHANGELOG.mdMakefileREADME.mdclients/go/README.mdclients/go/client_test.goclients/go/cmd/wavehouse-codegen/main.goclients/go/conformance_test.goclients/go/dlq.goclients/go/e2e_test.goclients/go/errors.goclients/go/errors_test.goclients/go/example_test.goclients/go/go.modclients/go/http.goclients/go/http_test.goclients/go/live_query.goclients/go/namespaces_test.goclients/go/pipes.goclients/go/policy.goclients/go/query_builder.goclients/go/query_builder_test.goclients/go/schema.goclients/go/stream.goclients/go/sys.goclients/go/table.goclients/go/table_test.goclients/go/testdata/wire_cases.jsonclients/go/types.goclients/go/wavehouse.godocs/src/config/sidebar.tsdocs/src/content/docs/architecture.mddocs/src/content/docs/sdk/go/admin.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/go/pipes.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/sdk/go/streaming.mddocs/src/content/docs/sdk/index.mdxtests/conformance/conformance_ts.mjs
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Docs build
- GitHub Check: Coverage
- GitHub Check: E2E tests
- GitHub Check: Integration tests
- GitHub Check: Unit tests
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (9)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Never force-push or rebase PR branches; mergeorigin/maininstead.
Do not hand-write review or CI markers and do not bypass gates with--no-verify; use the prescribed tooling.
Files:
clients/go/go.modREADME.mddocs/src/content/docs/architecture.mdclients/go/sys.godocs/src/content/docs/sdk/go/index.mddocs/src/config/sidebar.tsdocs/src/content/docs/sdk/go/admin.mdclients/go/testdata/wire_cases.jsonclients/go/dlq.goclients/go/schema.godocs/src/content/docs/sdk/go/pipes.mddocs/src/content/docs/sdk/index.mdxclients/go/e2e_test.goclients/go/live_query.godocs/src/content/docs/sdk/go/reference.mdclients/go/README.mdtests/conformance/conformance_ts.mjsCHANGELOG.mdclients/go/wavehouse.goclients/go/policy.goclients/go/cmd/wavehouse-codegen/main.goclients/go/pipes.goclients/go/types.goAGENTS.mdclients/go/namespaces_test.goclients/go/conformance_test.goclients/go/errors.goclients/go/http_test.goclients/go/query_builder_test.goclients/go/table.goclients/go/errors_test.godocs/src/content/docs/sdk/go/queries.mdclients/go/example_test.goclients/go/table_test.goclients/go/client_test.goclients/go/query_builder.goclients/go/http.goclients/go/stream.goMakefiledocs/src/content/docs/sdk/go/streaming.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Every code change must update corresponding documentation and add a notable-change entry to
CHANGELOG.mdunder[Unreleased].
Files:
README.mddocs/src/content/docs/architecture.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/go/admin.mddocs/src/content/docs/sdk/go/pipes.mddocs/src/content/docs/sdk/go/reference.mdclients/go/README.mdCHANGELOG.mdAGENTS.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/streaming.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use Go 1.26 with strictgofumptformatting; return errors instead of panicking, wrapping them withfmt.Errorf("context: %w", err).
Use structured logging withlog/slog, and pass dependencies explicitly rather than using global state.
Files:
clients/go/sys.goclients/go/dlq.goclients/go/schema.goclients/go/e2e_test.goclients/go/live_query.goclients/go/wavehouse.goclients/go/policy.goclients/go/cmd/wavehouse-codegen/main.goclients/go/pipes.goclients/go/types.goclients/go/namespaces_test.goclients/go/conformance_test.goclients/go/errors.goclients/go/http_test.goclients/go/query_builder_test.goclients/go/table.goclients/go/errors_test.goclients/go/example_test.goclients/go/table_test.goclients/go/client_test.goclients/go/query_builder.goclients/go/http.goclients/go/stream.go
clients/go/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
When backend public APIs change, update the Go SDK's corresponding typed client, query, streaming, policy, pipe, or payload types as applicable.
Files:
clients/go/sys.goclients/go/dlq.goclients/go/schema.goclients/go/e2e_test.goclients/go/live_query.goclients/go/wavehouse.goclients/go/policy.goclients/go/cmd/wavehouse-codegen/main.goclients/go/pipes.goclients/go/types.goclients/go/namespaces_test.goclients/go/conformance_test.goclients/go/errors.goclients/go/http_test.goclients/go/query_builder_test.goclients/go/table.goclients/go/errors_test.goclients/go/example_test.goclients/go/table_test.goclients/go/client_test.goclients/go/query_builder.goclients/go/http.goclients/go/stream.go
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
The TypeScript SDK must have zero third-party runtime dependencies and maintain API-tree parity with the Go SDK.
Files:
docs/src/config/sidebar.ts
docs/src/content/docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
Author Mermaid diagrams primarily top-down, avoid placing large diagrams side-by-side, and keep labels short and readable.
Files:
docs/src/content/docs/sdk/index.mdx
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Use table-driven tests withtests := []struct{...}andt.Run(tt.name, ...)for multiple scenarios.
Use shared helpers and mocks frominternal/testutil/, including JWT, schema, policy, pipes, and HTTP response helpers, instead of ad-hoc implementations.
Files:
clients/go/e2e_test.goclients/go/namespaces_test.goclients/go/conformance_test.goclients/go/http_test.goclients/go/query_builder_test.goclients/go/errors_test.goclients/go/example_test.goclients/go/table_test.goclients/go/client_test.go
.github/workflows/**/*.yml
📄 CodeRabbit inference engine (AGENTS.md)
Pin third-party GitHub Actions to full commit SHAs with version comments; never use floating tags such as
@main.
Files:
.github/workflows/ci.yml
Makefile
📄 CodeRabbit inference engine (AGENTS.md)
Use
make cias the full local validation pipeline before every push; do not use CI as the first feedback loop.
Files:
Makefile
🧠 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:
README.mddocs/src/content/docs/architecture.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/go/admin.mddocs/src/content/docs/sdk/go/pipes.mddocs/src/content/docs/sdk/go/reference.mdclients/go/README.mdCHANGELOG.mdAGENTS.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/streaming.md
📚 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:
clients/go/sys.goclients/go/dlq.goclients/go/schema.goclients/go/e2e_test.goclients/go/live_query.goclients/go/wavehouse.goclients/go/policy.goclients/go/cmd/wavehouse-codegen/main.goclients/go/pipes.goclients/go/types.goclients/go/namespaces_test.goclients/go/conformance_test.goclients/go/errors.goclients/go/http_test.goclients/go/query_builder_test.goclients/go/table.goclients/go/errors_test.goclients/go/example_test.goclients/go/table_test.goclients/go/client_test.goclients/go/query_builder.goclients/go/http.goclients/go/stream.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:
clients/go/e2e_test.goclients/go/namespaces_test.goclients/go/conformance_test.goclients/go/http_test.goclients/go/query_builder_test.goclients/go/errors_test.goclients/go/example_test.goclients/go/table_test.goclients/go/client_test.go
📚 Learning: 2026-06-10T15:01:59.729Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: .github/workflows/ci.yml:232-237
Timestamp: 2026-06-10T15:01:59.729Z
Learning: In this repo’s CI workflow (ci.yml), treat `clickhouse/clickhouse-server:latest` in the workflow’s prefetch steps (`docker pull -q clickhouse/clickhouse-server:latest`) as an intentional canary: the `:latest` tag is meant to mirror the tag that testcontainers resolves at runtime. Do not flag it as a supply-chain concern during CI workflow reviews as long as it’s used specifically for this “latest mirrors testcontainers runtime” prefetch purpose. If the workflow pins a different tag/digest for a different reason, or uses `latest` outside of this prefetch/canary pattern, then it may warrant scrutiny.
Applied to files:
.github/workflows/ci.yml
🪛 LanguageTool
docs/src/content/docs/sdk/go/index.md
[style] ~11-~11: Since ownership is already implied, this phrasing may be redundant.
Context: ...ypeScript client (@wavehouse/sdk) has its own docs starting at SDK Overview —...
(PRP_OWN)
[style] ~228-~228: Since ownership is already implied, this phrasing may be redundant.
Context: ... returned *StreamController manages its own background goroutine and connection, ...
(PRP_OWN)
[style] ~237-~237: Consider using the typographical ellipsis character here instead.
Context: ...ed row slices, not just maps.** Passing []ClickRow{...} (any slice type, detected via reflect...
(ELLIPSIS)
docs/src/content/docs/sdk/go/reference.md
[style] ~216-~216: Since ownership is already implied, this phrasing may be redundant.
Context: ... unit tests colocated in clients/go/ (its own Go module — clients/go/go.mod — separ...
(PRP_OWN)
[style] ~221-~221: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...lls and asserts the Go SDK produces the exact same HTTP method, path, content type, and bo...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
docs/src/content/docs/sdk/go/queries.md
[style] ~211-~211: Consider using the typographical ellipsis character here instead.
Context: ...ent type ([]string, []int, []any, ...) | | wavehouse.OpLike | like | SQL ...
(ELLIPSIS)
[typographical] ~332-~332: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...east that many rows, HasMore is true. Cursor-based pagination's Next walks ...
(WRB_QUESTION_MARK)
[style] ~389-~389: Consider using the typographical ellipsis character here instead.
Context: ...put — use the structured query builder (wh.From(table)...). :::
(ELLIPSIS)
docs/src/content/docs/sdk/go/streaming.md
[style] ~86-~86: Consider using “who” when you are referring to a person instead of an object.
Context: ... buffered (256 events); a slow consumer that never drains it causes the SDK to **dro...
(THAT_WHO)
[typographical] ~160-~160: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...ueryBuilderwith.Where()filters or.Select()columns calls.Stream()`, the...
(WRB_QUESTION_MARK)
[typographical] ~173-~173: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...ilterOpset.Where()takes everywhere.like/not_like` match SQL LIKE sema...
(WRB_QUESTION_MARK)
[style] ~176-~176: Consider using the typographical ellipsis character here instead.
Context: ...and side ([]string, []int, []any, ...), not just []any. --- ## Live Queri...
(ELLIPSIS)
[style] ~231-~231: Since ownership is already implied, this phrasing may be redundant.
Context: ...query outside a live query. Decode into your own type inside the callback if you need on...
(PRP_OWN)
🔇 Additional comments (37)
clients/go/live_query.go (2)
95-126: LGTM!Also applies to: 142-147
18-35: 🎯 Functional CorrectnessNo change needed for
filters.
QueryBuilder.LiveQuerypasses the query filters throughQueryBuilder.Stream, which wraps the raw stream withnewFilteredStreamControllerbeforenewLiveQuery. Client-side filtering is already applied to live events.clients/go/query_builder.go (1)
35-45: LGTM!Also applies to: 239-273
clients/go/schema.go (1)
11-33: LGTM!clients/go/dlq.go (1)
15-42: LGTM!clients/go/stream.go (2)
169-169: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Reachability: External
Reachability path
● Entry clients/go/e2e_test.go:313 TestE2E_PipesCRUD: Create │ ▼ ● Hop clients/go/errors.go:26 Error │ ▼ ● Sink clients/go/stream.goReplace
log.Printfwithlog/slog, and stop logging raw payloads.Two problems:
- The coding guidelines require structured logging with
log/slogand explicit dependency passing. Both call sites write to the global standard logger, which an SDK consumer cannot redirect or silence.- Line 340 writes the whole malformed SSE frame into the log. That frame carries user record data, so a truncated or malformed frame leaks record contents into logs the consumer did not opt into. Log the length and the decode error instead of the payload.
Accept an optional
*slog.LoggeronClientOptions, store it onhttpContext, and default it toslog.Default().🛡️ Proposed change
- log.Printf("[wavehouse] stream event dropped: channel buffer full") + sc.logger.Warn("stream event dropped", "reason", "channel buffer full", "table", event.Table)var msg sseMessage if err := json.Unmarshal([]byte(data), &msg); err != nil { - log.Printf("[wavehouse] SSE received malformed message: %s", data) + sc.logger.Warn("malformed SSE message", "error", err, "bytes", len(data)) return }Then drop the
logimport and addlog/slog.Run the following script to find every global-logger call in the SDK:
#!/bin/bash # Description: Locate global log package usage and any existing slog wiring in the Go SDK. set -euo pipefail fd -e go . clients/go --exec rg -n -C2 '\blog\.(Printf|Println|Print|Fatal|Fatalf)\b' {} fd -e go . clients/go --exec rg -n -C3 'slog|Logger' {}Also applies to: 337-342
363-393: 🩺 Stability & AvailabilityNo change needed for the filtered controller callback scope.
StreamController.closeEventChis not exported, the filtered path uses internal access toeventCh, andemitEventuses a non-blocking send so it does not panic whensc.eventChis closed.> Likely an incorrect or invalid review comment.clients/go/go.mod (1)
1-3: 📐 Maintainability & Code QualityNo patch-version change needed for
go 1.26.5.The module explicitly supports Go 1.26.5 and no earlier Go 1.26 patch, so this matches the declared Go version.
> Likely an incorrect or invalid review comment.clients/go/types.go (1)
7-180: LGTM!Also applies to: 193-245
clients/go/http_test.go (1)
13-199: LGTM!clients/go/wavehouse.go (1)
44-48: LGTM!Also applies to: 96-114, 132-142
clients/go/client_test.go (1)
11-100: LGTM!clients/go/table.go (1)
20-76: LGTM!clients/go/table_test.go (1)
12-209: LGTM!clients/go/cmd/wavehouse-codegen/main.go (2)
300-338: LGTM!
42-46: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-214)
Reachability: Internal
Accept the bearer token from an environment variable.
--authrequires the token on the command line. The token then appears in the process argument list and in shell history. Any local user can read/proc/<pid>/cmdlinewhile the command runs. Add an environment-variable source such asWAVEHOUSE_AUTHand keep the flag as an override only.🔐 Proposed change
func parseArgs() cliArgs { args := cliArgs{url: "http://localhost:8080", out: "./wavehouse_types.go", pkg: "main"} + if tok := os.Getenv("WAVEHOUSE_AUTH"); tok != "" { + args.auth = tok + } for i := 1; i < len(os.Args); i++ {Also document the environment variable in the
--helptext.tests/conformance/conformance_ts.mjs (2)
73-146: LGTM!
258-261: 🎯 Functional CorrectnessNo change needed for health empty-response parsing.
The TS
request<T>implementation treats successful responses without text bodies asundefinedand does not callJSON.parsefor the empty/v1/healthbody, so this cannot hide a correct request for that fixture.> Likely an incorrect or invalid review comment.clients/go/testdata/wire_cases.json (1)
374-388: 🗄️ Data Integrity & IntegrationNo change needed.
Both SDKs send array insert as
\n-joined NDJSON without a trailing newline, andinsertNDJSONpasses NDJSON sources through unchanged.clients/go/e2e_test.go (1)
286-311: 🗄️ Data Integrity & IntegrationNo change needed.
The SDK
Policy, SDKPolicyNamespace.Get/Set, and serverpolicy.Policyshare the same JSON-marshaled fields, so this round-trip does not drop server-side policy fields.> Likely an incorrect or invalid review comment..github/workflows/ci.yml (1)
207-208: LGTM!AGENTS.md (1)
63-63: LGTM!Also applies to: 334-349, 390-397
CHANGELOG.md (1)
14-14: LGTM!Makefile (2)
431-435: LGTM!
735-739: LGTM!Also applies to: 777-777
docs/src/content/docs/sdk/go/queries.md (2)
1-363: LGTM!Also applies to: 369-382, 390-390
383-389: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winInjection (CWE-89): Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection')
State the safe boundary for raw SQL.
SQL[Row]has no parameter binding. The phrase “Inline literals into the SQL” can be read as permission to concatenate untrusted values into/v1/admin/query. State that only trusted constants may be inlined. Require the structured query builder for user-supplied values.Suggested wording
-Inline literals into the SQL, or — for safe binding from user-supplied input — use the structured query builder (`wh.From(table)...`). +Inline only trusted constants. For user-supplied input, use the structured query builder (`wh.From(table)...`).docs/src/content/docs/sdk/go/reference.md (1)
1-40: LGTM!Also applies to: 45-136, 144-229, 234-234
docs/src/content/docs/sdk/go/streaming.md (1)
1-255: LGTM!docs/src/content/docs/sdk/index.mdx (1)
17-18: LGTM!Also applies to: 423-434
clients/go/README.md (1)
1-218: LGTM!Also applies to: 221-221, 223-235
docs/src/config/sidebar.ts (1)
28-30: LGTM!Also applies to: 41-51
docs/src/content/docs/architecture.md (1)
279-279: LGTM!docs/src/content/docs/sdk/go/admin.md (1)
1-62: LGTM!Also applies to: 66-118
docs/src/content/docs/sdk/go/index.md (1)
1-179: LGTM!Also applies to: 196-249
docs/src/content/docs/sdk/go/pipes.md (1)
1-101: LGTM!README.md (1)
63-63: 🎯 Functional CorrectnessNo change needed. Both SDKs provide a
wavehouse-codegenCLI, and the summary sentence correctly scopes schema codegen to both clients.
github-project-automation
Bot
moved this from Backlog
to In review
in WaveHouse Task Board
Source: extract helpers (errAborted, aggDefault, emptyInsertResult, marshalNDJSON, dlq.stats, sortedKeys), collapse like/not_like, inline trimTrailingSlashes, one-line IsRetryable, map-based numeric type lookup. Tests: table-driven parseErrorResponse, loop namespace nil checks, merge DLQ List+Table subcases, skipIfUnauthorized helper. Docs: dedupe Quick Start + Error Handling in index.md (link to README and reference.md), drop codegen type table from README (link to docs).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated no new comments.
Suppressed comments (2)
clients/go/query_builder_test.go:28
- captureQueryBody reads the request body with a single Read into a fixed 32KiB buffer, which can truncate JSON (and make tests flaky) if the encoded query ever grows beyond one read. It’s safer to decode the whole JSON body.
wrapper := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
raw := make([]byte, 32*1024)
n, _ := r.Body.Read(raw)
body = raw[:n]
handler.ServeHTTP(w, r)
clients/go/query_builder.go:288
- Pagination cursor extraction coerces typed-row fields through map[string]any via json.Unmarshal, which turns numbers into float64 (and can lose integer precision). That value is then sent back as a filter in the next request, which can fail schema validation or page incorrectly for numeric cursor columns.
// Extract the last row's value for the cursor column.
lastRow := any(prevRows[len(prevRows)-1])
m, ok := lastRow.(map[string]any)
if !ok {
// ponytail: marshal/unmarshal round-trip to get a map — optimize with reflect if perf matters.
…indings CI blockers: - Bump google.golang.org/grpc 1.82.1, golang.org/x/text 0.39.0, klauspost/compress 1.18.7 (GO-2026-6061/-5970/-5841) — vulncheck green - Drop forbidden trailing slash in sdk/go/index.md anchor link SDK behavior (CodeRabbit/Copilot review): - Retry: 429 now retryable, Retry-After honored for 429 and clamped to 30s, ±20% backoff jitter, dead 503 clause removed - NewClient uses a fresh http.Client instead of mutable http.DefaultClient - LiveQueryHandle: close state applied synchronously in Close(); dedup bound is the max backfilled timestamp compared as parsed time.Time - Stream reconnect backoff resets after a connection reaches live - Pagination replaces the cursor filter instead of stacking one per page - PolicyFilter operators marshal with omitempty (absent, never null) - Errors wrapped with operation context at every SDK boundary - codegen: unknown args rejected, 30s HTTP timeout, wrapped errors, field/type collision detection, WAVEHOUSE_AUTH env var for the token, Int64/UInt64 map to string (ClickHouse quotes 64-bit ints in JSON output) Tests: - httptest servers closed via t.Cleanup; handler captures synchronized; captureQueryBody uses io.ReadAll; multi-scenario tables use t.Run subtests - e2e: probe timeout, deterministic table pick returning its schema, polling instead of fixed sleeps, errors.As, unsupported-type skip - conformance: SDK errors logged, arg guards, normalizePath compares decoded query values (Go + TS), TS harness counts unhandled endpoints as skipped, stubs aligned to real server shapes, deterministic exit, cacheTTL fixture Docs/Makefile: policyDraft defined in admin example, (T, error) claim scoped to request-response ops, operator-key path documented for admin SQL, test-go-sdk-e2e documented, test-all includes test-go-sdk, --allow-parallel-runners on SDK lint, HTTPS caution for bearer tokens.
… doc sync Streaming correctness: - Filtered stream no longer panics with send-on-closed-channel when Close() races in-flight inner deliveries: channel close and channel send now serialize under the controller mutex, and the wrapper unsubscribes from the inner stream before closing - Events() channel is fed only once Events() has been called, so Subscribe-only consumers no longer overflow a channel nobody reads; the buffer-full drop is logged once, not per event - Malformed-SSE log omits the payload (can carry tenant/PII fields) - Unparseable Retry-After falls back to backoff(attempt), not the 30s max; parsing extracted to retryAfterDelay for testability Tests (SDK coverage 33% → 80%+; streaming subsystem was 0%): - stream_test.go: SSE lifecycle over httptest, filtered close-under-load regression test, Events()/Connected, full filter-engine tables - live_query_test.go: backfill buffering, desc-order dedup bound, fetch error, no-callbacks-after-Close - http_test.go: retryAfterDelay table + live 429 Retry-After flow - Remaining unclosed httptest servers in table_test/http_test now use t.Cleanup Build plumbing: - test-go-sdk runs with -race; test/lint/fix aggregates now include the nested clients/go module; mangled test-go-sdk comment restored - New test-conformance-ts target runs the TS conformance runner (was wired to nothing); CI unit job runs it Docs: - development.md synced: suites/targets tables, CI unit job, project structure with clients/, Go SDK in the dev-loop section - reference.md: real codegen output (EventId not EventID; no bare int), initialism note, missing type-mapping rows (SimpleAggregateFunction, Time/Time64, Boolean, BFloat16), two-runner conformance wording, 401 row corrected (missing token → 403) here and in sdk/reference.md - Go SDK added alongside TS on the landing page, getting-started, and why-wavehouse comparison tables
…gen pointers, precision SDK behavior: - Non-retryable SSE connect errors (401/403/404) are now terminal: connect surfaces the parsed API error, run emits it and closes the stream instead of reconnecting forever; Connected() unblocks with "stream closed" - codegen: defaulted columns generate pointer fields (*T + omitempty) — the Go spelling of the TS codegen's `field?: T` — so an explicit zero value is sent instead of silently dropped in favor of the server default - Typed-row pagination cursor decodes with json.Number, keeping int64 cursor values past 2^53 exact Tests: - Pagination: page.Next walked across three pages asserting exactly one replaced cursor filter with the right op/value, desc → lt, quiet end when the projection omits the order column, int64 precision regression test - Terminal 403 stream test: error surfaced, StatusClosed, Connected fails - e2e: buildMarkerRow returns the column it used (markerColumn could pick a defaulted column the row never wrote); http_test raw-body via io.ReadAll - conformance_ts exits non-zero when nothing ran or any case was skipped Docs/Makefile: - Error-model claim scoped: HTTP-exchange errors are *wavehouse.Error; pre-request failures (auth provider, marshal) are plain wrapped errors — examples gain the else branch (go/index, go/reference, README) - codegen README example uses WAVEHOUSE_AUTH; reference.md documents the pointer rule and sample output - queries.md: real aggregation signatures/alias defaults, pagination example gains the OrderBy it needs - development.md: coverage sentence scoped to instrumented suites, "four suites" count dropped, Releasing the SDKs covers the Go module - access-control.mdx + pipes.mdx list the Go SDK method equivalents - make ci runs test-conformance-ts (parity with the CI unit job)
…rray(UInt8), codegen tests - Untyped-path cursor precision: acknowledged as a documented ceiling rather than claimed fixed — FetchUntyped rows are float64-decoded before pagination sees them (same 2^53 ceiling as the TS SDK's JS numbers), so the code comment now says exactly that, queries.md documents the caveat next to the pagination example, and a regression test pins the behavior; FetchTyped and codegen structs remain exact - codegen: Array(UInt8) no longer generates []uint8 ([]byte, which encoding/json base64-encodes and the server rejects) — widened to []uint16; new main_test.go covers chTypeToGo (incl. this case), pascalCase's digit guard, findTopLevelComma, pointer-default output, and both collision failures (codegen package was 0% covered) - Docs: streaming.md documents terminal non-retryable stream errors, SSE_ERROR row added to both SDK reference tables, make test/fmt/ci descriptions synced, internal/stream added to the project tree, SQL example no longer redeclares rows :=, CONTRIBUTING gains the SDK-sync bullet (+ configuration.mdx path, also in SUPPORT.md), AGENTS.md drops the nonexistent "wavehouse-go" name for the real module path, landing/ why-wavehouse name the Go module importably, 404 page links the Go SDK
…IKE compile, doc precision - codegen: Int64/UInt64 map to int64/uint64 and 128/256-bit ints to json.Number (with a conditional encoding/json import) — generated structs target /v1/query and /v1/pipes/*, where the server scans ClickHouse values into Go types and re-marshals them as UNQUOTED numbers; the round-1 string mapping only held for /v1/admin/query, which forwards ClickHouse's own quoted JSON (use map[string]any with SQL[Row] there). Decode round-trip test pins the wire shape; docs type table and parity paragraph updated, pagination caveat now notes 64-bit codegen columns decode exactly - Filtered streams compile LIKE patterns once at construction — the process-global likeRegexCache sync.Map (unbounded, keyed on caller input) is gone, and per-event matching is a plain regex call - Docs: /sdk/go Quick Start is compilable (package main + func main, like the README); client concurrency-safety documented; Events() first-call feeding note; SELECT * expansion claim scoped to column-restricted roles (both SDK pages); Array(UInt8) exception in the type table; codegen go run uses @latest so it works outside the repo; stale TableRef "NOT safe for mutations" comment corrected (it holds no mutable state) - AGENTS.md: configuration.mdx path, both SDK readmes in the prose list - CHANGELOG: Go SDK entry expanded to house style (module path, nested- module caveat, new targets, conformance wiring)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
clients/go/cmd/wavehouse-codegen/main.go (1)
41-123: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd tests for the new CLI parsing and schema retrieval paths.
main_test.gocovers generation helpers only. It has no cases forparseArgs,flagValue, orfetchSchemas.Add table-driven cases for argument values, missing values, unknown arguments,
WAVEHOUSE_AUTH, array and map schema responses, malformed JSON, non-200 responses, and the authorization header. Usehttptest.NewServerforfetchSchemas. Test exit paths in a subprocess if needed.As per coding guidelines: “Every new function should have corresponding test cases.”
Source: Coding guidelines
Makefile (1)
460-464: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude the nested SDK in
make fmt.
GO_DIRScomes from the root module, sofmt-godoes not visitclients/go. The newverify-go-sdktarget checks formatting only duringmake verify;make fmtcan still report success with unformatted SDK files. Add a nested-module formatter leaf tofmtor extend the formatter command to coverclients/go.scripts/cov/main.go (1)
278-282: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winApply the standalone SDK gate in
merge-all.When
go-sdkis the only coverage data,hasAnyCoveragereturns true, butmerge-allcallsmerge, which iterates onlygoSuites, and thenmergeTS. The command can exit successfully without rendering or checking thego-sdkthreshold. Add standalone suites to this command or reject standalone-only input.docs/src/content/docs/sdk/go/pipes.md (1)
35-41: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the stream in the example.
The Go SDK documentation states that
StreamControllerowns the connection untilClose. This example opensstreamwithout closing it. Adddefer stream.Close()immediately after creation.
♻️ Duplicate comments (1)
docs/src/content/docs/sdk/go/reference.md (1)
45-45: 🎯 Functional Correctness | 🟡 MinorAlign the
SSE_PARSE_ERRORrecovery contract across both Go SDK pages.The pages disagree about whether a malformed SSE frame causes reconnection or an in-place skip.
docs/src/content/docs/sdk/go/reference.md#L45-L45: document the retryability value and description consistently with the intended in-place-skip behavior.docs/src/content/docs/sdk/go/streaming.md#L111-L111: removeSSE_PARSE_ERRORfrom the reconnect-trigger list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3ecec750-2db4-49cc-8168-fe03e9485d33
📒 Files selected for processing (41)
.claude/commands/cover.md.github/dependabot.yml.github/workflows/ci.yml.testcoverage.ymlAGENTS.mdMakefileclients/go/README.mdclients/go/client_test.goclients/go/cmd/wavehouse-codegen/main.goclients/go/cmd/wavehouse-codegen/main_test.goclients/go/conformance_test.goclients/go/dlq.goclients/go/e2e_test.goclients/go/errors.goclients/go/errors_test.goclients/go/http.goclients/go/http_test.goclients/go/live_query.goclients/go/live_query_test.goclients/go/namespaces_test.goclients/go/pipes.goclients/go/query_builder.goclients/go/query_builder_test.goclients/go/schema.goclients/go/stream.goclients/go/stream_test.goclients/go/sys.goclients/go/table.goclients/go/table_test.goclients/go/testdata/wire_cases.jsonclients/go/types.goclients/go/wavehouse.godocs/src/content/docs/sdk/go/admin.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/go/pipes.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/sdk/go/streaming.mddocs/src/content/docs/sdk/index.mdxscripts/cov/main.gotests/conformance/conformance_ts.mjs
💤 Files with no reviewable changes (1)
- clients/go/schema.go
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
- **Opt a page into the Cloud CTA with `cloudCta` frontmatter**, not by importing the component.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/go/pipes.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/streaming.mddocs/src/content/docs/sdk/go/admin.md
Return errors, don't panic. Wrap with `fmt.Errorf("context: %w", err)`.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
clients/go/errors.goclients/go/conformance_test.goclients/go/live_query.goclients/go/types.goclients/go/pipes.goclients/go/table_test.goclients/go/sys.goclients/go/wavehouse.goscripts/cov/main.goclients/go/errors_test.goclients/go/cmd/wavehouse-codegen/main_test.goclients/go/query_builder.goclients/go/e2e_test.goclients/go/live_query_test.goclients/go/query_builder_test.goclients/go/table.goclients/go/dlq.goclients/go/http_test.goclients/go/http.goclients/go/client_test.goclients/go/cmd/wavehouse-codegen/main.goclients/go/stream_test.goclients/go/stream.goclients/go/namespaces_test.go
Both ship from this repo with full API-tree parity.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
clients/go/errors.goclients/go/conformance_test.goclients/go/live_query.goclients/go/types.goclients/go/pipes.goclients/go/table_test.goclients/go/sys.goclients/go/wavehouse.goclients/go/errors_test.goclients/go/cmd/wavehouse-codegen/main_test.goclients/go/query_builder.goclients/go/e2e_test.goclients/go/README.mdclients/go/live_query_test.goclients/go/testdata/wire_cases.jsonclients/go/query_builder_test.goclients/go/table.goclients/go/dlq.goclients/go/http_test.goclients/go/http.goclients/go/client_test.goclients/go/cmd/wavehouse-codegen/main.goclients/go/stream_test.goclients/go/stream.goclients/go/namespaces_test.go
- **Every new function should have corresponding test cases.** Run `make lint` and `make test` before considering work complete.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
clients/go/conformance_test.goclients/go/table_test.goclients/go/errors_test.goclients/go/cmd/wavehouse-codegen/main_test.goclients/go/e2e_test.goclients/go/live_query_test.goclients/go/query_builder_test.goclients/go/http_test.goclients/go/client_test.goclients/go/stream_test.goclients/go/namespaces_test.go
- **Never hard-wrap prose. One paragraph is one line.** No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/go/pipes.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/streaming.mddocs/src/content/docs/sdk/go/admin.mdclients/go/README.mdAGENTS.md
- **In MDX, leave a blank line between a JSX tag and a code fence.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/sdk/index.mdx
🧠 Learnings (5)
📚 Learning: 2026-08-11T21:55:53.726Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/stream_test.go:159-176
Timestamp: 2026-08-11T21:55:53.726Z
Learning: For Go files in this repository, do not report direct type assertions solely because the `forcetypeassert` rule is commented out in `.golangci.yml`. Only flag a type assertion when there is an independent correctness, safety, or maintainability issue.
Applied to files:
clients/go/conformance_test.goclients/go/stream_test.go
📚 Learning: 2026-08-11T21:55:39.391Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/cmd/wavehouse-codegen/main_test.go:55-59
Timestamp: 2026-08-11T21:55:39.391Z
Learning: In Go SDK tests, do not require named t.Run subtests for table-driven test loops when assertion errors already identify the failing input and expected and actual values. Avoid raising style-only findings to add t.Run in these cases.
Applied to files:
clients/go/conformance_test.goclients/go/table_test.goclients/go/e2e_test.goclients/go/query_builder_test.goclients/go/http_test.goclients/go/stream_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:
clients/go/conformance_test.goclients/go/table_test.goclients/go/errors_test.goclients/go/e2e_test.goclients/go/live_query_test.goclients/go/query_builder_test.goclients/go/http_test.goclients/go/client_test.goclients/go/stream_test.go
📚 Learning: 2026-08-11T21:55:32.845Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: docs/src/content/docs/sdk/go/index.md:180-180
Timestamp: 2026-08-11T21:55:32.845Z
Learning: Go SDK documentation should accurately distinguish the query APIs: `Client.From(table)` returns a `*TableRef`, whose `Fetch(ctx)` method fetches all columns as untyped rows. `QueryBuilder.FetchUntyped(ctx)` is available only on a `*QueryBuilder`, such as one returned by `TableRef.Select(...)` or `TableRef.SelectAll()`.
Applied to files:
docs/src/content/docs/sdk/go/queries.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:
AGENTS.md
🪛 golangci-lint (2.12.2)
clients/go/query_builder_test.go
[error] 15-15: Error return value of (*encoding/json.Encoder).Encode is not checked: unsafe type any found
(errchkjson)
[error] 200-200: type assertion must be checked
(forcetypeassert)
🪛 LanguageTool
docs/src/content/docs/sdk/go/reference.md
[style] ~170-~170: Since ownership is already implied, this phrasing may be redundant.
Context: ...re colocated in clients/go/, which is its own module (clients/go/go.mod), separate ...
(PRP_OWN)
[style] ~179-~179: Since ownership is already implied, this phrasing may be redundant.
Context: ...n against a live WaveHouse instance via their own Make target: ```bash WAVEHOUSE_URL=htt...
(PRP_OWN)
docs/src/content/docs/sdk/go/index.md
[grammar] ~143-~143: Use a hyphen to join words.
Context: ...uthprovider, marshal errors) are plain wrapped errors, so handle theerrors.As...
(QB_NEW_EN_HYPHEN)
[style] ~170-~170: Consider using the typographical ellipsis character here instead.
Context: ... Any slice batches. Reflection lets []ClickRow{...} take the same NDJSON batch path as `[...
(ELLIPSIS)
docs/src/content/docs/sdk/go/queries.md
[style] ~105-~105: Consider using the typographical ellipsis character here instead.
Context: ...eaming). ## Query Builder Returned by tableRef.Select(...) or tableRef.SelectAll(). Immutable ...
(ELLIPSIS)
docs/src/content/docs/sdk/go/streaming.md
[style] ~66-~66: Consider using the typographical ellipsis character here instead.
Context: ... Status are delivered exclusively via .Subscribe(...). The channel closes on terminal error...
(ELLIPSIS)
[typographical] ~136-~136: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...ilterOpset.Where()takes everywhere.OpLike/OpNotLike` use SQL LIKE seman...
(WRB_QUESTION_MARK)
[grammar] ~142-~142: Ensure spelling is correct
Context: ...lly equal row. Both sides are parsed as instants instead. - **Only unambiguous spellings...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~144-~144: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...o it is not treated as a timestamp. - Ordering an instant against a non-instant fails closed. If one side parses as a timestamp and the other does not, OpGt/OpGte/OpLt/OpLte withhold the row rather than falling back to text comparison, which could admit rows the query path excludes. The usual cause is a zone-less filter c...
(TOO_LONG_SENTENCE)
[style] ~149-~149: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...age domain — so a client-side filter on a very large UInt64 can disagree with the server's...
(EN_WEAK_ADJECTIVE)
AGENTS.md
[style] ~128-~128: Consider using the typographical ellipsis character here instead.
Context: ...module — invisible to the root module's -coverpkg=./..., so it is gated on its own `suites.go-...
(ELLIPSIS)
[uncategorized] ~128-~128: The official name of this software platform is spelled with a capital “H”.
Context: ... comments via GitHub Code Quality — see .github/workflows/README.md "Coverage publishi...
(GITHUB)
🔇 Additional comments (41)
clients/go/types.go (1)
7-17: LGTM!Also applies to: 137-138, 164-165, 196-202, 213-216
clients/go/wavehouse.go (1)
17-17: LGTM!Also applies to: 27-32, 44-46, 61-63, 76-83, 125-126
clients/go/errors.go (1)
14-18: LGTM!Also applies to: 42-42
clients/go/http.go (1)
33-34: LGTM!Also applies to: 51-52, 87-88, 147-147, 186-187, 202-203
clients/go/table.go (1)
12-13: LGTM!Also applies to: 55-57, 108-109, 145-147
clients/go/query_builder.go (1)
11-12: LGTM!Also applies to: 29-30, 176-178, 203-205, 248-255, 268-281, 283-296
clients/go/stream.go (1)
31-49: LGTM!Also applies to: 60-78, 92-94, 146-147, 151-171, 186-187, 202-217, 254-280, 284-285, 287-310, 333-335, 348-378, 390-403, 420-428, 450-457, 460-472, 499-500, 515-516, 531-531, 546-546, 584-585, 607-623, 631-632, 646-675
clients/go/live_query.go (1)
23-24: LGTM!Also applies to: 37-38, 86-99
clients/go/client_test.go (1)
9-46: LGTM!Also applies to: 70-86
clients/go/http_test.go (2)
26-49: LGTM!Also applies to: 51-127
148-286: LGTM!Also applies to: 388-396, 438-442, 457-464
clients/go/errors_test.go (1)
49-52: LGTM!Also applies to: 97-106
clients/go/namespaces_test.go (1)
10-195: LGTM!clients/go/table_test.go (1)
9-181: LGTM!clients/go/query_builder_test.go (2)
28-143: LGTM!Also applies to: 163-171, 178-203, 208-277, 302-355
12-16: 📐 Maintainability & Code QualityNo change needed.
.golangci.ymlcomments outerrchkjson, so it is not part of the configured lint gate.clients/go/live_query_test.go (1)
37-111: LGTM!clients/go/stream_test.go (2)
18-50: LGTM!Also applies to: 63-91
190-197: LGTM!Also applies to: 251-340, 384-403, 405-494, 508-510, 559-559, 610-650, 661-661
clients/go/conformance_test.go (2)
56-209: LGTM!
211-333: LGTM!clients/go/testdata/wire_cases.json (1)
1-336: LGTM!tests/conformance/conformance_ts.mjs (1)
124-154: LGTM!Also applies to: 192-209, 248-248
clients/go/e2e_test.go (2)
55-57: LGTM!Also applies to: 118-139
141-185: LGTM!Also applies to: 187-343, 353-413
clients/go/README.md (2)
3-12: LGTM!Also applies to: 15-59
13-13: 📐 Maintainability & Code QualityKeep the Go 1.24 requirement.
clients/go/go.moddeliberately declares Go 1.24 as the library floor. The rootgo.modversion 1.26.6 is separate, and CI reads that root module. The README statement is correct..claude/commands/cover.md (1)
3-3: LGTM!Also applies to: 16-20
.github/dependabot.yml (1)
3-8: LGTM!.github/workflows/ci.yml (1)
207-210: LGTM!docs/src/content/docs/sdk/go/queries.md (1)
6-18: LGTM!Also applies to: 34-59, 61-101, 103-199, 201-260, 262-263, 265-284
docs/src/content/docs/sdk/go/reference.md (1)
6-44: LGTM!Also applies to: 46-49, 104-117, 129-166, 170-185
docs/src/content/docs/sdk/go/streaming.md (1)
6-109: LGTM!Also applies to: 115-217
docs/src/content/docs/sdk/index.mdx (1)
11-11: LGTM!Also applies to: 562-566
.testcoverage.yml (1)
15-16: LGTM!Also applies to: 34-38
AGENTS.md (1)
63-63: LGTM!Also applies to: 297-297, 363-378, 419-420
Makefile (1)
202-203: LGTM!Also applies to: 369-378, 495-496, 542-543, 561-561, 762-762, 805-839, 879-879
scripts/cov/main.go (1)
62-71: LGTM!Also applies to: 225-226, 235-252, 329-335, 589-605, 972-978
docs/src/content/docs/sdk/go/admin.md (1)
1-23: LGTM!Also applies to: 25-58, 62-76, 80-94
docs/src/content/docs/sdk/go/index.md (1)
6-24: LGTM!Also applies to: 26-57, 59-120, 122-139, 141-158, 160-170, 172-178
docs/src/content/docs/sdk/go/pipes.md (1)
1-6: LGTM!Also applies to: 8-33, 45-79
- conformance: the Go harness had no /v1/ingest branch, so both insert cases
decoded the default `[]` into InsertResult, failed, and were only logged by
logErr — the request assertions still passed, so the divergence was
invisible. Both harnesses now answer ingest and health the same way, which
is what the TS runner's comment already claimed. (/v1/health never actually
failed: Sys.Health passes a nil decode target.)
- conformance_ts.mjs: index an aggregation op's args defensively, matching the
Go harness's stringArg, so a fixture like {"method":"count"} cannot crash one
runner while the other accepts it.
- pipes: PipeRef.Stream's doc comment claimed it works wherever the pipe name
is also a table name. It does not — the pipe's SQL and params are never sent,
so the caller gets that table's raw events. Say so, in the godoc and the docs
page, and point at #445.
- AGENTS.md: the second coverage summary at line 413 still said "sdk 50%" and
omitted go-sdk entirely.
- Makefile: GO_DIRS comes from `go list ./...` on the root module, which never yields the nested clients/go, so `make fmt` reported success on an unformatted SDK file. Only `make verify` caught it, via verify-go-sdk. gofumpt now gets clients/go explicitly; verified with a deliberately misformatted probe file. - scripts/cov: merge-all counted the standalone go-sdk suite in hasAnyCoverage but merge() only iterates goSuites, so go-sdk-only input exited 0 with no threshold applied. It now renders and gates the standalone suites. (`make cov` runs `report`, which already gated them — merge-all has no callers in the build, so this was reachable only by hand.) - docs/sdk/go/streaming.md: SSE_PARSE_ERROR was listed among the reconnect triggers. handleSSEData drops the frame and returns on the same connection, which is what reference.md already said. The two pages now agree with the code. - docs/sdk/go/pipes.md: the .Stream example never closed the controller, unlike every other stream example.
main_test.go exercised the generation helpers only, leaving parseArgs, flagValue, fetchSchemas and sortedKeys with no direct cases (CodeRabbit, PR #434). Adds table-driven coverage for flag values (long and short), defaults, the WAVEHOUSE_AUTH fallback and its --auth override, and, in a re-executed child process, the os.Exit branches for a missing flag value, an unknown argument and --help. fetchSchemas is driven against httptest servers for array- and map-shaped responses, a trimmed trailing slash, malformed JSON, a body that is neither shape, a non-200 status, an unbuildable URL and a transport failure, asserting the request path and that Authorization: Bearer is sent only when auth is supplied. Package coverage 55.8% -> 81.8%; parseArgs, flagValue, fetchSchemas and sortedKeys are now at 100%. Production code unchanged.
Review 5020447247 — the five findings that couldn't be posted inlineThe four inline threads from this review are answered and resolved in place. The five in the review body have no thread attached ("outside diff range", plus the duplicate), so they're answered here. All five were valid; all five are fixed.
Duplicate: Full |
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Not even started on reviewing the actual SDK yet, so far has been just on the structure around the SDK, and this is straight slop so far… idk if ur using a local model that is just not smart, or using some plugins or skills that are absolutely knee-capping its capabilities, but I swear GPT from 2 years ago would do better than whatever you're using now has been doing – it's to the point that it's taking more time for me to review the slop than for me to just get an agent to work on this PR myself… Left lots of comments on various things outside of the sdk code itself, but please if you are to continue on this PR make SOME sort of change so that the agent is actually competent and can do its own full passes to fix all of this stuff.
Addresses every thread @EricAndrechek left on #434. Docs — the documented decision, restored. PR #313 settled that SDK pages are topic-first: shared usage pages grow <Tabs syncKey="lang"> as languages land, each language keeps its own setup page, and the topic URLs never churn. The Go SDK's first draft deleted that comment and shipped a parallel /sdk/go/* tree with TypeScript left un-prefixed at the root of /sdk. Undone: - /sdk/{queries,streaming,pipes,admin,reference} are now .mdx, each carrying one <Tabs syncKey="lang"> block per section — shared prose outside, language-specific code and caveats inside. Nothing was dropped in the merge; both languages' full content is preserved. - /sdk/typescript (was the root /sdk) and /sdk/go are the per-language setup/caveats pages. /sdk is now a language-neutral overview. - The sidebar comment is back, and stronger: it says what a third language costs (one setup page, one TabItem per topic) and what it must not do (a parallel tree; a language at the root of /sdk). - AGENTS.md gains a §SDK docs layout section so the rule is enforceable rather than a comment one agent can delete. - SDK mentions across README, why-wavehouse, getting-started, the docs homepage, the 404 page and the footer drop the per-SDK dependency-count trivia and just name the clients we publish. The dependency posture stays where it is a real difference: the Go page's own comparison list. Make targets — one family, no new ad-hoc names. - test-ts -> test-sdk-ts, test-go-sdk -> test-sdk-go, plus test-sdk which runs both and test-sdk-go-e2e for the live-server suite. - test-conformance-ts is gone as a target: each language's half of the wire-format suite now rides that language's target, which is already how the Go half worked. - `make test` is the test-unit alias it was before this PR. - All four SDK targets use gotestsum and honor ARGS/V=1 like every other Go suite — the nested module has no tool directives, so they resolve the binary with `go tool -n gotestsum` from the root module first. Static checks — one entry point per tool, not per module. - lint-go-sdk and verify-go-sdk are deleted. lint-go now lints both modules (two invocations, because golangci-lint is module-scoped); verify-go-sdk was pure duplication — gofumpt is already fmt-go's job and `go vet` is golangci-lint's govet linter. - tidy covers the nested go.mod too, so verify checks exactly what fix rewrites. That closes the tidy half of #437; vulncheck remains open. - go-mod-download warms both modules, so anything reaching into clients/go inherits the prereq. - fix-go is one step per line instead of a five-command && chain, and a comment says the `cd` is the only difference between its two golangci-lint lines. Coverage — .testcoverage.yml now explains why the Go SDK has one gate key where the TS SDK has three (one instrumented suite vs two), and #518 tracks running the Go e2e suite from the orchestrator so it earns the same shape. Refs #434, #437, #518
Docs review (1 MUST, 2 SHOULD, 4 MAY):
- MUST: the topic-page merge hoisted `Last-Event-ID` into shared prose,
claiming it for both clients. Only the TS SDK sends that header
(clients/ts/src/stream/sse.ts:393); Go folds the last event id back
into `?since=` (clients/go/stream.go:246,294). Same server behavior —
internal/api/stream.go prefers the header and falls back to since —
but wrong for anyone configuring a gateway header allow-list or
reading a capture. Both spellings are now named.
- development.md's file tree still said "hence the *-go-sdk make
targets"; no target matches that glob any more.
- typescript.mdx had a dangling back-reference to "the SDK's single
~1.4 KB one" — the concision pass removed its antecedent, leaving the
TS setup page never stating the dependency. Restored there, which is
where per-language detail belongs; the overview pages stay trimmed.
- Restored the builder-level "Streaming from a builder" pointer, the
one substantive thing the merge dropped.
- `failed === 0` was JS syntax sitting in language-neutral prose.
- go.md linked "Reference → Context Cancellation" at a heading this PR
renamed to "Cancellation".
- The Go live-query tab said the stream-first guarantee "holds only
when the backfill completes cleanly" and never said what happens when
it doesn't.
Code review (0 MUST, 3 SHOULD, 2 MAY):
- test-sdk-go was the only Go suite target with no -timeout, on the
most concurrent code in the repo under -race — a deadlocked SSE
controller would have burned half of CI's 20-minute unit cap before
dumping goroutines. Pinned to 60s, matching test-sdk-go-e2e.
- .testcoverage.yml's go-sdk floor was justified by "75 vs 82.7%
measured; the headroom is the codegen CLI" — staled by this PR's own
e17152f, which added those tests. Now 87.1% (SDK 88.5%, codegen
81.0%), and the comment says why the floor stays a regression floor
rather than a target.
- scripts/cov had three open-coded suite->target hints, two of which
this PR hand-edited for the rename. All three now route through
makeTargetFor, so the next rename touches one map; `ternary` was its
last caller and goes with it. The map gains ts-e2e -> test-e2e, which
is not test-<suite>.
- Two comments still pointed at scripts/coverage.sh, which is
scripts/cov now.
The PR description is stale for the same reasons and is updated separately.
Refs #434
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/src/content/docs/development.md (1)
477-488: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale duplicate
clients/block in the project tree.Lines 477-480 document
clients/withts/and the nestedgo/module. Lines 487-488 repeatclients/with onlyts/. The tree now lists the same directory twice with conflicting content, and the second entry is also placed out of alphabetical order aftertests/.📝 Proposed fix
│ ├── fixtures/ # ClickHouse DDL + config/policy fixtures │ └── sdk/ # E2E specs driven through the TypeScript SDK (Vitest) -├── clients/ # Client SDKs -│ └── ts/ # TypeScript SDK (`@wavehouse/sdk`, pnpm workspace) ├── deployments/CHANGELOG.md (1)
21-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the doc paths in the two Fixed entries.
Line 21 cites
docs/src/content/docs/sdk/go/streaming.md. This PR does not create asdk/go/directory; line 13 states the docs are topic-first, so the Go streaming content lives indocs/src/content/docs/sdk/streaming.mdx. Line 23 citesdocs/src/content/docs/sdk/{reference,queries,streaming}.md, but those pages are.mdx.📝 Proposed fix
-- **Go SDK client-side stream filters compare timestamps as instants, not as text** (`clients/go/stream.go`, `clients/go/stream_test.go`, `docs/src/content/docs/sdk/go/streaming.md`): +- **Go SDK client-side stream filters compare timestamps as instants, not as text** (`clients/go/stream.go`, `clients/go/stream_test.go`, `docs/src/content/docs/sdk/streaming.mdx`):-- **TypeScript SDK documentation corrections found while writing the Go SDK's parity docs** (`docs/src/content/docs/sdk/{reference,queries,streaming}.md`): +- **TypeScript SDK documentation corrections found while writing the Go SDK's parity docs** (`docs/src/content/docs/sdk/{reference,queries,streaming}.mdx`):
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c809ec9-54eb-4356-9582-ce84b01dddf3
📒 Files selected for processing (37)
.claude/commands/cover.md.github/workflows/ci.yml.testcoverage.ymlAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdMakefileREADME.mdclients/go/README.mdclients/go/cmd/wavehouse-codegen/main_test.goclients/go/conformance_test.goclients/go/pipes.goclients/ts/README.mddocs/src/components/Footer.astrodocs/src/config/sidebar.tsdocs/src/content/docs/404.mddocs/src/content/docs/access-control.mdxdocs/src/content/docs/development.mddocs/src/content/docs/getting-started.mddocs/src/content/docs/index.mdxdocs/src/content/docs/pipes.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/admin.mddocs/src/content/docs/sdk/admin.mdxdocs/src/content/docs/sdk/go.mddocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/pipes.mddocs/src/content/docs/sdk/pipes.mdxdocs/src/content/docs/sdk/queries.mddocs/src/content/docs/sdk/queries.mdxdocs/src/content/docs/sdk/reference.mdxdocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/sdk/streaming.mdxdocs/src/content/docs/sdk/typescript.mdxdocs/src/content/docs/why-wavehouse.mdscripts/cov/main.gotests/conformance/conformance_ts.mjs
💤 Files with no reviewable changes (4)
- docs/src/content/docs/sdk/streaming.md
- docs/src/content/docs/sdk/pipes.md
- docs/src/content/docs/sdk/admin.md
- docs/src/content/docs/sdk/queries.md
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
- **Opt a page into the Cloud CTA with `cloudCta` frontmatter**, not by importing the component.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/access-control.mdxdocs/src/content/docs/pipes.mdxdocs/src/content/docs/index.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/pipes.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/admin.mdxdocs/src/content/docs/sdk/reference.mdxdocs/src/content/docs/sdk/queries.mdxdocs/src/content/docs/sdk/typescript.mdxdocs/src/content/docs/sdk/streaming.mdx
- **In MDX, leave a blank line between a JSX tag and a code fence.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/access-control.mdxdocs/src/content/docs/pipes.mdxdocs/src/content/docs/index.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/pipes.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/admin.mdxdocs/src/content/docs/sdk/reference.mdxdocs/src/content/docs/sdk/queries.mdxdocs/src/content/docs/sdk/typescript.mdxdocs/src/content/docs/sdk/streaming.mdx
- **Table-driven tests**: Use `tests := []struct{ name string; ... }` with `t.Run(tt.name, ...)` for test cases.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
clients/go/conformance_test.goclients/go/cmd/wavehouse-codegen/main_test.go
- **Never hard-wrap prose. One paragraph is one line.** No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/404.mddocs/src/content/docs/access-control.mdxdocs/src/content/docs/why-wavehouse.mdCONTRIBUTING.mdREADME.mddocs/src/content/docs/pipes.mdxdocs/src/content/docs/index.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/getting-started.mdclients/ts/README.mdCHANGELOG.mddocs/src/content/docs/sdk/pipes.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/go.mdAGENTS.mddocs/src/content/docs/sdk/admin.mdxdocs/src/content/docs/sdk/reference.mdxdocs/src/content/docs/sdk/queries.mdxdocs/src/content/docs/sdk/typescript.mdxdocs/src/content/docs/development.mddocs/src/content/docs/sdk/streaming.mdxclients/go/README.md
The TypeScript SDK (`@wavehouse/sdk` in `clients/ts/`) and Go SDK (`github.com/Wave-RF/WaveHouse/clients/go`, in `clients/go/`) are both canonical, officially supported clients. Both ship from this repo with full API-tree parity.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/config/sidebar.ts
- **No global state**: Dependencies are passed explicitly (constructor injection).
📄 CodeRabbit inference engine (AGENTS.md)
Files:
clients/go/pipes.goclients/go/conformance_test.goclients/go/cmd/wavehouse-codegen/main_test.goscripts/cov/main.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:
CHANGELOG.mdAGENTS.mddocs/src/content/docs/development.md
📚 Learning: 2026-08-11T21:55:39.391Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/cmd/wavehouse-codegen/main_test.go:55-59
Timestamp: 2026-08-11T21:55:39.391Z
Learning: In Go SDK tests, do not require named t.Run subtests for table-driven test loops when assertion errors already identify the failing input and expected and actual values. Avoid raising style-only findings to add t.Run in these cases.
Applied to files:
clients/go/cmd/wavehouse-codegen/main_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:
clients/go/cmd/wavehouse-codegen/main_test.go
📚 Learning: 2026-08-13T12:17:52.620Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 470
File: docs/src/content/docs/reverse-proxy.mdx:137-144
Timestamp: 2026-08-13T12:17:52.620Z
Learning: For Wave-RF/WaveHouse documentation, verify claims about implementation control flow against the authoritative implementation source (for example, internal/auth/auth.go) rather than relying solely on docs/** content. Documentation may lag behind or paraphrase behavior, so control-flow claims should be confirmed in source code.
Applied to files:
docs/src/content/docs/sdk/admin.mdxdocs/src/content/docs/sdk/reference.mdxdocs/src/content/docs/sdk/queries.mdxdocs/src/content/docs/sdk/typescript.mdxdocs/src/content/docs/sdk/streaming.mdx
🪛 checkmake (0.3.2)
Makefile
[warning] 499-499: Target body for "fix-go" exceeds allowed length of 5 lines (7).
(maxbodylength)
[warning] 828-828: Target body for "test-sdk-go" exceeds allowed length of 5 lines (7).
(maxbodylength)
[warning] 846-846: Target body for "test-sdk-ts" exceeds allowed length of 5 lines (8).
(maxbodylength)
🪛 LanguageTool
docs/src/content/docs/index.mdx
[grammar] ~112-~112: Please add a punctuation mark at the end of paragraph.
Context: ...like a database. Subscribe to it like a socket The client SDKs wrap the whole...
(PUNCTUATION_PARAGRAPH_END)
CHANGELOG.md
[uncategorized] ~13-~13: The official name of this software platform is spelled with a capital “H”.
Context: ...stcoverage.yml, scripts/cov/main.go, .github/workflows/ci.yml, .claude/commands/co...
(GITHUB)
[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...ance/conformance_ts.mjs`) — each riding its own language's SDK target, both run by CI's...
(PRP_OWN)
[style] ~13-~13: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ... added, and each language keeps a setup/caveats page — /sdk/typescript (moved off the...
(CAVEAT)
docs/src/content/docs/sdk/pipes.mdx
[style] ~27-~27: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...clared* as RequestOptions is rejected whether or not it actually carries a limit, since the ...
(WHETHER)
docs/src/content/docs/sdk/index.mdx
[style] ~80-~80: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ...} ``` ## Setup & caveats, per language Installation, client con...
(CAVEAT)
[style] ~82-~82: Since ownership is already implied, this phrasing may be redundant.
Context: ...el are language-specific — each SDK has its own page for them. <LinkCard ...
(PRP_OWN)
docs/src/content/docs/sdk/go.md
[style] ~6-~6: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ...d the (T, error) model." --- Setup and caveats for `github.com/Wave-RF/WaveHouse/clien...
(CAVEAT)
[grammar] ~139-~139: Use a hyphen to join words.
Context: ...uthprovider, marshal errors) are plain wrapped errors, so handle theerrors.As...
(QB_NEW_EN_HYPHEN)
[style] ~166-~166: Consider using the typographical ellipsis character here instead.
Context: ... Any slice batches. Reflection lets []ClickRow{...} take the same NDJSON batch path as `[...
(ELLIPSIS)
AGENTS.md
[uncategorized] ~143-~143: The official name of this software platform is spelled with a capital “H”.
Context: ...me gates CI will run — the CI workflow (.github/workflows/ci.yml) is a job DAG over th...
(GITHUB)
docs/src/content/docs/sdk/reference.mdx
[style] ~43-~43: Since ownership is already implied, this phrasing may be redundant.
Context: ...he returned *StreamController manages its own context and goroutine, closed via `.Clo...
(PRP_OWN)
[grammar] ~52-~52: Use a hyphen to join words.
Context: ... are values. Both raise (or return plain wrapped errors) for caller and environme...
(QB_NEW_EN_HYPHEN)
[style] ~57-~57: Since ownership is already implied, this phrasing may be redundant.
Context: ... or .liveQuery(), described under If your own callback throws below. | Status | Cod...
(PRP_OWN)
[style] ~400-~400: Since ownership is already implied, this phrasing may be redundant.
Context: ...re colocated in clients/go/, which is its own module (clients/go/go.mod), separate ...
(PRP_OWN)
[style] ~413-~413: Since ownership is already implied, this phrasing may be redundant.
Context: ...-e2e orchestrator — wiring it in, with its own coverage gate, is tracked in [#518`](htt...
(PRP_OWN)
docs/src/content/docs/sdk/queries.mdx
[style] ~75-~75: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...f size; bounded-concurrency chunking of very large batches is tracked in [#196](https://gi...
(EN_WEAK_ADJECTIVE)
docs/src/content/docs/sdk/typescript.mdx
[style] ~8-~8: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ...trojs/starlight/components"; Setup and caveats for @wavehouse/sdk, the TypeScript cl...
(CAVEAT)
[style] ~345-~345: 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)
[style] ~380-~380: 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] ~400-~400: Since ownership is already implied, this phrasing may be redundant.
Context: ...row if it is set at all. See Supplying your own fetch for w...
(PRP_OWN)
[style] ~413-~413: 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] ~413-~413: 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)
[style] ~431-~431: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...used with .stream() or .liveQuery() needs a different set: .ok, .status, `.ty...
(EN_REPEATEDWORDS_NEED)
[style] ~464-~464: 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] ~477-~477: 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] ~477-~477: Consider using the typographical ellipsis character here instead.
Context: ...the two aren't structurally assignable. { ...init, dispatcher } as never covers the ...
(ELLIPSIS)
[style] ~477-~477: 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)
docs/src/content/docs/development.md
[style] ~197-~197: Since ownership is already implied, this phrasing may be redundant.
Context: ... both languages. Frontend devs running their own dev server (Vite, Next.js, etc.) can `i...
(PRP_OWN)
[style] ~289-~289: Consider using the typographical ellipsis character here instead.
Context: ...a summary, and every one of them honors ARGS="..." and V=1. Tool versions are pinned i...
(ELLIPSIS)
[style] ~334-~334: Consider using the typographical ellipsis character here instead.
Context: ... module, invisible to the root module's -coverpkg=./..., so its statements can never reach `tm...
(ELLIPSIS)
[style] ~338-~338: Consider using the typographical ellipsis character here instead.
Context: ...suite target plus test-sdk-ts accepts ARGS="..." for pass-through flags (e.g., -run,...
(ELLIPSIS)
[style] ~569-~569: Consider using the typographical ellipsis character here instead.
Context: ...sdk-go-e2e) plus test-sdk-tsacceptsARGS="..."` for pass-through flags; those Go targ...
(ELLIPSIS)
[style] ~569-~569: Consider using the typographical ellipsis character here instead.
Context: ...t-sdk-tsignores. Build targets acceptTAGS="..."` for Go build tags. ## Dependency Man...
(ELLIPSIS)
docs/src/content/docs/sdk/streaming.mdx
[style] ~136-~136: Consider using the typographical ellipsis character here instead.
Context: ... Status are delivered exclusively via .Subscribe(...). The channel closes on terminal error...
(ELLIPSIS)
[style] ~266-~266: Since ownership is already implied, this phrasing may be redundant.
Context: ... seam only — so key on timestamp plus your own row identity if duplicates matter. Rep...
(PRP_OWN)
[style] ~282-~282: Since ownership is already implied, this phrasing may be redundant.
Context: ...ed more of on this path; see [Supplying your own fetch](/sdk/typescript#supplying-your-o...
(PRP_OWN)
[style] ~301-~301: ‘On top of that’ might be wordy. Consider a shorter alternative.
Context: ...ed). ### Client-side stream filtering On top of that, when a query builder carrying filters ...
(EN_WORDINESS_PREMIUM_ON_TOP_OF_THAT)
[typographical] ~332-~332: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...ilterOpset.Where()takes everywhere.OpLike/OpNotLike` use SQL LIKE seman...
(WRB_QUESTION_MARK)
[grammar] ~336-~336: Ensure spelling is correct
Context: ...lly equal row. Both sides are parsed as instants instead. - **Only unambiguous spellings...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~338-~338: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...o it is not treated as a timestamp. - Ordering an instant against a non-instant fails closed. If one side parses as a timestamp and the other does not, OpGt/OpGte/OpLt/OpLte withhold the row rather than falling back to text comparison, which could admit rows the query path excludes. The usual cause is a zone-less filter c...
(TOO_LONG_SENTENCE)
[style] ~343-~343: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...age domain — so a client-side filter on a very large UInt64 can disagree with the server's...
(EN_WEAK_ADJECTIVE)
[style] ~468-~468: Since ownership is already implied, this phrasing may be redundant.
Context: ...ers — treat initial() never firing as its own failure. Where auth rejects and the s...
(PRP_OWN)
[typographical] ~468-~468: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...es auth or the URL, never the backfill. Re-run the fetch then; you never have t...
(WRB_QUESTION_MARK)
[style] ~468-~468: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...ver have to work out which row you hit. Leave it a moment first: events reach a stream from the message queue before the ingest worker lands them in ClickHouse, and its per-table batcher flushes on size or a deadline with the insert still to complete after that (see Ingest pipeline), so an immediate re-fetch can miss the newest rows. Why auth splits the way it does....
(TOO_LONG_SENTENCE)
🔇 Additional comments (23)
clients/go/pipes.go (1)
99-103: LGTM!clients/go/conformance_test.go (1)
115-122: LGTM!Also applies to: 136-214
clients/go/cmd/wavehouse-codegen/main_test.go (1)
193-257: LGTM!Also applies to: 262-308, 310-389, 391-417
tests/conformance/conformance_ts.mjs (1)
84-121: LGTM!clients/go/README.md (1)
1-59: LGTM!clients/ts/README.md (1)
11-11: LGTM!Also applies to: 60-60, 173-173
docs/src/components/Footer.astro (1)
149-149: LGTM!docs/src/config/sidebar.ts (1)
28-44: LGTM!docs/src/content/docs/404.md (1)
48-48: LGTM!docs/src/content/docs/access-control.mdx (1)
454-454: LGTM!docs/src/content/docs/sdk/go.md (1)
2-6: LGTM!Also applies to: 94-94, 109-109, 135-176
docs/src/content/docs/sdk/index.mdx (1)
2-102: LGTM!Also applies to: 122-122
docs/src/content/docs/sdk/pipes.mdx (1)
1-140: LGTM!docs/src/content/docs/development.md (1)
289-291: LGTM!Also applies to: 334-338, 347-350, 530-552, 713-713
docs/src/content/docs/getting-started.md (1)
79-79: LGTM!Also applies to: 108-108
docs/src/content/docs/index.mdx (1)
105-114: LGTM!Also applies to: 215-216
docs/src/content/docs/sdk/queries.mdx (1)
1-651: LGTM!docs/src/content/docs/sdk/reference.mdx (1)
8-46: LGTM!Also applies to: 108-119, 137-242, 246-371, 375-416
docs/src/content/docs/sdk/streaming.mdx (1)
8-286: LGTM!Also applies to: 290-492
docs/src/content/docs/sdk/typescript.mdx (1)
1-567: LGTM!scripts/cov/main.go (1)
72-88: LGTM!Also applies to: 171-180, 246-260, 265-279, 306-306, 328-328, 353-359, 613-628, 987-996
.github/workflows/ci.yml (1)
207-210: LGTM!CHANGELOG.md (1)
13-17: LGTM!
Four threads, all raised against f778e41 and all correct. Two of them are cross-language leaks from the topic-page merge, which is now the second time that restructure has produced this class of bug (the first was `Last-Event-ID` in 23c58e2): nothing type-checks the snippets inside a `<TabItem>`, so a Go identifier surviving into the TypeScript tab builds clean. - admin.mdx's TypeScript tab called `wh.policy.validate(policyDraft)` against a variable it never declared. `policyDraft` exists only in the Go tab. Declared it and passed it to both `set` and `validate`, mirroring the Go tab's shape. - streaming.mdx's Go tab said `Auth` provider errors are retryable as `SSE_ERROR`. clients/go/stream.go:310 returns `SSE_AUTH_ERROR`; `SSE_ERROR` (:270) is the fallback for a failure that isn't already an *Error. The same page two paragraphs earlier, and the error table in reference.mdx, both had it right, so the page contradicted itself. - .claude/commands/cover.md said `make test-all` runs "every suite". It runs test-unit, test-sdk, test-integration, test-e2e and cov — test-sdk is test-sdk-go + test-sdk-ts, so test-sdk-go-e2e is never invoked. Named the exclusion and why it's excluded. - parseArgs and flagValue read os.Args directly, and the test helper mutated the global to drive them, against the `**/*.go` guideline that dependencies are passed explicitly. Both now take argv, and parseArgs takes the resolved WAVEHOUSE_AUTH value, so main owns the two reads of process state. withArgs and its t.Setenv are gone. Not done: CodeRabbit's stated motive for the last one was enabling t.Parallel(). There is no t.Parallel() anywhere in clients/go, so adding it here would invent a convention the package doesn't have. The injection fixes the guideline violation on its own.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/src/content/docs/development.md (1)
477-483: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicate
clients/tree entry.This new
clients/entry duplicates the stale root-levelclients/entry at Lines 487-488. Keep oneclients/section and list both SDKs there.clients/go/cmd/wavehouse-codegen/main.go (5)
96-100: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External · Exploitability: Moderate
Require HTTPS for authenticated remote URLs.
Reject authenticated
http://requests, except where explicitly limited to loopback endpoints, before sending theAuthorizationheader.
294-311: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInjection (CWE-94): Improper Control of Generation of Code ('Code Injection')
Reachability: External · Exploitability: Difficult
Validate schema names before embedding them as Go identifiers.
column.Nameis copied into generated Go source throughpascalCasewithout identifier validation or escaping. Reject names that cannot produce a valid exported Go identifier before callingfmt.Fprintf;format.Sourcedoes not prevent source injection.
79-87: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDecode into the shared schema types
clients/go/types.goalready definesColumn,TableSchema, andSchemasfor/v1/ops/schema. Reuse these types infetchSchemasinstead of maintaining separate wire contracts that can drift.
99-100: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-522): Insufficiently Protected Credentials
Reachability: External · Exploitability: Difficult
Constrain redirects before sending bearer credentials.
When
authis set,fetchSchemassendsAuthorizationthrough the default redirect policy. Go retains this header for same-host or subdomain redirects without scoping the check to scheme or port. Reject redirects that change the scheme, host, or port, or clearAuthorizationon every non-origin hop.Source: MCP tools
199-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject ClickHouse map keys that are not comparable in Go.
chTypeToGoconvertsMap(Array(Map(Float64, String)), String)tomap[[]map[float64]string]string. Go forbids slices as map keys, so the generated source does not compile even thoughformat.Sourceaccepts its syntax. Reject non-comparable key types or generate a slice-of-pairs representation.Source: MCP tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 36d7025e-8f8f-4175-b0c5-534cb7a3c6c5
📒 Files selected for processing (20)
.claude/commands/cover.mdAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdclients/go/README.mdclients/go/cmd/wavehouse-codegen/main.goclients/go/cmd/wavehouse-codegen/main_test.goclients/ts/README.mddocs/src/config/sidebar.tsdocs/src/content/docs/development.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/admin.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/pipes.mdxdocs/src/content/docs/sdk/queries.mdxdocs/src/content/docs/sdk/reference.mdxdocs/src/content/docs/sdk/setup/go.mddocs/src/content/docs/sdk/setup/index.mddocs/src/content/docs/sdk/setup/typescript.mdxdocs/src/content/docs/sdk/streaming.mdx
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
In MDX, leave a blank line between a JSX tag and a code fence.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/queries.mdxdocs/src/content/docs/sdk/reference.mdxdocs/src/content/docs/sdk/pipes.mdxdocs/src/content/docs/sdk/admin.mdxdocs/src/content/docs/sdk/streaming.mdxdocs/src/content/docs/sdk/setup/typescript.mdx
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T17:59:28.171Z
Learning: Never force-push or rebase a PR branch
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T17:59:28.171Z
Learning: Never hand-write markers or `--no-verify`
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T17:59:28.171Z
Learning: Every new function should have corresponding test cases.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T17:59:28.171Z
Learning: Never hand-write `®` or `™` in prose.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T17:59:28.171Z
Learning: Agents must create PRs with `gh pr create --draft`.
📚 Learning: 2026-08-13T12:17:52.620Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 470
File: docs/src/content/docs/reverse-proxy.mdx:137-144
Timestamp: 2026-08-13T12:17:52.620Z
Learning: For Wave-RF/WaveHouse documentation, verify claims about implementation control flow against the authoritative implementation source (for example, internal/auth/auth.go) rather than relying solely on docs/** content. Documentation may lag behind or paraphrase behavior, so control-flow claims should be confirmed in source code.
Applied to files:
docs/src/content/docs/sdk/streaming.mdx
📚 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/sdk/queries.mdx
[style] ~75-~75: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...f size; bounded-concurrency chunking of very large batches is tracked in [#196](https://gi...
(EN_WEAK_ADJECTIVE)
docs/src/content/docs/sdk/reference.mdx
[grammar] ~52-~52: Use a hyphen to join words.
Context: ... are values. Both raise (or return plain wrapped errors) for caller and environme...
(QB_NEW_EN_HYPHEN)
[style] ~57-~57: Since ownership is already implied, this phrasing may be redundant.
Context: ... or .liveQuery(), described under If your own callback throws below. | Status | Cod...
(PRP_OWN)
[style] ~90-~90: Consider an alternative for the overused word “exactly”.
Context: ...roxy. That silent-downgrade behavior is exactly why auth is re-read on every connecti...
(EXACTLY_PRECISELY)
[style] ~94-~94: Since ownership is already implied, this phrasing may be redundant.
Context: ...cts rather than feeding it again. If your own callback throws. For anything deliver...
(PRP_OWN)
[style] ~94-~94: Since ownership is already implied, this phrasing may be redundant.
Context: ...r` callback, so a handler that swallows its own failures fails silently. **Wrap your h...
(PRP_OWN)
[style] ~103-~103: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...aller-side failure that isn't terminal. A rejecting auth callback propagates out of a REST call, but on a stream, where auth is invoked on every connection attempt rather than once per stream, a token endpoint having a bad minute is treated as transient and retried, rather than tearing down a stream that is otherwise healthy. T...
(TOO_LONG_SENTENCE)
[style] ~413-~413: Since ownership is already implied, this phrasing may be redundant.
Context: ...t-e2e orchestrator. Wiring it in, with its own coverage gate, is tracked in [#518`](htt...
(PRP_OWN)
docs/src/content/docs/sdk/pipes.mdx
[style] ~27-~27: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...clared* as RequestOptions is rejected whether or not it actually carries a limit, since the ...
(WHETHER)
docs/src/content/docs/sdk/setup/go.md
[style] ~6-~6: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ...d the (T, error) model." --- Setup and caveats for `github.com/Wave-RF/WaveHouse/clien...
(CAVEAT)
docs/src/content/docs/sdk/streaming.mdx
[style] ~266-~266: Since ownership is already implied, this phrasing may be redundant.
Context: ... seam only), so key on timestamp plus your own row identity if duplicates matter. Rep...
(PRP_OWN)
[style] ~282-~282: Since ownership is already implied, this phrasing may be redundant.
Context: ...ed more of on this path; see [Supplying your own fetch](/sdk/setup/typescript#supplying-...
(PRP_OWN)
[typographical] ~332-~332: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...ilterOpset.Where()takes everywhere.OpLike/OpNotLike` use SQL LIKE seman...
(WRB_QUESTION_MARK)
[grammar] ~336-~336: Ensure spelling is correct
Context: ...lly equal row. Both sides are parsed as instants instead. - **Only unambiguous spellings...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~338-~338: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...o it is not treated as a timestamp. - Ordering an instant against a non-instant fails closed. If one side parses as a timestamp and the other does not, OpGt/OpGte/OpLt/OpLte withhold the row rather than falling back to text comparison, which could admit rows the query path excludes. The usual cause is a zone-less filter c...
(TOO_LONG_SENTENCE)
[style] ~343-~343: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...rage domain, so a client-side filter on a very large UInt64 can disagree with the server's...
(EN_WEAK_ADJECTIVE)
[grammar] ~449-~449: Ensure spelling is correct
Context: ...at omits received_timestamp skips the pass entirely. Tracked in [#449](https://git...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~468-~468: Since ownership is already implied, this phrasing may be redundant.
Context: ...ters, treat initial() never firing as its own failure. Where auth rejects and the s...
(PRP_OWN)
[typographical] ~468-~468: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...es auth or the URL, never the backfill. Re-run the fetch then; you never have t...
(WRB_QUESTION_MARK)
[style] ~468-~468: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...ver have to work out which row you hit. Leave it a moment first: events reach a stream from the message queue before the ingest worker lands them in ClickHouse, and its per-table batcher flushes on size or a deadline with the insert still to complete after that (see Ingest pipeline), so an immediate re-fetch can miss the newest rows. Why auth splits the way it does....
(TOO_LONG_SENTENCE)
docs/src/content/docs/sdk/setup/typescript.mdx
[style] ~8-~8: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ...trojs/starlight/components"; Setup and caveats for @wavehouse/sdk, the TypeScript cl...
(CAVEAT)
CHANGELOG.md
[uncategorized] ~13-~13: The official name of this software platform is spelled with a capital “H”.
Context: ...stcoverage.yml, scripts/cov/main.go, .github/workflows/ci.yml, .claude/commands/co...
(GITHUB)
[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...ance/conformance_ts.mjs`) — each riding its own language's SDK target, both run by CI's...
(PRP_OWN)
[style] ~13-~13: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ... added, and each language keeps a setup/caveats page — /sdk/typescript (moved off the...
(CAVEAT)
[style] ~17-~17: Since ownership is already implied, this phrasing may be redundant.
Context: ...ter,LiveDemo}.astro`): the site tracked its own CTAs but nothing a reader did on the wa...
(PRP_OWN)
[style] ~17-~17: Since ownership is already implied, this phrasing may be redundant.
Context: ...docs area without each tracker carrying its own copy; it's stamped at capture time by a...
(PRP_OWN)
[style] ~17-~17: Since ownership is already implied, this phrasing may be redundant.
Context: ...ressive Code, and the 404 route all own their own markup — some of it created after page ...
(PRP_OWN)
[typographical] ~21-~21: Consider using an em dash in dialogues and enumerations.
Context: - **Go SDK client-side stream filters com...
(DASH_RULE)
[style] ~21-~21: You can shorten this phrase to improve clarity and avoid wordiness.
Context: ...elow* the constant, so OpGte withheld a row that was chronologically equal. Both sides now parse as instants, mirr...
(NNS_THAT_ARE_JJ)
docs/src/content/docs/development.md
[style] ~197-~197: Since ownership is already implied, this phrasing may be redundant.
Context: ... both languages. Frontend devs running their own dev server (Vite, Next.js, etc.) can `i...
(PRP_OWN)
🔇 Additional comments (12)
clients/go/cmd/wavehouse-codegen/main_test.go (1)
197-197: LGTM!Also applies to: 239-239, 255-255
docs/src/config/sidebar.ts (1)
28-47: LGTM!docs/src/content/docs/development.md (1)
21-21: LGTM!Also applies to: 44-44, 189-197, 289-338, 347-350, 363-365, 530-556, 569-569, 613-615, 649-652, 713-713
docs/src/content/docs/sdk/setup/index.md (1)
1-13: LGTM!docs/src/content/docs/sdk/setup/typescript.mdx (1)
8-8: LGTM!Also applies to: 565-565
docs/src/content/docs/sdk/streaming.mdx (1)
16-16: LGTM!Also applies to: 64-68, 90-90, 132-132, 174-174, 188-195, 225-225, 243-243, 256-256, 266-280, 282-282, 287-298, 316-318, 332-343, 353-353, 444-472, 482-489
AGENTS.md (1)
363-388: LGTM!CONTRIBUTING.md (1)
42-48: LGTM!Also applies to: 93-93
.claude/commands/cover.md (1)
3-3: LGTM!Also applies to: 16-20
clients/go/cmd/wavehouse-codegen/main.go (3)
29-75: LGTM!
212-226: LGTM!
254-260: LGTM!Also applies to: 319-356
CodeRabbit's 18:11Z review on ce066c9: two inline threads plus six findings GitHub could not post inline, which live only in the review body and never show up as unresolved threads. Seven of the eight are taken here; the type dedup is deliberately not (see below). Code injection via schema names (CWE-94) was real and demonstrable, not theoretical. pascalCase splits only on space/_/-/., so tabs and newlines survive it, and a keyword inside a part keeps its lowercase; the result is written unquoted and format.Source only *parses*. A column named with a tab-separated payload closed the struct and appended a top-level `func Pwn() string { return "owned" }` that format.Source accepted with no error. Type and field identifiers are now validated before emission, which also covers the backtick case (a backtick in a name closes the struct tag's raw string literal). The regression test uses that exact payload. --package is validated on the same path -- not a trust boundary, but the same class of broken output. - chTypeToGo mapped Map(Array(...), String) to `map[[]T]V`, which parses and then fails to compile in the caller's build. ClickHouse restricts Map keys to comparable types so a real server shouldn't send this, but format.Source can't catch it. Falls back to `any`. - fetchSchemas sent the bearer token over whatever scheme the URL carried (CWE-319), and left redirects to net/http, which drops Authorization only on a host change and ignores scheme and port (CWE-522) -- so an https->http hop on the same host handed the token over in cleartext. Credentials now require https or loopback, and any redirect changing scheme/host/port is refused. This matches the stance the SDK already takes on streams (SSE_REDIRECT). Documented in reference.mdx, since it's user-visible CLI behavior; both documented invocations use http://localhost and are unaffected. - CHANGELOG still gave the setup routes as /sdk/typescript and /sdk/go after f778e41 moved them under /sdk/setup/, contradicting the paths listed earlier in its own entry, and claimed all four SDK targets use gotestsum and honor V=1 -- test-sdk-ts is vitest. - CONTRIBUTING said the topic pages carry "one <Tabs> block per topic". They carry one per section (queries.mdx has 19), which was deliberate so the ToC doesn't list every heading twice. - development.md had two `clients/` entries in the file tree; I added one without removing the old one, which listed only ts/. Folded the surviving detail (pnpm workspace) into the current entry. Not done: reusing Column/TableSchema from clients/go/types.go in the CLI. That makes cmd/ depend on the SDK package, which is a design change rather than a fix, and this PR is already large. go-sdk coverage 87.1% -> 87.4%.
|
@coderabbitai review |
✅ Action performedReview finished.
|
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: 43ed5c70-e4c6-4cfb-a043-4dc4c54b3298
📒 Files selected for processing (6)
CHANGELOG.mdCONTRIBUTING.mdclients/go/cmd/wavehouse-codegen/main.goclients/go/cmd/wavehouse-codegen/main_test.godocs/src/content/docs/development.mddocs/src/content/docs/sdk/reference.mdx
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
In MDX, leave a blank line between a JSX tag and a code fence.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/sdk/reference.mdx
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Never force-push or rebase a PR branch
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Never hand-write markers or `--no-verify`
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Return errors, don't panic.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Every new function should have corresponding test cases.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Never hand-write `®` or `™` in prose.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Never hand-write `utm_*` params or `rel` on a link to `wavehouse.cloud` or `wave-rf.com`.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Never sit two large diagrams side-by-side.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Add tests.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Add a matching `area/<pkg>` repo label
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Use table-driven tests with `t.Run(tt.name, ...)` for multiple scenarios.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Run `make test`
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Agents must create PRs with `gh pr create --draft`.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-27T20:15:47.101Z
Learning: Don't
📚 Learning: 2026-08-13T12:17:52.620Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 470
File: docs/src/content/docs/reverse-proxy.mdx:137-144
Timestamp: 2026-08-13T12:17:52.620Z
Learning: For Wave-RF/WaveHouse documentation, verify claims about implementation control flow against the authoritative implementation source (for example, internal/auth/auth.go) rather than relying solely on docs/** content. Documentation may lag behind or paraphrase behavior, so control-flow claims should be confirmed in source code.
Applied to files:
docs/src/content/docs/sdk/reference.mdx
📚 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
CHANGELOG.md
[uncategorized] ~13-~13: The official name of this software platform is spelled with a capital “H”.
Context: ...stcoverage.yml, scripts/cov/main.go, .github/workflows/ci.yml, .claude/commands/co...
(GITHUB)
[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...ance/conformance_ts.mjs`) — each riding its own language's SDK target, both run by CI's...
(PRP_OWN)
[style] ~13-~13: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ... added, and each language keeps a setup/caveats page under /sdk/setup/ — `/sdk/setup/...
(CAVEAT)
🔇 Additional comments (4)
CHANGELOG.md (1)
13-13: LGTM!Also applies to: 21-23
CONTRIBUTING.md (1)
42-42: LGTM!Also applies to: 93-93
docs/src/content/docs/development.md (1)
21-21: LGTM!Also applies to: 44-44, 189-197, 289-338, 347-350, 363-365, 477-486, 528-554, 567-567, 611-613, 647-650, 711-711
docs/src/content/docs/sdk/reference.mdx (1)
1-133: LGTM!Also applies to: 137-242, 246-371, 375-418
CodeRabbit's 20:24Z review on ff35c8b. All three findings hold. The map-key guard added in ff35c8b tested the wrong property. Go comparability keeps `map[[]T]V` out of the output, but `map[float64]V` and `map[bool]V` are comparable, compile, and then fail at Marshal and Unmarshal: encoding/json supports only string-kinded and integer-kinded keys plus encoding.TextMarshaler implementors. That is worse than the non-compiling case, because it surfaces at runtime in the caller's service rather than in their build. chTypeToGo returns no TextMarshaler, so the check is now a closed allow-list of its own outputs, which also covers `*T` from Nullable — comparable, and equally unmarshalable. TestJSONMapKeyMatchesEncodingJSON pins that allow-list to what encoding/json actually does rather than to a reading of its docs: it marshals a real map for each candidate key type and asserts the outcome agrees with jsonMapKey. - validGoIdent accepted "_" and keywords like "type". Neither can arise for a type or field name (pascalCase capitalizes the first rune of each part, and strips underscores entirely), but --package is passed through to the `package` clause verbatim, so `--package type` wrote a file that could not parse. Replaced the hand-rolled loop with token.IsIdentifier, which excludes keywords, plus an explicit "_" rejection since the stdlib check admits it. - CONTRIBUTING listed the two setup pages as `sdk/setup/typescript.mdx` and `sdk/setup/go.md` in a bullet whose other entries are repo-root paths, so neither resolved from the repository root. AGENTS.md is unaffected: it names the routes, not the files. go-sdk coverage holds at 87.4%.
The TypeScript codegen it was built for parity with is itself almost
untested and unused, so matching it bought a second copy of a feature
nobody runs. It was also the most defect-dense code on the branch: the
last three CodeRabbit rounds were all codegen findings (identifier
injection, non-comparable map keys, JSON-incompatible map keys, cleartext
credentials, redirect credential leakage), and it was the lowest-coverage
package in the module.
Removing it also removes a security surface that only existed because the
CLI compiled untrusted schema names into Go source the caller then built.
- Deleted clients/go/cmd/ entirely (main.go, main_test.go).
- /sdk/reference's "Codegen CLI" section becomes "Row types". The
TypeScript tab keeps the CLI; the Go tab documents hand-written row
structs and keeps the ClickHouse-to-Go field table, which is still
load-bearing without codegen and has no other home. It gains the
encoding/json map-key constraint learned in 607d000: `map[K]V` is
only usable where K is a string or integer type.
- Anchor moved to #row-types; both inbound links updated. The TS setup
page still points at the codegen CLI, which still exists.
- Swept the "codegen" claim out of the language-neutral surfaces that
asserted it for both SDKs: /sdk, the docs landing page, README,
why-wavehouse's comparison table, getting-started, AGENTS.md §SDK
Sync, and the CHANGELOG entry. TypeScript's own codegen docs are
untouched.
go-sdk coverage 87.4% -> 88.5%: the codegen CLI was the package holding
the module's number down. Branch diff 11,667 -> 10,580 lines.
…rects
Two ultrareview findings, both confirmed against the code.
MaxRetries was an int, so its zero value was indistinguishable from an
explicit 0, and the `>= 0` gate meant any caller who built ClientOptions
for another reason ran with retries off. Headers is exactly that caller:
the option the docs push operators toward for X-Operator-Key. Three of
our own tests were silently running with retries disabled and passed only
because they never reach the retry path.
The gate could not simply become `> 0` — http_test.go and
conformance_test.go set MaxRetries: 0 deliberately, and that gate would
have handed them 2. MaxRetries is now *int: nil means unset, and Ptr(0)
is a choice. Nothing is released, so the shape is free to change now.
Two decisions beyond the finding:
- Added `func Ptr[T any](v T) *T`. Without it every caller needs a
throwaway variable, and it retires the `func strPtr(s string) *string`
snippet the policy docs told readers to hand-roll for PolicyFilter.
- Negative values clamp to 0. The old `>= 0` gate quietly turned a
negative into the default; a pointer removes that, and an unclamped
-1 makes maxAttempts 0, skipping the request and returning nil error.
Second finding: doRequest had no redirect guard, while stream.go has
refused 3xx on credentialed connects since it was written. net/http
strips only its own four sensitive headers across hosts and forwards
configured ones verbatim, so a 302 handed X-Operator-Key to whatever
Location named, up to 10 hops. Same threat model, same SDK, opposite
postures. doRequest now installs the same CheckRedirect on the same
condition, on a copied client so a caller's own CheckRedirect survives,
and a refused 3xx returns a non-retryable REDIRECT rather than a bare
HTTP_302 — the REST analogue of SSE_REDIRECT.
TestRESTRedirectsWithCredentials pins all three cases and asserts the
redirect target received nothing. It also pins the scoping: an
unauthenticated request still follows redirects.
Docs: setup/go.md's ClientOptions table, the REDIRECT row in the
reference error table, admin.mdx's pointer advice, and the CHANGELOG.
setup/go.md also carried a :::caution titled "Options opts you out of the
default, not just in", which documented this bug as intended behavior and
told readers to set MaxRetries explicitly. Deleted: it is now false.
go-sdk coverage 88.5% -> 88.8%.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 9
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 783d3067-8235-40c2-971e-91839d979bba
📒 Files selected for processing (69)
.claude/commands/cover.md.github/dependabot.yml.github/labeler.yml.github/workflows/ci.yml.testcoverage.ymlAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdMakefileREADME.mdbiome.jsonclients/go/README.mdclients/go/client_test.goclients/go/conformance_test.goclients/go/dlq.goclients/go/e2e_test.goclients/go/errors.goclients/go/errors_test.goclients/go/example_test.goclients/go/go.modclients/go/http.goclients/go/http_test.goclients/go/live_query.goclients/go/live_query_test.goclients/go/namespaces_test.goclients/go/pipes.goclients/go/policy.goclients/go/query_builder.goclients/go/query_builder_test.goclients/go/schema.goclients/go/stream.goclients/go/stream_test.goclients/go/sys.goclients/go/table.goclients/go/table_test.goclients/go/testdata/wire_cases.jsonclients/go/types.goclients/go/wavehouse.goclients/ts/README.mddocs/src/components/Footer.astrodocs/src/config/sidebar.tsdocs/src/content/docs/404.mddocs/src/content/docs/access-control.mdxdocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/claude-code.mddocs/src/content/docs/deployment.mddocs/src/content/docs/development.mddocs/src/content/docs/getting-started.mddocs/src/content/docs/index.mdxdocs/src/content/docs/pipes.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/admin.mddocs/src/content/docs/sdk/admin.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/pipes.mddocs/src/content/docs/sdk/pipes.mdxdocs/src/content/docs/sdk/queries.mddocs/src/content/docs/sdk/queries.mdxdocs/src/content/docs/sdk/reference.mddocs/src/content/docs/sdk/reference.mdxdocs/src/content/docs/sdk/setup/go.mddocs/src/content/docs/sdk/setup/index.mddocs/src/content/docs/sdk/setup/typescript.mdxdocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/sdk/streaming.mdxdocs/src/content/docs/why-wavehouse.mdscripts/cov/main.gotests/conformance/conformance_ts.mjs
💤 Files with no reviewable changes (5)
- docs/src/content/docs/sdk/pipes.md
- docs/src/content/docs/sdk/admin.md
- docs/src/content/docs/sdk/streaming.md
- docs/src/content/docs/sdk/queries.md
- docs/src/content/docs/sdk/reference.md
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
In MDX, leave a blank line between a JSX tag and a code fence.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/index.mdxdocs/src/content/docs/sdk/pipes.mdxdocs/src/content/docs/access-control.mdxdocs/src/content/docs/pipes.mdxdocs/src/content/docs/sdk/admin.mdxdocs/src/content/docs/sdk/queries.mdxdocs/src/content/docs/sdk/setup/typescript.mdxdocs/src/content/docs/sdk/streaming.mdxdocs/src/content/docs/sdk/reference.mdxdocs/src/content/docs/sdk/index.mdx
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-28T00:18:12.057Z
Learning: Never force-push or rebase a PR branch
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-28T00:18:12.057Z
Learning: Never hand-write markers or `--no-verify`
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-28T00:18:12.057Z
Learning: Don't reintroduce cookie auth or `Allow-Credentials` without a design discussion
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-28T00:18:12.057Z
Learning: Point k8s at `/livez`/`/readyz`, SDK/online-checks at `/v1/health`, never the deprecated aliases.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-28T00:18:12.057Z
Learning: Return errors, don't panic.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-28T00:18:12.057Z
Learning: Comment the *why*, not the *what*.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-28T00:18:12.057Z
Learning: Every new function should have corresponding test cases.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-28T00:18:12.057Z
Learning: Use table-driven tests with `t.Run(tt.name, ...)` for multiple scenarios.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-28T00:18:12.057Z
Learning: Never hand-write `utm_*` params or `rel` on a link to `wavehouse.cloud` or `wave-rf.com`.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-28T00:18:12.057Z
Learning: Never sit two large diagrams side-by-side.
📚 Learning: 2026-08-13T12:17:52.620Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 470
File: docs/src/content/docs/reverse-proxy.mdx:137-144
Timestamp: 2026-08-13T12:17:52.620Z
Learning: For Wave-RF/WaveHouse documentation, verify claims about implementation control flow against the authoritative implementation source (for example, internal/auth/auth.go) rather than relying solely on docs/** content. Documentation may lag behind or paraphrase behavior, so control-flow claims should be confirmed in source code.
Applied to files:
docs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/pipes.mdxdocs/src/content/docs/sdk/admin.mdxdocs/src/content/docs/sdk/queries.mdxdocs/src/content/docs/sdk/setup/typescript.mdxdocs/src/content/docs/sdk/streaming.mdxdocs/src/content/docs/sdk/reference.mdx
📚 Learning: 2026-08-11T21:55:53.726Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/stream_test.go:159-176
Timestamp: 2026-08-11T21:55:53.726Z
Learning: For Go files in this repository, do not report direct type assertions solely because the `forcetypeassert` rule is commented out in `.golangci.yml`. Only flag a type assertion when there is an independent correctness, safety, or maintainability issue.
Applied to files:
clients/go/query_builder.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:
clients/go/conformance_test.goclients/go/namespaces_test.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:
AGENTS.mdCHANGELOG.md
🪛 ast-grep (0.45.2)
scripts/cov/main.go
[warning] 355-356: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: fmt.Printf(" %s%-13s%s %s %s(separate gate; not in merge above)%s\n",
cyan, s+":", reset, pct, yellow, reset)
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🪛 checkmake (0.3.2)
Makefile
[warning] 499-499: Target body for "fix-go" exceeds allowed length of 5 lines (7).
(maxbodylength)
[warning] 828-828: Target body for "test-sdk-go" exceeds allowed length of 5 lines (7).
(maxbodylength)
[warning] 846-846: Target body for "test-sdk-ts" exceeds allowed length of 5 lines (8).
(maxbodylength)
🪛 golangci-lint (2.12.2)
clients/go/query_builder_test.go
[error] 15-15: Error return value of (*encoding/json.Encoder).Encode is not checked: unsafe type any found
(errchkjson)
[error] 200-200: type assertion must be checked
(forcetypeassert)
clients/go/http_test.go
[medium] 484-484: G710: Open redirect via taint analysis
(gosec)
🪛 LanguageTool
docs/src/content/docs/claude-code.md
[uncategorized] ~84-~84: The official name of this software platform is spelled with a capital “H”.
Context: ...t changed but whose docs didn't), using .github/prompts/docs-review.md over the `scrip...
(GITHUB)
docs/src/content/docs/index.mdx
[grammar] ~112-~112: Please add a punctuation mark at the end of paragraph.
Context: ...like a database. Subscribe to it like a socket The client SDKs wrap the whole...
(PUNCTUATION_PARAGRAPH_END)
docs/src/content/docs/sdk/pipes.mdx
[style] ~27-~27: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...clared* as RequestOptions is rejected whether or not it actually carries a limit, since the ...
(WHETHER)
docs/src/content/docs/development.md
[style] ~197-~197: Since ownership is already implied, this phrasing may be redundant.
Context: ... both languages. Frontend devs running their own dev server (Vite, Next.js, etc.) can `i...
(PRP_OWN)
[style] ~289-~289: Consider using the typographical ellipsis character here instead.
Context: ...a summary, and every one of them honors ARGS="..." and V=1. Tool versions are pinned i...
(ELLIPSIS)
[style] ~334-~334: Consider using the typographical ellipsis character here instead.
Context: ... module, invisible to the root module's -coverpkg=./..., so its statements can never reach `tm...
(ELLIPSIS)
[style] ~338-~338: Consider using the typographical ellipsis character here instead.
Context: ...suite target plus test-sdk-ts accepts ARGS="..." for pass-through flags (e.g., -run,...
(ELLIPSIS)
[style] ~567-~567: Consider using the typographical ellipsis character here instead.
Context: ...sdk-go-e2e) plus test-sdk-tsacceptsARGS="..."` for pass-through flags; those Go targ...
(ELLIPSIS)
[style] ~567-~567: Consider using the typographical ellipsis character here instead.
Context: ...t-sdk-tsignores. Build targets acceptTAGS="..."` for Go build tags. ## Dependency Man...
(ELLIPSIS)
[uncategorized] ~647-~647: The official name of this software platform is spelled with a capital “H”.
Context: ... grouped into the categories defined in [.github/release.yml](https://github.com/Wave-R...
(GITHUB)
[uncategorized] ~647-~647: The official name of this software platform is spelled with a capital “H”.
Context: ...ease.yml). Grouping is by PR label: github_actions / documentation are applied ...
(GITHUB)
[style] ~647-~647: Since ownership is already implied, this phrasing may be redundant.
Context: ...github_actions, documentation` — mark our own PRs too; our CI work gets its own "CI &...
(PRP_OWN)
[uncategorized] ~647-~647: The official name of this software platform is spelled with a capital “H”.
Context: ...lf (dependencies, javascript, go, github_actions; javascript is in neither `l...
(GITHUB)
[style] ~649-~649: Since ownership is already implied, this phrasing may be redundant.
Context: .../beta/rc/next` (prerelease), plus its own GitHub Release. - Go SDK — nothing ...
(PRP_OWN)
docs/src/content/docs/sdk/admin.mdx
[style] ~117-~117: Consider using the typographical ellipsis character here instead.
Context: ...e the tenantFilter variable above, or wavehouse.Ptr("..."). --- ## Dead...
(ELLIPSIS)
docs/src/content/docs/sdk/queries.mdx
[style] ~75-~75: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...f size; bounded-concurrency chunking of very large batches is tracked in [#196](https://gi...
(EN_WEAK_ADJECTIVE)
AGENTS.md
[style] ~128-~128: Consider using the typographical ellipsis character here instead.
Context: ...module — invisible to the root module's -coverpkg=./..., so it is gated on its own `suites.go-...
(ELLIPSIS)
[uncategorized] ~128-~128: The official name of this software platform is spelled with a capital “H”.
Context: ... comments via GitHub Code Quality — see .github/workflows/README.md "Coverage publishi...
(GITHUB)
[uncategorized] ~143-~143: The official name of this software platform is spelled with a capital “H”.
Context: ...me gates CI will run — the CI workflow (.github/workflows/ci.yml) is a job DAG over th...
(GITHUB)
[uncategorized] ~297-~297: The official name of this software platform is spelled with a capital “H”.
Context: ...acked .md/.mdx EXCEPT .claude/**, .github/**, CHANGELOG.md, AGENTS.md, `CLAU...
(GITHUB)
docs/src/content/docs/sdk/setup/typescript.mdx
[style] ~8-~8: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ...trojs/starlight/components"; Setup and caveats for @wavehouse/sdk, the TypeScript cl...
(CAVEAT)
[style] ~345-~345: 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)
[style] ~380-~380: 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] ~400-~400: Since ownership is already implied, this phrasing may be redundant.
Context: ...row if it is set at all. See Supplying your own fetch for w...
(PRP_OWN)
[style] ~413-~413: 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] ~413-~413: 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)
[style] ~431-~431: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...used with .stream() or .liveQuery() needs a different set: .ok, .status, `.ty...
(EN_REPEATEDWORDS_NEED)
[style] ~464-~464: 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] ~477-~477: 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] ~477-~477: Consider using the typographical ellipsis character here instead.
Context: ...the two aren't structurally assignable. { ...init, dispatcher } as never covers the ...
(ELLIPSIS)
[style] ~477-~477: 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)
docs/src/content/docs/sdk/streaming.mdx
[style] ~266-~266: Since ownership is already implied, this phrasing may be redundant.
Context: ... seam only), so key on timestamp plus your own row identity if duplicates matter. Rep...
(PRP_OWN)
[style] ~282-~282: Since ownership is already implied, this phrasing may be redundant.
Context: ...ed more of on this path; see [Supplying your own fetch](/sdk/setup/typescript#supplying-...
(PRP_OWN)
[style] ~301-~301: ‘On top of that’ might be wordy. Consider a shorter alternative.
Context: ...ed). ### Client-side stream filtering On top of that, when a query builder carrying filters ...
(EN_WORDINESS_PREMIUM_ON_TOP_OF_THAT)
[typographical] ~332-~332: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...ilterOpset.Where()takes everywhere.OpLike/OpNotLike` use SQL LIKE seman...
(WRB_QUESTION_MARK)
[grammar] ~336-~336: Ensure spelling is correct
Context: ...lly equal row. Both sides are parsed as instants instead. - **Only unambiguous spellings...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~338-~338: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...o it is not treated as a timestamp. - Ordering an instant against a non-instant fails closed. If one side parses as a timestamp and the other does not, OpGt/OpGte/OpLt/OpLte withhold the row rather than falling back to text comparison, which could admit rows the query path excludes. The usual cause is a zone-less filter c...
(TOO_LONG_SENTENCE)
[style] ~343-~343: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...rage domain, so a client-side filter on a very large UInt64 can disagree with the server's...
(EN_WEAK_ADJECTIVE)
[style] ~468-~468: Since ownership is already implied, this phrasing may be redundant.
Context: ...ters, treat initial() never firing as its own failure. Where auth rejects and the s...
(PRP_OWN)
[typographical] ~468-~468: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...es auth or the URL, never the backfill. Re-run the fetch then; you never have t...
(WRB_QUESTION_MARK)
[style] ~468-~468: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...ver have to work out which row you hit. Leave it a moment first: events reach a stream from the message queue before the ingest worker lands them in ClickHouse, and its per-table batcher flushes on size or a deadline with the insert still to complete after that (see Ingest pipeline), so an immediate re-fetch can miss the newest rows. Why auth splits the way it does....
(TOO_LONG_SENTENCE)
docs/src/content/docs/sdk/reference.mdx
[style] ~43-~43: Since ownership is already implied, this phrasing may be redundant.
Context: ...he returned *StreamController manages its own context and goroutine, closed via `.Clo...
(PRP_OWN)
[grammar] ~52-~52: Use a hyphen to join words.
Context: ... are values. Both raise (or return plain wrapped errors) for caller and environme...
(QB_NEW_EN_HYPHEN)
[style] ~57-~57: Since ownership is already implied, this phrasing may be redundant.
Context: ... or .liveQuery(), described under If your own callback throws below. | Status | Cod...
(PRP_OWN)
[style] ~69-~69: A comma is missing here.
Context: ...OR| No | Stream could not be started (e.g. a non-absolutebaseURL) | | 0 | SSE_...
(EG_NO_COMMA)
[style] ~90-~90: Consider an alternative for the overused word “exactly”.
Context: ...roxy. That silent-downgrade behavior is exactly why auth is re-read on every connecti...
(EXACTLY_PRECISELY)
[style] ~94-~94: Since ownership is already implied, this phrasing may be redundant.
Context: ...cts rather than feeding it again. If your own callback throws. For anything deliver...
(PRP_OWN)
[style] ~94-~94: Since ownership is already implied, this phrasing may be redundant.
Context: ...r` callback, so a handler that swallows its own failures fails silently. **Wrap your h...
(PRP_OWN)
[style] ~103-~103: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...aller-side failure that isn't terminal. A rejecting auth callback propagates out of a REST call, but on a stream, where auth is invoked on every connection attempt rather than once per stream, a token endpoint having a bad minute is treated as transient and retried, rather than tearing down a stream that is otherwise healthy. T...
(TOO_LONG_SENTENCE)
[style] ~373-~373: Since ownership is already implied, this phrasing may be redundant.
Context: ...re colocated in clients/go/, which is its own module (clients/go/go.mod), separate ...
(PRP_OWN)
[style] ~386-~386: Since ownership is already implied, this phrasing may be redundant.
Context: ...t-e2e orchestrator. Wiring it in, with its own coverage gate, is tracked in [#518`](htt...
(PRP_OWN)
CONTRIBUTING.md
[style] ~44-~44: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...h the //go:build integration tag. 3. Update documentation if your change affects: - API endpoints → update docs/src/content/docs/api.md - Configuration options → update docs/src/content/docs/configuration.mdx - Deployment → update docs/src/content/docs/deployment.md - Architecture → update docs/src/content/docs/architecture.md - Client SDK surface → update both SDKs (clients/ts/src/, clients/go/), the shared topic pages under docs/src/content/docs/sdk/ (a <Tabs syncKey="lang"> block per language-specific section, with one <TabItem> per language) plus the per-language setup pages docs/src/content/docs/sdk/setup/typescript.mdx / docs/src/content/docs/sdk/setup/go.md, and the shared wire fixture clients/go/testdata/wire_cases.json; see AGENTS.md §SDK Sync 4. Follow the commit message format (s...
(TOO_LONG_SENTENCE)
CHANGELOG.md
[uncategorized] ~13-~13: The official name of this software platform is spelled with a capital “H”.
Context: ...stcoverage.yml, scripts/cov/main.go, .github/workflows/ci.yml, .claude/commands/co...
(GITHUB)
[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...E alike, because net/http strips only its own four sensitive headers across hosts and...
(PRP_OWN)
[style] ~13-~13: Since ownership is already implied, this phrasing may be redundant.
Context: ...ance/conformance_ts.mjs`) — each riding its own language's SDK target, both run by CI's...
(PRP_OWN)
[style] ~13-~13: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ... added, and each language keeps a setup/caveats page under /sdk/setup/ — `/sdk/setup/...
(CAVEAT)
[typographical] ~17-~17: Consider using an em dash in dialogues and enumerations.
Context: - **Docs-site analytics for search, code ...
(DASH_RULE)
[style] ~17-~17: Since ownership is already implied, this phrasing may be redundant.
Context: ...ter,LiveDemo}.astro`): the site tracked its own CTAs but nothing a reader did on the wa...
(PRP_OWN)
[style] ~17-~17: Since ownership is already implied, this phrasing may be redundant.
Context: ...docs area without each tracker carrying its own copy; it's stamped at capture time by a...
(PRP_OWN)
[style] ~17-~17: Since ownership is already implied, this phrasing may be redundant.
Context: ...ressive Code, and the 404 route all own their own markup — some of it created after page ...
(PRP_OWN)
[style] ~21-~21: You can shorten this phrase to improve clarity and avoid wordiness.
Context: ...elow* the constant, so OpGte withheld a row that was chronologically equal. Both sides now parse as instants, mirr...
(NNS_THAT_ARE_JJ)
[style] ~23-~23: Consider using the typographical ellipsis character here instead.
Context: ...etched rows' received_timestamp, so a .select(...) projection omitting that column silen...
(ELLIPSIS)
docs/src/content/docs/sdk/setup/go.md
[style] ~6-~6: The word ‘caveats’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “cautions” or “warnings”.
Context: ...d the (T, error) model." --- Setup and caveats for `github.com/Wave-RF/WaveHouse/clien...
(CAVEAT)
[style] ~101-~101: Since ownership is already implied, this phrasing may be redundant.
Context: ...d streams alike: net/http strips only its own four sensitive headers across hosts and...
(PRP_OWN)
[grammar] ~134-~134: Use a hyphen to join words.
Context: ...uthprovider, marshal errors) are plain wrapped errors, so handle theerrors.As...
(QB_NEW_EN_HYPHEN)
[style] ~161-~161: Consider using the typographical ellipsis character here instead.
Context: ... Any slice batches. Reflection lets []ClickRow{...} take the same NDJSON batch path as `[...
(ELLIPSIS)
🔇 Additional comments (33)
clients/go/schema.go (1)
1-38: LGTM!clients/go/policy.go (1)
1-48: LGTM!clients/go/dlq.go (1)
1-42: LGTM!clients/go/namespaces_test.go (1)
1-195: LGTM!clients/go/stream.go (9)
54-58: LGTM!Also applies to: 65-90, 202-215
137-149: LGTM!Also applies to: 151-168, 173-177, 179-200, 219-224
227-282: LGTM!
286-423: LGTM!
441-458: LGTM!
462-506: LGTM!
553-582: LGTM!Also applies to: 586-604, 614-629, 633-644
648-683: LGTM!
685-714: LGTM!clients/go/live_query.go (2)
11-21: LGTM!Also applies to: 39-66
144-154: LGTM!clients/go/stream_test.go (6)
21-91: LGTM!
93-153: LGTM!
155-228: LGTM!
257-382: LGTM!
387-494: LGTM!
499-718: LGTM!clients/go/live_query_test.go (3)
13-35: LGTM!
41-111: LGTM!
134-158: LGTM!clients/go/http_test.go (1)
372-385: 📐 Maintainability & Code Quality | ⚡ Quick winConvert the two base URL variants into named subtests.
The loop still runs both inputs in one test function. A failure does not isolate the failing input. The coding guidelines require
t.Runfor multi-scenario tables. This was flagged before and is still present in the code shown.As per coding guidelines,
**/*_test.go: use table-driven tests witht.Run(tt.name, ...)for multiple scenarios.Source: Coding guidelines
clients/go/errors.go (1)
34-71: LGTM!clients/go/http.go (1)
84-97: LGTM!Also applies to: 159-198, 208-234
clients/go/errors_test.go (1)
11-129: LGTM!clients/go/table_test.go (1)
11-181: LGTM!clients/go/testdata/wire_cases.json (1)
330-336: LGTM!clients/go/conformance_test.go (1)
79-217: LGTM!Also applies to: 233-341
tests/conformance/conformance_ts.mjs (1)
21-30: LGTM!Also applies to: 40-55, 186-272
clients/go/e2e_test.go (1)
22-109: LGTM!Also applies to: 143-185, 353-413
|
|
||
| - **Go SDK client-side stream filters compare timestamps as instants, not as text** (`clients/go/stream.go`, `clients/go/stream_test.go`, `docs/src/content/docs/sdk/streaming.mdx`): the server canonicalizes every top-level `DateTime`/`DateTime64` value to RFC 3339 UTC before publishing (#402), so a payload reads `2026-06-21T04:00:00Z` while a caller's filter constant may name the same instant as `2026-06-21T06:00:00+02:00`. Compared as text those disagree in both directions — lexically the payload sorts *below* the constant, so `OpGte` withheld a row that was chronologically equal. Both sides now parse as instants, mirroring the server's row-filter rule for DateTime columns. Only unambiguous spellings count (RFC 3339 with an explicit offset or `Z`): a zone-less constant names an instant only relative to the column's declared timezone, which a stream subscriber doesn't have, so reading it as UTC would move the instant. Ordering an instant against a non-instant now fails closed rather than falling back to text comparison. Also fixes a missing column matching the literal string `"<nil>"` through the equality fallback. The TypeScript SDK's `matchesFilters` has the same text-comparison behavior and needs the same change for parity. | ||
|
|
||
| - **TypeScript SDK documentation corrections found while writing the Go SDK's parity docs** (`docs/src/content/docs/sdk/{reference,queries,streaming}.md`): the error-code table described `401` as "missing or invalid JWT" when a *missing* token is actually evaluated as `default_role` — succeeding or denied with `403`, never `401` (`internal/auth/auth.go`, `internal/api/errors.go`) — and only a present-but-invalid or expired token yields `401`. `.aggregate()` was documented as accepting a "custom fn", but the server enforces an allowlist (`internal/query/builder.go`); the allowed set is now listed. Live queries gained a caution for a real footgun: the backfill dedup boundary comes from the fetched rows' `received_timestamp`, so a `.select(...)` projection omitting that column silently disables dedup and delivers overlap-window events twice. No SDK code changed — these were pre-existing gaps between the TS docs and the server's behavior. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the SDK page file extensions.
Line 23 identifies docs/src/content/docs/sdk/{reference,queries,streaming}.md. These topic pages use .mdx. The entry points contributors to files that do not exist.
Change the suffix to .mdx.
🧰 Tools
🪛 LanguageTool
[style] ~23-~23: Consider using the typographical ellipsis character here instead.
Context: ...etched rows' received_timestamp, so a .select(...) projection omitting that column silen...
(ELLIPSIS)
| func TestRESTRedirectsWithCredentials(t *testing.T) { | ||
| // The redirect target records anything that reaches it. Nothing should. | ||
| var leaked http.Header | ||
| dst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| leaked = r.Header.Clone() | ||
| _, _ = io.WriteString(w, `[]`) | ||
| })) | ||
| defer dst.Close() | ||
|
|
||
| redirector := func(w http.ResponseWriter, r *http.Request) { | ||
| http.Redirect(w, r, dst.URL+r.URL.Path, http.StatusFound) | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| opts *ClientOptions | ||
| auth func(context.Context) (string, error) | ||
| wantErr bool | ||
| }{ | ||
| {name: "configured header", opts: &ClientOptions{Headers: map[string]string{"X-Operator-Key": "secret"}}, wantErr: true}, | ||
| {name: "bearer token", auth: StaticToken("secret"), wantErr: true}, | ||
| // Nothing to protect, so the ordinary redirect-following stands. | ||
| {name: "no credential follows the redirect", wantErr: false}, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| leaked = nil | ||
| src := httptest.NewServer(http.HandlerFunc(redirector)) | ||
| defer src.Close() | ||
|
|
||
| client := NewClient(Config{BaseURL: src.URL, Auth: tt.auth, Options: tt.opts, HTTPClient: src.Client()}) | ||
| _, err := client.Schema.List(context.Background()) | ||
|
|
||
| if !tt.wantErr { | ||
| if err != nil { | ||
| t.Fatalf("uncredentialed request should follow the redirect, got %v", err) | ||
| } | ||
| return | ||
| } | ||
| var apiErr *Error | ||
| if !errors.As(err, &apiErr) { | ||
| t.Fatalf("want a *wavehouse.Error, got %v", err) | ||
| } | ||
| if apiErr.Code != "REDIRECT" { | ||
| t.Errorf("want code REDIRECT, got %q (%v)", apiErr.Code, apiErr) | ||
| } | ||
| if apiErr.Retryable { | ||
| t.Error("a refused redirect is terminal, not retryable") | ||
| } | ||
| if leaked != nil { | ||
| t.Errorf("credentialed request reached the redirect target: %v", leaked) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Run the credentialed-redirect test with the race detector.
set -euo pipefail
cd clients/go
go test -race -run 'TestRESTRedirectsWithCredentials' -count=5 ./... 2>&1 | tail -40Repository: Wave-RF/WaveHouse
Length of output: 1630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- applicable repository conventions ---'
find /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9 -type f -name '*.md' -print \
| sort
printf '%s\n' '--- test and directly bound client definitions ---'
wc -l clients/go/http_test.go
sed -n '430,545p' clients/go/http_test.go
printf '%s\n' '--- relevant symbols and imports ---'
rg -n 'func TestRESTRedirectsWithCredentials|func NewClient|type ClientOptions|type Config|func StaticToken|CheckRedirect|REDIRECT|recordRequests' clients/goRepository: Wave-RF/WaveHouse
Length of output: 6932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository conventions ---'
cat /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9/conventions/repo-wide.md
printf '%s\n' '--- Go client learnings ---'
cat /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9/learnings/clients-go.md
printf '%s\n' '--- request capture helper ---'
sed -n '1,125p' clients/go/http_test.go
printf '%s\n' '--- client request and redirect handling ---'
sed -n '1,190p' clients/go/http.go
printf '%s\n' '--- client construction and token binding ---'
sed -n '1,115p' clients/go/wavehouse.goRepository: Wave-RF/WaveHouse
Length of output: 13667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Go toolchain and relevant synchronization points ---'
go version
go env GOROOT CGO_ENABLED
rg -n 'ErrUseLastResponse|func \(c \*Client\) do|func \(c \*Client\) Do|type responseAndError|responseAndError|send\(' "$(go env GOROOT)/src/net/http/client.go" "$(go env GOROOT)/src/net/http/httptest" "$(go env GOROOT)/src/net/http/transport.go"
printf '%s\n' '--- client redirect return path ---'
sed -n '620,760p' "$(go env GOROOT)/src/net/http/client.go"
printf '%s\n' '--- response body and transport synchronization context ---'
rg -n 'type bodyEOFSignal|earlyCloseFn|waitForBodyRead|bodyEOF|respHeaderRecv|bodyChan|responseAndError' "$(go env GOROOT)/src/net/http"Repository: Wave-RF/WaveHouse
Length of output: 14416
Synchronize access to leaked
The dst handler writes leaked on the server goroutine. The test goroutine resets and reads it without a happens-before edge. The uncredentialed case can race with the reset. Store the header on a buffered channel, and drain it in the uncredentialed branch before returning.
🧰 Tools
🪛 golangci-lint (2.12.2)
[medium] 484-484: G710: Open redirect via taint analysis
(gosec)
| func TestLiveQuery_FetchErrorReportedOnce(t *testing.T) { | ||
| sc := bareStream() | ||
| errCh := make(chan error, 1) | ||
|
|
||
| lq := newLiveQuery(sc, | ||
| func(context.Context) ([]map[string]any, error) { return nil, errors.New("boom") }, | ||
| &StreamSubscriber{ | ||
| Initial: func(_ []map[string]any, err error) { errCh <- err }, | ||
| }) | ||
| defer lq.Close() | ||
|
|
||
| select { | ||
| case err := <-errCh: | ||
| if err == nil || err.Error() != "boom" { | ||
| t.Fatalf("want boom, got %v", err) | ||
| } | ||
| case <-time.After(5 * time.Second): | ||
| t.Fatal("Initial never fired on fetch error") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the behavior the test name promises, and cover the buffer discard.
Two gaps:
-
The name is
FetchErrorReportedOnce, but the body receives fromerrChonce and stops.errChhas capacity 1, so a secondInitialcall would block the producing goroutine instead of failing the test. Count the calls to assert once-only delivery. -
The fetch-error path in
clients/go/live_query.goLines 78-84 setsbuffering = falseand discardslq.buffer. Events that arrived while the failed fetch was in flight are therefore dropped, and events arriving after it are delivered. No test pins that decision, so a future change could silently flush the buffer instead.
💚 Proposed test change
func TestLiveQuery_FetchErrorReportedOnce(t *testing.T) {
sc := bareStream()
- errCh := make(chan error, 1)
+ errCh := make(chan error, 4)
+ var initialCalls atomic.Int64
+ var delivered atomic.Int64
+ gate := make(chan struct{})
lq := newLiveQuery(sc,
- func(context.Context) ([]map[string]any, error) { return nil, errors.New("boom") },
+ func(context.Context) ([]map[string]any, error) {
+ <-gate
+ return nil, errors.New("boom")
+ },
&StreamSubscriber{
- Initial: func(_ []map[string]any, err error) { errCh <- err },
+ Initial: func(_ []map[string]any, err error) {
+ initialCalls.Add(1)
+ errCh <- err
+ },
+ Next: func(StreamEvent) { delivered.Add(1) },
})
defer lq.Close()
+ // Buffered while the fetch is gated: the error path discards these.
+ sc.emitEvent(liveEvent("2026-01-01T00:00:01Z"))
+ close(gate)
+
select {
case err := <-errCh:
if err == nil || err.Error() != "boom" {
t.Fatalf("want boom, got %v", err)
}
case <-time.After(5 * time.Second):
t.Fatal("Initial never fired on fetch error")
}
+
+ // A fetch error reports Initial exactly once and drops the gated buffer.
+ time.Sleep(50 * time.Millisecond)
+ if got := initialCalls.Load(); got != 1 {
+ t.Fatalf("want exactly 1 Initial call, got %d", got)
+ }
+ if got := delivered.Load(); got != 0 {
+ t.Fatalf("want the gated buffer dropped on fetch error, got %d deliveries", got)
+ }
+
+ // Events after the failed fetch flow straight through.
+ sc.emitEvent(liveEvent("2026-01-01T00:00:02Z"))
+ time.Sleep(50 * time.Millisecond)
+ if got := delivered.Load(); got != 1 {
+ t.Fatalf("want post-error events delivered, got %d", got)
+ }
}Based on learnings: "Every new function should have corresponding test cases."
Source: Learnings
| if lq.buffering { | ||
| lq.buffer = append(lq.buffer, event) | ||
| lq.mu.Unlock() | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Cap the backfill buffer.
lq.buffer grows without a limit while buffering is true. The buffering window is the whole fetchFn duration. A slow historical query against a high-rate table therefore accumulates every stream event in memory. StreamController already bounds the same hazard: eventCh holds 256 events and drops the rest with a one-time log at clients/go/stream.go Line 196. LiveQueryHandle has no equivalent bound.
Apply the same bound and the same drop signal so a slow backfill cannot grow the heap without a limit.
♻️ Proposed change
+// maxLiveQueryBuffer bounds the events held while the historical fetch runs.
+// Matches the StreamController event-channel bound so a slow backfill on a
+// high-rate table cannot grow the heap without a limit.
+const maxLiveQueryBuffer = 256
+
// LiveQueryHandle controls a live query that combines historical backfill
// with a real-time stream.
type LiveQueryHandle struct {
stream *StreamController
cancel context.CancelFunc
unsub func()
closeOnce sync.Once
+ dropOnce sync.Once
mu sync.Mutex
buffer []StreamEvent
buffering bool
closed bool
} if lq.buffering {
+ if len(lq.buffer) >= maxLiveQueryBuffer {
+ lq.mu.Unlock()
+ lq.dropOnce.Do(func() {
+ log.Printf("[wavehouse] live query event dropped: backfill buffer full (further drops not logged)")
+ })
+ return
+ }
lq.buffer = append(lq.buffer, event)
lq.mu.Unlock()
return
}Add the log import.
Also applies to: 100-113
| // Dedup bound: the maximum backfilled timestamp as a parsed time. The | ||
| // last row can be the oldest, and RFC3339 strings with varying | ||
| // fractional digits do not sort lexically. | ||
| var lastTS time.Time | ||
| for _, row := range rows { | ||
| if s, ok := row["received_timestamp"].(string); ok { | ||
| if ts, perr := time.Parse(time.RFC3339Nano, s); perr == nil && ts.After(lastTS) { | ||
| lastTS = ts | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Establish the wire precision of received_timestamp emitted by the server
# and returned by the query path, to decide whether the dedup bound can drop a distinct row.
set -euo pipefail
echo "=== received_timestamp formatting / column type ==="
rg -n -C 5 'received_timestamp' --glob '!clients/go/**' --glob '!**/node_modules/**' | head -120
echo "=== RFC3339 / DateTime64 formatting helpers ==="
rg -nP -C 4 'DateTime64|RFC3339(Nano)?|toRFC3339|Format\(time\.' --glob '!**/node_modules/**' | head -80
echo "=== conformance fixtures showing the emitted spelling ==="
fd -t f 'wire_cases.json' | xargs -r -I{} sh -c 'jq -r ".. | .received_timestamp? // empty" {} | sort -u | head -20'Repository: Wave-RF/WaveHouse
Length of output: 563
🏁 Script executed:
set -euo pipefail
printf '%s\n' '=== repository conventions ==='
find /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '=== changed file ==='
cat -n clients/go/live_query.go | sed -n '1,180p'
printf '%s\n' '=== timestamp references outside the Go client ==='
rg -n -C 4 'received_timestamp' --glob '!clients/go/**' --glob '!**/node_modules/**' --glob '!**/vendor/**' | head -240Repository: Wave-RF/WaveHouse
Length of output: 6191
🏁 Script executed:
set -euo pipefail
printf '%s\n' '=== applicable repository rules ==='
cat /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9/conventions/repo-wide.md
printf '%s\n' '=== applicable Go-client learnings ==='
cat /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9/learnings/clients-go.md
printf '%s\n' '=== repository files likely to define the wire contract ==='
git ls-files | rg '(^|/)(server|api|schema|proto|types|models|queries|clients/go|.*test.*|.*fixture.*)' | head -240
printf '%s\n' '=== exact timestamp symbols and formatters ==='
rg -n -C 3 'received_timestamp|receivedTimestamp|timestamp.*format|format.*timestamp|DateTime64|RFC3339Nano|RFC3339' --glob '*.go' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.sql' --glob '*.json' --glob '*.yaml' --glob '*.yml' --glob '*.proto' . | head -260Repository: Wave-RF/WaveHouse
Length of output: 23071
🏁 Script executed:
set -euo pipefail
printf '%s\n' '=== backend files containing the reserved timestamp ==='
rg -n -C 6 'received_timestamp|ReceivedTimestamp|DateTime64' internal cmd tests --glob '!**/*_test.go' --glob '!**/node_modules/**' | head -260
printf '%s\n' '=== record and stream implementation outlines ==='
ast-grep outline internal/api/record_reader.go
ast-grep outline internal/api/stream.go
printf '%s\n' '=== relevant backend source ==='
sed -n '1,260p' internal/api/record_reader.go
sed -n '1,300p' internal/api/stream.goRepository: Wave-RF/WaveHouse
Length of output: 36581
🏁 Script executed:
set -euo pipefail
printf '%s\n' '=== timestamp canonicalization contract ==='
sed -n '1,90p' internal/discovery/timestamp.go
printf '%s\n' '=== DateTime64 precision and canonical wire examples ==='
sed -n '110,165p' internal/discovery/timestamp.go
sed -n '110,165p' tests/e2e/sdk/streaming.test.ts
sed -n '35,72p' tests/e2e/sdk/tables.ts
printf '%s\n' '=== live-query tests covering equal timestamps ==='
sed -n '1,180p' clients/go/live_query_test.go
printf '%s\n' '=== live-query API and related SDK behavior ==='
rg -n -C 5 'LiveQuery|live query|dedup|received_timestamp' clients/go clients/ts docs/src tests/e2e/sdk --glob '!**/node_modules/**' | head -260Repository: Wave-RF/WaveHouse
Length of output: 31479
Add row identity to live-query deduplication. The server preserves each column’s precision, and the repository’s e2e schema uses DateTime64(3). Distinct rows can share lastTS, so !ts.After(lastTS) can silently drop a live event during the backfill handoff.
| func Fetch[Row any](ctx context.Context, p *PipeRef) ([]Row, error) { | ||
| body := p.params | ||
| if body == nil { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject a nil PipeRef before dereference.
Fetch[Row](ctx, nil) dereferences p.params on Line 79 and panics. Return an SDK error before accessing p.
Based on learnings: “Return errors, don't panic.”
Proposed fix
import (
"context"
+ "errors"
"fmt"
"net/url"
)
func Fetch[Row any](ctx context.Context, p *PipeRef) ([]Row, error) {
+ if p == nil {
+ return nil, errors.New("execute pipe: nil PipeRef")
+ }
body := p.params📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func Fetch[Row any](ctx context.Context, p *PipeRef) ([]Row, error) { | |
| body := p.params | |
| if body == nil { | |
| func Fetch[Row any](ctx context.Context, p *PipeRef) ([]Row, error) { | |
| if p == nil { | |
| return nil, errors.New("execute pipe: nil PipeRef") | |
| } | |
| body := p.params | |
| if body == nil { |
Source: Learnings
| func TestStream_EventsBufferBeforeFirstEventsCall(t *testing.T) { | ||
| sc := &StreamController{eventCh: make(chan StreamEvent, 256)} | ||
| sc.emitEvent(StreamEvent{Table: "clicks", Data: map[string]any{"page": "/"}}) | ||
|
|
||
| select { | ||
| case e := <-sc.Events(): | ||
| if e.Table != "clicks" { | ||
| t.Fatalf("want event for clicks, got %+v", e) | ||
| } | ||
| default: | ||
| t.Fatal("event emitted before Events() was not buffered") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Reuse bareStream() instead of a second hand-built controller.
clients/go/live_query_test.go Line 13 defines bareStream(), which builds a StreamController with status, eventCh, done, and cancel set. This literal sets only eventCh, so the package now has two partially-initialized StreamController values built by hand. bareStream() is in the same package and is directly usable here.
♻️ Proposed change
- sc := &StreamController{eventCh: make(chan StreamEvent, 256)}
+ sc := bareStream()
sc.emitEvent(StreamEvent{Table: "clicks", Data: map[string]any{"page": "/"}})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestStream_EventsBufferBeforeFirstEventsCall(t *testing.T) { | |
| sc := &StreamController{eventCh: make(chan StreamEvent, 256)} | |
| sc.emitEvent(StreamEvent{Table: "clicks", Data: map[string]any{"page": "/"}}) | |
| select { | |
| case e := <-sc.Events(): | |
| if e.Table != "clicks" { | |
| t.Fatalf("want event for clicks, got %+v", e) | |
| } | |
| default: | |
| t.Fatal("event emitted before Events() was not buffered") | |
| } | |
| } | |
| func TestStream_EventsBufferBeforeFirstEventsCall(t *testing.T) { | |
| sc := bareStream() | |
| sc.emitEvent(StreamEvent{Table: "clicks", Data: map[string]any{"page": "/"}}) | |
| select { | |
| case e := <-sc.Events(): | |
| if e.Table != "clicks" { | |
| t.Fatalf("want event for clicks, got %+v", e) | |
| } | |
| default: | |
| t.Fatal("event emitted before Events() was not buffered") | |
| } | |
| } |
| func compileLike(pattern string) *regexp.Regexp { | ||
| escaped := regexp.QuoteMeta(pattern) | ||
| escaped = strings.ReplaceAll(escaped, "%", ".*") | ||
| escaped = strings.ReplaceAll(escaped, "_", ".") | ||
| re, err := regexp.Compile("(?i)^" + escaped + "$") | ||
| if err != nil { | ||
| return nil | ||
| } | ||
| return re | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🌐 Web query:
Does ClickHouse LIKE % wildcard match newline characters in string values?
💡 Result:
Yes, the ClickHouse LIKE operator's % wildcard matches newline characters (and any other arbitrary characters) within string values [1][2][3]. In ClickHouse, the % wildcard is defined as representing an arbitrary number of characters, including zero characters [1][2]. It does not treat newline characters as special delimiters or line terminators; it simply treats them as part of the string's content [1][2]. As a result, a pattern such as '%pattern%' will successfully match a string containing a newline if the rest of the pattern aligns with the surrounding text [1][3]. Matching is performed based on UTF-8 encoding [1][2]. You can verify this behavior by executing a query such as: SELECT 'line1\nline2' LIKE '%1%2%'; This query will return 1 (true) because the % wildcard correctly bridges the newline character (\n) between the '1' and '2'. Note that when constructing these patterns in ClickHouse, you must account for standard SQL string literal escaping (where \n represents a newline character) [4] and the specific escaping requirements of the LIKE operator (where backslashes are used to escape LIKE metacharacters like % and _) [1][2].
Citations:
- 1: https://clickhouse.com/docs/reference/functions/regular-functions/string-search-functions
- 2: https://clickhouse.com/docs/sql-reference/functions/string-search-functions
- 3: https://clickhouse.com/docs/reference/statements/select
- 4: https://clickhouse.com/docs/reference/syntax
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- applicable repository guidance ---'
find /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9 -type f -name '*.md' -print
printf '%s\n' '--- stream.go target and direct references ---'
sed -n '500,555p' clients/go/stream.go
rg -n -C 4 'compileLike|OpLike|Where\(' clients/go/stream.go clients/go
printf '%s\n' '--- Go regexp contract in the local source ---'
go version 2>/dev/null || true
go env GOROOT 2>/dev/null || trueRepository: Wave-RF/WaveHouse
Length of output: 13480
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- repository-wide convention ---'
cat /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9/conventions/repo-wide.md
printf '%s\n' '--- Go client learnings ---'
cat /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9/learnings/clients-go.md
printf '%s\n' '--- filter evaluation path ---'
sed -n '532,610p' clients/go/stream.goRepository: Wave-RF/WaveHouse
Length of output: 3243
Add the s flag so % and _ match newlines.
evaluateFilter uses this expression for like and not_like. Go's . excludes newlines by default, so stream filtering can reject multi-line values that ClickHouse LIKE accepts. Compile the expression with (?is).
| The motivating case is a transport bug in the runtime's own HTTP stack, which you can't fix from inside the SDK — for example [undici #5600](https://github.com/nodejs/undici/issues/5600): reusing a keep-alive socket while the event loop is idle stalls the request before it goes out. How bad it gets varies with the runtime and the idle gap: the upstream report measured ~450–465 ms against a 10 ms server, and we have measured anything from ~100 ms to tens of seconds on a server answering instantly. It affects undici 8.8.0–8.9.0, and Node 26 bundles 8.9.0. | ||
|
|
||
| **Upgrading undici is the actual fix** — it landed in 8.10.0. What `options.fetch` buys you is a way to get there without waiting for a new runtime: install undici yourself and route requests through it. The one non-obvious part is that you must pass its dispatcher **explicitly**. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- applicable repository guidance ---'
find /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9 -type f -name '*.md' -print
printf '%s\n' '--- target excerpt ---'
sed -n '420,455p' docs/src/content/docs/sdk/setup/typescript.mdx
printf '%s\n' '--- related Node/Undici references ---'
rg -n -C 2 'Node 26|undici 8\.|process\.versions\.undici|options\.fetch' docs/src/content/docs/sdk/setup/typescript.mdxRepository: Wave-RF/WaveHouse
Length of output: 15506
🏁 Script executed:
printf '%s\n' '--- repository-wide conventions ---'
cat /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9/conventions/repo-wide.md
printf '%s\n' '--- documentation learnings ---'
cat /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9/learnings/docs.md
cat /tmp/coderabbit-repo-knowledge/wave-rf-wavehouse-8d4c42a9/learnings/docs-src-content-docs.mdRepository: Wave-RF/WaveHouse
Length of output: 2222
🌐 Web query:
Node.js 26 release versions bundled undici 8.9.0 process.versions.undici Node 26.0.0 26.1.0
💡 Result:
Node.js 26.0.0 was released on May 5, 2026, and bundled Undici 8.0.0 [1][2]. The property process.versions.undici in Node.js 26.0.0 reflects this initial version [3][4]. Regarding Node.js 26.1.0, while the Node.js documentation mentions that process.versions.undici can be used to check the bundled version [5][6], Undici 8.0.0 was the initial version included in the 26.x release line [2]. Subsequent minor or patch releases of Node.js 26 have updated the bundled Undici version (for example, the Node.js 26 changelog notes an update to Undici 8.10.0 in a later release) [7]. The property process.versions.undici will always return the specific version of Undici bundled within that specific Node.js binary [3][8].
Citations:
- 1: https://nodejs.org/en/blog/release/v26.0.0
- 2: GitHub issue 62351 in nodejs/node (link omitted to avoid creating a cross-reference)
- 3: https://undici.nodejs.org/best-practices/undici-vs-builtin-fetch
- 4: https://undici.nodejs.org/
- 5: https://nodejs.org/docs/latest-v26.x/api/globals.html
- 6: https://nodejs.org/api/globals.html
- 7: https://github.com/nodejs/node/blob/main/doc/changelogs/CHANGELOG_V26.md
- 8: https://github.com/nodejs/undici?tab=readme-ov-file
🏁 Script executed:
set -eu
for url in \
https://nodejs.org/en/blog/release/v26.0.0 \
https://raw.githubusercontent.com/nodejs/node/main/doc/changelogs/CHANGELOG_V26.md
do
echo "--- $url ---"
curl -L --fail --silent "$url" |
rg -n -C 3 'undici|Undici|26\.0\.0|26\.1\.0|8\.0\.|8\.9\.|8\.10\.'
doneRepository: Wave-RF/WaveHouse
Length of output: 50374
Scope the Node.js version claim. Node.js 26.0.0 bundles Undici 8.0.2, so “Node 26 bundles 8.9.0” is too broad and can send users to an unnecessary workaround. Name the affected Node.js release, or tell users to check process.versions.undici.
Source: MCP tools
Summary
Complete Go SDK at
clients/go/— a canonical, officially supported client with full API-tree parity against the TypeScript SDK. Zero third-party runtime dependencies (stdlib only).What's included
SDK core (
clients/go/)FetchTyped[Row],SQL[Row])[]T, not just[]map[string]anyCodegen CLI (
clients/go/cmd/wavehouse-codegen/)/v1/schemaendpointTesting
-race(SDK coverage ~87%)//go:build e2etag (against live server)make test-sdk(both),make test-sdk-go,make test-sdk-ts(renamed fromtest-ts),make test-sdk-go-e2e. Each language target runs its own half of the conformance suite, so there is no separate conformance target. All use gotestsum and honorARGS/V=1like every other Go suite.Documentation — topic-first, per the decision recorded in #313
<Tabs syncKey="lang">so both SDKs live on one page and the topic URLs never churn as languages are added/sdk/typescript(moved off the root of/sdk) and/sdk/go;/sdkis now a language-neutral overviewCI
make test-unit test-sdk, so the Go SDK suite and its coverage gate are enforced in CI (previously only localmake ciran them)fmt-go,lint-go,tidyandfix-goeach span both modules, soverify-parallelis unchanged apart from dropping two leavesCommits
43+ commits: the initial implementation (SDK, docs, CI wiring), then a review-response series addressing every Copilot/CodeRabbit inline thread plus successive local pre-push review rounds (each thread gets an inline reply; false positives are rebutted rather than patched).
Notes
clients/go/go.mod) — invisible to rootgo list ./....tidy,fmt,lintand the coverage gate now reach it;vulncheckis still root-only and remains tracked in Extend root verify tooling to the nested clients/go module (tidy, vulncheck, coverage gate) #437//go:build e2e) runs only against a server you started yourself. Wiring it into the orchestrator so it runs in CI with ago-sdk-unit/go-sdk-e2e/go-sdk-totalgate shape matching the TS SDK is tracked in Run the Go SDK e2e suite from the orchestrator and gate its coverage like the TS SDK #518Array(UInt8)base64 normalization tracked in query: normalize []byte scan values in /v1/query responses (Array(UInt8) arrives as base64) #436 (SDK maps it tojson.RawMessageuntil fixed)