Repository navigation
fix(policy): fail closed on unresolvable row-filter claims - #457
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (6)**/*.{md,mdx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/**/*.{go,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/api/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (36)📓 Common learnings📚 Learning: 2026-06-10T15:01:09.027ZApplied to files:
📚 Learning: 2026-08-11T21:56:03.206ZApplied to files:
📚 Learning: 2026-08-12T21:55:01.697ZApplied to files:
📚 Learning: 2026-07-07T12:38:12.052ZApplied to files:
📚 Learning: 2026-08-11T21:55:53.726ZApplied to files:
📚 Learning: 2026-08-12T21:55:01.697ZApplied to files:
📚 Learning: 2026-07-24T18:23:07.472ZApplied to files:
📚 Learning: 2026-08-12T21:55:01.697ZApplied to files:
📚 Learning: 2026-06-26T12:23:22.696ZApplied to files:
📚 Learning: 2026-08-11T21:55:41.475ZApplied to files:
📚 Learning: 2026-05-13T20:40:56.906ZApplied to files:
📚 Learning: 2026-08-12T21:55:01.697ZApplied to files:
📚 Learning: 2026-05-23T01:24:02.141ZApplied to files:
📚 Learning: 2026-08-11T21:55:46.227ZApplied to files:
📚 Learning: 2026-05-20T01:02:03.228ZApplied to files:
📚 Learning: 2026-05-20T20:35:48.141ZApplied to files:
📚 Learning: 2026-08-11T16:02:20.914ZApplied to files:
📚 Learning: 2026-05-13T20:41:09.256ZApplied to files:
📚 Learning: 2026-06-30T14:22:44.209ZApplied to files:
📚 Learning: 2026-07-08T12:46:29.364ZApplied to files:
📚 Learning: 2026-05-20T01:02:00.784ZApplied to files:
📚 Learning: 2026-08-12T21:55:01.697ZApplied to files:
📚 Learning: 2026-05-23T01:23:59.268ZApplied to files:
📚 Learning: 2026-05-25T11:24:16.432ZApplied to files:
📚 Learning: 2026-05-20T20:30:22.556ZApplied to files:
📚 Learning: 2026-08-12T21:55:01.697ZApplied to files:
📚 Learning: 2026-06-26T12:23:26.034ZApplied to files:
📚 Learning: 2026-05-25T11:25:14.412ZApplied to files:
📚 Learning: 2026-08-13T12:17:52.620ZApplied to files:
📚 Learning: 2026-06-10T19:54:03.032ZApplied to files:
📚 Learning: 2026-08-13T12:17:52.620ZApplied to files:
📚 Learning: 2026-06-10T23:32:24.497ZApplied to files:
📚 Learning: 2026-06-29T14:21:45.067ZApplied to files:
📚 Learning: 2026-06-29T14:21:45.067ZApplied to files:
📚 Learning: 2026-08-12T20:33:30.744ZApplied to files:
🪛 LanguageTooldocs/src/content/docs/access-control.mdx[style] ~284-~284: Consider using “who” when you are referring to a person instead of an object. (THAT_WHO) [typographical] ~417-~417: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence. (WRB_QUESTION_MARK) [typographical] ~418-~418: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence. (WRB_QUESTION_MARK) 🔇 Additional comments (39)
📝 WalkthroughSummary by CodeRabbit
WalkthroughJWT claim templates now fail closed when unresolved or invalid. Canonical scalar handling preserves numeric precision. Query construction emits policy predicates and limits directly. Policy-filtered stream paths reject undecoded and malformed payloads. ChangesPolicy authorization
Query policy assembly
Claim precision and stream safety
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The change makes row filters fail closed for absent or null claims and preserves exact numeric claim values, reducing the chance of unauthorized broad reads. It is mergeable with owner awareness of a required test-format follow-up and several bounded documentation and changelog corrections. Sequence Diagram(s)sequenceDiagram
participant Client
participant Policy
participant QueryBuilder
participant ClickHouse
Client->>Policy: submit JWT and query
Policy-->>QueryBuilder: resolved predicate or 1 = 0
QueryBuilder->>ClickHouse: execute SQL with policy predicate and limit
ClickHouse-->>Client: return filtered result
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
documentation
|
📚 Docs preview is live → https://31ef5c5b-wavehouse-docs.wave-rf.workers.dev |
Contributor
Code Coverage OverviewLanguages: Go GoThe overall coverage in commit b2747c9 in the Show a code coverage summary of the most impacted files.
Updated |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 252af621-04dc-4466-8a78-4f1e72149e72
📒 Files selected for processing (4)
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy.gointernal/policy/policy_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Write tests in table-driven form witht.Run(tt.name, ...)for multiple cases.
Use shared mocks frominternal/testutil/instead of ad-hoc mocks in tests.
Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (testutil.MakeJWT,testutil.MakeExpiredJWT,NewTestSchemaRegistry,policy.NewMemoryStore,pipes.NewMemoryStore,AssertJSONResponse,AssertJSONContains) where applicable.
Every new function should have corresponding test cases, and new code should aim for 80%+ coverage.
Files:
internal/policy/policy_test.go
docs/src/content/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Documentation prose under the Starlight docs site must stay accurate against code, include runnable examples where relevant, and reflect code↔docs sync for changed behavior.
Files:
docs/src/content/docs/access-control.mdx
🧠 Learnings (20)
📓 Common learnings
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
📚 Learning: 2026-05-20T20:30:22.556Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/policy/**/*.{go} : Policy code must preserve fail-closed access control: `IsAdmin` is the single admin check, empty roles match nothing, `Validate` rejects empty role keys, and policy deletion denies everyone except the operator-key break-glass path.
Applied to files:
CHANGELOG.mdinternal/policy/policy.gointernal/policy/policy_test.godocs/src/content/docs/access-control.mdx
📚 Learning: 2026-07-08T12:46:29.364Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Applied to files:
CHANGELOG.mdinternal/policy/policy.gointernal/policy/policy_test.godocs/src/content/docs/access-control.mdx
📚 Learning: 2026-06-30T14:22:44.209Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 358
File: internal/policy/policy.go:289-305
Timestamp: 2026-06-30T14:22:44.209Z
Learning: In the Go policy/ingest path, `internal/policy/policy.go:resolveInValues` returns `[]any`, so `return nil` produces a typed nil slice. When that value is stored in `ResolvedPermissions.CheckClauses` and later type-asserted in `internal/api/ingest.go`, it still matches `[]any` and is handled as an `_in` membership check, preserving fail-closed behavior for absent claims. This is covered by `internal/api/ingest_test.go:TestIngest_Policy_CheckIn_AbsentClaim_FailsClosed`.
Applied to files:
CHANGELOG.mdinternal/policy/policy.gointernal/policy/policy_test.godocs/src/content/docs/access-control.mdx
📚 Learning: 2026-08-11T15:22:47.380Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: docs/src/content/docs/sdk/index.mdx:330-334
Timestamp: 2026-08-11T15:22:47.380Z
Learning: In WaveHouse Go server authentication, `internal/auth/auth.go` `bearerToken` returns from the `Authorization` header path before modifying `r.URL`. It removes the `token` query parameter only when authentication uses the query parameter without an `Authorization` header. Documentation must state that this protects WaveHouse's own logs only; reverse proxies, CDNs, load balancers, and other upstream intermediaries require query-string redaction.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/query/**/*.{go} : Structured-query code must enforce schema validation, permission checks, timestamp bucketing, and fail-closed column authorization inside `query.Build`.
Applied to files:
CHANGELOG.mdinternal/policy/policy.gointernal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/pipes/**/*.{go} : Named query pipes must remain fail-closed: per-pipe `allowed_roles` is the only execute-path gate, with admin-only behavior when no allowlist is present.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/auth/**/*.{go} : JWT auth middleware must always run, verify with either HMAC or JWKS (not both), pin accepted `alg` to the active verifier, and keep authN/authZ decoupled except for the sanctioned operator key.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-06-10T19:54:03.032Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: CHANGELOG.md:0-0
Timestamp: 2026-06-10T19:54:03.032Z
Learning: In the Wave-RF/WaveHouse repository, CHANGELOG.md entries under `[Unreleased]` use descriptive Keep-a-Changelog leads (e.g. "The structured-query column allowlist is now a hard cap…"), NOT the Conventional Commit PR title verbatim. Do not flag CHANGELOG entry leads for not matching the PR title — that is not a rule in this repo. There is no `.coderabbit.yaml`, and neither `AGENTS.md` nor `CONTRIBUTING.md` requires CHANGELOG leads to match PR titles.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/policy/policy.gointernal/policy/policy_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:
internal/policy/policy.gointernal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (`testutil.MakeJWT`, `testutil.MakeExpiredJWT`, `NewTestSchemaRegistry`, `policy.NewMemoryStore`, `pipes.NewMemoryStore`, `AssertJSONResponse`, `AssertJSONContains`) where applicable.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-20T20:35:48.141Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:147-153
Timestamp: 2026-05-20T20:35:48.141Z
Learning: In WaveHouse internal/api pipes tests, when testing the non-forbidden (allowed) path via `safeHandle`, the response body is empty because `safeHandle` recovers the nil-Conn panic before any body is written. Use plain `assert.NotEqual(t, http.StatusForbidden, w.Code)` / `assert.NotEqual(t, http.StatusNotFound, w.Code)` rather than JSON-body helpers, which would fail on `json.Unmarshal` of an empty body.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-13T20:41:09.256Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/api/health_test.go:100-163
Timestamp: 2026-05-13T20:41:09.256Z
Learning: In the WaveHouse repository (`internal/testutil/testutil.go`), `testutil.AssertJSONResponse(t, rec, expectedStatus, expected any)` does full-body equality (`assert.Equal`) and `testutil.AssertJSONContains(t, rec, expectedStatus, expectedKeys map[string]any)` does per-key equality (`assert.Equal` per key). Neither helper supports substring/Contains checks. Passing a string to `AssertJSONContains` would not compile.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-20T01:02:03.228Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:03.228Z
Learning: In the WaveHouse project (`internal/api/**/*_test.go`), the convention for testing `RequireRole` middleware is to inject `ContextKeyRole` directly into the request context rather than using `testutil.MakeJWT`. JWT token parsing is covered separately in `middleware_test.go` (17 dedicated tests). Do not suggest switching role-gate tests to JWT-driven tests — the separation of concerns is intentional to keep failure surfaces isolated.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Write tests in table-driven form with `t.Run(tt.name, ...)` for multiple cases.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-13T20:40:56.906Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/discovery/discovery_test.go:404-513
Timestamp: 2026-05-13T20:40:56.906Z
Learning: In `internal/discovery/discovery_test.go`, the five `TestRetryRefresh_*` tests (SucceedsOnFirstAttempt, RetriesUntilSuccess, ReturnsOnContextCancel, BackoffIsBounded, NilOnAttemptIsSafe) are intentionally written as individual named tests rather than a table-driven suite. Their setup pipelines and assertion shapes are fundamentally heterogeneous: ReturnsOnContextCancel requires goroutine + channel + select-with-timeout orchestration, BackoffIsBounded uses wall-clock elapsed bounds, and NilOnAttemptIsSafe is a nil-callback panic-safety check. Forcing them into a table would produce mostly-null rows with nested `if` branches, which is worse readability. The table-driven pattern is correctly applied to `TestClampBackoff` in the same file (pure function, uniform I/O shape). Do not suggest converting these RetryRefresh tests to a table-driven suite.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/policy/policy_test.go
🪛 LanguageTool
docs/src/content/docs/access-control.mdx
[style] ~241-~241: Consider using a more formal/concise alternative here.
Context: ...njected as '', and any supplied value other than '' is rejected with `403 check failed...
(OTHER_THAN)
🔇 Additional comments (2)
internal/policy/policy.go (1)
205-210: LGTM!Also applies to: 231-248, 268-288, 314-320
docs/src/content/docs/access-control.mdx (1)
222-235: LGTM!Also applies to: 237-240
github-project-automation
Bot
moved this from In progress
to In review
in WaveHouse Task Board
…ejection; parallelize TestResolveTemplate subtests
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e8c0ba5-f1a3-4644-b604-5b359c7cce68
📒 Files selected for processing (3)
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Integration tests
- GitHub Check: Unit tests
- GitHub Check: Coverage
- GitHub Check: Docs build
- GitHub Check: E2E tests
🧰 Additional context used
📓 Path-based instructions (2)
docs/src/content/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Documentation prose under the Starlight docs site must stay accurate against code, include runnable examples where relevant, and reflect code↔docs sync for changed behavior.
Files:
docs/src/content/docs/access-control.mdx
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Write tests in table-driven form witht.Run(tt.name, ...)for multiple cases.
Use shared mocks frominternal/testutil/instead of ad-hoc mocks in tests.
Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (testutil.MakeJWT,testutil.MakeExpiredJWT,NewTestSchemaRegistry,policy.NewMemoryStore,pipes.NewMemoryStore,AssertJSONResponse,AssertJSONContains) where applicable.
Every new function should have corresponding test cases, and new code should aim for 80%+ coverage.
Files:
internal/policy/policy_test.go
🧠 Learnings (22)
📓 Common learnings
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/policy/**/*.{go} : Policy code must preserve fail-closed access control: `IsAdmin` is the single admin check, empty roles match nothing, `Validate` rejects empty role keys, and policy deletion denies everyone except the operator-key break-glass path.
Applied to files:
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy_test.go
📚 Learning: 2026-05-20T20:30:22.556Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
Applied to files:
CHANGELOG.mddocs/src/content/docs/access-control.mdx
📚 Learning: 2026-06-30T14:22:44.209Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 358
File: internal/policy/policy.go:289-305
Timestamp: 2026-06-30T14:22:44.209Z
Learning: In the Go policy/ingest path, `internal/policy/policy.go:resolveInValues` returns `[]any`, so `return nil` produces a typed nil slice. When that value is stored in `ResolvedPermissions.CheckClauses` and later type-asserted in `internal/api/ingest.go`, it still matches `[]any` and is handled as an `_in` membership check, preserving fail-closed behavior for absent claims. This is covered by `internal/api/ingest_test.go:TestIngest_Policy_CheckIn_AbsentClaim_FailsClosed`.
Applied to files:
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy_test.go
📚 Learning: 2026-07-08T12:46:29.364Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/query/**/*.{go} : Structured-query code must enforce schema validation, permission checks, timestamp bucketing, and fail-closed column authorization inside `query.Build`.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-11T15:22:47.380Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: docs/src/content/docs/sdk/index.mdx:330-334
Timestamp: 2026-08-11T15:22:47.380Z
Learning: In WaveHouse Go server authentication, `internal/auth/auth.go` `bearerToken` returns from the `Authorization` header path before modifying `r.URL`. It removes the `token` query parameter only when authentication uses the query parameter without an `Authorization` header. Documentation must state that this protects WaveHouse's own logs only; reverse proxies, CDNs, load balancers, and other upstream intermediaries require query-string redaction.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/pipes/**/*.{go} : Named query pipes must remain fail-closed: per-pipe `allowed_roles` is the only execute-path gate, with admin-only behavior when no allowlist is present.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/auth/**/*.{go} : JWT auth middleware must always run, verify with either HMAC or JWKS (not both), pin accepted `alg` to the active verifier, and keep authN/authZ decoupled except for the sanctioned operator key.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Use the repo’s JWT, schema, policy, pipes, and JSON response test helpers (`testutil.MakeJWT`, `testutil.MakeExpiredJWT`, `NewTestSchemaRegistry`, `policy.NewMemoryStore`, `pipes.NewMemoryStore`, `AssertJSONResponse`, `AssertJSONContains`) where applicable.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Write tests in table-driven form with `t.Run(tt.name, ...)` for multiple cases.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-08-11T21:55:41.475Z
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:41.475Z
Learning: In the Go SDK tests, table-driven test loops do not require named `t.Run` subtests when the assertion error already identifies the failing input and expected and actual values. Do not raise a style-only finding to add `t.Run` in that case.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-13T20:40:56.906Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/discovery/discovery_test.go:404-513
Timestamp: 2026-05-13T20:40:56.906Z
Learning: In `internal/discovery/discovery_test.go`, the five `TestRetryRefresh_*` tests (SucceedsOnFirstAttempt, RetriesUntilSuccess, ReturnsOnContextCancel, BackoffIsBounded, NilOnAttemptIsSafe) are intentionally written as individual named tests rather than a table-driven suite. Their setup pipelines and assertion shapes are fundamentally heterogeneous: ReturnsOnContextCancel requires goroutine + channel + select-with-timeout orchestration, BackoffIsBounded uses wall-clock elapsed bounds, and NilOnAttemptIsSafe is a nil-callback panic-safety check. Forcing them into a table would produce mostly-null rows with nested `if` branches, which is worse readability. The table-driven pattern is correctly applied to `TestClampBackoff` in the same file (pure function, uniform I/O shape). Do not suggest converting these RetryRefresh tests to a table-driven suite.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Every new function should have corresponding test cases, and new code should aim for 80%+ coverage.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to **/*_test.go : Use shared mocks from `internal/testutil/` instead of ad-hoc mocks in tests.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-08-11T21:55:46.227Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/client_test.go:40-44
Timestamp: 2026-08-11T21:55:46.227Z
Learning: In `clients/go/client_test.go`, do not validate typed pointer fields by storing them in `map[string]any` and checking `ns == nil`. A nil typed pointer stored in an interface value is non-nil. Compare each concrete pointer field directly, such as `c.Sys == nil`, so constructor tests detect missing namespace assignments.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-20T01:02:03.228Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:03.228Z
Learning: In the WaveHouse project (`internal/api/**/*_test.go`), the convention for testing `RequireRole` middleware is to inject `ContextKeyRole` directly into the request context rather than using `testutil.MakeJWT`. JWT token parsing is covered separately in `middleware_test.go` (17 dedicated tests). Do not suggest switching role-gate tests to JWT-driven tests — the separation of concerns is intentional to keep failure surfaces isolated.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-05-20T20:35:48.141Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:147-153
Timestamp: 2026-05-20T20:35:48.141Z
Learning: In WaveHouse internal/api pipes tests, when testing the non-forbidden (allowed) path via `safeHandle`, the response body is empty because `safeHandle` recovers the nil-Conn panic before any body is written. Use plain `assert.NotEqual(t, http.StatusForbidden, w.Code)` / `assert.NotEqual(t, http.StatusNotFound, w.Code)` rather than JSON-body helpers, which would fail on `json.Unmarshal` of an empty body.
Applied to files:
internal/policy/policy_test.go
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/policy/policy_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:
internal/policy/policy_test.go
🔇 Additional comments (4)
internal/policy/policy_test.go (2)
416-418: 🎯 Functional CorrectnessVerify the Go target before keeping these subtests parallel.
The subtest closure captures
ttand callst.Parallel(). If the module targets Go before 1.22, all subtests can observe the final loop value. Check the declared Go version. If it is below Go 1.22, shadowttinside the loop. If it is Go 1.22 or later, no code change is required.The supplied files do not include the repository's declared Go target.
Portable fix for targets below Go 1.22
for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) {
399-415: LGTM!Also applies to: 419-422, 654-681, 683-723, 725-743, 745-762
docs/src/content/docs/access-control.mdx (1)
222-222: LGTM!Also applies to: 235-237, 241-244
CHANGELOG.md (1)
40-40: LGTM!
This was referenced Aug 12, 2026
Closed
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 96a2ef34-5424-4029-a332-3c8dc1f42bea
📒 Files selected for processing (14)
AGENTS.mdCHANGELOG.mddocs/src/content/docs/access-control.mdxdocs/src/content/docs/architecture.mddocs/src/content/docs/pipes.mdxinternal/api/structured_query.gointernal/api/structured_query_test.gointernal/auth/auth.gointernal/auth/auth_test.gointernal/policy/policy.gointernal/policy/policy_test.gointernal/query/builder.gointernal/query/builder_test.gointernal/stream/hub_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Integration tests
- GitHub Check: E2E tests
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (6)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Go 1.26, strict formatting (gofumpt, enforced by CI)
- No global state: Dependencies are passed explicitly (constructor injection).
Files:
internal/auth/auth.gointernal/auth/auth_test.gointernal/policy/policy.gointernal/api/structured_query.gointernal/api/structured_query_test.gointernal/query/builder.gointernal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
internal/auth/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Auth: always on, fail-loud, decoupled from authz (security) — the JWT middleware always runs (no
auth.enabled/dev_modeflag); it verifies with HMAC or JWKS (not both), with acceptedalgpinned to the active verifier and checked before any key is used (rejectsalg:noneand cross-family confusion).
Files:
internal/auth/auth.gointernal/auth/auth_test.go
internal/*/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Package naming: Lowercase, single word (or abbreviated).
internal/enforces module privacy.
Files:
internal/auth/auth.gointernal/auth/auth_test.gointernal/policy/policy.gointernal/api/structured_query.gointernal/api/structured_query_test.gointernal/query/builder.gointernal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.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:
internal/auth/auth_test.gointernal/api/structured_query_test.gointernal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
internal/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Bearer-token-only CORS posture (security) — Bearer JWT on every request, no cookies/sessions;
corsMiddlewaredeliberately never emitsAccess-Control-Allow-Credentials(not needed, and*+ credentials is a spec violation browsers reject).
Files:
internal/api/structured_query.gointernal/api/structured_query_test.go
internal/query/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Structured queries: column authz fail-closed (security) —
POST /v1/query?table={table}: typed AST validated against schema, permission-enforced, timestamp-bucketed for cache,DefaultMaxRows(10,000) cap. Every column reference — projection, aggregation args,filters,group_by,order_by,time_range— is authorized insidequery.Build(the single chokepoint that enumerates them all), so no clause can skip the role'sallow_columns/deny_columnscheck (#223).
Files:
internal/query/builder.gointernal/query/builder_test.go
🧠 Learnings (44)
📓 Common learnings
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/query_builder.go:278-291
Timestamp: 2026-08-11T21:56:03.206Z
Learning: In `clients/go/query_builder.go`, `fetchNextTyped` intentionally treats a failed JSON decode of a non-object typed `Row` as normal end-of-pagination. This behavior matches the existing “cursor column was not in the projection” path and TypeScript SDK parity. The broader behavior change is tracked in GitHub issue `#452`.
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 358
File: internal/policy/policy.go:289-305
Timestamp: 2026-06-30T14:22:44.209Z
Learning: In the Go policy/ingest path, `internal/policy/policy.go:resolveInValues` returns `[]any`, so `return nil` produces a typed nil slice. When that value is stored in `ResolvedPermissions.CheckClauses` and later type-asserted in `internal/api/ingest.go`, it still matches `[]any` and is handled as an `_in` membership check, preserving fail-closed behavior for absent claims. This is covered by `internal/api/ingest_test.go:TestIngest_Policy_CheckIn_AbsentClaim_FailsClosed`.
📚 Learning: 2026-05-20T20:30:22.556Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
Applied to files:
docs/src/content/docs/pipes.mdxCHANGELOG.mdinternal/policy/policy.godocs/src/content/docs/access-control.mdxinternal/query/builder.gointernal/stream/hub_test.go
📚 Learning: 2026-06-29T14:21:45.067Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 343
File: internal/api/pipe_deps.go:0-0
Timestamp: 2026-06-29T14:21:45.067Z
Learning: In `internal/api/pipes.go`, direct table-function reads and direct cross-database table reads are intentionally omitted from the pipe dependency set and continue using the normal query-derived TTL; only resolved-but-unmaintainable dependencies (such as unknown or unfoldable view-derived names) trigger the unresolved-dependency TTL cap.
Applied to files:
docs/src/content/docs/pipes.mdxAGENTS.mddocs/src/content/docs/access-control.mdx
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/auth/auth.gointernal/auth/auth_test.gointernal/policy/policy.gointernal/api/structured_query.gointernal/api/structured_query_test.gointernal/query/builder.gointernal/stream/hub_test.gointernal/policy/policy_test.gointernal/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:
internal/auth/auth.gointernal/auth/auth_test.gointernal/policy/policy.gointernal/api/structured_query.gointernal/api/structured_query_test.gointernal/query/builder.gointernal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to internal/query/**/*.go : **Structured queries: column authz fail-closed (security)**
Applied to files:
docs/src/content/docs/architecture.mdAGENTS.mdCHANGELOG.mdinternal/policy/policy.gointernal/api/structured_query.gointernal/api/structured_query_test.godocs/src/content/docs/access-control.mdxinternal/query/builder.gointernal/query/builder_test.go
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
docs/src/content/docs/architecture.mdAGENTS.mdCHANGELOG.md
📚 Learning: 2026-05-25T11:25:11.992Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 180
File: internal/observability/instruments.go:40-117
Timestamp: 2026-05-25T11:25:11.992Z
Learning: In the WaveHouse project (Go), package-level `var` declarations of OTel metric instruments (e.g., `metric.Float64Histogram`, `metric.Int64Counter`) created via `Meter().Float64Histogram(...)` / `Meter().Int64Counter(...)` are idiomatic and intentional — they follow the OTel Go SDK global proxy pattern and are NOT considered "global state" violations under the AGENTS.md constructor-injection rule. That rule targets swappable application-level interface dependencies (Cache, Publisher, Subscriber, Deduplicator), not OTel proxy instruments. Do not suggest wrapping these into an `Instruments` struct for injection.
Applied to files:
AGENTS.md
📚 Learning: 2026-05-13T14:35:40.574Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 116
File: internal/observability/provider.go:0-0
Timestamp: 2026-05-13T14:35:40.574Z
Learning: In `internal/observability/provider.go` (Go), `runtime.Start` from `go.opentelemetry.io/contrib/instrumentation/runtime` is wrapped in a package-level `runtimeStartOnce sync.Once`. The key design decision: `runtime.Start` errors must NOT route through `handleErr` (which rolls back OTel globals) — they should go through `slog.Warn` so the rest of the pipeline stays initialized with degraded host metrics. With non-fatal error handling in place, `sync.Once` is a clean goroutine-leak guard rather than a behavior-changing one. Production `main.go` calls `InitProvider` exactly once; the Once guard caps the leak in test re-init paths. Resolved in commit 6de31ee.
Applied to files:
AGENTS.md
📚 Learning: 2026-05-13T14:12:20.026Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 116
File: internal/observability/provider.go:155-158
Timestamp: 2026-05-13T14:12:20.026Z
Learning: In `internal/observability/provider.go` (Go), `runtime.Start` from `go.opentelemetry.io/contrib/instrumentation/runtime` spawns a goroutine with no shutdown/stop API. A `sync.Once` guard was deliberately NOT added around `runtime.Start` because it would mask the issue: a second `InitProvider` call would silently omit runtime metrics for the new MeterProvider, which is a worse failure mode than the goroutine leak. Production `main.go` calls `InitProvider` exactly once per process (leak surface bounded to tests). The integration test `TestOTel_UnreachableEndpoint_DoesNotBlockStartupOrEmits` documents and intentionally accepts this leak. Will revisit when upstream OTel adds a `Stop()` to the runtime package.
Applied to files:
AGENTS.md
📚 Learning: 2026-05-25T11:24:24.022Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 180
File: internal/cache/local.go:0-0
Timestamp: 2026-05-25T11:24:24.022Z
Learning: In the WaveHouse codebase (`internal/cache/local.go` and related packages), package-level `var` declarations of immutable `metric.MeasurementOption` / OTel attribute sets (e.g., `cacheL1Attrs = metric.WithAttributes(attribute.String("tier", "L1"))`) are intentional and acceptable. These are pre-allocated constants analogous to `regexp.MustCompile(...)`, not mutable global state. The AGENTS.md "no global state / constructor injection" rule applies to application dependencies (Cache, Publisher, Deduplicator), not to stateless OTel metric attribute options. Do not flag these as violations of the constructor-injection guideline.
Applied to files:
AGENTS.md
📚 Learning: 2026-05-25T11:25:14.412Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 180
File: internal/observability/instruments.go:22-38
Timestamp: 2026-05-25T11:25:14.412Z
Learning: In WaveHouse's `internal/observability/instruments.go`, the `mustFloat64Histogram` and `mustInt64Counter` helpers intentionally panic at package init time if OTel instrument registration fails. This follows the `regexp.MustCompile`/`template.Must` Go idiom for build-time-constant invariants. The "return errors, don't panic" coding guideline applies to runtime/request-response paths only, not to init-time instrument registration. Do not flag this pattern as a violation.
Applied to files:
AGENTS.md
📚 Learning: 2026-07-07T12:38:15.328Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:15.328Z
Learning: Repo: Wave-RF/WaveHouse. WaveHouse deliberately does not log or trace any client IP address anywhere in the codebase. `middleware.RealIP` was removed in PR `#332` due to IP-spoofing GHSAs, and trusted-proxy-aware client-IP extraction for logs/traces is tracked as a future cross-cutting effort in issue `#333`. Do not suggest adding `r.RemoteAddr` or naive `X-Forwarded-For`-derived IPs to logs (e.g., audit logs in internal/auth/auth.go for the operator-key path) until `#333` lands with proper trusted-proxy handling.
Applied to files:
AGENTS.md
📚 Learning: 2026-07-24T18:23:07.472Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 418
File: internal/observability/metrics_test.go:108-240
Timestamp: 2026-07-24T18:23:07.472Z
Learning: In `internal/observability/metrics_test.go`, tests in package `observability` cannot import shared `internal/testutil/` mocks because `internal/testutil/` imports `mq`, which imports `observability` and would create an import cycle. Keep minimal local test stubs (such as `stubDeduplicator`, `stubCHConn`, and `stubPartsRows`) in this package unless the dependency structure changes.
Applied to files:
AGENTS.mdinternal/stream/hub_test.go
📚 Learning: 2026-07-08T12:46:29.364Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Applied to files:
AGENTS.mdCHANGELOG.mdinternal/policy/policy.godocs/src/content/docs/access-control.mdx
📚 Learning: 2026-06-29T14:21:45.067Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 343
File: internal/api/pipe_deps.go:0-0
Timestamp: 2026-06-29T14:21:45.067Z
Learning: In `internal/api/pipes.go`, pipe dependency handling deliberately distinguishes `fallback` from `unresolved`: `fallback` means dependency analysis failed and the pipe over-resolves to `SchemaRegistry.AllBaseTables()` without TTL flooring, while `unresolved` means EXPLAIN succeeded but at least one resolved dependency is not reliably version-maintained, so `Execute` caps the cache TTL with `cache.UnresolvedDepsTTLCap`.
Applied to files:
AGENTS.mddocs/src/content/docs/access-control.mdx
📚 Learning: 2026-06-10T19:54:03.032Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: CHANGELOG.md:0-0
Timestamp: 2026-06-10T19:54:03.032Z
Learning: In the Wave-RF/WaveHouse repository, CHANGELOG.md entries under `[Unreleased]` use descriptive Keep-a-Changelog leads (e.g. "The structured-query column allowlist is now a hard cap…"), NOT the Conventional Commit PR title verbatim. Do not flag CHANGELOG entry leads for not matching the PR title — that is not a rule in this repo. There is no `.coderabbit.yaml`, and neither `AGENTS.md` nor `CONTRIBUTING.md` requires CHANGELOG leads to match PR titles.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-06-30T14:22:44.209Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 358
File: internal/policy/policy.go:289-305
Timestamp: 2026-06-30T14:22:44.209Z
Learning: In the Go policy/ingest path, `internal/policy/policy.go:resolveInValues` returns `[]any`, so `return nil` produces a typed nil slice. When that value is stored in `ResolvedPermissions.CheckClauses` and later type-asserted in `internal/api/ingest.go`, it still matches `[]any` and is handled as an `_in` membership check, preserving fail-closed behavior for absent claims. This is covered by `internal/api/ingest_test.go:TestIngest_Policy_CheckIn_AbsentClaim_FailsClosed`.
Applied to files:
CHANGELOG.mdinternal/policy/policy.godocs/src/content/docs/access-control.mdxinternal/policy/policy_test.go
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to internal/policy/**/*.go : **Hasura-style access control: fail-closed (security)**
Applied to files:
CHANGELOG.mdinternal/policy/policy.godocs/src/content/docs/access-control.mdxinternal/query/builder.go
📚 Learning: 2026-08-12T15:28:20.891Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: docs/src/content/docs/sdk/index.mdx:0-0
Timestamp: 2026-08-12T15:28:20.891Z
Learning: For `docs/src/content/docs/sdk/index.mdx`, the documented workaround for the undici idle-event-loop keep-alive stall is to upgrade to undici 8.10.0 or later. If a consumer is pinned to an affected version, `new Agent({ pipelining: 0 })` must be merged as `dispatcher` into the SDK-provided `RequestInit`; this disables keep-alive reuse. Configuring `keepAliveTimeout` does not mitigate this stall because the socket retirement timer is starved by the same idle event loop.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-06-26T15:07:28.749Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 0
File: :0-0
Timestamp: 2026-06-26T15:07:28.749Z
Learning: In the Go SSE implementation in `internal/api/stream.go`, keepalive frames from `internal/stream.Heartbeater` are only written from the post-replay select loop. The replay/gap-fill step is synchronous before entering that loop, so registering the `internal/stream.Subscriber` before replay does not materially improve idle-time coverage during replay; it can at most buffer one heartbeat in the subscriber's capacity-1 queue. Covering a genuinely long replay would require interleaving replay with the select loop and is tied to the broader delivery-path rework tracked by Issue `#294`.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-12T20:33:30.744Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: clients/ts/src/pipes.ts:28-28
Timestamp: 2026-08-12T20:33:30.744Z
Learning: In the TypeScript SDK, `PipeRef.fetch` in `clients/ts/src/pipes.ts` accepts only a signal option. Pipe row limits are not generic request options. A pipe SQL definition can declare a `{{limit}}` parameter, and callers provide that parameter through `wh.pipe(name, { limit })`. The API binds the pipe request body as pipe parameters through `pipes.BindParams` in `internal/api/pipes.go`.
Applied to files:
CHANGELOG.mddocs/src/content/docs/access-control.mdx
📚 Learning: 2026-08-12T21:45:38.018Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: clients/ts/src/pipes.ts:0-0
Timestamp: 2026-08-12T21:45:38.018Z
Learning: In the TypeScript SDK, `PipeRef.fetch` uses the exported `PipeRequestOptions` type rather than `Pick<RequestOptions, "signal">`. `PipeRequestOptions` declares `limit?: never` so both object literals and named `RequestOptions` values that include `limit` fail type checking instead of silently dropping the limit. A value declared as `RequestOptions` is intentionally not assignable to `PipeRequestOptions`, even if it has no runtime `limit`; consumers can use `PipeRequestOptions` for shared pipe, table, and query-builder fetch options, or use an inferred `{ signal }` object.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-11T15:22:47.380Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: docs/src/content/docs/sdk/index.mdx:330-334
Timestamp: 2026-08-11T15:22:47.380Z
Learning: In WaveHouse Go server authentication, `internal/auth/auth.go` `bearerToken` returns from the `Authorization` header path before modifying `r.URL`. It removes the `token` query parameter only when authentication uses the query parameter without an `Authorization` header. Documentation must state that this protects WaveHouse's own logs only; reverse proxies, CDNs, load balancers, and other upstream intermediaries require query-string redaction.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-05-20T01:02:03.228Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:03.228Z
Learning: In the WaveHouse project (`internal/api/**/*_test.go`), the convention for testing `RequireRole` middleware is to inject `ContextKeyRole` directly into the request context rather than using `testutil.MakeJWT`. JWT token parsing is covered separately in `middleware_test.go` (17 dedicated tests). Do not suggest switching role-gate tests to JWT-driven tests — the separation of concerns is intentional to keep failure surfaces isolated.
Applied to files:
internal/auth/auth_test.gointernal/stream/hub_test.gointernal/policy/policy_test.go
📚 Learning: 2026-05-23T01:24:02.141Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:24:02.141Z
Learning: In WaveHouse tests under internal/api/**/*_test.go, use `testutil.AssertJSONErrorResponse(t, w)` (from `internal/testutil`) for HTTP error-path assertions — NOT a package-local `assertJSONErrorResponse` helper. The package-local helper was removed in PR `#174` and its functionality was promoted to `internal/testutil.AssertJSONErrorResponse`. This helper asserts `Content-Type: application/json`, `X-Content-Type-Options: nosniff` headers, and the presence of an `"error"` field in the JSON body.
Applied to files:
internal/auth/auth_test.gointernal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to **/*.go : **Structured logging** with `log/slog` (JSON handler)
Applied to files:
internal/auth/auth_test.go
📚 Learning: 2026-05-20T20:35:48.141Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:147-153
Timestamp: 2026-05-20T20:35:48.141Z
Learning: In WaveHouse internal/api pipes tests, when testing the non-forbidden (allowed) path via `safeHandle`, the response body is empty because `safeHandle` recovers the nil-Conn panic before any body is written. Use plain `assert.NotEqual(t, http.StatusForbidden, w.Code)` / `assert.NotEqual(t, http.StatusNotFound, w.Code)` rather than JSON-body helpers, which would fail on `json.Unmarshal` of an empty body.
Applied to files:
internal/auth/auth_test.gointernal/policy/policy.gointernal/api/structured_query_test.godocs/src/content/docs/access-control.mdxinternal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
📚 Learning: 2026-08-11T16:02:20.914Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: internal/auth/auth.go:0-0
Timestamp: 2026-08-11T16:02:20.914Z
Learning: In `internal/auth/auth.go`, `Middleware` must call `bearerToken(r)` before any authentication branch that can return early, including operator-key authentication. `bearerToken` removes a non-empty `token` query parameter from `r.URL.RawQuery` before selecting the Bearer-header or query-token credential, so WaveHouse handlers and logs do not retain an unused query token.
Applied to files:
internal/auth/auth_test.go
📚 Learning: 2026-05-13T20:41:09.256Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/api/health_test.go:100-163
Timestamp: 2026-05-13T20:41:09.256Z
Learning: In the WaveHouse repository (`internal/testutil/testutil.go`), `testutil.AssertJSONResponse(t, rec, expectedStatus, expected any)` does full-body equality (`assert.Equal`) and `testutil.AssertJSONContains(t, rec, expectedStatus, expectedKeys map[string]any)` does per-key equality (`assert.Equal` per key). Neither helper supports substring/Contains checks. Passing a string to `AssertJSONContains` would not compile.
Applied to files:
internal/auth/auth_test.gointernal/policy/policy_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/auth/auth_test.gointernal/api/structured_query_test.gointernal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
📚 Learning: 2026-08-11T21:56:03.206Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/query_builder.go:278-291
Timestamp: 2026-08-11T21:56:03.206Z
Learning: In `clients/go/query_builder.go`, `fetchNextTyped` intentionally treats a failed JSON decode of a non-object typed `Row` as normal end-of-pagination. This behavior matches the existing “cursor column was not in the projection” path and TypeScript SDK parity. The broader behavior change is tracked in GitHub issue `#452`.
Applied to files:
internal/policy/policy.godocs/src/content/docs/access-control.mdxinternal/query/builder.go
📚 Learning: 2026-05-25T11:24:16.432Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 182
File: internal/discovery/validation.go:107-123
Timestamp: 2026-05-25T11:24:16.432Z
Learning: In `internal/discovery/validation.go` (WaveHouse project, Go), the `isTypeCompatible` function is intentionally permissive: it accepts any string for Bool and numeric ClickHouse types (and similarly broad coercions for other types) because the design philosophy is to avoid false-negative rejections at the pre-validation layer. ClickHouse's own type coercion is more forgiving and will handle the final validation. Stricter lexical/value checks (e.g., `strconv.ParseFloat` for numerics, allowlisting "true"/"false" for bools) should NOT be suggested, as accepting incorrect types is preferred over rejecting values ClickHouse would accept.
Applied to files:
internal/policy/policy.go
📚 Learning: 2026-05-13T20:40:56.906Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/discovery/discovery_test.go:404-513
Timestamp: 2026-05-13T20:40:56.906Z
Learning: In `internal/discovery/discovery_test.go`, the five `TestRetryRefresh_*` tests (SucceedsOnFirstAttempt, RetriesUntilSuccess, ReturnsOnContextCancel, BackoffIsBounded, NilOnAttemptIsSafe) are intentionally written as individual named tests rather than a table-driven suite. Their setup pipelines and assertion shapes are fundamentally heterogeneous: ReturnsOnContextCancel requires goroutine + channel + select-with-timeout orchestration, BackoffIsBounded uses wall-clock elapsed bounds, and NilOnAttemptIsSafe is a nil-callback panic-safety check. Forcing them into a table would produce mostly-null rows with nested `if` branches, which is worse readability. The table-driven pattern is correctly applied to `TestClampBackoff` in the same file (pure function, uniform I/O shape). Do not suggest converting these RetryRefresh tests to a table-driven suite.
Applied to files:
internal/policy/policy.gointernal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
📚 Learning: 2026-06-26T12:23:26.034Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:26.034Z
Learning: In this Go repository, the `**/*_test.go` table-driven test guideline is intended for genuinely multi-scenario tests. Single sequential behavioral-flow tests, such as `internal/stream/subscriber_test.go`'s `TestSubscriber_SendDeliversThenDropsWhenFull`, do not need to be rewritten into `[]struct{...}` + `t.Run(...)` when that would be artificial and less clear.
Applied to files:
internal/policy/policy.go
📚 Learning: 2026-08-11T21:55:41.475Z
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:41.475Z
Learning: In the Go SDK tests, table-driven test loops do not require named `t.Run` subtests when the assertion error already identifies the failing input and expected and actual values. Do not raise a style-only finding to add `t.Run` in that case.
Applied to files:
internal/policy/policy.gointernal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
📚 Learning: 2026-05-20T01:02:00.784Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:00.784Z
Learning: In WaveHouse’s internal API tests (files matching internal/api/**/*_test.go), follow the existing separation-of-concerns convention for testing the RequireRole middleware: inject `ContextKeyRole` directly into the request `context.Context` instead of using `testutil.MakeJWT`/JWT-driven flows. Do not refactor role-gate tests to use JWT tokens—JWT parsing and token handling are covered separately in `middleware_test.go` (the dedicated JWT parsing tests), and mixing those concerns would expand the failure surface and reduce isolation.
Applied to files:
internal/api/structured_query_test.go
📚 Learning: 2026-05-23T01:23:59.268Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:23:59.268Z
Learning: In WaveHouse Go tests in internal/api/**/*_test.go, use internal/testutil.AssertJSONErrorResponse(t, w) for HTTP error-path JSON assertions. Do not use (or reintroduce) package-local assertJSONErrorResponse helpers. AssertJSONErrorResponse verifies the response Content-Type is application/json, includes the X-Content-Type-Options: nosniff header, and that the JSON body contains an "error" field.
Applied to files:
internal/api/structured_query_test.go
📚 Learning: 2026-05-13T20:41:09.256Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/api/health_test.go:100-163
Timestamp: 2026-05-13T20:41:09.256Z
Learning: In `internal/api/health_test.go` (WaveHouse), every handler test explicitly asserts `Content-Type: application/json` and `X-Content-Type-Options: nosniff` headers, including on 503 responses. This is deliberate regression coverage: the comment in `TestHealth_Readiness_PingFails` explains that without the 503-path header test, a future refactor moving header setup into the success branch would silently drop headers on error responses. New boot-degraded tests should follow the same pattern.
Applied to files:
internal/stream/hub_test.go
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to internal/**/*.go : - **Table-driven tests**: Use `tests := []struct{ name string; ... }` with `t.Run(tt.name, ...)` for test cases.
Applied to files:
internal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to **/*_test.go : - **Every new function should have corresponding test cases.** Run `make lint` and `make test` before considering work complete.
Applied to files:
internal/stream/hub_test.gointernal/policy/policy_test.gointernal/query/builder_test.go
📚 Learning: 2026-06-10T23:32:24.497Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 330
File: internal/api/pipes_test.go:421-423
Timestamp: 2026-06-10T23:32:24.497Z
Learning: In Wave-RF/WaveHouse, `testutil.AssertJSONContains` (internal/testutil/testutil.go) has the signature `func AssertJSONContains(t *testing.T, rec *httptest.ResponseRecorder, expectedStatus int, expectedKeys map[string]any)`. The fourth argument must be a `map[string]any` of JSON key-value pairs to check in the response body (e.g., `map[string]any{"error": "some message"}`), NOT a plain substring string. Passing a bare string as the fourth argument will not compile.
Applied to files:
internal/stream/hub_test.gointernal/policy/policy_test.go
📚 Learning: 2026-05-19T03:04:07.206Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 164
File: internal/ingest/bento.go:121-122
Timestamp: 2026-05-19T03:04:07.206Z
Learning: In `internal/ingest/bento.go`, the condition `raw.Action != "insert" && raw.Action != ""` is intentional. `EventMessage` in `internal/ingest/types.go` has no `Action` field (only `TableName`, `ReceivedTimestamp`, `Data`), and `internal/api/ingest.go` builds every ingest envelope using `ingest.EventMessage` without an `action` key. This means `raw.Action` is always `""` for envelopes from `IngestHandler`, so rejecting the empty string would break the only legitimate insert producer in the pipeline.
Applied to files:
internal/stream/hub_test.go
📚 Learning: 2026-08-11T21:55:46.227Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/client_test.go:40-44
Timestamp: 2026-08-11T21:55:46.227Z
Learning: In `clients/go/client_test.go`, do not validate typed pointer fields by storing them in `map[string]any` and checking `ns == nil`. A nil typed pointer stored in an interface value is non-nil. Compare each concrete pointer field directly, such as `c.Sys == nil`, so constructor tests detect missing namespace assignments.
Applied to files:
internal/policy/policy_test.gointernal/query/builder_test.go
🔇 Additional comments (15)
internal/query/builder.go (1)
37-40: LGTM!Also applies to: 97-110, 139-149, 221-222, 242-243
internal/api/structured_query.go (1)
115-117: LGTM!internal/query/builder_test.go (1)
5-5: LGTM!Also applies to: 181-298
internal/api/structured_query_test.go (1)
266-280: LGTM!Also applies to: 335-363
docs/src/content/docs/architecture.md (1)
153-153: LGTM!AGENTS.md (1)
43-43: LGTM!internal/auth/auth.go (1)
206-211: LGTM!internal/auth/auth_test.go (1)
8-8: LGTM!Also applies to: 112-137
internal/policy/policy.go (1)
265-299: LGTM!Also applies to: 309-336, 519-520, 538-538, 572-606
internal/policy/policy_test.go (1)
4-4: LGTM!Also applies to: 397-428, 697-712, 721-729, 847-847, 1034-1034
docs/src/content/docs/access-control.mdx (1)
222-222: LGTM!Also applies to: 233-243, 284-285, 530-531
CHANGELOG.md (1)
46-46: LGTM!internal/stream/hub_test.go (2)
119-135: LGTM!
308-325: LGTM!docs/src/content/docs/pipes.mdx (1)
122-122: LGTM!
EricAndrechek
requested changes
Aug 13, 2026
EricAndrechek
left a comment
EricAndrechek
left a comment
Member
There was a problem hiding this comment.
Not sure if you were ready/wanted me to re-review again, but did anyway to try and help keep this moving. Plus I went to start on #322 and get a PR open for it myself and Claude reviewing/working on that flagged your commits here did it already so I scrapped that and came to look here instead.
Structural fix looks good, I can't seem to craft something to evade the predicate anymore.
Two minor issues on the claim-value stuff in comments:
-
_inarray elements still stringify per-element - the
json.Numberedge, whereWithJSONNumber()fails on numeric columns
The rest are minor docs type things etc, save for one: isValidAggFn – it's the same family as that U+0131 splice bug where we have unicode case folding used where we want an exact match. I think it may be worth searching/grepping for other ToLower/ToUpper comparisons against fixed ASCII sets while it's fresh if you don't mind.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/content/docs/access-control.mdx (1)
535-535: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument malformed
checktemplates in the field reference.
validateRolePermsappliesrejectMalformedTemplatestoperms.Checkas well asperms.Filterininternal/policy/policy.goLines 600-657. Thefilterrow documents this rejection, but thecheckrow only says “Supports templating”. Add the write-time rejection so the field reference does not imply that every template form is accepted.Proposed wording
- ... Supports templating. + ... Supports templating; malformed templates are rejected when the policy is written.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 585c26f4-28c8-4606-8c85-8070716edd70
📒 Files selected for processing (12)
CHANGELOG.mddocs/src/content/docs/access-control.mdxdocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.mdinternal/api/ingest.gointernal/api/ingest_test.gointernal/auth/auth.gointernal/auth/auth_test.gointernal/policy/policy.gointernal/policy/policy_test.gointernal/query/builder.gointernal/query/builder_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Docs preview
- GitHub Check: Coverage
- GitHub Check: E2E tests
🧰 Additional context used
📓 Path-based instructions (6)
**/*.md
📄 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/deployment.mdCHANGELOG.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Table-driven tests: Use
tests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
Files:
internal/api/ingest_test.gointernal/api/ingest.gointernal/auth/auth.gointernal/query/builder_test.gointernal/auth/auth_test.gointernal/query/builder.gointernal/policy/policy_test.gointernal/policy/policy.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
Files:
internal/api/ingest_test.gointernal/query/builder_test.gointernal/auth/auth_test.gointernal/policy/policy_test.go
internal/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Bearer-token-only CORS posture (security) — Bearer JWT on every request, no cookies/sessions;
corsMiddlewaredeliberately never emitsAccess-Control-Allow-Credentials(not needed, and*+ credentials is a spec violation browsers reject).
Files:
internal/api/ingest_test.gointernal/api/ingest.go
internal/query/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Structured queries: column authz fail-closed (security) —
POST /v1/query?table={table}: typed AST validated against schema, permission-enforced, timestamp-bucketed for cache,DefaultMaxRows(10,000) cap.
Files:
internal/query/builder_test.gointernal/query/builder.go
internal/policy/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Hasura-style access control: fail-closed (security) —
policy.IsAdmin(role ==admin_role, exact case-sensitive, default"admin") is the single admin check, shared byEvaluate/ResolveRole/Validate/the/v1/admingate/RoleAllowed.
Files:
internal/policy/policy_test.gointernal/policy/policy.go
🧠 Learnings (34)
📓 Common learnings
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
📚 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.mdCHANGELOG.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/configuration.mdxdocs/src/content/docs/access-control.mdx
📚 Learning: 2026-06-30T14:22:44.209Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 358
File: internal/policy/policy.go:289-305
Timestamp: 2026-06-30T14:22:44.209Z
Learning: In the Go policy/ingest path, `internal/policy/policy.go:resolveInValues` returns `[]any`, so `return nil` produces a typed nil slice. When that value is stored in `ResolvedPermissions.CheckClauses` and later type-asserted in `internal/api/ingest.go`, it still matches `[]any` and is handled as an `_in` membership check, preserving fail-closed behavior for absent claims. This is covered by `internal/api/ingest_test.go:TestIngest_Policy_CheckIn_AbsentClaim_FailsClosed`.
Applied to files:
internal/api/ingest_test.gointernal/api/ingest.goCHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy_test.gointernal/policy/policy.go
📚 Learning: 2026-05-20T20:35:48.141Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:147-153
Timestamp: 2026-05-20T20:35:48.141Z
Learning: In WaveHouse internal/api pipes tests, when testing the non-forbidden (allowed) path via `safeHandle`, the response body is empty because `safeHandle` recovers the nil-Conn panic before any body is written. Use plain `assert.NotEqual(t, http.StatusForbidden, w.Code)` / `assert.NotEqual(t, http.StatusNotFound, w.Code)` rather than JSON-body helpers, which would fail on `json.Unmarshal` of an empty body.
Applied to files:
internal/api/ingest_test.gointernal/auth/auth_test.godocs/src/content/docs/access-control.mdxinternal/policy/policy_test.gointernal/policy/policy.go
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to internal/policy/**/*.go : **Hasura-style access control: fail-closed (security)**
Applied to files:
internal/api/ingest_test.goCHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy.go
📚 Learning: 2026-05-20T01:02:00.784Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:00.784Z
Learning: In WaveHouse’s internal API tests (files matching internal/api/**/*_test.go), follow the existing separation-of-concerns convention for testing the RequireRole middleware: inject `ContextKeyRole` directly into the request `context.Context` instead of using `testutil.MakeJWT`/JWT-driven flows. Do not refactor role-gate tests to use JWT tokens—JWT parsing and token handling are covered separately in `middleware_test.go` (the dedicated JWT parsing tests), and mixing those concerns would expand the failure surface and reduce isolation.
Applied to files:
internal/api/ingest_test.go
📚 Learning: 2026-05-23T01:23:59.268Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:23:59.268Z
Learning: In WaveHouse Go tests in internal/api/**/*_test.go, use internal/testutil.AssertJSONErrorResponse(t, w) for HTTP error-path JSON assertions. Do not use (or reintroduce) package-local assertJSONErrorResponse helpers. AssertJSONErrorResponse verifies the response Content-Type is application/json, includes the X-Content-Type-Options: nosniff header, and that the JSON body contains an "error" field.
Applied to files:
internal/api/ingest_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/api/ingest_test.gointernal/query/builder_test.gointernal/auth/auth_test.gointernal/policy/policy_test.go
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/api/ingest_test.gointernal/api/ingest.gointernal/auth/auth.gointernal/query/builder_test.gointernal/auth/auth_test.gointernal/query/builder.gointernal/policy/policy_test.gointernal/policy/policy.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:
internal/api/ingest_test.gointernal/api/ingest.gointernal/auth/auth.gointernal/query/builder_test.gointernal/auth/auth_test.gointernal/query/builder.gointernal/policy/policy_test.gointernal/policy/policy.go
📚 Learning: 2026-06-10T23:32:24.497Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 330
File: internal/api/pipes_test.go:421-423
Timestamp: 2026-06-10T23:32:24.497Z
Learning: In Wave-RF/WaveHouse, `testutil.AssertJSONContains` (internal/testutil/testutil.go) has the signature `func AssertJSONContains(t *testing.T, rec *httptest.ResponseRecorder, expectedStatus int, expectedKeys map[string]any)`. The fourth argument must be a `map[string]any` of JSON key-value pairs to check in the response body (e.g., `map[string]any{"error": "some message"}`), NOT a plain substring string. Passing a bare string as the fourth argument will not compile.
Applied to files:
internal/api/ingest.gointernal/policy/policy_test.go
📚 Learning: 2026-05-13T20:41:09.256Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/api/health_test.go:100-163
Timestamp: 2026-05-13T20:41:09.256Z
Learning: In the WaveHouse repository (`internal/testutil/testutil.go`), `testutil.AssertJSONResponse(t, rec, expectedStatus, expected any)` does full-body equality (`assert.Equal`) and `testutil.AssertJSONContains(t, rec, expectedStatus, expectedKeys map[string]any)` does per-key equality (`assert.Equal` per key). Neither helper supports substring/Contains checks. Passing a string to `AssertJSONContains` would not compile.
Applied to files:
internal/api/ingest.gointernal/auth/auth_test.gointernal/policy/policy_test.go
📚 Learning: 2026-05-25T11:24:16.432Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 182
File: internal/discovery/validation.go:107-123
Timestamp: 2026-05-25T11:24:16.432Z
Learning: In `internal/discovery/validation.go` (WaveHouse project, Go), the `isTypeCompatible` function is intentionally permissive: it accepts any string for Bool and numeric ClickHouse types (and similarly broad coercions for other types) because the design philosophy is to avoid false-negative rejections at the pre-validation layer. ClickHouse's own type coercion is more forgiving and will handle the final validation. Stricter lexical/value checks (e.g., `strconv.ParseFloat` for numerics, allowlisting "true"/"false" for bools) should NOT be suggested, as accepting incorrect types is preferred over rejecting values ClickHouse would accept.
Applied to files:
internal/api/ingest.gointernal/policy/policy_test.gointernal/policy/policy.go
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to internal/query/**/*.go : **Structured queries: column authz fail-closed (security)**
Applied to files:
internal/api/ingest.goCHANGELOG.mdinternal/query/builder_test.godocs/src/content/docs/access-control.mdxinternal/query/builder.gointernal/policy/policy.go
📚 Learning: 2026-08-11T21:56:03.206Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/query_builder.go:278-291
Timestamp: 2026-08-11T21:56:03.206Z
Learning: In `clients/go/query_builder.go`, `fetchNextTyped` intentionally treats a failed JSON decode of a non-object typed `Row` as normal end-of-pagination. This behavior matches the existing “cursor column was not in the projection” path and TypeScript SDK parity. The broader behavior change is tracked in GitHub issue `#452`.
Applied to files:
internal/auth/auth.goCHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/query/builder.gointernal/policy/policy.go
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to internal/discovery/**/*.go : **Canonical timestamp wire form (fail-open at ingest)**
Applied to files:
internal/auth/auth.go
📚 Learning: 2026-06-10T19:54:03.032Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: CHANGELOG.md:0-0
Timestamp: 2026-06-10T19:54:03.032Z
Learning: In the Wave-RF/WaveHouse repository, CHANGELOG.md entries under `[Unreleased]` use descriptive Keep-a-Changelog leads (e.g. "The structured-query column allowlist is now a hard cap…"), NOT the Conventional Commit PR title verbatim. Do not flag CHANGELOG entry leads for not matching the PR title — that is not a rule in this repo. There is no `.coderabbit.yaml`, and neither `AGENTS.md` nor `CONTRIBUTING.md` requires CHANGELOG leads to match PR titles.
Applied to files:
CHANGELOG.mddocs/src/content/docs/access-control.mdx
📚 Learning: 2026-05-20T20:30:22.556Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:22.556Z
Learning: In WaveHouse's pipes authorization fix (PR `#172`), fixing the AllowedRoles fail-open bug requires two changes: (1) remove the outer `if role != ""` guard in PipesHandler.Execute so empty roles are evaluated against the allowlist, AND (2) add an `ar != ""` guard inside the allowlist scan (i.e., `ar != "" && ar == role`) so a malformed allowlist containing empty strings (e.g., `[""]`) cannot match an empty role via `"" == ""`. Doing only (1) is insufficient and would make `[""]` fail-open.
Applied to files:
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/query/builder.gointernal/policy/policy.go
📚 Learning: 2026-07-08T12:46:29.364Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 381
File: internal/stream/hub.go:142-178
Timestamp: 2026-07-08T12:46:29.364Z
Learning: In `internal/stream/hub.go`, the per-subscriber `policy.Evaluate(...)` call in `Broadcast` (on the row-filtered path, when `perms.HasRowFilter()` is true) is intentionally not memoized per distinct claim set. Rationale from maintainer taitelee: the claims-independent fast path (`!HasRowFilter()`) already ensures high-fanout public streams without a row-filter pay no per-subscriber cost; for topics that do carry a row-filter, visibility is inherently per-connection (different JWT claims → different rows) so the per-subscriber evaluation can't be hoisted without losing correctness, and memoization by claim set would rarely hit since subscribers in a row-filtered bucket typically have distinct tenant claims (plus `map[string]any` claims aren't cheaply hashable). This tradeoff is intentional; don't flag it as a perf issue unless profiling on a real filtered-high-fanout topic shows it matters.
Applied to files:
CHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/policy.go
📚 Learning: 2026-08-11T16:02:20.914Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: internal/auth/auth.go:0-0
Timestamp: 2026-08-11T16:02:20.914Z
Learning: In `internal/auth/auth.go`, `Middleware` must call `bearerToken(r)` before any authentication branch that can return early, including operator-key authentication. `bearerToken` removes a non-empty `token` query parameter from `r.URL.RawQuery` before selecting the Bearer-header or query-token credential, so WaveHouse handlers and logs do not retain an unused query token.
Applied to files:
CHANGELOG.mdinternal/auth/auth_test.go
📚 Learning: 2026-05-20T01:02:03.228Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:03.228Z
Learning: In the WaveHouse project (`internal/api/**/*_test.go`), the convention for testing `RequireRole` middleware is to inject `ContextKeyRole` directly into the request context rather than using `testutil.MakeJWT`. JWT token parsing is covered separately in `middleware_test.go` (17 dedicated tests). Do not suggest switching role-gate tests to JWT-driven tests — the separation of concerns is intentional to keep failure surfaces isolated.
Applied to files:
CHANGELOG.mdinternal/auth/auth_test.gointernal/policy/policy_test.go
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to internal/**/*.go : - **Table-driven tests**: Use `tests := []struct{ name string; ... }` with `t.Run(tt.name, ...)` for test cases.
Applied to files:
internal/query/builder_test.gointernal/policy/policy_test.go
📚 Learning: 2026-07-24T18:23:07.472Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 418
File: internal/observability/metrics_test.go:108-240
Timestamp: 2026-07-24T18:23:07.472Z
Learning: In `internal/observability/metrics_test.go`, tests in package `observability` cannot import shared `internal/testutil/` mocks because `internal/testutil/` imports `mq`, which imports `observability` and would create an import cycle. Keep minimal local test stubs (such as `stubDeduplicator`, `stubCHConn`, and `stubPartsRows`) in this package unless the dependency structure changes.
Applied to files:
internal/query/builder_test.gointernal/policy/policy_test.go
📚 Learning: 2026-08-11T21:55:41.475Z
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:41.475Z
Learning: In the Go SDK tests, table-driven test loops do not require named `t.Run` subtests when the assertion error already identifies the failing input and expected and actual values. Do not raise a style-only finding to add `t.Run` in that case.
Applied to files:
internal/query/builder_test.gointernal/policy/policy_test.gointernal/policy/policy.go
📚 Learning: 2026-05-13T20:40:56.906Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/discovery/discovery_test.go:404-513
Timestamp: 2026-05-13T20:40:56.906Z
Learning: In `internal/discovery/discovery_test.go`, the five `TestRetryRefresh_*` tests (SucceedsOnFirstAttempt, RetriesUntilSuccess, ReturnsOnContextCancel, BackoffIsBounded, NilOnAttemptIsSafe) are intentionally written as individual named tests rather than a table-driven suite. Their setup pipelines and assertion shapes are fundamentally heterogeneous: ReturnsOnContextCancel requires goroutine + channel + select-with-timeout orchestration, BackoffIsBounded uses wall-clock elapsed bounds, and NilOnAttemptIsSafe is a nil-callback panic-safety check. Forcing them into a table would produce mostly-null rows with nested `if` branches, which is worse readability. The table-driven pattern is correctly applied to `TestClampBackoff` in the same file (pure function, uniform I/O shape). Do not suggest converting these RetryRefresh tests to a table-driven suite.
Applied to files:
internal/query/builder_test.gointernal/policy/policy_test.gointernal/policy/policy.go
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to **/*_test.go : - **Every new function should have corresponding test cases.** Run `make lint` and `make test` before considering work complete.
Applied to files:
internal/query/builder_test.gointernal/policy/policy_test.go
📚 Learning: 2026-05-23T01:24:02.141Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:24:02.141Z
Learning: In WaveHouse tests under internal/api/**/*_test.go, use `testutil.AssertJSONErrorResponse(t, w)` (from `internal/testutil`) for HTTP error-path assertions — NOT a package-local `assertJSONErrorResponse` helper. The package-local helper was removed in PR `#174` and its functionality was promoted to `internal/testutil.AssertJSONErrorResponse`. This helper asserts `Content-Type: application/json`, `X-Content-Type-Options: nosniff` headers, and the presence of an `"error"` field in the JSON body.
Applied to files:
internal/query/builder_test.gointernal/auth/auth_test.gointernal/policy/policy_test.go
📚 Learning: 2026-08-11T21:55:46.227Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/client_test.go:40-44
Timestamp: 2026-08-11T21:55:46.227Z
Learning: In `clients/go/client_test.go`, do not validate typed pointer fields by storing them in `map[string]any` and checking `ns == nil`. A nil typed pointer stored in an interface value is non-nil. Compare each concrete pointer field directly, such as `c.Sys == nil`, so constructor tests detect missing namespace assignments.
Applied to files:
internal/query/builder_test.gointernal/policy/policy_test.go
📚 Learning: 2026-06-29T14:21:45.067Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 343
File: internal/api/pipe_deps.go:0-0
Timestamp: 2026-06-29T14:21:45.067Z
Learning: In `internal/api/pipes.go`, direct table-function reads and direct cross-database table reads are intentionally omitted from the pipe dependency set and continue using the normal query-derived TTL; only resolved-but-unmaintainable dependencies (such as unknown or unfoldable view-derived names) trigger the unresolved-dependency TTL cap.
Applied to files:
docs/src/content/docs/access-control.mdx
📚 Learning: 2026-06-29T14:21:45.067Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 343
File: internal/api/pipe_deps.go:0-0
Timestamp: 2026-06-29T14:21:45.067Z
Learning: In `internal/api/pipes.go`, pipe dependency handling deliberately distinguishes `fallback` from `unresolved`: `fallback` means dependency analysis failed and the pipe over-resolves to `SchemaRegistry.AllBaseTables()` without TTL flooring, while `unresolved` means EXPLAIN succeeded but at least one resolved dependency is not reliably version-maintained, so `Execute` caps the cache TTL with `cache.UnresolvedDepsTTLCap`.
Applied to files:
docs/src/content/docs/access-control.mdx
📚 Learning: 2026-08-12T20:33:30.744Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 456
File: clients/ts/src/pipes.ts:28-28
Timestamp: 2026-08-12T20:33:30.744Z
Learning: In the TypeScript SDK, `PipeRef.fetch` in `clients/ts/src/pipes.ts` accepts only a signal option. Pipe row limits are not generic request options. A pipe SQL definition can declare a `{{limit}}` parameter, and callers provide that parameter through `wh.pipe(name, { limit })`. The API binds the pipe request body as pipe parameters through `pipes.BindParams` in `internal/api/pipes.go`.
Applied to files:
docs/src/content/docs/access-control.mdx
📚 Learning: 2026-08-12T21:55:01.697Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-12T21:55:01.697Z
Learning: Applies to internal/api/**/*.go : **Chi v5** for HTTP routing
Applied to files:
internal/query/builder.gointernal/policy/policy.go
📚 Learning: 2026-06-26T12:23:26.034Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:26.034Z
Learning: In this Go repository, the `**/*_test.go` table-driven test guideline is intended for genuinely multi-scenario tests. Single sequential behavioral-flow tests, such as `internal/stream/subscriber_test.go`'s `TestSubscriber_SendDeliversThenDropsWhenFull`, do not need to be rewritten into `[]struct{...}` + `t.Run(...)` when that would be artificial and less clear.
Applied to files:
internal/policy/policy.go
🪛 LanguageTool
docs/src/content/docs/access-control.mdx
[typographical] ~417-~417: 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)
[typographical] ~418-~418: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ... you seed one via PUT /v1/admin/policy. :::caution Validation guards only the ...
(WRB_QUESTION_MARK)
🔇 Additional comments (6)
internal/query/builder.go (1)
9-9: LGTM!Also applies to: 38-41, 98-111, 140-150, 159-159, 222-223, 243-244, 481-489, 502-502
internal/query/builder_test.go (2)
5-5: LGTM!Also applies to: 181-225, 228-264, 266-299, 478-478
181-225: 📐 Maintainability & Code QualityConfirm lint and test execution.
Before considering this test change complete, run
make lintandmake test. Their results are not present in the review context.As per coding guidelines,
**/*_test.gorequiresmake lintandmake testbefore work is complete.Also applies to: 228-264, 266-313
Source: Coding guidelines
docs/src/content/docs/access-control.mdx (1)
222-243: LGTM!Also applies to: 284-285, 399-399
docs/src/content/docs/configuration.mdx (1)
171-171: LGTM!Also applies to: 270-271
docs/src/content/docs/deployment.md (1)
146-149: LGTM!
EricAndrechek
left a comment
EricAndrechek
left a comment
Member
There was a problem hiding this comment.
Ok, re-reviewed after the canonicalization work. The forgery is closed — all three repros now 403 through the real ingest handler, and between claude and the reviewers we put roughly 5M literals plus 300k random ones through it against big.Rat ground truth with zero collisions, zero value drift, zero panics. ParseFloat is gone from the bind path entirely. Going exact rather than the 1<<53 guard I suggested was the better call; that would only have failed closed where this is actually correct.
Nothing blocking this round. Seven comments, and four of them are things I raised last round that are still open:
- the required side of the check comparison still isn't canonicalized, so a static
_eq: "1.0"against body1.0is a live regression - the
!comparableguard has no test — mutated it out and the whole api suite stayed green access-control.mdx:281still doesn't say how equality is decided nowapi.mdstill lists no ingest 403 rows for the two the handler emits (column "x" not allowed for insert,check failed for column "x"). Couldn't inline that one, api.md isn't in the branch diff — the structured-query table two sections down already documents its own policy 403s, so the file contradicts itself.
New ones are mostly small: two branches of canonicalDecimal (the sign and the exponent DoS guard) can each be deleted without failing a test, the byte-vs-digit precheck makes acceptance spelling-dependent, and access-control.mdx:235 still describes the old rule so values like 1e150 and 1e-150 now fail closed with no doc saying why.
The one I'd not skip is on CHANGELOG — this is a data migration, not just a code change. Numeric claims above 2^53 used to canonicalize to a rounded value, so rows written by the old build carry a tenant id the new build's filter won't match. They don't become wrong, they become unreachable, silently. Snowflake-style ids are ~1.7e18 so it's a real shape.
Also filed #474 off this review — aggregations with a * argument skip column authorization entirely, so argMax(*) returns a denied column's value and uniq(*) leaks its cardinality. Pre-existing, live on main, not this PR's problem, but the fix is a breaking change to the query API and that's free before a tagged release and costly after.
Still need to sort #381 ordering — it collides in auth.go and policy.go now, and its RowVisible has to agree with this PR's fail-closed rule or the two read paths drift.
EricAndrechek
approved these changes
Aug 13, 2026
EricAndrechek
left a comment
EricAndrechek
left a comment
Member
There was a problem hiding this comment.
I think we've covered every case we can think to throw at this OR moved things to separate issues at this point. The policy, type checking, and coercion logic have gotten so bloated and out of hand at this point, but they also are important and should just be shipped as they fix things to be done with them, they can just become tech debt instead. Approved.
github-project-automation
Bot
moved this from In review
to In progress
in WaveHouse Task Board
github-project-automation
Bot
moved this from In progress
to Done
in WaveHouse Task Board
taitelee
added a commit
that referenced
this pull request
Aug 13, 2026
EricAndrechek
added a commit
that referenced
this pull request
Aug 18, 2026
## Summary Moves `.stream()` / `.liveQuery()` off `EventSource` and onto `fetch`, so the JWT travels as `Authorization: Bearer` instead of `?token=` in the request URI — where every proxy, CDN, and load balancer in front of WaveHouse is free to log it. **Server untouched.** `bearerToken()` has always preferred the header, and the CORS preflight has allow-listed `Authorization` + `Last-Event-ID` since #215 — `internal/api/router_test.go:352` is an existing test naming #203 as its reason. `?token=` stays accepted for clients that genuinely can't set headers. **Advances #203; does not close it.** Tasks 1 and 2 of 3 are done (header auth, cURL flow). The third — retiring `?token=` server-side — is #468, and closing that closes this. **This does not make streams authenticated, and (regarding #203) does not close it.** `/v1/stream` stays ungated: an expired or missing token still resolves to `default_role` and gets a filtered `200`, never a `401`. What changes is that the JWT stops appearing in request URIs, and therefore in proxy, CDN, and load-balancer logs. Enforcing expiry on a live stream is #239 and is deliberately not delivered here. Part of #194. ## Decisions worth your eyes **1. Redirects are refused only when the request carries a credential.** Platforms strip `Authorization` on a cross-origin redirect ([whatwg/fetch#1544](whatwg/fetch#1544) — the mitigation for the class behind [CVE-2022-1650](GHSA-6h5x-7c5m-7cr7), which was this exact bug in the `eventsource` package) while forwarding other headers intact. A credentialed hop therefore either silently downgrades the stream to `default_role` — this endpoint answers an unauthenticated caller rather than rejecting them — or hands a configured proxy secret to whatever the redirect names. Refusing costs nothing, because following would never have produced an *authenticated* stream anyway. With no credential there's nothing to protect, so redirects are followed and CDN canonicalization, geo/LB indirection, and http→https upgrades all work. `options.fetch` overrides `redirect` if you need the credentialed case followed regardless. `manual` rather than `error`: `error` rejects with a bare `TypeError` indistinguishable from a connection failure, which the reconnect loop would retry forever against a redirect that will never stop happening. **2. `FetchLike`'s URL parameter narrows to `string`.** The design note on #269 said the type was narrow so hand-written `(url: string, init?) => Promise<Response>` middleware would assign. What shipped was `string | URL | Request`, which — parameters being contravariant — rejects exactly that. Purely additive for implementers; only code that *imported* `FetchLike` and called through it with a `URL` breaks. **3. The SDK gains its first runtime dependency: `eventsource-parser@^3.1.0`.** MIT, **zero transitive deps**, npm provenance-attested (SLSA v1), no install scripts, 61.7M downloads/week, same maintainer as the canonical `eventsource`. Dual CJS/ESM, so it composes with our `dist/index.cjs`. Measured cost in the CDN IIFE bundle: 3402 B minified, 1430 B gzipped. A caret rather than an exact pin because pinning in a *published library* duplicates the package in any consumer tree already resolving a 3.x and freezes them out of patch/security releases until we cut one — our lockfile still governs CI. The range stops below the ESM-only 4.0.0. Why rent rather than hand-roll: the correct parser is ~440 lines, and the parts that matter — reassembling a frame split across chunk boundaries without quadratic recopying, disambiguating a trailing `\r` at a chunk boundary between a bare-CR terminator and half a split `\r\n`, and capping buffered input against a hostile stream — are what a naive version gets wrong, and are unavoidable even though we control the server, since HTTP/2 and intermediaries re-chunk freely. The buffer cap is set explicitly to 16 MiB; the parser defaults to unbounded. **This retires the "zero-dependency" claim** everywhere it appeared — both READMEs, AGENTS.md invariant 14, four site pages, and an older `Unreleased` CHANGELOG entry that would otherwise have shipped in the same release notes as the entry introducing the dependency. The only surviving mentions are in released CHANGELOG history, which is a record rather than a claim. ## What this fixes beyond the headline - **Expired-token silent downgrade.** `auth()` was called once and baked into the URL; `EventSource` then reconnected forever with that token. Once expired the stream stayed open serving a reduced view. This is why #203 is a **prerequisite for #239**. - **Blank `id:` clearing resumption.** The hub emits `id: ` for passthrough payloads; per spec an empty `id` *clears* the last-event-id. The transport retains the last **non-empty** id. - **Real errors.** A rejection carries its actual status and message instead of `EventSource`'s status-free `onerror` — which, in the old transport, meant a gateway `401` surfaced as a silent `closed` with no `error` callback at all. Note the limit: a browser stream going cross-origin only sees the status if the rejection passes CORS and the gateway answered the `Authorization` preflight; otherwise it degrades to a retryable network error. - **Non-SSE `200`s refused** (`SSE_BAD_CONTENT_TYPE`). An auth gateway's login page would otherwise feed HTML to the parser, which per the SSE grammar parses to *nothing* — leaving the stream live and permanently silent. - **No Node polyfill.** `polyfills.ts` and the `eventsource` devDependency are deleted. - **`options.fetch` / `headers` / `fetchOptions` reach streams**, closing the carve-out documented in #456. ## Behavior changes to be aware of - **A credentialed cross-origin browser stream now preflights on the initial connect.** `EventSource` never preflighted at all (its request isn't a `fetch()`, so Fetch's unsafe-request flag is never set). A proxy that answers CORS itself must allow `Authorization` on `OPTIONS /v1/stream` or the stream never opens. Documented in `reverse-proxy.mdx`. - **A proxy that strips `Authorization` on `/v1/stream`, or redirects it while credentialed, now breaks.** Both previously "worked" by accident. - **Resumption is at-least-once and time-bounded** — this was always true; the docs now say so. The last event you saw is *certainly* redelivered (the id is a `received_timestamp` and replay is inclusive), the SDK does not dedupe live frames, and replay is capped by `mq.gap_window_minutes` (15 default). ## Testing `sse.test.ts` is rewritten against an injected fetch returning a scripted `ReadableStream`. The old harness stubbed a global `FakeEventSource` and could only assert on URL strings — framing, reconnect, and resumption had **no coverage at all**. 204 tests now, with 14 behavioral fixes mutation-verified: reverting each one makes a test fail, and — after review caught a case where it didn't — fail on the assertion that names it. The e2e auth test was rewritten to be discriminating: `anon` is denied `payload` on the events table, so dropping the `Authorization` header flips the assertion. The previous version could not fail. ## Beyond the nominal scope Two REST-path fixes ride along, both surfaced by review of the streaming work and both in `clients/ts/src/http.ts`. Flagging them because an SSE PR is not where you would look for them, and either can be split out on request. - **A cancelled request could throw instead of returning `ABORTED`.** The network-error backoff is the one `sleep` inside `request()`'s catch, so its rejection had no handler and escaped as a raw `DOMException`. Nothing wraps `request()`, so it reached callers as an unhandled rejection — and the `AbortController` example in our own reference demonstrated a branch that could not be taken against an unreachable server. - **Abort is now classified from the signal, not the rejection's type.** Keying off the error made the outcome depend on `maxRetries`: `AbortSignal.timeout()` raises a `TimeoutError`, so it reported `NETWORK_ERROR` at `maxRetries: 0` and `ABORTED` at `2`. It also mis-handled middleware — an `options.fetch` enforcing its own per-attempt deadline aborts an internal controller while the caller never cancelled, which is transient and should be retried, not reported as a terminal `ABORTED`. The same rule then had to be applied to the stream transport, where the old error-type check was worse: an `AbortError` from `auth()` or a custom `fetch` ended the stream terminally and emitted **nothing**. ## Review Fifteen pre-push rounds against both gating reviewers, who verify by executing the code rather than reading it. Worth knowing what they caught, since none of it was reachable by CI: | | | |---|---| | Behavior bugs | `SSE_CONNECT_ERROR` never reaching a subscriber; a closed stream stuck reporting `live`; a consumed-body guard that didn't guard; an unhandled rejection that killed the host process; a stranded reconnect timer | | Regression vs `EventSource` | `close()` from inside a handler no longer stopped delivery | | Coverage holes | deleting the bearer half of the credentialed-redirect rule left the suite green | | False claims in docs | `EventSource` "preflighted on reconnect" (it never preflights); "WaveHouse does not reject a stream" (it 400s on a missing table); "the SDK isolates a throwing handler" (true only inside the transport — several paths outside it are not, now enumerated in the SDK reference and filed as #473) | Two recurring shapes, both worth knowing before you read the diff. **In the code: a guard or cleanup applied to N−1 of N call sites.** Six defects shared it. `if (this._closed) return` now appears eight times in `sse.ts`, several added a round apart. Assume any new early-exit path in this transport is the one that got missed. **In the prose: a true mechanism attached to a wider case set than it holds for.** This accounts for essentially every documentation defect found here, and it recurred for eight consecutive rounds — three times *inside the sentence written to fix the previous instance*. The reviewers' diagnosis is the useful part: none of these were factual errors about the system, they were missing quantifiers. Nearly every claim in this area is a function of a variable the docs cannot name — the reader's token-provider latency, which origin, which credentials mode — so the domain lives only in the author's head at the moment of writing, and the next revision reaches for the deepest true mechanism and silently re-attaches it to the whole case set. The empirical tell was sharp: the rule-shaped sentences never needed correcting; the value-shaped ones were corrected every round. The Live Queries failure section is written to that conclusion — it states rules and gives the reader a test, rather than reporting which outcome is typical. Two amplifiers were also removed: sentences that counted table rows (a ninth row would have silently falsified five of them) and facts restated independently in four or five files. **If you are reviewing prose here, the question that finds bugs is "for which cases is this true?", not "is this true?"** Four axes account for essentially every defect found on this branch, and a claim that is silent about which side it means is the shape to distrust: 1. **Who rejected it** — WaveHouse (a `400` on the stream route; a `404`/`405` off it) versus something in front. Never "the server said 401": `/v1/stream` is ungated. 2. **Where the caller runs** — server-side or same-origin (statuses visible) versus browser cross-origin, where CORS can make any rejection, including a rejected preflight, indistinguishable from a network drop. 3. **Whether the request carries a credential** — decides `redirect: "manual"` versus `"follow"`, and the test is `Authorization`-or-`headers`, so cookies are *not* credentials by it (#478). 4. **Whether the failure is semantically transient** — the 4xx-is-terminal rule is a transport mechanism, not a claim about the world (#469). One structural note so it isn't rediscovered: `CHANGELOG.md` is denylisted from the docs-prose gate (`scripts/docs-prose.sh:38`), so that entry has never been read by the automated docs reviewer. It is worth reading at docs scrutiny rather than skimming as boilerplate — a false claim survived three rounds there for exactly that reason. **The Go diff is one comment, so this looks deployment-free. It isn't.** A credentialed cross-origin browser stream now preflights where `EventSource` never did, so a proxy that answers CORS itself must allow `Authorization` on `OPTIONS /v1/stream` or streams stop opening — silently, in a retry loop, not with a visible error. That break is documented in `reverse-proxy.mdx`; a reviewer reading only `internal/` will conclude nothing operational changed. ## Follow-ups filed Design work deferred out of this PR: - **#465** — gzip on `/v1/stream` (per-frame flush; measure before adopting) - **#466** — normalize a schemeless `baseURL`, with a loopback exception - **#467** — binary framing negotiated via `Accept`, sequenced behind #465 - **#468** — retire `?token=` server-side; closes #203 - **#204** — commented with the POST-body analysis that unblocks multiplexing Defects found by review of this branch and left unfixed here, each because the fix lands outside the transport or carries a design question I didn't want to answer unilaterally in a PR about auth: - **#469** — a stream `429` is terminal; should honor `Retry-After` - **#471** — a rejected resumption preflight leaves a stream re-dialing forever - **#473** — a throwing subscriber silently stops delivery to the others. Widened during review to cover every path outside the transport's guard — `.subscribe()`'s initial unguarded `status` call (worse via `liveQuery()`, which returns no handle at all), the fan-out dropping the event for a concurrent `for await` and leaving it un-terminated on a terminal close, a throwing `status` handler making `.connected()` time out against a live stream, and `liveQuery()`'s backfill flush discarding its buffer. The docs and CHANGELOG now describe the real contract rather than the one I first wrote. - **#476** — `http.ts`'s `sleep()` leaks an abort listener when the timer wins - **#484** — transport hardening against a non-conforming peer, raised as "what I'd watch" in the final review: a parser-buffer overflow re-dials at a flat rate forever because the backoff reset counts the overflowing connection as healthy, and `FetchLike`'s contract never states that `init.signal` must be honored. Neither is reachable with a conforming peer; both are cheap now. - **#477** — `StreamController` retains every event when nothing iterates. The buffer's only drain is the async iterator's `next()`, so a `.subscribe()`-only consumer — the pattern the docs lead with — holds every event it has ever received, unbounded. Doubled on a filtered or live stream, since both controller layers buffer. - **#478** — a cookie-authenticated stream bypasses the redirect guard. `credentialed` tests for a bearer token or configured `headers`; cookies are neither, so the request follows a redirect and can arrive unauthenticated. - **#449** — pre-existing liveQuery dedup boundary, re-confirmed by review - **#445** — not touched here, but surfaced again while reviewing the streaming docs and worth a person's eye: `wh.pipe(name).stream()` is documented as working in three places while `pipes.ts` streams `?table=<pipeName>`, so it subscribes to a topic nothing publishes and silently yields nothing. The DLQ variant carries an inline caveat for the same class of gap; the pipe one doesn't. ## Reviewer notes The interesting file is `clients/ts/src/stream/sse.ts`. `controller.ts` is untouched — the transport sits behind the same `StreamTransport` interface — so the diff is scoped to the transport, its tests, and the docs the change invalidated. `clients/ts/src/stream/live-query.test.ts` is new and is the first test coverage `LiveQuery` has had. It pins one thing worth knowing about: the backfill, not the stream, spends the first `auth()` call, and that ordering is emergent from four independent details rather than declared anywhere. One added `await` on the REST path silently swaps the two failure modes the docs describe, so the test exists to make that a red build rather than a documentation drift. Merged with `main` at `1064a4fe`, which brought #381 (per-subscriber SSE row filtering) and #457. Both conflicted textually with this branch and both were resolved keeping each side; #381 adds no new status code, so this PR's claim that `/v1/stream` raises exactly one 4xx itself still holds. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
jfwoods
added a commit
that referenced
this pull request
Aug 21, 2026
Three catch-up changes, all client-side. The server is untouched. Routes: main merged every admin-gated endpoint under /v1/ops (#479) with no aliases, so thirteen call sites were 404ing against a current server — schema list/refresh, DLQ stats, raw SQL, policy get/put/validate, and pipes CRUD, plus the codegen CLI's schema fetch. Rewrote them along with the tests, the shared wire_cases.json fixture, and the Go SDK docs. The fixture is replayed by both conformance runners, so the stale paths broke the TypeScript half too; `make test-conformance-ts` is back to 45/45. ClientOptions.Headers: the TypeScript SDK gained options.headers in #456 and Go had no equivalent. Headers now apply to every request the client makes, REST and SSE alike — which is also how an operator sends the server's non-JWT X-Operator-Key. The SDK's own headers are set afterwards and win a collision; net/http canonicalizes names, so matching is case-insensitive; the map is copied at construction so later mutation can't reach into requests. SSE robustness, mirroring main's fetch-based rewrite (#470). The Go SDK already authenticated by header, so that part was never stale, but three gaps were: - A credentialed stream followed redirects. net/http drops Authorization on a cross-host hop while forwarding custom headers verbatim, so a redirect either downgraded the stream to default_role in silence or handed configured secrets to wherever it pointed. Now refused with a terminal SSE_REDIRECT. Uncredentialed streams still follow. - A 200 with any content type was treated as an event stream, so an auth gateway's login page left the stream sitting in StatusLive delivering nothing. Now a terminal SSE_BAD_CONTENT_TYPE. - Every failure collapsed into one retryable SSE_ERROR, and malformed frames came back as a bare fmt.Errorf, so errors.As and IsRetryable didn't work on them. Replaced with the taxonomy the TypeScript SDK uses — SSE_AUTH_ERROR, SSE_NETWORK_ERROR, SSE_CONNECT_ERROR, SSE_REDIRECT, SSE_BAD_CONTENT_TYPE, SSE_PARSE_ERROR, SSE_READ_ERROR — each with its own retryable flag, all delivered as *Error. Also documents what main changed underneath the Go SDK without changing its code: DateTime values arrive canonicalized to RFC 3339 UTC (#402), SSE applies policy row-filters per subscriber and fails closed (#381, #457), /v1/stream is ungated so WaveHouse never 401s a stream, and /v1/ops/dlq/stats is absent (404) when the DLQ is disabled rather than returning empty stats. Tests: terminal-failure table (bad content type, missing content type, credentialed redirect, non-HTTP scheme), redirect-followed-when- uncredentialed, typed retryable parse errors, and header precedence and copying on both transports.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A row filter whose
{{ jwt.* }}template references a claim the token doesn't carry (absent ornull) previously rendered to''and bound a real predicate —tenant_id != ''matches essentially every row, erasing the restriction.resolveTemplatenow reports resolution failure andresolveFiltersemits the constant-false1 = 0for every operator, matching the fail-closed behavior_inalready had from #224.nullfails closed.checkkeeps its resolve-to-''semantics (security(policy): define insert-check behavior when a claim template can't be resolved #463), now pinned by a test.access-control.mdx.Also pins the untested half of #323. That issue's behavior fix landed incidentally in #353 (a perf refactor), which is why it was never closed; both of its conditions — non-
EventMessageand emptytable_name— collapse into thedecodedflag atinternal/stream/hub.go:160, but only the first half has a regression test. This adds the second.The row filter and the role's
max_rowscap are now emitted structurally insideBuildrather than spliced into rendered SQL afterward —InjectPermissionFilters,findInsertPoint,ApplyMaxRowsand the interimspliceKeywordReidentifier guard are all deleted (#322). That splice was what let a crafted aggregation alias capture the injectedWHEREand drop the row filter entirely; it is not separable from this PR, because making_eq/_neq/_gt/_ltfail closed is what made the constant-false predicate reachable enough to exploit. Identifier names go back to unrestricted.Claim values are also handled honestly now: numeric claims decode as
json.Number(jwt.WithJSONNumber()), so a large tenant id no longer rounds throughfloat64, and a claim resolving to an object or array fails closed for the scalar operators while a bare-claim array stays the_inset.Test plan
""and present-but-empty claims bind normally, check-path_eqresolution pinnedEvaluate: unresolvable claim yields constant-falseWhereClausetable_namefail-closed path, driven throughReplayFrame(security(streaming): applyStreamPolicy passes non-EventMessage / empty table_name payloads through unfiltered (fail-open) #323)e WHERE z, U+0131e lımıt z,ORDER BYalias-ref) assert the policy predicate survives in the emitted SQL, not merely thatBuilderrors (security(query): RLS predicate is spliced into rendered SQL by substring match — a crafted alias deletes it (and max_rows with it) #322)Related Issues
Closes #385
Closes #323
Closes #322