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
WalkthroughThe pull request adds an official Go SDK with query, ingestion, streaming, administration, code generation, conformance tests, documentation, and CI integration. It also adds independent Go SDK coverage reporting and TypeScript wire-format conformance execution. ChangesGo SDK implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to This PR adds a new Go SDK and related documentation, but the current head still has merge-blocking issues: REST requests may leak configured credentials across cross-host redirects, schema listing can fail on a supported response shape, and race-enabled tests can fail because shared header state is unsynchronized. Several credential-handling examples also need correction before publication, so the PR is not ready to merge until these are addressed. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📚 Docs preview is live → https://b00566b7-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)
…se-split The LIKE caution added last round was right about like and wrong about not_like. internal/query/builder.go's filterToSQL has cases for eq/neq/gt/gte/lt/lte/like/in only, so not_like falls through to default and /v1/query rejects it with 400 — a live query filtering on it fails its Initial callback rather than quietly disagreeing with itself. Both streaming pages now scope the case-sensitivity claim to like and describe not_like separately, matching what the two queries pages and api.md already said. Also corrects "the Makefile uses go run" to go tool (Makefile:144-150 defines every pinned tool as `go tool <name>`; the same page already said so thirty lines earlier), and gives the TS selectAll() description the same restricted-vs-unrestricted split this branch added everywhere else — an unrestricted role gets a bare SELECT *, not an expansion.
# Conflicts: # docs/src/content/docs/sdk/reference.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/content/docs/development.md (1)
496-498: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
make test-unitfor the root unit-test row.Line 341 describes tests in
internal/*/_test.go, but Line 496 definesmake testastest-unitplustest-go-sdk. The table therefore runs the separately listed Go SDK suite for the root unit-test entry.Proposed documentation fix
- | Unit tests | `internal/*/_test.go` | No | `make test` | + | Unit tests | `internal/*/_test.go` | No | `make test-unit` |As per coding guidelines, “Keep documentation synchronized with code changes.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 73533654-680d-4551-9ffa-18c2728ddbde
📒 Files selected for processing (4)
docs/src/content/docs/development.mddocs/src/content/docs/sdk/go/streaming.mddocs/src/content/docs/sdk/queries.mddocs/src/content/docs/sdk/streaming.md
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Keep documentation synchronized with code changes, including API, configuration, architecture, deployment, development, and changelog updates as applicable.
Files:
docs/src/content/docs/sdk/queries.mddocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/sdk/go/streaming.mddocs/src/content/docs/development.md
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Runmake cilocally before every push, using the documented background execution method, and do not use CI as the first feedback loop.
Never force-push or rebase PR branches; mergeorigin/maininstead.
Do not hand-write review markers or bypass hooks with--no-verify; run or formally skip every required pre-push reviewer and resolve all findings.
Files:
docs/src/content/docs/sdk/queries.mddocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/sdk/go/streaming.mddocs/src/content/docs/development.md
🧠 Learnings (1)
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
docs/src/content/docs/sdk/queries.mddocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/sdk/go/streaming.mddocs/src/content/docs/development.md
🪛 LanguageTool
docs/src/content/docs/sdk/queries.md
[style] ~129-~129: Consider using the typographical ellipsis character here instead.
Context: ...et SELECT *). Mutually exclusive with .select(...) and with aggregations (.count(), `....
(ELLIPSIS)
🔇 Additional comments (6)
docs/src/content/docs/sdk/streaming.md (1)
2-2: LGTM!Also applies to: 157-170
docs/src/content/docs/development.md (1)
191-199: LGTM!Also applies to: 291-305, 318-333, 342-345, 359-360, 370-370, 382-382, 392-392, 432-440, 484-493, 499-505, 519-519, 552-556, 596-596
docs/src/content/docs/sdk/go/streaming.md (2)
210-283: LGTM!Also applies to: 285-296, 298-305
1-18: 📐 Maintainability & Code QualityNo documentation changes required. The Go streaming documentation matches the implementation.
docs/src/content/docs/sdk/queries.md (2)
2-2: LGTM!Also applies to: 89-89, 129-129
165-174: 🗄️ Data Integrity & IntegrationVerify the aggregation allowlist against backend validation.
clients/go/query_builder_test.goconfirms only the wire serialization ofuniqExact. It does not prove that the backend accepts the documented case-insensitive allowlist or returns400 unsupported aggregation functionfor unsupported names.
… to Go Resolving the sdk/reference.md conflict by taking main's page wholesale was too blunt. Main's SSE rows and their explanatory paragraph are correct and stay — the branch's "the stream reconnects automatically" was wrong for the TS SDK, which never re-dials a stream itself. But the same resolution also reverted two unrelated branch edits on that page: - The 401 row went back to "Missing or invalid JWT". internal/auth/auth.go returns no auth error for an empty token, so a missing token resolves to default_role and is denied 403; only a present-but-invalid or expired one yields 401. api.md and the Go reference page still said so, so the page contradicted both. - The title lost its "TypeScript SDK" prefix, leaving the only unqualified "SDK ..." title in a two-SDK tree — it reads as the shared reference in breadcrumbs and search results when it is TS-only. Main's new path-prefix guidance is TypeScript-only, which understates it now that the Go SDK is co-canonical and preserves a prefix on both transports (wavehouse.go trims the trailing slash; http.go and stream.go concatenate). reverse-proxy.mdx now shows both clients and scopes the "upgrade your SDK" caution to TypeScript, since the Go client never had #428. The Go README gains the sentence its TypeScript counterpart got in the merge.
The merge documented path-prefix support as a promise on the Go SDK's Config row and README, but nothing tested it — every test server in clients/go was root-hosted. The TS side shipped url.test.ts and friends pinning exactly this after #428. Adds a guard per transport, since the SSE URL is built in stream.go independently of buildURL: a mux serving only the prefixed path, so a dropped prefix 404s (REST) or never arrives (SSE). Both verified failing against a deliberately broken buildURL and stream URL before being kept. Qualifies the 401 row on both reference pages: "denied with 403" was too absolute. A missing token is evaluated as default_role, which may well succeed — the point is that it never yields 401. Also trims the changelog's claim that this branch added SSE_ERROR/SSE_CONNECT_ERROR to the TS error table; post-merge those rows come from #448 in the same Unreleased section.
…inters Triage of the 24 open review threads surfaced one real defect. retryAfterDelay computed time.Duration(secs) * time.Second before any range check, which wraps negative past ~9.2e9 seconds — and because the wrap goes negative, the min(delay, maxRetryAfter) clamp below selected it rather than capping. The retry timer then fired immediately, so a server answering 429/503 with a large Retry-After got hammered with zero-delay retries: the exact inverse of the header's purpose, and a silent breach of the documented 30s cap. Verified: "10000000000" yielded -2346317h47m53s, MaxInt64 yielded -1s. The HTTP-date branch is unaffected — time.Sub saturates rather than wrapping. emptyInsertResult aliased one &z across Total/Succeeded/Failed/Duplicates, so a caller writing through any one of those exported *int fields mutated all four. Separate vars now. fetchNextTyped discarded its json.Marshal error, truncating a result set to look like normal end-of-pagination. Propagated. The Decode error stays deliberate — a Row marshaling to a non-object ends pagination the same way an absent cursor column does — now commented as such, with the parity question tracked in #452. Test fixes for guards that could not fail: - InsertByteSliceNotBatch asserted only the path and OK, both of which the batch path also satisfies; it now pins Content-Type and body, and was confirmed to fail with the []byte carve-out removed. - backoff's CappedAt30s accepted 24-36s where the cap is applied after jitter and returns exactly 30s, so moving the cap before the jitter still passed. - retryAfterDelay's HTTPDateFuture window subsumed the ~1s parse-failure fallback; tightened, and two overflow cases added. - TestNewClient_HasNamespaces boxed typed pointers into map[string]any, where a nil typed pointer is never == nil — it could not fail. Compares concrete fields now. - TestClient_From discarded Fetch's error, so an early return left its handler assertions unreached. Docs: a pagination snippet that did not compile, three shell examples that were bash syntax errors (unquoted <placeholder>), the cursor tie-breaker caveat (#452), and the raw-SQL claim that a tokenless request is rejected — untrue when default_role is the admin role, which AGENTS.md permits as dev-only. Fixed on both SDKs' pages.
The guard added in 0a46251 had no test — the same gap that commit existed to close. A Row that unmarshals cleanly but fails to marshal back (an exported func field, absent from the response) previously produced an empty page, indistinguishable from real exhaustion. Verified failing with the guard swallowing the error instead of returning it.
The Go queries page documents two pagination footguns and names the TS SDK as sharing both, but the TS page carried neither — so a data-loss caveat lived only on the page TS readers don't open. Ports both: the strict single-column cursor that drops rows tying the page boundary (#452, verified against clients/ts/src/query-builder.ts), and the Number.MAX_SAFE_INTEGER ceiling on integer cursors. Also fills the not_like row's Backend column on the TS operator table — the SDK does send a wire token (query-builder.ts maps not_like to "not_like"), and a reader debugging the resulting 400 needs to know which token the server rejected. The Go table already said so. Two smaller corrections: reverse-proxy.mdx claimed the Go SDK "preserves the prefix on every released version", asserting a property of a set that development.md says is empty (no tagged Go releases yet); and the key-targets table under-reported make lint / verify / fix, all three of which fan out to markdown, prose, shell, workflow, and astro checks beyond what was listed.
The TypeScript codegen type table promised `number` for Decimal* columns, but /v1/query returns them as quoted strings: transformRow in internal/api/clickhouse_exec.go converts only UUID and time.Time, so a shopspring decimal.Decimal reaches json.Marshal and marshals quoted. A TS user with a Decimal price column gets a number-typed field holding "12.34" and silently wrong arithmetic, with no type error. The Go page already documented the real shape, which is what made the discrepancy visible. Footnoted both that row and Array(UInt8) (base64 on the same path, #436); the codegen fix itself is tracked in #453. Also corrects the path-prefix sentence I got wrong in both directions: it now says the prefix is preserved in every version `go get` can resolve, rather than asserting a release history that doesn't exist yet.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
43-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the access-control documentation path.
Line 43 names
docs/src/content/docs/access-control.md, but this PR usesdocs/src/content/docs/access-control.mdx. Update the changelog reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6080abc9-b86a-422e-921a-0065022485f2
📒 Files selected for processing (19)
CHANGELOG.mdclients/go/README.mdclients/go/client_test.goclients/go/http.goclients/go/http_test.goclients/go/query_builder.goclients/go/query_builder_test.goclients/go/stream_test.goclients/go/table.goclients/go/table_test.godocs/src/content/docs/api.mddocs/src/content/docs/development.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/queries.mddocs/src/content/docs/sdk/reference.md
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: Coverage
- GitHub Check: Docs build
- GitHub Check: E2E tests
- GitHub Check: Integration tests
- GitHub Check: Unit tests
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (8)
docs/src/content/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Keep documentation synchronized with code changes, including endpoint, configuration, architecture, ingest-format, deployment, and development-process changes.
Files:
docs/src/content/docs/api.mddocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/reference.mddocs/src/content/docs/sdk/queries.mddocs/src/content/docs/development.md
docs/src/content/docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Author Mermaid diagrams vertically by default, avoid large side-by-side diagrams, and keep labels short enough to remain legible in the documentation column.
Files:
docs/src/content/docs/api.mddocs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/reference.mddocs/src/content/docs/sdk/queries.mddocs/src/content/docs/development.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Comment the why rather than restating what the code does; keep comments concise and consistent with surrounding code.
Files:
docs/src/content/docs/api.mdCHANGELOG.mddocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/go/queries.mdclients/go/README.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/reference.mddocs/src/content/docs/sdk/queries.mddocs/src/content/docs/development.md
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Before every push, validate locally with the documentedmake ciworkflow; do not use CI as the first feedback loop.
Never force-push or rebase PR branches; mergeorigin/maininstead. Do not hand-write review markers or bypass hooks with--no-verify.
Every review finding requires a substantive response, a fix or linked tracking issue, and thread resolution before merge.
Agent-created pull requests must be drafts and use a Conventional Commits title of at most 72 characters, with a lowercase-first subject and no trailing period.
Files:
docs/src/content/docs/api.mdCHANGELOG.mddocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/reverse-proxy.mdxclients/go/http.godocs/src/content/docs/sdk/go/queries.mdclients/go/client_test.goclients/go/README.mdclients/go/table_test.godocs/src/content/docs/sdk/go/index.mdclients/go/table.godocs/src/content/docs/sdk/reference.mdclients/go/stream_test.godocs/src/content/docs/sdk/queries.mdclients/go/http_test.goclients/go/query_builder.goclients/go/query_builder_test.godocs/src/content/docs/development.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Every notable code or user-visible change must update the
[Unreleased]section ofCHANGELOG.mdin the same PR.
Files:
CHANGELOG.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use Go 1.26, format code withgofumpt, return wrapped errors instead of panicking, use structuredlog/sloglogging, and pass dependencies explicitly rather than using global state.
New Go code should target at least 80% project coverage overall and add tests for new functions; runmake lintandmake testbefore completion.
Files:
clients/go/http.goclients/go/client_test.goclients/go/table_test.goclients/go/table.goclients/go/stream_test.goclients/go/http_test.goclients/go/query_builder.goclients/go/query_builder_test.go
clients/go/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Maintain the Go SDK as a canonical, zero-runtime-dependency client with API-tree parity, typed query builders, SSE/live-query support, and code generation.
Files:
clients/go/http.goclients/go/client_test.goclients/go/table_test.goclients/go/table.goclients/go/stream_test.goclients/go/http_test.goclients/go/query_builder.goclients/go/query_builder_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Use table-driven tests with
t.Run, shared mocks and test helpers frominternal/testutil, and provide corresponding tests for every new function.
Files:
clients/go/client_test.goclients/go/table_test.goclients/go/stream_test.goclients/go/http_test.goclients/go/query_builder_test.go
🧠 Learnings (6)
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
docs/src/content/docs/api.mdCHANGELOG.mddocs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/sdk/go/queries.mdclients/go/README.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/reference.mddocs/src/content/docs/sdk/queries.mddocs/src/content/docs/development.md
📚 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/reference.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/index.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/http.goclients/go/client_test.goclients/go/table_test.goclients/go/table.goclients/go/stream_test.goclients/go/http_test.goclients/go/query_builder.goclients/go/query_builder_test.go
📚 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/http.goclients/go/client_test.goclients/go/table_test.goclients/go/table.goclients/go/stream_test.goclients/go/http_test.goclients/go/query_builder.goclients/go/query_builder_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/client_test.goclients/go/table_test.goclients/go/stream_test.goclients/go/http_test.goclients/go/query_builder_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/client_test.goclients/go/table_test.goclients/go/stream_test.goclients/go/http_test.goclients/go/query_builder_test.go
🪛 LanguageTool
docs/src/content/docs/sdk/index.mdx
[style] ~349-~349: 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)
docs/src/content/docs/reverse-proxy.mdx
[style] ~40-~40: Since ownership is already implied, this phrasing may be redundant.
Context: ... the proxy's job, and the server having its own opinion about it is a second place for ...
(PRP_OWN)
docs/src/content/docs/sdk/reference.md
[style] ~40-~40: A comma is missing here.
Context: ...RROR| Yes | Stream failed to connect (e.g. a non-absolutebaseURL) | | 0 | SSE_...
(EG_NO_COMMA)
🔇 Additional comments (18)
CHANGELOG.md (1)
39-42: LGTM!Also applies to: 48-79
clients/go/README.md (1)
83-83: LGTM!Also applies to: 198-203
clients/go/client_test.go (1)
40-56: LGTM!Also applies to: 70-74
clients/go/http_test.go (1)
213-322: LGTM!clients/go/table.go (1)
122-125: LGTM!docs/src/content/docs/sdk/go/queries.md (1)
322-330: LGTM!Also applies to: 362-367
docs/src/content/docs/sdk/index.mdx (1)
337-359: LGTM!Also applies to: 382-395
docs/src/content/docs/sdk/queries.md (1)
9-11: LGTM!Also applies to: 91-91, 131-131, 155-155, 167-175, 258-260
docs/src/content/docs/sdk/reference.md (1)
28-43: LGTM!Also applies to: 139-143
clients/go/http.go (1)
176-182: 🎯 Functional CorrectnessNo 32-bit target is supported by the release matrix.
GoReleaser targets only
amd64andarm64, so thisstrconv.Atoibehavior does not affect a supported target.> Likely an incorrect or invalid review comment.clients/go/query_builder.go (2)
287-300: LGTM!
7-7: 📐 Maintainability & Code QualityKeep the Go SDK compatibility floor at Go 1.24.
The root module and CI use Go 1.26.5.
clients/go/go.modintentionally declares Go 1.24 as its compatibility floor, not its CI toolchain.> Likely an incorrect or invalid review comment.clients/go/table_test.go (1)
129-162: LGTM!docs/src/content/docs/api.md (1)
26-26: LGTM!docs/src/content/docs/development.md (1)
342-345: LGTM!Also applies to: 369-370, 382-382, 392-392, 486-489, 519-519, 554-556
docs/src/content/docs/sdk/go/reference.md (1)
57-57: LGTM!Also applies to: 150-150, 266-266
docs/src/content/docs/reverse-proxy.mdx (1)
38-96: LGTM!Also applies to: 147-147
docs/src/content/docs/sdk/go/index.md (1)
88-88: LGTM!
| func TestQueryBuilder_Pagination_UnmarshalableRowErrors(t *testing.T) { | ||
| type row struct { | ||
| ID string `json:"id"` | ||
| Cb func() `json:"cb"` | ||
| } | ||
| c, _ := pagingServer(t, [][]map[string]any{{{"id": "a"}, {"id": "b"}}}) | ||
| page, err := FetchTyped[row](context.Background(), | ||
| c.From("clicks").Select("id").OrderBy("id", "asc").Limit(2)) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if page.Next == nil { | ||
| t.Fatal("want a Next cursor") | ||
| } | ||
| if _, err := page.Next(context.Background()); err == nil { | ||
| t.Fatal("want a marshal error, got a silently empty page") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use table-driven subtests for the new Go tests.
Both new test functions use one linear case. The repository rule requires table-driven tests with t.Run.
clients/go/query_builder_test.go#L464-L481: wrap the marshal-error case in a table-drivent.Runtest.clients/go/stream_test.go#L436-L468: wrap the BaseURL-prefix case in a table-drivent.Runtest.
As per coding guidelines: "**/*_test.go: Use table-driven tests with t.Run".
📍 Affects 2 files
clients/go/query_builder_test.go#L464-L481(this comment)clients/go/stream_test.go#L436-L468
Source: Coding guidelines
) ## Summary Release-readiness pass ahead of #149, plus #431 and #475. The through-line: **every component now derives its version from the tag it was built at**, so cutting a release is one tag and nothing else. That isn't just convenience — the `main` ruleset forbids direct pushes, so a version-bump commit would have needed its own reviewed PR before *every* SDK release. ### Two bugs that would have fired on the first tag - GoReleaser's `release.prerelease` defaults to a flat `false`, so a `v0.1.0-alpha.1` tag would have published as a **stable** GitHub Release and taken the "Latest release" badge. Now `auto`. - `dockers_v2` tagged `:latest` unconditionally on any `v*` tag, so a future `v1.3.0-rc.1` would have moved `ghcr.io/wave-rf/wavehouse:latest` off the shipped `v1.2.0`. Each build now gets one immutable ref plus one *channel* pointer (`:latest` stable, `:alpha`/`:beta`/`:rc`/`:next` prerelease, `:dev` on main), resolved by `scripts/ci/release-channel.sh` — shared with the npm dist-tag selection so `:rc` and `@rc` can't drift apart. ### Release notes could come out empty Two tag families share this repo, and both release paths asked GitHub to generate notes without saying which tag to diff from. GitHub's default is the previous *release*, so cutting the SDK tag on the same commit as an already-published server release yields a **completely empty** changelog. The server side is fixed at the root with `git.ignore_tags: ["clients/*"]`, so GoReleaser's own detection is family-aware and no workflow has to pass an override; the SDK side passes `--notes-start-tag`, resolved by walking git history from the tag's parent rather than version-sorting (sorting picked a tag *ahead* of the one being released). The SDK release step also no longer fails outright when a release already exists — it previously ran a bare `gh release create`, which reddens a run whose npm publish had already succeeded. Release bodies now come from GitHub's own per-PR generator (`changelog.use: github-native`) grouped by `.github/release.yml`, rather than raw commit subjects. ### Tag naming: `clients/<lang>/vX.Y.Z` `sdk-v*` becomes `clients/ts/v*`. Not cosmetic — Go resolves a subdirectory module only against a tag carrying that subdirectory as a prefix ([go.dev/ref/mod](https://go.dev/ref/mod)), so the incoming Go SDK (#434, whose `clients/go/go.mod` declares `github.com/Wave-RF/WaveHouse/clients/go`) *must* be `clients/go/vX.Y.Z`. Every client follows that shape rather than leaving Go as the exception. Done now because it was free exactly once: no `sdk-v*` tag had ever been created. The `release tag protection` ruleset was updated to `refs/tags/v*` + `refs/tags/clients/*/v*`. ### Fixes - **#431** — `triage.yml` retired. GitHub Models was [retired 2026-07-30](https://github.blog/changelog/2026-07-30-github-models-is-now-retired/); the endpoint returns `410` unconditionally and the workflow had failed on **every** issue event since. The issue's suggested `continue-on-error` + fallback was written while it still looked like the announced brownout; with the service gone it would mean always taking the fallback. Triage is manual now. `.github/board-config.env` went with it (no other consumer), and `PROJECT_BOARD_TOKEN` is now unused. - **#475** — dev versions move from `0.0.0-dev.h<hash>` to `0.0.1-dev.<utc-stamp>.h<hash>`. semver compares alphanumeric prerelease identifiers *lexically*, so a content hash made the channel's order arbitrary with respect to time — and since `npm i …@dev` records a **range**, resolution walked backwards to a two-month-old build. A numeric identifier compares numerically; the `0.0.1` base outranks the permanently-published legacy versions and keeps the channel isolated from any release range. - **`-X main.BuildTime` has never worked** — the variable was initialized to `time.Now()`, and `-X` silently no-ops on a non-constant initializer, so `/version` reported process start time as build time. - **`go install …@vX.Y.Z` reported `version=dev`** — added a `debug.ReadBuildInfo()` fallback, gated so ldflags always win. ### Release-notes taxonomy `changelog.use: github-native` hands the release body to GitHub's per-PR generator, grouped by a new `.github/release.yml`. Getting that grouping right took several passes and the reasoning is recorded in the file, because the failure mode is silent — a wrong-looking changelog, not a red build: - **Dependabot is separated by author, not by label.** `github_actions` is path-based (labeler maps `.github/workflows/**`), so it marks our own CI PRs too — excluding the *label* filed all our CI work under Dependencies, and dropping it instead empties that category, since Dependabot's action bumps carry `github_actions` and not `dependencies`. - **`documentation` needed the same exclude**, because the npm Dependabot config points at the workspace root, so every npm bump touches `docs/package.json`. - **`🔧 CI & build` sits above `📚 Documentation`**, or it never matches: AGENTS.md requires a doc update with every change, so 10 of the last 12 human `github_actions` PRs also carry `documentation`. It's a trade — a few docs PRs that touch a workflow now file under CI — and both sides are counted in the file. The invariant, recorded inline: *any category keyed on a label a Dependabot PR can carry needs the author exclude* — labeler's path labels **and** the ecosystem labels Dependabot applies itself (`javascript` is in neither `labeler.yml` nor our categories). This routing can't be linted locally, but it can be dry-run against real history from the branch — `configuration_file_path` resolves relative to `target_commitish`: ```bash gh api -X POST repos/Wave-RF/WaveHouse/releases/generate-notes \ -f tag_name=v0.1.0 -f target_commitish=ci-tagger \ -f configuration_file_path=.github/release.yml --jq .body ``` Run on this config: 23/23 merged Dependabot PRs in 📦 Dependencies, zero leaking into 🔧 CI & build or 📚 Documentation. ### Tooling and housekeeping - `make release-server` / `release-sdk-ts` / `release-sdk-go` → `scripts/release.sh`, which preflights (on main, clean tree, synced with origin, tag free both sides, `CI` green on that exact commit), prints what will publish, and prompts. `DRY_RUN=1` stops after the plan. - `release.yml` verifies its own attestations before finishing, so a release publishing unverifiable provenance goes red. - Lowercase artifact names (`wavehouse_linux_amd64.tar.gz`), `.zip` on Windows, archives no longer bundle the 324 KB CHANGELOG. - `CHANGELOG.md` merged from dozens of duplicate `###` bands under one `## Unreleased` into 5 canonical sections and stamped `## [0.1.0]`. Verified content-preserving — every entry byte-identical, checked by diffing the sorted multiset of non-heading lines. (No count is quoted: the base moves with every merge, and a stale figure beside a "verified" claim is worse than none.) ## Test plan - [x] `make ci` green - [x] All four goreleaser tag modes rendered through a real registry push (stable → `:v1.2.3`+`:latest`; prerelease → `:v0.1.0-alpha.1`+`:alpha`; dev → `:dev-<sha>`+`:dev`; env-unset → safe `:latest` fallback) - [x] #475 reproduced and fixed against real `node-semver` using the ten actually-published versions — old scheme resolves `^0.0.0-dev.h04a…` to the June 8 build; new scheme resolves forward and is unsatisfiable by `^0.1.0` - [x] Full 8-target build matrix (4 GOOS × 2 GOARCH), archives + checksums, Windows `.zip` - [x] Version stamping verified across all four build paths: `go build` in-VCS, `-ldflags`, no-VCS, and `make build` - [x] `scripts/release.sh` exercised end-to-end in a scratch repo — real tag creation and push, re-run refusal, abort at prompt, dirty tree, wrong branch, bad version, missing `VERSION` - [x] Container image smoke-tested (`wavehouse health` → clean exit 1, labels/user/healthcheck correct) - [x] Ten rounds of the pre-push gate (`pre-push-reviewer` + `docs-reviewer`), both `ship_it` at zero findings - [x] Release-notes routing dry-run against real history — all 23 merged Dependabot PRs land in 📦 Dependencies, none leak into 🔧 CI & build (41) or 📚 Documentation (54). `generate-notes` accepts `configuration_file_path` resolved from `target_commitish`, so this validates from the branch *before* merge - [ ] Post-merge: first real tag exercises the release path end-to-end ## Notes for review - **The `## [0.1.0] - 2026-08-19` stamp is a judgement call.** This branch promotes `## Unreleased`, which its own new docs prescribe as a separate pre-tag PR. The date is the *intended* cut date and the `[0.1.0]` link 404s until the tag exists. If tagging slips, correct the date and move anything merged in between up from `## Unreleased` — or say the word and I'll split it out. - **`PROJECT_BOARD_TOKEN` should be deleted.** Retiring `triage.yml` left it with no consumers — an unused PAT with Projects scope on a public repo. A branch can't remove a repo secret, so `gh secret delete PROJECT_BOARD_TOKEN` plus revoking the PAT is a manual step. The docs now say so rather than claiming it's already gone. - **`:latest` and the bare CDN URL don't resolve yet.** `ghcr.io/wave-rf/wavehouse:latest` 404s and npm's `latest` still points at the June 4 bootstrap `0.0.0-dev.0f8826c`. Both self-heal the moment a **stable** `v0.1.0` / `clients/ts/v0.1.0` ships — which is why the first release should not be a prerelease, despite #149's title. - **Deferred**: adding `/wavehouse` to `.gitignore` (the pre-push reviewer's `go build ./cmd/...` drops a binary there that voids the `make ci` marker), and a post-release smoke test that installs the *published* SDK against the *published* image. ## Related Issues Closes #431 Closes #475 Advances #149 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_015bVwHtNakQgmnBhMcfW8pe --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reconciles the Go SDK branch with main for the first time since 2026-08-11 (merge base e945ecc). All 14 conflicts were prose; no Go or TypeScript source conflicted. Ten of the conflicts share one cause: #489's WH001 rule unwrapped hard-wrapped prose across the docs tree on main, so the branch's edits sat on pre-reflow text. Each was resolved by taking main's reflowed paragraph and re-applying the branch's semantic edit into it. Resolutions of note: - AGENTS.md #14, README.md, docs/index.mdx, why-wavehouse.md: the branch's "zero third-party runtime dependencies in both SDKs" is no longer true — main added eventsource-parser to the TypeScript SDK. Kept the branch's two-SDK structure with main's dependency facts: one runtime dependency in TypeScript, none in Go. - CHANGELOG.md: main cut 0.1.0 on 2026-08-19, so the branch's Go SDK entry had landed inside a released section. Moved it to the top of main's new Unreleased/Added, and re-homed the branch-only TypeScript docs-corrections entry under Unreleased/Fixed. - Makefile: unioned the verify-parallel leaf list (16 leaves) and kept main's per-leaf inventory comment. - sdk/streaming.md: main's rewrite already covers both cautions the branch added — the projection-dedup caveat (#449) in step 3 and the like/not_like backfill-vs-live divergence (#451) in the operator section — so main's version supersedes it wholesale. - sdk/queries.md: kept main's /v1/ops/query routing and operator rows, grafted on the branch's select_all carve-out (unrestricted/admin roles do get SELECT *), the not_like wire token, and the aggregation allowlist. - sdk/index.mdx: dropped the branch's stale CDN paragraph — main's reflowed copy carries the post-#470 fetch wording and the correct /development#cutting-a-release anchor. The branch's dead #releasing-the-sdks link would have failed the docs build. - reverse-proxy.mdx: #428 is closed and the fix shipped in 0.1.0, so main's deletion of the prefix caution stands; kept a Go example on main's renamed /api/wavehouse prefix. - development.md: discarded the branch's release section entirely — main already documents the clients/go/vX.Y.Z tag scheme it was guessing at. Also clears the forward-references main left for this PR: the "(pending #434)" marker on make release-sdk-go, the paragraph saying it refuses to run until clients/go/ exists, and the missing Go line in the release example. Documents what a Go SDK release publishes (the module proxy serves the tag; no workflow fires, so no GitHub Release).
Three catch-up changes, all client-side. The server is untouched. Routes: main merged every admin-gated endpoint under /v1/ops (#479) with no aliases, so thirteen call sites were 404ing against a current server — schema list/refresh, DLQ stats, raw SQL, policy get/put/validate, and pipes CRUD, plus the codegen CLI's schema fetch. Rewrote them along with the tests, the shared wire_cases.json fixture, and the Go SDK docs. The fixture is replayed by both conformance runners, so the stale paths broke the TypeScript half too; `make test-conformance-ts` is back to 45/45. ClientOptions.Headers: the TypeScript SDK gained options.headers in #456 and Go had no equivalent. Headers now apply to every request the client makes, REST and SSE alike — which is also how an operator sends the server's non-JWT X-Operator-Key. The SDK's own headers are set afterwards and win a collision; net/http canonicalizes names, so matching is case-insensitive; the map is copied at construction so later mutation can't reach into requests. SSE robustness, mirroring main's fetch-based rewrite (#470). The Go SDK already authenticated by header, so that part was never stale, but three gaps were: - A credentialed stream followed redirects. net/http drops Authorization on a cross-host hop while forwarding custom headers verbatim, so a redirect either downgraded the stream to default_role in silence or handed configured secrets to wherever it pointed. Now refused with a terminal SSE_REDIRECT. Uncredentialed streams still follow. - A 200 with any content type was treated as an event stream, so an auth gateway's login page left the stream sitting in StatusLive delivering nothing. Now a terminal SSE_BAD_CONTENT_TYPE. - Every failure collapsed into one retryable SSE_ERROR, and malformed frames came back as a bare fmt.Errorf, so errors.As and IsRetryable didn't work on them. Replaced with the taxonomy the TypeScript SDK uses — SSE_AUTH_ERROR, SSE_NETWORK_ERROR, SSE_CONNECT_ERROR, SSE_REDIRECT, SSE_BAD_CONTENT_TYPE, SSE_PARSE_ERROR, SSE_READ_ERROR — each with its own retryable flag, all delivered as *Error. Also documents what main changed underneath the Go SDK without changing its code: DateTime values arrive canonicalized to RFC 3339 UTC (#402), SSE applies policy row-filters per subscriber and fails closed (#381, #457), /v1/stream is ungated so WaveHouse never 401s a stream, and /v1/ops/dlq/stats is absent (404) when the DLQ is disabled rather than returning empty stats. Tests: terminal-failure table (bad content type, missing content type, credentialed redirect, non-HTTP scheme), redirect-followed-when- uncredentialed, typed retryable parse errors, and header precedence and copying on both transports.
The six Go SDK pages were authored before #489 landed WH001 (no-hard-wrapped-prose), so they wrapped prose at ~76 columns while the rest of the docs tree had been reflowed. `make fix` output, whitespace only — verified no content changed in any of the four files.
biome.json's files.includes covered clients/ts, tests/e2e/sdk, docs, and scripts, but not tests/conformance — so the 311-line conformance_ts.mjs this branch adds was invisible to make lint-ts / fmt-ts / fix-ts. Including it surfaced one formatting fix, applied here.
WH001's autofix deliberately never runs over .mdx, so this aside — added on this branch before the rule landed — had to be joined by hand. Text unchanged.
The gomod entry was `directory: /`, which covers the root module only — Dependabot does not descend into nested modules, so clients/go/go.mod was untracked. Converted to `directories:` (the form the github-actions entry already uses for the same reason) so both share one schedule, group, and commit prefix rather than forking the policy into a second block. clients/go is stdlib-only today, so this catches the first dependency it takes on rather than closing a gap that already exists — the same argument the github-actions comment block makes about composite actions.
area/sdk globbed clients/ts/** and tests/e2e/sdk/** only, so every file in the new Go SDK and the shared conformance runner went unlabeled. Adds clients/go/** and tests/conformance/**. Also broadens the dependencies label from root-anchored go.mod/go.sum to **/go.mod and **/go.sum, so the nested clients/go module's manifests get labeled the way clients/ts/package.json already does via **/package.json. Left area/infra root-anchored — the TypeScript SDK's manifest doesn't carry that label either. Verified by replaying the config's globs over the branch's diff: clients/go sources land on area/sdk + go, the codegen CLI on area/sdk, and clients/go/go.mod on area/sdk + dependencies. Note this takes effect from the next PR, not this one: housekeeping.yml resolves labeler.yml from the default branch, not the PR head.
test-go-sdk ran a bare `go test -race ./...` — no profile, no threshold — so the Go SDK would have shipped as the only component in the repo with no coverage gate while its TypeScript peer has three. It now collects covdata the same way the root-module Go suites do and renders through scripts/cov, with a `go-sdk` suite gated at 75% against a measured 82.7% (clients/go 88.6%, cmd/wavehouse-codegen 55.8%). That ratio sits inside the band the existing floors already use — unit is 80 against ~91% actual — leaving headroom without being slack enough to rot. Gated standalone rather than merged into the Go total: the total feeds threshold.total and the README badge, and folding a shipped client library into the server's project-wide number would move that badge for unrelated reasons. The same separation the TypeScript SDK gets via ts-*. Being a nested module, clients/go is invisible to the root -coverpkg anyway, so it cannot leak into the total and needs no exclude.paths entry — verified: zero clients/go rows in the merged profile, Go total unmoved at 81.2%. One wrinkle worth recording: `go tool cover -html` resolves a profile's package paths through the module in the working directory, so rendering from the repo root fails to find the nested packages. renderHTML now runs with cmd.Dir set to clients/go and absolute paths. CI needed no functional change — the unit job already uploads the whole tmp/coverage tree, so the fragment arrives and the coverage job gates it.
The client-side filter behind .Stream() compared every string operand lexically. That was defensible before #402; it isn't now. The server canonicalizes every top-level DateTime/DateTime64 value to RFC 3339 UTC before publishing, 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 — and lexically the payload sorts BELOW the constant, so OpGte withheld a row that was chronologically equal, and OpEq called two spellings of one instant different. Both sides now parse as instants, mirroring what internal/policy's row filter does for a DateTime column. Deliberately narrow: only RFC 3339 with an explicit offset or Z counts. A zone-less spelling names an instant only relative to the column's declared timezone, which the server reads from the schema and a stream subscriber does not have — reading it as UTC would move the instant, so those fall through rather than being silently reinterpreted. A ',' fraction is ISO 8601 but not ClickHouse, and is refused for the same reason the ingest grammar refuses it. The operand length is pre-gated at 64 bytes like the server's, so a megabyte 'timestamp' isn't scanned once per filter per event. Ordering an instant against a non-instant now fails closed instead of falling back to text, which could admit rows the query path excludes — the same direction the server errs in. The usual trigger is a zone-less constant, which now yields no rows rather than wrong ones. Also: a column missing from the payload no longer matches the literal string "<nil>" through the fmt.Sprint equality fallback. Not fixed here, and worth its own change: the TypeScript SDK's matchesFilters compares the same way, and its compareOrdered comment still asserts that lexicographic order 'is correct for ISO-8601 timestamps' — true only while both sides share an offset spelling, which #402 stopped guaranteeing. Its unknown-operator branch also returns true where Go and the server fail closed.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
clients/go/http.go (1)
96-105: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply the same credentialed-redirect refusal to REST requests.
doRequestuseshctx.httpClientwith the default redirect policy.net/httpstripsAuthorizationon a cross-host hop, but it forwards custom headers verbatim. TheClientOptions.Headersdocumentation names "a gateway credential" as an intended use, so a redirect to another host sends that credential to the redirect target.
connectinclients/go/stream.go(lines 347-361) already refuses redirects when the request carries a credential, and its comment states the samenet/httpbehavior. The REST transport lacks that protection, so the two transports enforce different credential-scope rules for the same configured headers.Copy the client and set
CheckRedirectwhen configured headers or anAuthorizationheader are present, as the stream path does. If following redirects on REST is required, strip the configured headers on a cross-host hop instead.🔒 Proposed fix
+ client := hctx.httpClient + if authHeader != "" || len(hctx.headers) > 0 { + // Same rule as the SSE path: never carry a credential across a + // redirect. net/http drops Authorization on a cross-host hop but + // forwards configured headers verbatim. Copy so a caller-supplied + // client keeps its own CheckRedirect elsewhere. + c := *client + c.CheckRedirect = func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + } + client = &c + } + var lastErr error maxAttempts := hctx.maxRetries + 1Then use
client.Do(req)in place ofhctx.httpClient.Do(req)at line 107.Note that
TestConfiguredHeadersOnRESTRequestsandTestConfiguredHeadersAreCopiednever exercise a redirect, so neither test discriminates this case.Source: Linters/SAST tools
clients/go/schema.go (1)
14-20: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDecode both supported schema response shapes.
SchemaNamespace.Listdecodes/v1/ops/schemadirectly into[]TableSchema. ThefetchSchemasimplementation inclients/go/cmd/wavehouse-codegen/main.gosupports both an array and a name-keyed object. A map response therefore fails before this method buildsSchemas. Add dual-shape decoding and a test for the object response.clients/go/namespaces_test.gocurrently covers only the array form.Proposed fix
- var raw []TableSchema + var payload json.RawMessage if err := doRequest(ctx, s.ctx, requestOptions{ method: "GET", path: "/v1/ops/schema", - }, &raw); err != nil { + }, &payload); err != nil { return nil, fmt.Errorf("list schemas: %w", err) } + + var raw []TableSchema + if err := json.Unmarshal(payload, &raw); err == nil { + schemas := make(Schemas, len(raw)) + for _, t := range raw { + schemas[t.Name] = t + } + return schemas, nil + } + + var schemas Schemas + if err := json.Unmarshal(payload, &schemas); err != nil { + return nil, fmt.Errorf("decode schemas: %w", err) + } + return schemas, nilSource: Linters/SAST tools
clients/go/cmd/wavehouse-codegen/main.go (3)
31-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the remaining CLI helpers and failure paths.
main_test.gocoverschTypeToGo,pascalCase,findTopLevelComma, andgenerate, but notflagValue,parseArgs,fetchSchemas,sortedKeys, oros.Exitpaths. Add these tests. Update table-driven tests to includenameand callt.Run(tt.name, ...).Source: Coding guidelines
258-281: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck the first rune before validating the identifier.
result[0]reads the first UTF-8 byte. For"٢fa_events", the check misses the leading digit, andgo/formatrejects the generated identifier. Use the first rune and add a table-driven test for a non-ASCII digit.
352-390: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winEnforce
0600when overwriting the output file.
os.WriteFilepreserves the permissions of an existing file. Applyos.Chmod(args.out, 0o600)after a successful write, or replace the file through a0600temporary file before reporting success.docs/src/content/docs/sdk/index.mdx (2)
361-370: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep the Cloudflare Access secret out of browser examples.
This section also documents browser and CDN usage. A reader can copy this example into frontend code and bundle
process.env.CF_ACCESS_CLIENT_SECRET. That exposes the service-token credential. Cloudflare documents this credential for automated systems, not browser bundles. (developers.cloudflare.com)Mark this example as server-side only. Direct browser clients through a trusted BFF or proxy that injects the headers.
Source: MCP tools
441-441: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire an origin allowlist before following credentialed redirects.
options.fetchreceives the configured headers, and non-Authorizationheaders can cross an unexpected redirect target. Require an explicit trusted-origin check before overridingredirect, or remove this recommendation.Source: MCP tools
docs/src/content/docs/reverse-proxy.mdx (1)
237-238: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQualify the NDJSON batch-size claim.
This nginx setting caps the complete NDJSON request at 16 MiB. WaveHouse also applies a 16 MiB request cap and a 10 MiB per-line cap. State these limits so “unbounded batch” does not imply unlimited uploads.
Source: MCP tools
docs/src/content/docs/access-control.mdx (1)
92-92: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep privileged credentials out of process arguments.
- In
docs/src/content/docs/access-control.mdx#L92,$(cat operator.key)places the operator key incurl’sargv. Use a protectedcurl --configfile or stdin.- In
docs/src/content/docs/sdk/reference.md#L136, the TypeScript CLI currently accepts credentials only through--auth/-a. Add a non-argvinput path and make it the primary documented method. Update the options table and CLI help.docs/src/content/docs/index.mdx (1)
189-194: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
cloudCtafrontmatter for this CTA.This MDX page renders
<CloudCta>directly. The repository rule requires Cloud CTA opt-in throughcloudCtafrontmatter, not through a component import. Move this CTA configuration into frontmatter and remove the component usage.As per coding guidelines:
docs/src/content/docs/**/*.mdxmust opt a page into the Cloud CTA withcloudCtafrontmatter, not by importing the component.Source: Coding guidelines
docs/src/content/docs/deployment.md (1)
159-162: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the conditional configuration comment.
Line 159 currently starts with
WH_POLICY_FILE_PATH is set, which is not a complete condition. Change it toIf WH_POLICY_FILE_PATH is setso the boot-failure requirement is unambiguous.Proposed wording fix
-# WH_POLICY_FILE_PATH is set, the file MUST exist, parse, and pass policy +# If WH_POLICY_FILE_PATH is set, the file MUST exist, parse, and pass policydocs/src/content/docs/development.md (1)
480-481: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicate SDK tree entry.
The project tree already documents
clients/with bothts/andgo/at Lines 470-473. Lines 480-481 add a secondclients/section and omit the Go SDK, so the documented tree contradicts itself.Proposed tree cleanup
-├── clients/ # Client SDKs -│ └── ts/ # TypeScript SDK (`@wavehouse/sdk`, pnpm workspace)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 040aefe8-4d1f-4226-9dbb-01d1a99c59c6
📒 Files selected for processing (53)
.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/cmd/wavehouse-codegen/main.goclients/go/conformance_test.goclients/go/dlq.goclients/go/http.goclients/go/http_test.goclients/go/namespaces_test.goclients/go/pipes.goclients/go/policy.goclients/go/schema.goclients/go/stream.goclients/go/stream_test.goclients/go/table.goclients/go/testdata/wire_cases.jsonclients/go/wavehouse.godocs/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/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.mdxdocs/src/content/docs/sdk/pipes.mddocs/src/content/docs/sdk/queries.mddocs/src/content/docs/sdk/reference.mddocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/why-wavehouse.mdscripts/cov/main.gotests/conformance/conformance_ts.mjs
💤 Files with no reviewable changes (1)
- CHANGELOG.md
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (6)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
- Never hard-wrap prose. One paragraph is one line. No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
Files:
docs/src/content/docs/sdk/admin.mddocs/src/content/docs/404.mddocs/src/content/docs/getting-started.mddocs/src/content/docs/claude-code.mddocs/src/content/docs/sdk/streaming.mdCONTRIBUTING.mddocs/src/content/docs/sdk/go/admin.mddocs/src/content/docs/deployment.mddocs/src/content/docs/sdk/pipes.mddocs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/api.mddocs/src/content/docs/why-wavehouse.mdclients/go/README.mddocs/src/content/docs/architecture.mddocs/src/content/docs/sdk/queries.mdREADME.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/index.mdAGENTS.mddocs/src/content/docs/sdk/reference.mddocs/src/content/docs/development.mddocs/src/content/docs/sdk/go/streaming.mddocs/src/content/docs/sdk/go/pipes.md
docs/src/content/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Every code change should update the corresponding docs in the same PR. A code change without its doc update is incomplete.
Files:
docs/src/content/docs/sdk/admin.mddocs/src/content/docs/404.mddocs/src/content/docs/index.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/getting-started.mddocs/src/content/docs/claude-code.mddocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/sdk/go/admin.mddocs/src/content/docs/deployment.mddocs/src/content/docs/sdk/pipes.mddocs/src/content/docs/sdk/go/reference.mddocs/src/content/docs/api.mddocs/src/content/docs/why-wavehouse.mddocs/src/content/docs/architecture.mddocs/src/content/docs/access-control.mdxdocs/src/content/docs/pipes.mdxdocs/src/content/docs/sdk/queries.mddocs/src/content/docs/sdk/go/queries.mddocs/src/content/docs/sdk/go/index.mddocs/src/content/docs/sdk/reference.mddocs/src/content/docs/development.mddocs/src/content/docs/sdk/go/streaming.mddocs/src/content/docs/sdk/go/pipes.md
**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
- In MDX, leave a blank line between a JSX tag and a code fence.
Files:
docs/src/content/docs/index.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/access-control.mdxdocs/src/content/docs/pipes.mdx
docs/src/content/docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/docs/**/*.mdx: - Opt a page into the Cloud CTA withcloudCtafrontmatter, not by importing the component.
- Never hand-write
®or™in prose.
Files:
docs/src/content/docs/index.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/access-control.mdxdocs/src/content/docs/pipes.mdx
**/*.{go,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
- No global state: Dependencies are passed explicitly (constructor injection).
Files:
clients/go/schema.goclients/go/dlq.goclients/go/cmd/wavehouse-codegen/main.goclients/go/namespaces_test.goclients/go/policy.goclients/go/pipes.goclients/go/table.goclients/go/http.goclients/go/http_test.goclients/go/wavehouse.goclients/go/client_test.goclients/go/conformance_test.goscripts/cov/main.goclients/go/stream_test.goclients/go/stream.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Table-driven tests: Usetests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
Files:
clients/go/schema.goclients/go/dlq.goclients/go/cmd/wavehouse-codegen/main.goclients/go/namespaces_test.goclients/go/policy.goclients/go/pipes.goclients/go/table.goclients/go/http.goclients/go/http_test.goclients/go/wavehouse.goclients/go/client_test.goclients/go/conformance_test.goscripts/cov/main.goclients/go/stream_test.goclients/go/stream.go
🧠 Learnings (4)
📚 Learning: 2026-08-20T15:42:29.972Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 505
File: docs/src/content/docs/404.md:14-19
Timestamp: 2026-08-20T15:42:29.972Z
Learning: For Markdown files under docs/src/content/docs, do not flag multi-line HTML comment interiors as violations of the one-paragraph-per-line rule. Markdownlint masks HTML comment contents, and joining those lines during autofix could write masked text back into the file; preserve the comment formatting instead.
Applied to files:
docs/src/content/docs/404.md
📚 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/index.mdxdocs/src/content/docs/access-control.mdxdocs/src/content/docs/pipes.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:
docs/src/content/docs/deployment.mdREADME.mdAGENTS.mddocs/src/content/docs/development.md
📚 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/http_test.goclients/go/stream_test.go
🪛 ast-grep (0.45.1)
scripts/cov/main.go
[warning] 348-349: 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)
🪛 Betterleaks (1.7.3)
docs/src/content/docs/development.md
[high] 212-214: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 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 TypeScript SDK wraps the w...
(PUNCTUATION_PARAGRAPH_END)
docs/src/content/docs/sdk/index.mdx
[style] ~347-~347: 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] ~382-~382: 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] ~402-~402: 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] ~415-~415: 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] ~415-~415: 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] ~433-~433: 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] ~466-~466: 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] ~479-~479: 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] ~479-~479: Consider using the typographical ellipsis character here instead.
Context: ...the two aren't structurally assignable. { ...init, dispatcher } as never covers the ...
(ELLIPSIS)
[style] ~479-~479: 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.md
[style] ~115-~115: Since ownership is already implied, this phrasing may be redundant.
Context: ...sues/449)) — so key on timestamp plus your own row identity if duplicates matter. Rep...
(PRP_OWN)
[style] ~122-~122: Since ownership is already implied, this phrasing may be redundant.
Context: ...ed more of on this path; see Supplying your own fetch. ...
(PRP_OWN)
[grammar] ~126-~126: Ensure spelling is correct
Context: ...nection's JWT claims. A stream on a row-policied table therefore delivers only the rows the policy admit...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~129-~129: ‘On top of that’ might be wordy. Consider a shorter alternative.
Context: ...ect. ### Client-Side Stream Filtering On top of that, when a QueryBuilder with .where() ...
(EN_WORDINESS_PREMIUM_ON_TOP_OF_THAT)
[style] ~210-~210: 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] ~210-~210: 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] ~210-~210: 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/deployment.md
[uncategorized] ~104-~104: The official name of this software platform is spelled with a capital “H”.
Context: ...tion — verify a downloaded archive with gh attestation verify <file> --repo Wave-RF/WaveHouse --signer-workflow Wave-RF/WaveHouse/.github/workflows/release.yml. (This covers th...
(GITHUB)
[typographical] ~376-~376: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...NATS stream under subjects dlq.{table}. This prevents infinite retry loops. Mon...
(WRB_QUESTION_MARK)
docs/src/content/docs/sdk/pipes.md
[style] ~22-~22: 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/go/reference.md
[style] ~25-~25: 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] ~31-~31: Use a hyphen to join words.
Context: ...hprovider, marshal failures) are plain wrapped errors; handle theerrors.As ==...
(QB_NEW_EN_HYPHEN)
docs/src/content/docs/api.md
[style] ~249-~249: Consider using the typographical ellipsis character here instead.
Context: ...n"}| Malformed request body | | 400 |{"error":"unknown column ... for table ..."}(also:missing requir...
(ELLIPSIS)
[style] ~249-~249: Consider using the typographical ellipsis character here instead.
Context: ...nown column ... for table ..."}(also:missing required column ..., type mismatch for column ..., null...
(ELLIPSIS)
[style] ~249-~249: Consider using the typographical ellipsis character here instead.
Context: ...(also:missing required column ..., type mismatch for column ..., null value for non-nullable column ....
(ELLIPSIS)
[style] ~249-~249: Consider using the typographical ellipsis character here instead.
Context: ...n ..., type mismatch for column ..., null value for non-nullable column ...`) | Schema validation failure (unknown ...
(ELLIPSIS)
[style] ~282-~282: Consider using a more formal/concise alternative here.
Context: ...edge cases] - Digit-strings of lengths other than 9–10 are ClickHouse's own forms — calen...
(OTHER_THAN)
[grammar] ~292-~292: Ensure spelling is correct
Context: ...nds (shorter runs it rejected outright, where best_effort reads "2026" as a year)...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~531-~531: Redundant conjunctions can lead to confusion; consider removing a conjunction here.
Context: ...meters can be supplied via query string and/or JSON body. Results are cached in the sh...
(AND_OR)
docs/src/content/docs/architecture.md
[style] ~75-~75: Since ownership is already implied, this phrasing may be redundant.
Context: ...e JWT/JWKS authentication middleware is its own package, [auth/](#auth--authenticatio...
(PRP_OWN)
[typographical] ~125-~125: Consider using an em dash in dialogues and enumerations.
Context: - worker.go — StartIngestWorker lau...
(DASH_RULE)
docs/src/content/docs/access-control.mdx
[style] ~302-~302: Consider using “who” when you are referring to a person instead of an object.
Context: ...e rows are also invisible to the writer that produced them. (A Float column may in...
(THAT_WHO)
[typographical] ~414-~414: Consider using an em dash in dialogues and enumerations.
Context: - Numeric columns (Int*/UInt*/`Fl...
(DASH_RULE)
[typographical] ~461-~461: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...lidation**, or WaveHouse refuses to boot. That turns a typo, a missing mount, or ...
(WRB_QUESTION_MARK)
[grammar] ~461-~461: Ensure spelling is correct
Context: ...s to boot. That turns a typo, a missing mount, or a policy invalid under a newly-tigh...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[typographical] ~462-~462: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...s denied (logged loudly, admin included). Seed one via PUT /v1/ops/policy using...
(WRB_QUESTION_MARK)
[style] ~493-~493: Since ownership is already implied, this phrasing may be redundant.
Context: ...ous callers get nothing, viewer reads its own tenant's rows with sensitive columns ma...
(PRP_OWN)
docs/src/content/docs/sdk/queries.md
[style] ~84-~84: Consider using the typographical ellipsis character here instead.
Context: ....fetch()does. Mutually exclusive with.select(...) and with aggregations (.count(), ....
(ELLIPSIS)
docs/src/content/docs/sdk/go/index.md
[style] ~9-~9: 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)
[locale-violation] ~115-~115: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...s copied at NewClient, so mutating it afterwards changes nothing. For the two remaining...
(AFTERWARDS_US)
[grammar] ~163-~163: Use a hyphen to join words.
Context: ...uthprovider, marshal errors) are plain wrapped errors; handle theerrors.As ==...
(QB_NEW_EN_HYPHEN)
[style] ~190-~190: Consider using the typographical ellipsis character here instead.
Context: ...Any slice batches.* Reflection allows []ClickRow{...} to use the same NDJSON batch path as ...
(ELLIPSIS)
AGENTS.md
[style] ~121-~121: Consider using the typographical ellipsis character here instead.
Context: ...ventions - Table-driven tests: Use tests := []struct{ name string; ... } with t.Run(tt.name, ...) for test ...
(ELLIPSIS)
[style] ~121-~121: Consider using the typographical ellipsis character here instead.
Context: ...s := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases. - **Shared mocks in...
(ELLIPSIS)
[style] ~126-~126: Consider using the typographical ellipsis character here instead.
Context: ... without NATS. - Pipes helpers: Use pipes.NewMemoryStore(queries...) for in-memory pipes testing without N...
(ELLIPSIS)
[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)
[style] ~326-~326: 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: ...prose makes every later edit rewrap the whole block, so a one-word change lands as a ...
(EN_REPEATEDWORDS_WHOLE)
[uncategorized] ~342-~342: The official name of this software platform is spelled with a capital “H”.
Context: ...fter doing that. - WH001 is off under .github/ and .claude/ (CI docs and agent p...
(GITHUB)
docs/src/content/docs/sdk/reference.md
[style] ~26-~26: 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] ~38-~38: A comma is missing here.
Context: ...OR| No | Stream could not be started (e.g. a non-absolutebaseURL) | | 0 | SSE_...
(EG_NO_COMMA)
[style] ~59-~59: 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] ~63-~63: 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] ~63-~63: 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)
docs/src/content/docs/development.md
[style] ~327-~327: 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] ~380-~380: Since ownership is already implied, this phrasing may be redundant.
Context: .../`. The orchestrator always provisions its own stack — a fresh ClickHouse testcontaine...
(PRP_OWN)
[uncategorized] ~588-~588: The official name of this software platform is spelled with a capital “H”.
Context: ...ps:- **GitHub Actions** (root **and**/.github/actions/setup-env`) — outdated action v...
(GITHUB)
[uncategorized] ~588-~588: The official name of this software platform is spelled with a capital “H”.
Context: ...ons tracked against the SHA pins across .github/workflows/* and the setup-env compos...
(GITHUB)
[uncategorized] ~593-~593: The official name of this software platform is spelled with a capital “H”.
Context: ...o** directories. directory: / reaches .github/workflows/ but does not descend into `...
(GITHUB)
[uncategorized] ~593-~593: The official name of this software platform is spelled with a capital “H”.
Context: ...b/workflows/but does not descend into.github/actions/*/action.yml, so the setup-en...
(GITHUB)
[uncategorized] ~595-~595: The official name of this software platform is spelled with a capital “H”.
Context: ...de job down at once. See the comment in .github/dependabot.yml for the condition that ...
(GITHUB)
[typographical] ~609-~609: To join two clauses or introduce examples, consider using an em dash.
Context: ..., so open a PR renaming that heading to ## [X.Y.Z] - YYYY-MM-DD and adding the matching link...
(DASH_RULE)
[style] ~609-~609: ‘at the foot of’ might be wordy. Consider a shorter alternative.
Context: ... and adding the matching link reference at the foot of the file. Nothing in the release pipeli...
(EN_WORDINESS_PREMIUM_AT_THE_FOOT_OF)
[style] ~637-~637: Consider using the typographical ellipsis character here instead.
Context: ...f the ref name, so v* never matches a clients/... tag — which is what keeps a client rel...
(ELLIPSIS)
[typographical] ~641-~641: Consider using an em dash in dialogues and enumerations.
Context: - Server — a GitHub Release with ...
(DASH_RULE)
[uncategorized] ~641-~641: Do not mix variants of the same word (‘pre-release’ and ‘prerelease’) within a single text.
Context: ...v0.1.0-alpha.1) is marked as a GitHub pre-release, so it never takes the "Latest release"...
(EN_WORD_COHERENCY)
[uncategorized] ~642-~642: 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] ~642-~642: 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] ~642-~642: 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] ~642-~642: 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] ~644-~644: 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)
[style] ~646-~646: Since ownership is already implied, this phrasing may be redundant.
Context: ...image in GHCR. The release job verifies its own attestations before finishing, so a rel...
(PRP_OWN)
[style] ~666-~666: To elevate your writing, consider using more formal language here.
Context: ...wo things the make targets do for you and the UI does not: none of the preflight ...
(AND_WHEREAS)
docs/src/content/docs/sdk/go/streaming.md
[typographical] ~161-~161: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...where (mapped to wire tokens eq/neq). OpLike/OpNotLike use SQL LIKE seman...
(WRB_QUESTION_MARK)
🔇 Additional comments (50)
clients/go/wavehouse.go (2)
44-53: LGTM!Also applies to: 85-92, 107-107
147-147: 🗄️ Data Integrity & IntegrationNo route mismatch found. The server registers
POST /v1/ops/query, and both SDKs use the same path.clients/go/http.go (1)
32-39: LGTM!Also applies to: 186-202, 204-210, 212-224
clients/go/table.go (1)
84-84: LGTM!clients/go/stream.go (1)
10-10: LGTM!Also applies to: 234-247, 249-272, 289-308, 320-327, 340-361, 363-390, 396-407, 458-467, 471-477, 493-498
clients/go/client_test.go (1)
79-80: LGTM!clients/go/http_test.go (1)
116-116: LGTM!Also applies to: 138-138, 202-202
clients/go/namespaces_test.go (1)
26-27: LGTM!Also applies to: 135-135
clients/go/stream_test.go (1)
470-554: LGTM!Also applies to: 556-581, 583-612, 614-654
clients/go/testdata/wire_cases.json (1)
457-457: LGTM!Also applies to: 470-477, 503-510, 522-522, 541-541, 554-560, 575-575, 590-590
clients/go/conformance_test.go (1)
95-105: LGTM!tests/conformance/conformance_ts.mjs (1)
25-34: LGTM!Also applies to: 54-64, 310-311
clients/go/schema.go (1)
31-38: LGTM!clients/go/policy.go (1)
9-23: LGTM!Also applies to: 26-35, 38-48
clients/go/dlq.go (1)
10-18: LGTM!Also applies to: 20-40, 42-45
clients/go/pipes.go (2)
10-23: LGTM!Also applies to: 27-35, 39-47, 51-67, 69-92, 94-106
15-23: 📐 Maintainability & Code QualityRun the required Go validation checks.
Confirm that every new Go function has corresponding test cases. Run
make lintandmake testbefore completion. Use table-driven tests witht.Runfor test cases.As per coding guidelines, every new Go function must have corresponding test cases, and
make lintplusmake testmust run before completion.Source: Coding guidelines
clients/go/cmd/wavehouse-codegen/main.go (5)
1-29: LGTM!
76-125: LGTM!
127-239: LGTM!
241-256: LGTM!
284-350: LGTM!clients/go/README.md (1)
5-5: LGTM!Also applies to: 83-83, 209-209, 221-221
docs/src/content/docs/sdk/index.mdx (1)
3-12: LGTM!Also applies to: 14-342, 345-357, 375-439, 443-483, 485-532, 563-574
docs/src/content/docs/sdk/pipes.md (1)
2-2: LGTM!Also applies to: 6-6, 8-43, 51-53, 76-84
docs/src/content/docs/sdk/queries.md (1)
6-6: LGTM!Also applies to: 27-49, 58-65, 82-84, 122-124, 147-148, 150-165, 220-227, 247-249, 255-262
docs/src/content/docs/reverse-proxy.mdx (1)
4-5: LGTM!Also applies to: 23-23, 44-85, 133-142, 178-178, 201-215, 272-272
docs/src/content/docs/sdk/admin.md (1)
2-2: LGTM!Also applies to: 6-6, 23-25, 29-29, 61-72
docs/src/content/docs/sdk/go/admin.md (1)
6-23: LGTM!Also applies to: 27-64, 68-82, 86-100
docs/src/content/docs/sdk/go/index.md (1)
6-9: LGTM!Also applies to: 14-26, 61-61, 81-100, 103-138, 140-159, 163-190, 192-198
docs/src/content/docs/sdk/go/pipes.md (1)
6-10: LGTM!Also applies to: 18-43, 51-53, 76-84
docs/src/content/docs/sdk/reference.md (1)
2-2: LGTM!Also applies to: 6-6, 26-73, 92-93, 169-173, 187-187
docs/src/content/docs/sdk/streaming.md (2)
2-2: LGTM!Also applies to: 6-6, 43-43, 68-81, 100-100, 102-146, 161-161, 163-226, 229-235
227-227: 🎯 Functional CorrectnessNo change needed. Both caution blocks already close with
:::.> Likely an incorrect or invalid review comment.docs/src/content/docs/why-wavehouse.md (1)
4-6: LGTM!Also applies to: 160-160, 200-200
docs/src/content/docs/404.md (1)
16-19: LGTM!Also applies to: 49-49
docs/src/content/docs/access-control.mdx (1)
4-5: LGTM!Also applies to: 59-60, 62-63, 77-79, 104-105, 107-108, 122-123, 156-164, 178-179, 187-188, 191-194, 196-205, 220-221, 236-238, 242-243, 247-253, 257-260, 272-273, 294-303, 312-325, 364-375, 407-418, 439-466, 498-526, 528-567, 584-593
docs/src/content/docs/sdk/go/queries.md (1)
6-6: LGTM!Also applies to: 8-18, 32-32, 34-65, 67-85, 104-118, 122-126, 132-168, 183-226, 228-301, 303-304, 307-325
docs/src/content/docs/sdk/go/reference.md (1)
6-6: LGTM!Also applies to: 8-43, 53-68, 123-192, 194-209
docs/src/content/docs/sdk/go/streaming.md (1)
6-6: LGTM!Also applies to: 8-14, 23-48, 50-133, 138-226, 229-235
README.md (1)
10-23: LGTM!Also applies to: 35-92, 94-160, 164-164
docs/src/content/docs/api.md (1)
8-8: LGTM!Also applies to: 26-34, 155-155, 161-183, 199-204, 225-230, 249-250, 269-305, 308-308, 370-370, 391-393, 406-406, 409-409, 457-457, 470-470, 498-498, 512-512, 531-531, 548-548, 593-600, 615-619, 642-642, 668-670, 694-704, 727-727, 767-767, 781-781, 821-821, 841-843
docs/src/content/docs/architecture.md (1)
4-5: LGTM!Also applies to: 74-75, 84-84, 90-94, 118-126, 145-149, 158-158, 176-199, 211-249, 261-270
docs/src/content/docs/claude-code.md (1)
28-28: LGTM!Also applies to: 50-52, 84-84, 212-212
docs/src/content/docs/deployment.md (1)
4-5: LGTM!Also applies to: 86-100, 118-123, 134-136, 268-268, 281-281, 372-376
docs/src/content/docs/development.md (1)
21-22: LGTM!Also applies to: 41-41, 123-130, 132-150, 154-154, 189-197, 212-212, 223-226, 289-291, 327-331, 341-343, 356-358, 368-369, 380-400, 402-402, 407-408, 435-446, 468-479, 510-510, 523-528, 535-549, 562-562, 588-595, 604-609, 611-613, 625-649, 653-663, 673-680, 682-706, 772-775
docs/src/content/docs/getting-started.md (1)
50-50: LGTM!Also applies to: 76-79, 98-98, 108-109
docs/src/content/docs/index.mdx (1)
18-21: LGTM!Also applies to: 57-57, 106-110, 114-160, 185-187, 219-223, 228-228
docs/src/content/docs/pipes.mdx (2)
124-151: LGTM!Also applies to: 187-209, 225-231
122-122: 🔒 Security & PrivacyRemove this verification request.
PipesHandler.Executechecksallowed_rolesbefore binding parameters or executing SQL. It does not evaluate table column or row policies. Existing execution tests cover role authorization.> Likely an incorrect or invalid review comment.
| - **Pipes helpers**: Use `pipes.NewMemoryStore(queries...)` for in-memory pipes testing without NATS. | ||
| - **Response assertions**: Use `testutil.AssertJSONResponse(t, rec, status, expected)` and `testutil.AssertJSONContains(t, rec, status, substring)`. | ||
| - **Coverage target**: 80% project-wide (CI enforces `threshold.total` in `.testcoverage.yml` against the merged unit + integration + e2e profile). Per-suite minima also enforced: unit 80%, integration 20%, e2e 60%, sdk 50%. Aim for 80%+ on new code. Coverage is published as a README badge (Go merged-total) and as PR comments via GitHub Code Quality — see `.github/workflows/README.md` "Coverage publishing"; the gate is unchanged. | ||
| - **Coverage target**: 80% project-wide (CI enforces `threshold.total` in `.testcoverage.yml` against the merged unit + integration + e2e profile). Per-suite minima also enforced: unit 80%, integration 20%, e2e 60%, go-sdk 75%, ts SDK 50%. The Go SDK (`clients/go`) is a nested module — invisible to the root module's `-coverpkg=./...`, so it is gated on its own `suites.go-sdk` floor and never merged into the project-wide total. Aim for 80%+ on new code. Coverage is published as a README badge (Go merged-total) and as PR comments via GitHub Code Quality — see `.github/workflows/README.md` "Coverage publishing"; the gate is unchanged. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
List each TypeScript coverage floor.
Line 128 states a 50% TypeScript SDK minimum. .testcoverage.yml sets ts-unit and ts-e2e to 40%, and ts-total to 50%. State all three thresholds so contributors do not treat the per-suite gates as informational.
🧰 Tools
🪛 LanguageTool
[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)
| var got http.Header | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| got = r.Header.Clone() | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = io.WriteString(w, `[]`) | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| client := NewClient(Config{ | ||
| BaseURL: srv.URL, | ||
| Auth: tc.auth, | ||
| HTTPClient: srv.Client(), | ||
| Options: &ClientOptions{Headers: tc.configured}, | ||
| }) | ||
| if _, err := client.Schema.List(context.Background()); err != nil { | ||
| t.Fatalf("schema list: %v", err) | ||
| } | ||
| if v := got.Values(tc.header); len(v) != 1 { | ||
| t.Fatalf("want exactly one %s header, got %v", tc.header, v) | ||
| } | ||
| if v := got.Get(tc.header); v != tc.want { | ||
| t.Fatalf("want %s: %q, got %q", tc.header, tc.want, v) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestConfiguredHeadersAreCopied: mutating the caller's map after NewClient | ||
| // must not change what later requests send. | ||
| func TestConfiguredHeadersAreCopied(t *testing.T) { | ||
| var got http.Header | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| got = r.Header.Clone() | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = io.WriteString(w, `[]`) | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| headers := map[string]string{"X-Tenant-Id": "acme"} | ||
| client := NewClient(Config{ | ||
| BaseURL: srv.URL, | ||
| HTTPClient: srv.Client(), | ||
| Options: &ClientOptions{Headers: headers}, | ||
| }) | ||
| headers["X-Tenant-Id"] = "attacker" | ||
| delete(headers, "X-Tenant-Id") | ||
|
|
||
| if _, err := client.Schema.List(context.Background()); err != nil { | ||
| t.Fatalf("schema list: %v", err) | ||
| } | ||
| if v := got.Get("X-Tenant-Id"); v != "acme" { | ||
| t.Fatalf("want the value captured at construction, got %q", v) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Synchronize access to captured headers.
The handlers write got on server goroutines. The tests read got on test goroutines. The HTTP round trip does not create a race-detector synchronization edge for these variables. go test -race can fail for both tests.
Send the cloned header through a buffered channel, or guard both accesses with one mutex.
Proposed fix
- var got http.Header
+ gotHeaders := make(chan http.Header, 1)
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- got = r.Header.Clone()
+ gotHeaders <- r.Header.Clone()
w.Header().Set("Content-Type", "application/json")
_, _ = io.WriteString(w, `[]`)
}))
@@
if _, err := client.Schema.List(context.Background()); err != nil {
t.Fatalf("schema list: %v", err)
}
+ got := <-gotHeadersApply the same pattern in TestConfiguredHeadersAreCopied.
📝 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.
| var got http.Header | |
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| got = r.Header.Clone() | |
| w.Header().Set("Content-Type", "application/json") | |
| _, _ = io.WriteString(w, `[]`) | |
| })) | |
| defer srv.Close() | |
| client := NewClient(Config{ | |
| BaseURL: srv.URL, | |
| Auth: tc.auth, | |
| HTTPClient: srv.Client(), | |
| Options: &ClientOptions{Headers: tc.configured}, | |
| }) | |
| if _, err := client.Schema.List(context.Background()); err != nil { | |
| t.Fatalf("schema list: %v", err) | |
| } | |
| if v := got.Values(tc.header); len(v) != 1 { | |
| t.Fatalf("want exactly one %s header, got %v", tc.header, v) | |
| } | |
| if v := got.Get(tc.header); v != tc.want { | |
| t.Fatalf("want %s: %q, got %q", tc.header, tc.want, v) | |
| } | |
| }) | |
| } | |
| } | |
| // TestConfiguredHeadersAreCopied: mutating the caller's map after NewClient | |
| // must not change what later requests send. | |
| func TestConfiguredHeadersAreCopied(t *testing.T) { | |
| var got http.Header | |
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| got = r.Header.Clone() | |
| w.Header().Set("Content-Type", "application/json") | |
| _, _ = io.WriteString(w, `[]`) | |
| })) | |
| defer srv.Close() | |
| headers := map[string]string{"X-Tenant-Id": "acme"} | |
| client := NewClient(Config{ | |
| BaseURL: srv.URL, | |
| HTTPClient: srv.Client(), | |
| Options: &ClientOptions{Headers: headers}, | |
| }) | |
| headers["X-Tenant-Id"] = "attacker" | |
| delete(headers, "X-Tenant-Id") | |
| if _, err := client.Schema.List(context.Background()); err != nil { | |
| t.Fatalf("schema list: %v", err) | |
| } | |
| if v := got.Get("X-Tenant-Id"); v != "acme" { | |
| t.Fatalf("want the value captured at construction, got %q", v) | |
| gotHeaders := make(chan http.Header, 1) | |
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| gotHeaders <- r.Header.Clone() | |
| w.Header().Set("Content-Type", "application/json") | |
| _, _ = io.WriteString(w, `[]`) | |
| })) | |
| defer srv.Close() | |
| client := NewClient(Config{ | |
| BaseURL: srv.URL, | |
| Auth: tc.auth, | |
| HTTPClient: srv.Client(), | |
| Options: &ClientOptions{Headers: tc.configured}, | |
| }) | |
| if _, err := client.Schema.List(context.Background()); err != nil { | |
| t.Fatalf("schema list: %v", err) | |
| } | |
| got := <-gotHeaders | |
| if v := got.Values(tc.header); len(v) != 1 { | |
| t.Fatalf("want exactly one %s header, got %v", tc.header, v) | |
| } | |
| if v := got.Get(tc.header); v != tc.want { | |
| t.Fatalf("want %s: %q, got %q", tc.header, tc.want, v) | |
| } | |
| }) | |
| } | |
| } | |
| // TestConfiguredHeadersAreCopied: mutating the caller's map after NewClient | |
| // must not change what later requests send. | |
| func TestConfiguredHeadersAreCopied(t *testing.T) { | |
| gotHeaders := make(chan http.Header, 1) | |
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| gotHeaders <- r.Header.Clone() | |
| w.Header().Set("Content-Type", "application/json") | |
| _, _ = io.WriteString(w, `[]`) | |
| })) | |
| defer srv.Close() | |
| headers := map[string]string{"X-Tenant-Id": "acme"} | |
| client := NewClient(Config{ | |
| BaseURL: srv.URL, | |
| HTTPClient: srv.Client(), | |
| Options: &ClientOptions{Headers: headers}, | |
| }) | |
| headers["X-Tenant-Id"] = "attacker" | |
| delete(headers, "X-Tenant-Id") | |
| if _, err := client.Schema.List(context.Background()); err != nil { | |
| t.Fatalf("schema list: %v", err) | |
| } | |
| got := <-gotHeaders | |
| if v := got.Get("X-Tenant-Id"); v != "acme" { | |
| t.Fatalf("want the value captured at construction, got %q", v) | |
| } |
| | 0 | `SSE_CONNECT_ERROR` | No | `BaseURL` is unparseable, or its scheme is not `http`/`https` — retrying cannot fix it | | ||
| | *3xx* | `SSE_REDIRECT` | No | The stream endpoint redirected while the request carried a credential, and the SDK refused to follow it | | ||
| | 200 | `SSE_BAD_CONTENT_TYPE` | No | A `200` that wasn't `text/event-stream` — something between you and WaveHouse answered (a captive portal, an auth gateway's login page) | | ||
| | 0 | `SSE_PARSE_ERROR` | Yes | A frame's JSON didn't decode; the frame is dropped and the stream continues | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(docs/src/content/docs/sdk/go/(reference|streaming)\.md$|.*\.(go|md|mdx)$)' | head -200
printf '%s\n' '--- relevant documentation ---'
sed -n '1,75p' docs/src/content/docs/sdk/go/reference.md
sed -n '105,155p' docs/src/content/docs/sdk/go/streaming.md
printf '%s\n' '--- parse/reconnect symbols ---'
rg -n -i 'SSE_PARSE_ERROR|SSE_READ_ERROR|SSE_AUTH_ERROR|SSE_CONNECT_ERROR|SSE_BAD_CONTENT_TYPE|reconnect|parse error|malformed|event stream' --glob '*.go' --glob '*.md' --glob '*.mdx' . | head -300Repository: Wave-RF/WaveHouse
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Go stream structure ---'
ast-grep outline clients/go/stream.go
printf '%s\n' '--- Go stream implementation ---'
sed -n '210,285p' clients/go/stream.go
sed -n '380,515p' clients/go/stream.go
printf '%s\n' '--- Go stream tests for malformed frames and reconnects ---'
sed -n '185,225p' clients/go/stream_test.go
sed -n '560,625p' clients/go/stream_test.go
sed -n '325,445p' clients/go/stream_test.go
printf '%s\n' '--- relevant Go error definitions and helpers ---'
sed -n '1,220p' clients/go/errors.go
rg -n 'IsRetryable|Retryable|SSE_PARSE_ERROR|parseSSE|bufio.Scanner|Split' clients/go --glob '*.go'Repository: Wave-RF/WaveHouse
Length of output: 20747
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
source = Path("clients/go/stream.go").read_text()
def body(name):
marker = f"func (sc *StreamController) {name}("
start = source.index(marker)
brace = source.index("{", start)
depth = 0
for i in range(brace, len(source)):
if source[i] == "{":
depth += 1
elif source[i] == "}":
depth -= 1
if depth == 0:
return source[brace + 1:i]
raise RuntimeError(f"unterminated {name}")
run = body("run")
handle = body("handleSSEData")
print("handleSSEData returns an error:", "error" in handle.split("{", 1)[0])
print("handleSSEData emits SSE_PARSE_ERROR:", 'Code: "SSE_PARSE_ERROR"' in handle or 'Code: "SSE_PARSE_ERROR"' in handle)
print("handleSSEData invokes reconnect control:", any(x in handle for x in ("connect(", "setStatus(StatusReconnecting)", "backoff(")))
print("run invokes handleSSEData:", "sc.handleSSEData(dataLine)" in run)
print("run reconnects after connect errors:", "if err != nil" in run and "sc.setStatus(StatusReconnecting)" in run)
print("run can observe a parse error as connect error:", "err := sc.handleSSEData" in run or "if err := sc.handleSSEData" in run)
for path in ("docs/src/content/docs/sdk/go/reference.md",
"docs/src/content/docs/sdk/go/streaming.md"):
lines = Path(path).read_text().splitlines()
print(f"{path}:")
for n, line in enumerate(lines, 1):
if "SSE_PARSE_ERROR" in line or "Reconnect covers" in line:
print(f" {n}: {line}")
PYRepository: Wave-RF/WaveHouse
Length of output: 1210
Document SSE_PARSE_ERROR as an in-place skip. Update the Go reference table to state that the error is reported, but the malformed frame is dropped without reconnecting. Remove SSE_PARSE_ERROR from the reconnect list in the Go streaming guide.
📍 Affects 2 files
docs/src/content/docs/sdk/go/reference.md#L49-L49(this comment)docs/src/content/docs/sdk/go/streaming.md#L134-L134
| var standaloneGoSuites = []string{"go-sdk"} | ||
|
|
||
| // suiteModuleDir maps a suite to the module directory its covdata was | ||
| // produced in, for suites that aren't the root module. `go tool cover | ||
| // -html` reads the source of every package named in the profile and | ||
| // resolves it through the module in the process's working directory, so a | ||
| // nested module's profile has to be rendered from inside that module — | ||
| // from the repo root the tool fails with "no required module provides | ||
| // package github.com/Wave-RF/WaveHouse/clients/go/...". `go tool covdata | ||
| // textfmt` has no such constraint (it only reads the covdata files), so | ||
| // only the HTML step needs the chdir. | ||
| var suiteModuleDir = map[string]string{"go-sdk": "clients/go"} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Inject the coverage plan and command runner.
Lines 73 and 84 add mutable package-level state. Lines 254-267 and 997-1002 bind rendering to direct process execution. Pass an immutable coverage plan and a command runner dependency into the coverage functions. Add table-driven tests for root-module and nested-module rendering without invoking the real Go toolchain.
As per coding guidelines: “No global state: Dependencies are passed explicitly (constructor injection).” “Table-driven tests: Use tests := []struct{ name string; ... } with t.Run(tt.name, ...) for test cases.” “Every new function should have corresponding test cases.”
Also applies to: 254-268, 991-1002
Source: Coding guidelines
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-go-sdk,make test-go-sdk-e2e,make test-conformance-ts,make lint-go-sdktargetsDocumentation
CI
test-go-sdk+test-conformance-tswired into the CI unit joblint-go-sdk+verify-go-sdkwired intoverify-parallelCommits
33 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 ./...; root tidy/vulncheck/coverage-gate coverage for it is tracked in Extend root verify tooling to the nested clients/go module (tidy, vulncheck, coverage gate) #437Array(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)