fix(stream): apply policy row-filter per subscriber on SSE - #381
Open
taitelee wants to merge 25 commits into
Open
fix(stream): apply policy row-filter per subscriber on SSE#381taitelee wants to merge 25 commits into
taitelee wants to merge 25 commits into
Conversation
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds claims-aware, schema-aware row-level filtering to live and replayed SSE delivery. Subscribers store immutable JWT claim snapshots. Shared policy predicates support SQL and in-memory evaluation, with fail-closed comparisons and withheld-row metrics. ChangesStreaming row-level security
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant StreamHandler
participant Hub
participant Policy
participant SchemaRegistry
StreamHandler->>Policy: resolve role and claims
StreamHandler->>Hub: register subscriber with claims
Client->>StreamHandler: open SSE stream
Hub->>SchemaRegistry: resolve column comparison types
Hub->>Policy: evaluate row visibility
Hub-->>Client: deliver projected visible event
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📚 Docs preview is live → https://9002a59c-wavehouse-docs.wave-rf.workers.dev |
Contributor
Code Coverage OverviewLanguages: Go GoThe overall coverage in commit 2abde27 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/content/docs/access-control.mdx (1)
373-373: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the Live stream row in the enforcement table to include row-level filtering.
The "Where each rule is enforced" table at line 373 still reads "denied columns are masked from each event" with no mention of row-level
filter, but this PR adds exactly that enforcement. The Structured read row (line 371) explicitly lists "row filter", so the table is now inconsistent with both the caution block below (lines 377–378) and the actual code (hub.goBroadcastcallsRowVisibleper subscriber). Per the docs code↔docs sync guideline, the table should reflect the changed behavior.📝 Proposed fix to update the table row
-| Live stream | `GET /v1/stream` | table+role `select` required (a table the role can't read is skipped), then denied columns are masked from each event | +| Live stream | `GET /v1/stream` | table+role `select` required (a table the role can't read is skipped), then denied columns are masked from each event, and row-level `filter` predicates are evaluated per subscriber against their JWT claims (see caution below) |Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2efc09f7-0227-4f93-844b-1b18b486200f
📒 Files selected for processing (17)
CHANGELOG.mdcmd/wavehouse/main.godocs/src/content/docs/access-control.mdxdocs/src/content/docs/architecture.mdinternal/api/errors_test.gointernal/api/router_test.gointernal/api/stream.gointernal/api/stream_test.gointernal/discovery/validation.gointernal/discovery/validation_test.gointernal/policy/policy.gointernal/policy/rowfilter.gointernal/policy/rowfilter_test.gointernal/stream/hub.gointernal/stream/hub_test.gointernal/stream/subscriber.gotests/integration/setup_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: E2E tests
- GitHub Check: Docs build
- GitHub Check: Coverage
- GitHub Check: Lint
- GitHub Check: Analyze (go)
⚠️ CI failures not shown inline (2)
GitHub Actions: PR housekeeping / 0_PR housekeeping.txt: fix(stream): apply policy row-filter per subscriber on SSE
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
GitHub Actions: PR housekeeping / PR housekeeping: fix(stream): apply policy row-filter per subscriber on SSE
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
🧰 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/api/errors_test.gotests/integration/setup_test.gointernal/api/stream_test.gointernal/api/router_test.gointernal/discovery/validation_test.gointernal/policy/rowfilter_test.gointernal/stream/hub_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.mdxdocs/src/content/docs/architecture.md
🧠 Learnings (5)
📚 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/errors_test.gointernal/api/stream_test.gointernal/api/router_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/errors_test.gointernal/api/stream_test.gointernal/api/router_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/errors_test.gotests/integration/setup_test.gointernal/api/stream_test.gointernal/api/router_test.gointernal/discovery/validation_test.gointernal/policy/rowfilter_test.gointernal/stream/hub_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/errors_test.gocmd/wavehouse/main.gointernal/discovery/validation.gotests/integration/setup_test.gointernal/api/stream.gointernal/api/stream_test.gointernal/api/router_test.gointernal/discovery/validation_test.gointernal/policy/policy.gointernal/policy/rowfilter.gointernal/stream/subscriber.gointernal/policy/rowfilter_test.gointernal/stream/hub.gointernal/stream/hub_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.mdCHANGELOG.md
🔇 Additional comments (20)
CHANGELOG.md (1)
34-34: LGTM!docs/src/content/docs/access-control.mdx (1)
377-378: LGTM!docs/src/content/docs/architecture.md (1)
88-88: LGTM!cmd/wavehouse/main.go (1)
295-295: LGTM!tests/integration/setup_test.go (1)
315-315: LGTM!internal/api/errors_test.go (1)
197-197: LGTM!internal/api/router_test.go (1)
286-286: LGTM!Also applies to: 342-342, 414-414, 475-475, 526-526, 551-551, 648-648
internal/api/stream_test.go (1)
25-25: LGTM!Also applies to: 53-53, 78-78, 117-117
internal/stream/hub_test.go (2)
41-51: LGTM!
68-68: LGTM!Also applies to: 100-100, 136-136, 288-288, 331-331, 352-352, 373-373, 390-390, 411-411, 440-440
internal/policy/policy.go (1)
64-82: LGTM!Also applies to: 197-208, 228-313
internal/policy/rowfilter.go (2)
13-15: LGTM!
17-103: 🔒 Security & PrivacyLexicographic fallback is intentional
NewHubis already wired with a schema registry incmd/wavehouse/main.go, andnumericColsdeliberately falls back to string comparison when schema information is unavailable. That makes>/<best-effort by design, so this isn’t a security bug; the doc comment is the part that should be softened.> Likely an incorrect or invalid review comment.internal/policy/rowfilter_test.go (1)
1-108: LGTM!internal/discovery/validation.go (1)
51-69: LGTM!Also applies to: 152-158
internal/discovery/validation_test.go (1)
227-255: LGTM!internal/api/stream.go (1)
45-51: LGTM!Also applies to: 85-85, 103-103
internal/stream/hub.go (2)
25-31: LGTM!Also applies to: 43-45
185-198: LGTM!Also applies to: 218-233, 245-277
internal/stream/subscriber.go (1)
26-31: LGTM!Also applies to: 51-55
github-project-automation
Bot
moved this from In progress
to In review
in WaveHouse Task Board
github-actions
Bot
added
the
dependencies
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: c2faf18f-1e6f-44b3-92f4-aa3152c98c0c
📒 Files selected for processing (6)
go.modinternal/policy/policy.gointernal/policy/rowfilter.gointernal/policy/rowfilter_test.gointernal/stream/hub.gointernal/stream/hub_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: Integration tests
- GitHub Check: E2E tests
- GitHub Check: Docs build
- GitHub Check: Unit tests
- GitHub Check: Coverage
- GitHub Check: Lint
- GitHub Check: Analyze (go)
⚠️ CI failures not shown inline (2)
GitHub Actions: PR housekeeping / PR housekeeping: fix(stream): apply policy row-filter per subscriber on SSE
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
GitHub Actions: PR housekeeping / 0_PR housekeeping.txt: fix(stream): apply policy row-filter per subscriber on SSE
Conclusion: failure
##[group]Run # Single source of truth for the rule: scripts/lint-pr-title.sh — the
�[36;1m# Single source of truth for the rule: scripts/lint-pr-title.sh — the�[0m
�[36;1m# SAME validator the local agent gate runs (.claude/hooks/agent-bash-gate.sh),�[0m
�[36;1m# so CI and local can't drift. The checkout above is ref: main, so this is�[0m
�[36;1m# always the default-branch script. Dependabot's grouped-update titles�[0m
�[36;1m# routinely exceed the 72-char subject cap and the format isn't�[0m
�[36;1m# configurable, so Dependabot PRs are exempt from the length check�[0m
�[36;1m# (the format check still applies).�[0m
�[36;1mif [[ "$PR_AUTHOR" == "dependabot[bot]" || "$PR_AUTHOR" == "app/dependabot" ]]; then�[0m
�[36;1m export PR_TITLE_SKIP_LENGTH=1�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mif reason=$(bash scripts/lint-pr-title.sh "$PR_TITLE" 2>&1); then�[0m
�[36;1m echo "passed=true" >> "$GITHUB_OUTPUT"�[0m
�[36;1m echo "PR title OK: $PR_TITLE"�[0m
�[36;1melse�[0m
�[36;1m echo "passed=false" >> "$GITHUB_OUTPUT"�[0m
�[36;1m printf '%s\n' "$reason"�[0m
�[36;1m echo "::error::$(printf '%s' "$reason" | head -1)"�[0m
🧰 Additional context used
📓 Path-based instructions (1)
**/*_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/rowfilter_test.gointernal/stream/hub_test.go
🧠 Learnings (2)
📚 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/stream/hub.gointernal/policy/rowfilter.gointernal/policy/policy.gointernal/policy/rowfilter_test.gointernal/stream/hub_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/rowfilter_test.gointernal/stream/hub_test.go
🔇 Additional comments (8)
internal/policy/policy.go (2)
111-175: LGTM!Also applies to: 177-233
235-260: LGTM!Also applies to: 262-461
internal/policy/rowfilter.go (1)
78-139: LGTM!internal/policy/rowfilter_test.go (1)
111-167: LGTM!Also applies to: 169-232
internal/stream/hub.go (2)
142-171: LGTM!Also applies to: 221-236
172-173: 🩺 Stability & AvailabilityNo race here —
Subscriber.claimsis set once viaSetClaimsbeforeHub.Addand documented as read-only for the subscriber’s lifetime, soBroadcast’s read does not have a concurrent writer.> Likely an incorrect or invalid review comment.internal/stream/hub_test.go (1)
22-29: LGTM!Also applies to: 188-214, 440-464, 519-526, 530-545
go.mod (1)
3-3: LGTM!
This was referenced Aug 11, 2026
… type matrix checks shape only
…rejections in throw list
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6ce634f7-b823-4488-b378-9aa15726a00a
📒 Files selected for processing (13)
CHANGELOG.mdSECURITY.mdclients/ts/README.mddocs/src/content/docs/access-control.mdxdocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/sdk/reference.mddocs/src/content/docs/sdk/streaming.mdinternal/stream/hub.gointernal/stream/hub_test.gointernal/stream/metrics.gointernal/stream/subscriber.gotests/e2e/sdk/streaming.test.ts
📜 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 (5)
tests/e2e/sdk/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
tests/e2e/sdk/*.test.ts: Add new E2E scenarios astests/e2e/sdk/*.test.tsfiles using helpers fromtests/e2e/sdk/helpers.ts.
A new test file must (1) add its suite name toSUITESintables.tsand (2) get its names viaconst T = suiteTables("<suite>"), then referenceT.clicksetc. — never a bareclicks.
Files:
tests/e2e/sdk/streaming.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
The TypeScript SDK (
@wavehouse/sdkinclients/ts/) is the canonical client and ships from this repo.
Files:
tests/e2e/sdk/streaming.test.ts
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Error handling: Return errors, don't panic. Wrap withfmt.Errorf("context: %w", err).
- No global state: Dependencies are passed explicitly (constructor injection).
- Package naming: Lowercase, single word (or abbreviated).
internal/enforces module privacy.- Table-driven tests: Use
tests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
Files:
internal/stream/metrics.gointernal/stream/subscriber.gointernal/stream/hub.gointernal/stream/hub_test.go
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Comment the why, not the what. Add a comment only when the reason isn't obvious from the code; a line that matches the surrounding pattern needs none. Keep comments to 1–2 lines and match the file's existing density.
Files:
internal/stream/metrics.gointernal/stream/subscriber.gointernal/stream/hub.gointernal/stream/hub_test.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/stream/hub_test.go
🧠 Learnings (47)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/stream/**/*.{go} : Streaming/SSE code must preserve the hub’s per-role projection model, subscriber queues, bucket fan-out, heartbeating, and metrics semantics.
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 clients/ts/README.md : Keep the TypeScript SDK README in sync with SDK-facing changes and public client behavior.
Applied to files:
clients/ts/README.mdtests/e2e/sdk/streaming.test.tsdocs/src/content/docs/sdk/streaming.md
📚 Learning: 2026-08-11T15:22:23.813Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: clients/ts/src/types.ts:61-71
Timestamp: 2026-08-11T15:22:23.813Z
Learning: In the TypeScript SDK, `ClientConfig.baseURL` must be an absolute URL with a scheme and host. A relative `baseURL` causes `resolveURL` to throw a `TypeError` on the first request. REST requests reject because `resolveURL` runs outside `request()`'s retry `try` block. In `clients/ts/src/stream/sse.ts`, `SSETransport` catches this failure and reports `SSE_CONNECT_ERROR` through the optional `StreamSubscriber.error` callback, so a subscriber without that callback can observe no error.
Applied to files:
clients/ts/README.mdtests/e2e/sdk/streaming.test.tsdocs/src/content/docs/sdk/reference.md
📚 Learning: 2026-08-12T05:38:52.277Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 455
File: tests/e2e/sdk/helpers.ts:214-221
Timestamp: 2026-08-12T05:38:52.277Z
Learning: In `tests/e2e/sdk/helpers.ts`, `chQuery` must reclassify a caught error as a request timeout or caller abort only when the error is an abort error. On Node 22, `AbortSignal.timeout()` produces an error named `TimeoutError`, `AbortController.abort()` and caller cancellation through `AbortSignal.any()` produce `AbortError`, and `JSON.parse()` failures produce `SyntaxError`.
Applied to files:
clients/ts/README.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to clients/ts/**/*.{ts,tsx} : The TypeScript SDK in `clients/ts/` is the canonical client; when backend API changes affect users, update the SDK surface, auth handling, query builder, streaming helpers, pipes/policy helpers, or regenerated types as needed.
Applied to files:
clients/ts/README.mdtests/e2e/sdk/streaming.test.tsdocs/src/content/docs/sdk/streaming.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:
clients/ts/README.mdSECURITY.mddocs/src/content/docs/sdk/reference.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/sdk/streaming.mdCHANGELOG.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:
SECURITY.mddocs/src/content/docs/architecture.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:
SECURITY.mddocs/src/content/docs/architecture.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:
SECURITY.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.mdinternal/stream/hub.gointernal/stream/hub_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:
SECURITY.mdinternal/stream/hub_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:
SECURITY.mdCHANGELOG.mddocs/src/content/docs/access-control.mdx
📚 Learning: 2026-05-13T21:06:12.242Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 135
File: deployments/signoz/clickhouse/users.xml:68-68
Timestamp: 2026-05-13T21:06:12.242Z
Learning: In the WaveHouse repository, `deployments/signoz/clickhouse/users.xml` intentionally configures the ClickHouse `default` user with an empty password and global network access (`::0`). This is acceptable because the `deployments/signoz/` stack is a local-dev-only SigNoz observability stack; ClickHouse ports are not published to the host in `deployments/signoz/compose.yaml`. Do not flag this as a security issue in future reviews.
Applied to files:
SECURITY.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:
SECURITY.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/stream/**/*.{go} : Streaming/SSE code must preserve the hub’s per-role projection model, subscriber queues, bucket fan-out, heartbeating, and metrics semantics.
Applied to files:
tests/e2e/sdk/streaming.test.tsdocs/src/content/docs/sdk/reference.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.mdinternal/stream/subscriber.godocs/src/content/docs/sdk/streaming.mdCHANGELOG.mdinternal/stream/hub.godocs/src/content/docs/access-control.mdxinternal/stream/hub_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/ingest/types.go : `EventMessage` JSON tags and ingest event shape must stay aligned with docs, SSE examples, and ClickHouse INSERT column order.
Applied to files:
docs/src/content/docs/api.mdinternal/stream/hub.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:
docs/src/content/docs/api.md
📚 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:
docs/src/content/docs/api.mddocs/src/content/docs/access-control.mdx
📚 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:
internal/stream/metrics.godocs/src/content/docs/architecture.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: When adding comments, explain the reason rather than restating the code; keep comments short and only add them when the rationale is not obvious.
Applied to files:
internal/stream/metrics.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/stream/metrics.gointernal/stream/subscriber.gointernal/stream/hub.gointernal/stream/hub_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/stream/metrics.gointernal/stream/subscriber.gointernal/stream/hub.gointernal/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:
docs/src/content/docs/architecture.mdinternal/stream/subscriber.godocs/src/content/docs/sdk/streaming.mdCHANGELOG.mdinternal/stream/hub.godocs/src/content/docs/access-control.mdxinternal/stream/hub_test.go
📚 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:
docs/src/content/docs/architecture.mdCHANGELOG.mdinternal/stream/hub.godocs/src/content/docs/access-control.mdxinternal/stream/hub_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:
docs/src/content/docs/architecture.mdinternal/stream/hub.go
📚 Learning: 2026-08-11T12:41:08.161Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 446
File: .github/actions/setup-env/action.yml:112-127
Timestamp: 2026-08-11T12:41:08.161Z
Learning: In WaveHouse CI, the shared `gomod-v1` cache in `.github/actions/setup-env/action.yml` can be saved by multiple Go jobs on an exact-key miss. Every workflow path that can write this cache must fully populate `~/go/pkg/mod` through the Makefile `go-mod-download` prerequisite before the post-job cache save. The `cov` target must retain this prerequisite.
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.mdinternal/stream/hub.go
📚 Learning: 2026-06-10T15:02:09.425Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: .github/workflows/ci.yml:232-237
Timestamp: 2026-06-10T15:02:09.425Z
Learning: In the Wave-RF/WaveHouse repository, `clickhouse/clickhouse-server:latest` is used deliberately in `tests/integration/setup_test.go`, `scripts/orchestrator/main.go`, and the CI workflow prefetch steps (`docker pull -q clickhouse/clickhouse-server:latest`). The `:latest` tag in the prefetch steps intentionally mirrors the tag testcontainers resolves at runtime — this is a deliberate canary approach. Pinning to a concrete version/digest is a separate decision tracked as a follow-up issue and should not be flagged as a supply-chain concern in CI workflow reviews for this repo.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-12T05:35:22.804Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 455
File: clients/ts/README.md:11-11
Timestamp: 2026-08-12T05:35:22.804Z
Learning: For Wave-RF/WaveHouse, CI currently tests Node 22 only through `.nvmrc` and `.github/actions/setup-env` using `node-version-file`. Local `make ci` verification on Node 26 is not CI coverage and must not be documented as continuously tested runtime support.
Applied to files:
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.mddocs/src/content/docs/access-control.mdx
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to 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.mddocs/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`, 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:
CHANGELOG.md
📚 Learning: 2026-07-07T16:19:29.374Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/observability/**/*.{go} : Observability code must preserve the repo’s logging and telemetry invariants, including stdout fan-out, OTLP behavior, lazy dialing, and the private Prometheus registry.
Applied to files:
internal/stream/hub.go
📚 Learning: 2026-05-20T03:17:53.774Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-05-20T03:17:53.774Z
Learning: Refer to AGENTS.md for project conventions, architecture notes, and AI agent instructions - it is the single source of truth
Applied to files:
internal/stream/hub.go
📚 Learning: 2026-05-13T15:14:46.366Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-13T15:14:46.366Z
Learning: Refer to AGENTS.md for project conventions, architecture notes, and AI agent instructions
Applied to files:
internal/stream/hub.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:
docs/src/content/docs/access-control.mdx
📚 Learning: 2026-08-11T21:56:06.521Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: docs/src/content/docs/sdk/go/queries.md:356-362
Timestamp: 2026-08-11T21:56:06.521Z
Learning: In Wave-RF/WaveHouse Go SDK cursor pagination, `fetchNextTyped` uses only the first `QueryBuilder.OrderBy` column and a strict `gt` or `lt` filter. Duplicate values at a page boundary can skip rows. The Go SDK documentation must require a unique ordering column until the shared Go and TypeScript composite-cursor or tie-breaker implementation tracked in GitHub issue `#452` is available.
Applied to files:
docs/src/content/docs/access-control.mdx
📚 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:
docs/src/content/docs/access-control.mdxinternal/stream/hub_test.go
📚 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:
docs/src/content/docs/access-control.mdxinternal/stream/hub_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/stream/hub_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/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/stream/hub_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 **/*.{go} : Go code must use Go 1.26 conventions: gofumpt formatting, structured logging with `log/slog`, Chi v5 routing, explicit error returns, no global state, and package naming that is lowercase and single-word or abbreviated.
Applied to files:
internal/stream/hub_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/stream/hub_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/stream/hub_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/stream/hub_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/stream/hub_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/stream/hub_test.go
🪛 LanguageTool
docs/src/content/docs/sdk/reference.md
[style] ~40-~40: A comma is missing here.
Context: ...RROR| Yes | Stream failed to connect (e.g. a non-absolutebaseURL) | | 0 | SSE_...
(EG_NO_COMMA)
docs/src/content/docs/sdk/streaming.md
[style] ~115-~115: ‘On top of that’ might be wordy. Consider a shorter alternative.
Context: ...ed). ### Client-Side Stream Filtering On top of that, when a QueryBuilder with .where() ...
(EN_WORDINESS_PREMIUM_ON_TOP_OF_THAT)
🔇 Additional comments (20)
internal/stream/metrics.go (1)
89-91: Keep theRowWithheldcomment concise.This repeats the prior comment-density concern. Keep one short sentence that explains why stable
tableandrolelabels are used.As per coding guidelines: “Comment the why, not the what.” Keep comments to 1–2 lines.
Source: Coding guidelines
internal/stream/subscriber.go (1)
55-88: LGTM!internal/stream/hub.go (1)
6-6: LGTM!Also applies to: 220-231
internal/stream/hub_test.go (3)
214-244: LGTM!
658-691: LGTM!
693-723: LGTM!tests/e2e/sdk/streaming.test.ts (1)
168-193: LGTM!CHANGELOG.md (1)
38-38: LGTM!docs/src/content/docs/access-control.mdx (1)
373-393: LGTM!docs/src/content/docs/api.md (3)
26-26: LGTM!
214-217: LGTM!
542-542: LGTM!docs/src/content/docs/architecture.md (2)
88-92: LGTM!
117-117: LGTM!Also applies to: 142-143
docs/src/content/docs/sdk/reference.md (2)
28-28: LGTM!Also applies to: 40-43
90-90: LGTM!Also applies to: 113-113
clients/ts/README.md (2)
58-58: LGTM!
130-132: LGTM!docs/src/content/docs/sdk/streaming.md (2)
70-79: LGTM!
110-116: LGTM!
…forcement, dedup subscriber comment
…d UInt*, split row-level enforcement by path
3 tasks
EricAndrechek
requested changes
Aug 12, 2026
EricAndrechek
left a comment
EricAndrechek
left a comment
Member
There was a problem hiding this comment.
Nice work, basically everything from the previous round is fixed/addressed. Two new things:
- I think (Claude tested it for me and "confirmed" it, but obviously take that with a grain of salt) that the JWT claim mechanism may lead to improper number comparisons when really large
- A few fixes need to be made to timestamp filtering now that #402 has been merged in
| // compareExact compares two numeric strings at arbitrary precision — the tie-break | ||
| // for operands float64 cannot tell apart. ok=false when either side isn't an exact | ||
| // rational (±Inf, malformed), failing the predicate closed. | ||
| func compareExact(a, b string) (int, bool) { |
Member
There was a problem hiding this comment.
A subscriber can be delivered another tenant's rows. This fails open, and it's the one direction the CHANGELOG entry says is impossible.
compareExact compares the event's exact digits against the filter constant — but the constant was already rounded before it got here:
internal/auth/auth.go:206callsjwt.Parsewithoutjwt.WithJSONNumber(), so a numeric JWT claim decodes asfloat64.resolveTemplate(internal/policy/policy.go:328) stringifies it withfmt.Sprint, turning10000000000000001into"1e+16".compareExactthen does an exact comparison against a value whose digits were already lost.
Reproduced with a real signed HS256 token carrying tenant: 10000000000000001:
decoded claim: float64 1e+16 -> fmt.Sprint = "1e+16"
tenant 10000000000000001 -> WITHHELD (the subscriber's own rows)
tenant 10000000000000000 -> DELIVERED (a different tenant's rows)
The query path binds the same constant and ClickHouse rejects it outright (WHERE tenant_id = '1e+16' → Code: 53, cannot convert to UInt64), so the stream is strictly more permissive than the query path here.
Needs a bare-JSON-number integer claim above 2^53 against a numeric column — Snowflake-style tenant IDs, which is exactly the case the math/big tie-break was written for.
jwt.WithJSONNumber() on the parser fixes the stream, the query path and the check-clause auto-inject together, but it lands in internal/auth — outside this PR's footprint, so worth a call on scope. Failing the predicate closed when a claim arrives as a float64 at or above 2^53 would keep it in-package.
Member
Author
There was a problem hiding this comment.
Fixing so that the middleware now parses with jwt.WithJSONNumber() so numeric claims keep their exact digits, and claimString refuses any float64 at or above 2^53 as an in-package backstop, so the predicate matches no rows instead. Smaller floats render positionally now too, which also fixes the check clause auto-inject for those claims.
| hub := NewHub(policy.NewMemoryStore(p), reg, nil) | ||
| const topic = "ingest.clicks" | ||
|
|
||
| neighbor := NewSubscriber(map[string]any{"tenant": "10000000000000000"}) // float64-equal neighbor |
Member
There was a problem hiding this comment.
These claims are strings, and jwt.Parse never produces a string for a bare JSON number — it yields float64. So this test passes while the production path it stands for does not (see my note on rowfilter.go).
Constructing the subscriber from claims that came out of jwt.Parse, rather than a literal map, would have caught it. Worth doing for any test that asserts a claims-derived guarantee.
Member
Author
There was a problem hiding this comment.
Ok. Now I have the row filter hub testst build claims by running a real signed token through auth.Middleware (jwtClaims helper) instead of literal maps, which is what surfaced the float64 issue.
|
|
||
| A few more edges worth knowing when you write a policy — the stream evaluates the **ingested event payload**, not the stored row, and each of these follows from that: | ||
|
|
||
| - **A filtered column the payload doesn't carry withholds *every* event** for that subscriber, even though the same filter matches normally on the query path. That bites a `MATERIALIZED`/`ALIAS` column (never part of an ingest payload) or a `DEFAULT` column your clients omit. The recommended [`check` + `filter` pairing](#insert-checks) is unaffected: an `_eq` insert `check` auto-injects its claim value into any payload that omits the column *before* the event is published, so the streamed event carries it and the matching row filter evaluates normally. |
Member
There was a problem hiding this comment.
This guarantee doesn't hold for DateTime/DateTime64 columns since the #402 merge, and the problem is wider than the check + filter pairing.
The check runs at internal/api/ingest.go:374 and injects the raw claim value; CanonicalizeTimestamps rewrites it at :406, before the publish. The stream then compares the canonicalized payload against the un-canonicalized claim under ColumnOpaque byte-equality, so every row is withheld. Auto-injection isn't required — a client-sent value takes the same path, because the check compares pre-canonicalization and the filter compares post. In practice any _eq/_in row filter on a timestamp column is affected, since ingest rewrites every producer spelling.
The harder part: there is no constant that works on both surfaces. Verified against ClickHouse 25.10:
WHERE created_at = '2026-06-21 04:00:00' -> 1 row (stream withholds every row)
WHERE created_at = '2026-06-21T04:00:00Z' -> Code: 53 TYPE_MISMATCH
date_time_input_format=best_effort doesn't help; it governs insert parsing only. So the advice two bullets up to "pick a value that works on both surfaces" isn't actionable here — the two spellings are disjoint. Replay is affected identically, and no test currently uses a DateTime-typed filter column.
It fails closed, so this is availability rather than exposure. Two ways out, and the choice is worth making explicitly:
- Give
DateTime/DateTime64their ownColumnKindand run the filter constant through the same parser before comparing. That makes_eq/_inspelling-independent and would also unlock_gt/_lton timestamps — the natural shape for a time-window policy. - If that's out of scope here, say plainly that a timestamp row filter can't be enforced on the stream, and drop the "pairing is unaffected" sentence.
AGENTS.md:68points at fix(stream): apply policy row-filter per subscriber on SSE #381 for fail-closed enforcement on this type, so that pointer would need qualifying too.
|
|
||
| - **Numeric columns** (`Int*`/`UInt*`/`Float*`/`Decimal*`, unwrapping `Nullable`/`LowCardinality` in any nesting): all five operators compare numerically, matching ClickHouse (`9 < 100`). Equality ties beyond `float64`'s 2^53 resolve at full precision (a 64-bit ID never falsely matches a neighbor, whether it arrives string-encoded or as a bare JSON number); an unparseable or `NaN` operand withholds the row. | ||
| - **`String` columns** (again under any `Nullable`/`LowCardinality` wrapping): byte comparison *is* ClickHouse's String comparison — equality and ordering are both exact. | ||
| - **Every other type** (`Enum`, `UUID`, `Date`/`DateTime`, `Bool`, `IPv4`/`IPv6`, `FixedString`, …): only byte-equality is trusted. `_eq`/`_in` admit exactly the event's own text rendering — write the filter value the way your events carry it (`true`, not `1`; a lowercase UUID if that's what clients send). The same constant is bound into the query path's SQL, where ClickHouse compares it against the column's declared type rather than the event's text rendering — so pick a value that works on both surfaces, and verify the query path returns what you expect before relying on the filter. `_neq`, `_gt` and `_lt` withhold **every** row: a text difference can be pure representation (an uppercase UUID, an alternate date format, an Enum name vs. its number), so inequality and order are unprovable without ClickHouse — on these columns, use the query path for ordering/exclusion filters. |
Member
There was a problem hiding this comment.
"Write the filter value the way your events carry it" reads as "the way your clients send it," which is no longer true for timestamps: since #402, ingest rewrites every parseable top-level DateTime/DateTime64 value to canonical RFC 3339 UTC before publishing (internal/discovery/timestamp.go:36). WaveHouse decides the spelling, not the producer.
This is the page policy authors actually write filters on, and it never mentions canonicalization — the only statement of the rule is in api.md, in future tense (separate comment).
The bullet also groups Date/DateTime, but timestamp.go:25-27 excludes Date/Date32 from canonicalization, so the two need opposite advice: Date keeps the producer's spelling, DateTime doesn't.
Member
Author
There was a problem hiding this comment.
Timestamps moved out of that bullet entirely since they compare as instants now, and the byte-equality bullet calls out that Date keeps the producer's spelling while DateTime doesn't.
Member
There was a problem hiding this comment.
api.md:282 still describes this feature as unshipped:
…and the form the stream row-filter will require for timestamp comparisons once row-level enforcement lands ([#381])
This PR is #381 — internal/stream/hub.go:171-176 evaluates RowVisible per subscriber today. #402 anticipated this interaction and left the line as a placeholder for whoever landed row-level enforcement, so it's (y)ours to close out.
Present tense, and it should say what actually happens: _eq/_in match the canonical form, _neq/_gt/_lt withhold every row on a timestamp column. Worth repointing the link at the access-control enforcement caution rather than the issue.
(File-level because line 282 isn't in this diff.)
Member
Author
There was a problem hiding this comment.
Will update on next push.
|
|
||
| ### Security | ||
|
|
||
| - **Live SSE streams now apply a role's row-`filter` per subscriber, closing a query/stream row-level-security drift** (`internal/stream/hub.go`, `internal/stream/subscriber.go`, `internal/stream/metrics.go`, `internal/policy/policy.go`, `internal/policy/rowfilter.go` (new), `internal/discovery/validation.go`, `internal/api/stream.go`, `cmd/wavehouse/main.go`, `docs/src/content/docs/access-control.mdx`, `docs/src/content/docs/architecture.md`, `docs/src/content/docs/api.md`, `AGENTS.md`, `SECURITY.md`, `internal/stream/doc.go`, plus tests in `internal/policy/rowfilter_test.go` (new), `internal/discovery/validation_test.go`, `internal/stream/hub_test.go`, `tests/e2e/sdk/streaming.test.ts`): closes #319. The SSE delivery path stripped denied columns but never applied a role's row-level `filter` predicate, so a subscriber received rows the structured-query path would have filtered out for that same role — a data-exposure on the streaming surface for any table that combines a row-policy with a shared or role-scoped stream (harmless on the public Stats table today, which carries no restrictive row-policy, but real for any private/PII table fronted by a stream). The row-filter is now resolved once into predicates that feed **both** read surfaces — the query path renders them to SQL, the stream evaluates them in memory (`ResolvedPermissions.RowVisible`, evaluated per subscriber against that subscriber's claims via the same `Evaluate` call the query path uses) — so the two can't drift (the row-level analogue of the shared `IsColumnAllowed` decision from #223). Because a row-filter resolves against each subscriber's JWT claims, the #294/#353 once-per-role projection is now claims-aware: a role **without** a filter keeps the pure once-per-role fast path unchanged (zero regression on the public stream), while a role **with** a filter keeps the shared column projection but delivers it only to the subscribers whose claims admit each row (evaluated against the full event, so a filter may key on a column the role can't select). The in-memory comparison is type-aware and **fails closed on anything it can't prove** (`policy.ColumnKind`, seeded from the schema registry): numeric columns (`Int*`/`UInt*`/`Float*`/`Decimal*`) compare numerically, matching ClickHouse — float64-equal ties resolve at arbitrary precision (`math/big`) and the hub decodes event payloads with `UseNumber` (exact digit strings, also keeping big integers byte-faithful on the SSE wire), so 64-bit IDs never falsely collide whether they arrive string-encoded (the JS-precision-loss escape hatch ingest accepts) or as bare JSON numbers, and an unparseable/`NaN` operand withholds the row — `String` columns compare bytewise (exactly ClickHouse's String semantics, equality *and* ordering), and every other type (`Enum`, `UUID`, `Date`/`DateTime`, `Bool`, IPs, `FixedString`, …) — plus every column when no schema is available (an unknown table, or the boot-time discovery-failure window where the server serves while retrying) — trusts byte-equality only: `_eq`/`_in` admit exactly the event's own text rendering, while `_neq`/`_gt`/`_lt` withhold the row, because a byte difference can be pure representation and admitting on it would deliver rows the query path excludes (an uppercase UUID under `_neq`; `"9" > "100"` as text under `_gt`). Ambiguity therefore always costs availability (a row withheld), never confidentiality — a guarantee about the ingested payload the stream evaluates, whose one payload-vs-stored asymmetry (insert-time numeric narrowing: `Decimal` scale, `Float32` width) is documented in the enforcement caution — and each withhold is observable via the new `wavehouse_sse_rows_withheld_total` counter (labeled by table and role, on the live and replay paths alike), separating "no matching rows" from "a fail-closed filter is withholding everything". Subscriber claims are fixed at construction (`stream.NewSubscriber(claims)` — no setter), so the fan-out's unsynchronized claims read is race-free structurally, with a dedicated `-race` test driving concurrent row-filtered broadcasts. Replay (gap-fill) applies the same per-connection row check via `Hub.ReplayProjector`, which caches the per-table column-kind lookup across the replay loop. **Perf note:** on a topic whose role carries a row-filter, part of the #294/#353 once-per-role fan-out gain is traded back for correctness — `policy.Evaluate` runs per subscriber per event there (the column projection stays shared; roles without a filter are unaffected); #435 tracks memoizing it, and `BenchmarkBroadcast_RowFilteredFanout` exists to measure it. The resource limits (`max_rows`, `max_execution_time`, …) remain a query-path property and are still **not** applied to the stream (a separate, documented boundary). Supersedes the "stream path applies no row-level filter" invariant noted in the #294/#353 Changed entry below. |
Member
There was a problem hiding this comment.
Two things:
Direction. This entry closes with "the #294/#353 Changed entry below". That entry is at line 28, above — ### Changed precedes ### Security.
The superseded entry still reads as current. Line 28 says "the stream path applies no row-level filter (a documented invariant — if row-level filtering is ever added to streaming, the key must take claims into account)". Both halves are now wrong, and they ship in the same Unreleased block as this entry. The stream does apply a row filter; and the prescription is the design this PR deliberately rejected — hub.go:51 still keys buckets on (topic, role), with claims on the Subscriber gating delivery per subscriber. Someone reading only the Changed entry walks away with a false invariant and a stale instruction.
Worth a forward pointer on line 28 rather than leaving it to be inferred.
|
|
||
| - **A filtered column the payload doesn't carry withholds *every* event** for that subscriber, even though the same filter matches normally on the query path. That bites a `MATERIALIZED`/`ALIAS` column (never part of an ingest payload) or a `DEFAULT` column your clients omit. The recommended [`check` + `filter` pairing](#insert-checks) is unaffected: an `_eq` insert `check` auto-injects its claim value into any payload that omits the column *before* the event is published, so the streamed event carries it and the matching row filter evaluates normally. | ||
| - **A non-scalar event value** (array/object/null) under a filtered column withholds the row. | ||
| - **Insert-time numeric narrowing bounds the fail-closed guarantee itself** — this one edge can fail *open*. The guarantee holds for the *payload* value, and the one place that differs from the stored row is a payload with more precision than the column's declared type (a `Decimal`'s scale, `Float32` width), which is rounded on insert — so a numeric `_neq`/`_gt`/`_lt` filter on such a column compares the pre-rounding payload, and can deliver an event whose stored row lands on the other side of the comparison: a `_gt: "1.004"` on a `Decimal(10, 2)` column delivers a payload of `1.005` on the stream, while the query path compares the stored `1.00` and hides the row. `_eq`/`_in` cannot fail open here: ClickHouse narrows the bound filter constant to the column's declared type just as insert narrowed the payload, so an equality that matches on the stream matches the stored row too. Columns that store exactly (integer IDs, `String` tenants) are unaffected under every operator. |
Member
There was a problem hiding this comment.
ClickHouse truncates decimals, it doesn't round them — verified on 25.10, where 1.005, 1.006 and 1.009 all store as 1.00 in a Decimal(10,2).
That truncation is what makes this bullet's own example work. A reader who takes "rounded" literally predicts 1.01 for 1.005 and concludes the example is broken.
Float32 genuinely does round to the nearest representable value, so the two cases differ in direction as well as name and are worth stating separately.
Member
Author
There was a problem hiding this comment.
Right, verified the same on 25.10. The two cases are stated separately now: Decimal truncates, Float32 rounds.
| Row values of top-level `DateTime`/`DateTime64` columns inside `data` arrive in the canonical RFC 3339 UTC form (ingest rewrites them before publishing — see [timestamp canonicalization](#timestamp-canonicalization)), so a live event and a `/v1/query` read of the same row agree on the instant in zone-explicit form — a zone-less spelling no longer parses as local time in a browser ([#372](https://github.com/Wave-RF/WaveHouse/issues/372)). The two renderings are byte-identical regardless of the declared time zone or a `Nullable` wrapper — a column declared with a non-UTC zone also streams as `Z`, and `/v1/query` normalizes it (nullable or not) to UTC before rendering. Canonicalization is fail-open at ingest, so a value outside the accepted input forms streams in whatever spelling the producer sent — and for exactly those events the byte-identity above does not hold: a spelling ClickHouse accepts anyway is stored and still queries back canonical, while one it too rejects lands in the DLQ and never becomes queryable at all. Events ingested before this behavior shipped likewise replay in their original spelling. | ||
|
|
||
| **Note:** When access control policies are active, streamed events are filtered per the caller's role — denied columns are removed and tables without select permission are skipped. | ||
| **Note:** When access control policies are active, streamed events are filtered per the caller's role — tables without select permission are skipped, denied columns are removed from each event, and the role's row-level `filter` is evaluated per subscriber against the caller's JWT claims — so, for a filter constant written to be valid on both read surfaces (see the enforcement caution), a connection is never delivered a row the query path would hide for that role, with one documented exception (a numeric `_neq`/`_gt`/`_lt` filter over a column type that rounds the payload on insert). The claims come from the connection's token (the `Authorization` header, or the `?token=` fallback above), and replayed gap-fill events are filtered the same way. See [Access control — row-level security](/access-control#row-level-security) and the enforcement caution there for the stream's fail-closed comparison boundary. |
Member
There was a problem hiding this comment.
This note opens with a 93-word sentence carrying three enforcement rules, a conditional ("for a filter constant written to be valid on both read surfaces"), and a parenthetical exception. It's the primary user-facing statement of a security guarantee and shouldn't need a second read.
It also says "see the enforcement caution" without a link, then links that same caution two sentences later.
Three sentences would do it: the enumeration; then the guarantee and what it depends on; then the exception, with the link attached at first mention.
Note the conditional will need revisiting either way — see my comment on access-control.mdx:387 about there being no filter constant valid on both surfaces for timestamp columns.
Member
Author
There was a problem hiding this comment.
Rewrote it as three sentences with the link at first mention. The conditional got revisited as part of the timestamp fix since a zone-less constant now works on both surfaces.
github-project-automation
Bot
moved this from Ready
to In review
in WaveHouse Task Board
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3cb72044-b5b8-4ed3-9fec-b3c3e9cd1cc6
📒 Files selected for processing (16)
AGENTS.mdCHANGELOG.mddocs/src/content/docs/access-control.mdxdocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/sdk/streaming.mdinternal/auth/auth.gointernal/auth/auth_test.gointernal/discovery/timestamp.gointernal/discovery/timestamp_test.gointernal/policy/policy.gointernal/policy/policy_test.gointernal/policy/rowfilter.gointernal/policy/rowfilter_test.gointernal/stream/hub.gointernal/stream/hub_test.go
💤 Files with no reviewable changes (1)
- docs/src/content/docs/sdk/streaming.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Docs build
- GitHub Check: Coverage
- GitHub Check: E2E tests
🧰 Additional context used
📓 Path-based instructions (6)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Go 1.26, strict formatting (gofumpt, enforced by CI)
- Structured logging with
log/slog(JSON handler)- Chi v5 for HTTP routing
- Error handling: Return errors, don't panic. Wrap with
fmt.Errorf("context: %w", err).- No global state: Dependencies are passed explicitly (constructor injection).
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
- Create
*_test.gofiles in the same package as the code under test.
Files:
internal/auth/auth_test.gointernal/auth/auth.gointernal/discovery/timestamp.gointernal/discovery/timestamp_test.gointernal/policy/rowfilter.gointernal/stream/hub.gointernal/policy/policy_test.gointernal/policy/policy.gointernal/policy/rowfilter_test.gointernal/stream/hub_test.go
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- Package naming: Lowercase, single word (or abbreviated).
internal/enforces module privacy.
Files:
internal/auth/auth_test.gointernal/auth/auth.gointernal/discovery/timestamp.gointernal/discovery/timestamp_test.gointernal/policy/rowfilter.gointernal/stream/hub.gointernal/policy/policy_test.gointernal/policy/policy.gointernal/policy/rowfilter_test.gointernal/stream/hub_test.go
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: 1. Validate locally before every push — runmake cithe documented way (§Runningmake ci). Don't use CI as your first feedback loop.
3. Every code change updates its docs +CHANGELOG.mdin the same PR — a code change without its doc update is incomplete (§Documentation Sync).
Files:
internal/auth/auth_test.gointernal/auth/auth.gointernal/discovery/timestamp.godocs/src/content/docs/architecture.mdAGENTS.mdinternal/discovery/timestamp_test.goCHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/access-control.mdxinternal/policy/rowfilter.gointernal/stream/hub.gointernal/policy/policy_test.gointernal/policy/policy.gointernal/policy/rowfilter_test.gointernal/stream/hub_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
- Table-driven tests: Use
tests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
Files:
internal/auth/auth_test.gointernal/discovery/timestamp_test.gointernal/policy/policy_test.gointernal/policy/rowfilter_test.gointernal/stream/hub_test.go
internal/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/**/*_test.go: - Shared mocks ininternal/testutil/: UseMockPublisher,MockCache,MockDeduplicator,MockSubscriberinstead of creating ad-hoc mocks. Seetestutil/mocks.go.
- JWT helpers: Use
testutil.MakeJWT(t, claims)andtestutil.MakeExpiredJWT(t, claims)for auth tests. Seetestutil/jwt.go.
Files:
internal/auth/auth_test.gointernal/discovery/timestamp_test.gointernal/policy/policy_test.gointernal/policy/rowfilter_test.gointernal/stream/hub_test.go
**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
**/*.mdx: - Default to top-down:flowchart TB/TD, anddirection TBinside subgraphs — notLR/RL.
- Never sit two large diagrams side-by-side. Wrap comparisons in
<div class="diagram-pair">…</div>, which stacks them vertically so each gets the full column width.
Files:
docs/src/content/docs/access-control.mdx
🧠 Learnings (45)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-07-07T16:19:29.374Z
Learning: Applies to internal/stream/**/*.{go} : Streaming/SSE code must preserve the hub’s per-role projection model, subscriber queues, bucket fan-out, heartbeating, and metrics semantics.
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/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:
internal/auth/auth_test.gointernal/auth/auth.godocs/src/content/docs/architecture.mdAGENTS.mdCHANGELOG.mdinternal/policy/rowfilter.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/auth/auth_test.godocs/src/content/docs/architecture.mdinternal/policy/policy_test.gointernal/policy/rowfilter_test.gointernal/stream/hub_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/auth/auth_test.godocs/src/content/docs/architecture.mdCHANGELOG.mdinternal/stream/hub_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/policy/rowfilter_test.gointernal/stream/hub_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_test.gointernal/policy/rowfilter_test.gointernal/stream/hub_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.godocs/src/content/docs/architecture.mdCHANGELOG.md
📚 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/auth/auth_test.gointernal/policy/rowfilter_test.gointernal/stream/hub_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/rowfilter_test.gointernal/stream/hub_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/discovery/timestamp_test.gointernal/policy/policy_test.gointernal/policy/rowfilter_test.gointernal/stream/hub_test.go
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/auth/auth_test.gointernal/auth/auth.gointernal/discovery/timestamp.gointernal/discovery/timestamp_test.gointernal/policy/rowfilter.gointernal/stream/hub.gointernal/policy/policy_test.gointernal/policy/policy.gointernal/policy/rowfilter_test.gointernal/stream/hub_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_test.gointernal/auth/auth.gointernal/discovery/timestamp.gointernal/discovery/timestamp_test.gointernal/policy/rowfilter.gointernal/stream/hub.gointernal/policy/policy_test.gointernal/policy/policy.gointernal/policy/rowfilter_test.gointernal/stream/hub_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/stream/**/*.{go} : Streaming/SSE code must preserve the hub’s per-role projection model, subscriber queues, bucket fan-out, heartbeating, and metrics semantics.
Applied to files:
docs/src/content/docs/architecture.mdAGENTS.mdCHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/access-control.mdxinternal/stream/hub.gointernal/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:
docs/src/content/docs/architecture.mdAGENTS.mdCHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/access-control.mdxinternal/policy/rowfilter.gointernal/stream/hub.gointernal/policy/policy.gointernal/policy/rowfilter_test.gointernal/stream/hub_test.go
📚 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:
docs/src/content/docs/architecture.mdCHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/stream/hub.gointernal/stream/hub_test.go
📚 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:
docs/src/content/docs/architecture.mdAGENTS.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:
docs/src/content/docs/architecture.mdAGENTS.mdCHANGELOG.mdinternal/policy/rowfilter.gointernal/policy/policy.gointernal/policy/rowfilter_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/query/**/*.{go} : Structured-query code must enforce schema validation, permission checks, timestamp bucketing, and fail-closed column authorization inside `query.Build`.
Applied to files:
docs/src/content/docs/architecture.mdAGENTS.mdCHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/access-control.mdxinternal/policy/rowfilter.gointernal/policy/policy.gointernal/policy/rowfilter_test.go
📚 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:
docs/src/content/docs/architecture.mddocs/src/content/docs/access-control.mdxinternal/stream/hub.gointernal/stream/hub_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:
docs/src/content/docs/architecture.mddocs/src/content/docs/api.mddocs/src/content/docs/access-control.mdxinternal/policy/rowfilter.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/ingest/types.go : `EventMessage` JSON tags and ingest event shape must stay aligned with docs, SSE examples, and ClickHouse INSERT column order.
Applied to files:
docs/src/content/docs/architecture.mddocs/src/content/docs/api.mddocs/src/content/docs/access-control.mdx
📚 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:
docs/src/content/docs/architecture.mdinternal/stream/hub.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:
docs/src/content/docs/architecture.mdAGENTS.mdCHANGELOG.md
📚 Learning: 2026-06-29T14:21:45.067Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 343
File: internal/api/pipe_deps.go:0-0
Timestamp: 2026-06-29T14:21:45.067Z
Learning: In `internal/api/pipes.go`, direct table-function reads and direct cross-database table reads are intentionally omitted from the pipe dependency set and continue using the normal query-derived TTL; only resolved-but-unmaintainable dependencies (such as unknown or unfoldable view-derived names) trigger the unresolved-dependency TTL cap.
Applied to files:
docs/src/content/docs/architecture.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:
docs/src/content/docs/architecture.mdCHANGELOG.mddocs/src/content/docs/access-control.mdxinternal/policy/rowfilter.gointernal/policy/policy_test.gointernal/policy/policy.gointernal/policy/rowfilter_test.gointernal/stream/hub_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.mddocs/src/content/docs/api.md
📚 Learning: 2026-05-20T03:17:53.774Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-05-20T03:17:53.774Z
Learning: Refer to AGENTS.md for project conventions, architecture notes, and AI agent instructions - it is the single source of truth
Applied to files:
AGENTS.mdinternal/stream/hub.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/observability/**/*.{go} : Observability code must preserve the repo’s logging and telemetry invariants, including stdout fan-out, OTLP behavior, lazy dialing, and the private Prometheus registry.
Applied to files:
AGENTS.mdinternal/stream/hub.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:
AGENTS.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-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/discovery/timestamp_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/discovery/timestamp_test.godocs/src/content/docs/access-control.mdxinternal/policy/rowfilter_test.gointernal/stream/hub_test.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.mdinternal/stream/hub.go
📚 Learning: 2026-08-12T05:35:22.804Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 455
File: clients/ts/README.md:11-11
Timestamp: 2026-08-12T05:35:22.804Z
Learning: For Wave-RF/WaveHouse, CI currently tests Node 22 only through `.nvmrc` and `.github/actions/setup-env` using `node-version-file`. Local `make ci` verification on Node 26 is not CI coverage and must not be documented as continuously tested runtime support.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-11T12:41:08.161Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 446
File: .github/actions/setup-env/action.yml:112-127
Timestamp: 2026-08-11T12:41:08.161Z
Learning: In WaveHouse CI, the shared `gomod-v1` cache in `.github/actions/setup-env/action.yml` can be saved by multiple Go jobs on an exact-key miss. Every workflow path that can write this cache must fully populate `~/go/pkg/mod` through the Makefile `go-mod-download` prerequisite before the post-job cache save. The `cov` target must retain this prerequisite.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-06-10T15:02:09.425Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: .github/workflows/ci.yml:232-237
Timestamp: 2026-06-10T15:02:09.425Z
Learning: In the Wave-RF/WaveHouse repository, `clickhouse/clickhouse-server:latest` is used deliberately in `tests/integration/setup_test.go`, `scripts/orchestrator/main.go`, and the CI workflow prefetch steps (`docker pull -q clickhouse/clickhouse-server:latest`). The `:latest` tag in the prefetch steps intentionally mirrors the tag testcontainers resolves at runtime — this is a deliberate canary approach. Pinning to a concrete version/digest is a separate decision tracked as a follow-up issue and should not be flagged as a supply-chain concern in CI workflow reviews for this repo.
Applied to files:
CHANGELOG.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:
CHANGELOG.md
📚 Learning: 2026-08-11T21:56:03.206Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/query_builder.go:278-291
Timestamp: 2026-08-11T21:56:03.206Z
Learning: In `clients/go/query_builder.go`, `fetchNextTyped` intentionally treats a failed JSON decode of a non-object typed `Row` as normal end-of-pagination. This behavior matches the existing “cursor column was not in the projection” path and TypeScript SDK parity. The broader behavior change is tracked in GitHub issue `#452`.
Applied to files:
docs/src/content/docs/api.mddocs/src/content/docs/access-control.mdxinternal/policy/rowfilter.go
📚 Learning: 2026-08-11T21:56:06.521Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: docs/src/content/docs/sdk/go/queries.md:356-362
Timestamp: 2026-08-11T21:56:06.521Z
Learning: In Wave-RF/WaveHouse Go SDK cursor pagination, `fetchNextTyped` uses only the first `QueryBuilder.OrderBy` column and a strict `gt` or `lt` filter. Duplicate values at a page boundary can skip rows. The Go SDK documentation must require a unique ordering column until the shared Go and TypeScript composite-cursor or tie-breaker implementation tracked in GitHub issue `#452` is available.
Applied to files:
docs/src/content/docs/api.mddocs/src/content/docs/access-control.mdxinternal/policy/rowfilter.go
📚 Learning: 2026-05-13T15:14:46.366Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-13T15:14:46.366Z
Learning: Refer to AGENTS.md for project conventions, architecture notes, and AI agent instructions
Applied to files:
internal/stream/hub.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/rowfilter_test.gointernal/stream/hub_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/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/stream/hub_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 **/*.{go} : Go code must use Go 1.26 conventions: gofumpt formatting, structured logging with `log/slog`, Chi v5 routing, explicit error returns, no global state, and package naming that is lowercase and single-word or abbreviated.
Applied to files:
internal/stream/hub_test.go
📚 Learning: 2026-08-11T21:55:42.427Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/cmd/wavehouse-codegen/main.go:96-97
Timestamp: 2026-08-11T21:55:42.427Z
Learning: In `clients/go/cmd/wavehouse-codegen/main.go`, `fetchSchemas` is a self-contained CLI helper with one caller, `main`. The `AGENTS.md` convention to pass dependencies explicitly applies to package constructors, not to this type of CLI helper. Do not request HTTP-client injection unless HTTP-level test coverage or additional callers make that refactor necessary.
Applied to files:
internal/stream/hub_test.go
🪛 LanguageTool
docs/src/content/docs/access-control.mdx
[style] ~390-~390: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...self** — this one edge can fail open. The guarantee holds for the payload value, and the one place that differs from the stored row is a payload with more precision than the column's declared type, which the insert narrows — a Decimal truncates at its scale (1.005, 1.006 and 1.009 all store as 1.00 in a Decimal(10, 2)), while a Float32 rounds to its nearest representable value (16777217 stores as 16777216) — so a numeric _neq/_gt/_lt filter on such a column compares the pre-narrowing payload, and can deliver an event whose stored row lands on the other side of the comparison: a _gt: "1.004" on a Decimal(10, 2) column delivers a payload of 1.005 on the stream, while the query path compares the stored (truncated) 1.00 and hides the row. _eq/_in cannot fail open here: Clic...
(TOO_LONG_SENTENCE)
🔇 Additional comments (17)
internal/policy/policy.go (1)
4-8: LGTM!Also applies to: 200-211, 219-223, 251-282, 308-313, 341-365, 367-395, 403-435
internal/auth/auth_test.go (1)
8-8: LGTM!Also applies to: 112-129
internal/discovery/timestamp.go (1)
54-81: LGTM!internal/discovery/timestamp_test.go (1)
105-144: LGTM!Also applies to: 146-171
internal/policy/policy_test.go (1)
4-4: LGTM!Also applies to: 394-447, 678-704
internal/policy/rowfilter.go (1)
9-9: LGTM!Also applies to: 31-50, 52-68, 70-111, 115-149, 158-226
internal/policy/rowfilter_test.go (1)
6-6: LGTM!Also applies to: 54-72, 82-104, 109-115, 121-128, 138-156, 158-181, 183-222, 248-248
internal/stream/hub.go (1)
30-30: LGTM!Also applies to: 138-142, 166-179, 183-215, 263-289
internal/stream/hub_test.go (2)
7-14: LGTM!Also applies to: 26-46, 627-632
653-676: 🎯 Functional CorrectnessRemove the timezone warning
NewTestSchemaRegistryreturns"UTC"for the timezone probe, soRefreshresolves the bareDateTimecolumn to UTC and the filter constant parses correctly.> Likely an incorrect or invalid review comment.internal/auth/auth.go (1)
206-213: 🗄️ Data Integrity & IntegrationNo changes needed.
AGENTS.md (2)
61-61: LGTM!
44-44: 📐 Maintainability & Code QualityNo change needed to
AGENTS.md.The
stream/architecture sentence accurately describes immutable subscriber claims and per-subscriber row-filter evaluation.CHANGELOG.md (1)
28-28: LGTM!Also applies to: 40-40
docs/src/content/docs/access-control.mdx (1)
181-181: LGTM!Also applies to: 233-233, 373-375, 538-538
docs/src/content/docs/api.md (1)
580-580: LGTM!docs/src/content/docs/architecture.md (1)
88-92: LGTM!Also applies to: 117-118, 143-144, 266-275
|
|
||
| - **Numeric columns** (`Int*`/`UInt*`/`Float*`/`Decimal*`, unwrapping `Nullable`/`LowCardinality` in any nesting): all five operators compare numerically, matching ClickHouse (`9 < 100`). Equality ties beyond `float64`'s 2^53 resolve at full precision (a 64-bit ID never falsely matches a neighbor, whether it arrives string-encoded or as a bare JSON number); an unparseable or `NaN` operand withholds the row. | ||
| - **`String` columns** (again under any `Nullable`/`LowCardinality` wrapping): byte comparison *is* ClickHouse's String comparison — equality and ordering are both exact. | ||
| - **`DateTime`/`DateTime64` columns** (again under any wrapping): both operands are parsed as **instants** — through the same grammar [ingest canonicalization](/api#timestamp-canonicalization) reads — and compared chronologically, so all five operators are exact and the constant's spelling doesn't need to match the event's: ingest rewrites payload values to RFC 3339 UTC before publishing, and a zone-less constant (`2026-06-21 04:00:00`, read in the column's declared zone, else the discovered server default — ClickHouse's own rule) still matches the rewritten payload denoting that instant. **Write timestamp constants zone-less like that**: it's the one spelling that also works verbatim in the query path's SQL on every ClickHouse release (releases before 26.x reject the RFC 3339 `Z` form in a `WHERE` comparison with a type error; 26.x accepts it, and Unix-seconds strings, there too — the stream accepts every grammar spelling regardless). An operand the grammar can't read — on either side — withholds the row, as does an instant outside the column type's range (which insert-time *saturation* would have moved anyway). A column whose **declared** zone can't be loaded at runtime has no timestamp parser at all — it falls into the byte-equality bucket below (its values aren't canonicalized at ingest either), so `_neq`/`_gt`/`_lt` withhold every row. A column relying on the **server default** zone when that couldn't be resolved keeps instant comparison for zone-explicit operands (RFC 3339, Unix seconds) but refuses zone-less ones rather than guess the zone. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n 'before 26\.x|26\.x accepts|pre-26\.5|since 26\.5|date_time_input_format|RFC ?3339|Unix-seconds' \
AGENTS.md CHANGELOG.md docs/src/content/docs/access-control.mdx docs/src/content/docs/api.mdRepository: Wave-RF/WaveHouse
Length of output: 27590
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- relevant documentation ---'
sed -n '270,290p' docs/src/content/docs/api.md
sed -n '378,385p' docs/src/content/docs/access-control.mdx
printf '%s\n' '--- timestamp implementation and tests ---'
rg -n -C 3 'date_time_input_format|best_effort|basic|26\.5|26\.x|RFC3339|canonical.*timestamp|TimeParser' \
--glob '!docs/**' --glob '!CHANGELOG.md' .Repository: Wave-RF/WaveHouse
Length of output: 48311
🌐 Web query:
ClickHouse 26.5 date_time_input_format best_effort default release notes basic RFC3339 Z DateTime WHERE comparison
💡 Result:
In ClickHouse 26.5 and later, the default value for the date_time_input_format setting was changed from basic to best_effort [1][2][3]. This change was implemented to provide broader support for various date and time string formats (such as RFC 3339) when inserting data or performing comparisons in queries [1][3]. Key details regarding this change: 1. Default Behavior: With date_time_input_format set to best_effort, ClickHouse can parse not only the basic YYYY-MM-DD HH:MM:SS format but also a wide range of ISO 8601 and RFC 3339 date-time strings, including those with timezone offsets or 'Z' (UTC) suffixes (e.g., '2018-06-08T01:02:03.000Z') [4]. 2. Impact on WHERE Comparisons: When filtering DateTime columns in a WHERE clause using a string, the string is automatically converted to the target DateTime type [5][6]. Because the default is now best_effort, these strings are parsed using the more flexible engine, allowing for non-canonical formats that would have previously failed under the basic setting [1][3]. 3. Performance and Compatibility: The transition to best_effort as the default led to a performance regression for canonical date-time strings, which was subsequently addressed by adding a fast-path in the best-effort parser to handle standard YYYY-MM-DD HH:MM:SS formats as efficiently as the original basic parser [2][7]. If you require the original, stricter parsing behavior for specific use cases, you can set date_time_input_format back to basic at the session or query level [1][3]. 4. Related Settings: The cast_string_to_date_time_mode setting also had its default changed to best_effort to align with this behavior, ensuring consistency between explicit CAST operations and implicit parsing [1][3][8].
Citations:
- 1: Enable date time input format "best effort" by default ClickHouse/ClickHouse#89334
- 2: https://clickhouse.com/docs/resources/changelogs/oss/2026
- 3: https://github.com/ClickHouse/clickhouse-docs/blob/main/docs/whats-new/changelog/index.md
- 4: https://clickhouse.com/docs/reference/settings/formats/date-time
- 5: https://clickhouse.com/docs/reference/data-types/datetime
- 6: https://github.com/ClickHouse/ClickHouse/blob/master/docs/en/sql-reference/data-types/datetime.md
- 7: Add canonical-format fast path to parseDateTimeBestEffort ClickHouse/ClickHouse#108187
- 8: https://clickhouse.com/blog/parsing-dates-datetimes
Use the ClickHouse 26.5 boundary.
Replace before 26.x/26.x with before 26.5/since 26.5, and keep the wording consistent with docs/src/content/docs/api.md and the pinned date_time_input_format=best_effort behavior.
|
|
||
| - **A filtered column the payload doesn't carry withholds *every* event** for that subscriber, even though the same filter matches normally on the query path. That bites a `MATERIALIZED`/`ALIAS` column (never part of an ingest payload) or a `DEFAULT` column your clients omit. The recommended [`check` + `filter` pairing](#insert-checks) is unaffected: an `_eq` insert `check` auto-injects its claim value into any payload that omits the column *before* the event is published, so the streamed event carries it and the matching row filter evaluates normally. (That holds for timestamp columns too: the injected claim value is canonicalized with the rest of the payload before publish, and the stream compares timestamps as instants, so the claim's spelling and the canonical wire spelling meet.) | ||
| - **A non-scalar event value** (array/object/null) under a filtered column withholds the row. | ||
| - **Insert-time numeric narrowing bounds the fail-closed guarantee itself** — this one edge can fail *open*. The guarantee holds for the *payload* value, and the one place that differs from the stored row is a payload with more precision than the column's declared type, which the insert narrows — a `Decimal` **truncates** at its scale (`1.005`, `1.006` and `1.009` all store as `1.00` in a `Decimal(10, 2)`), while a `Float32` **rounds** to its nearest representable value (`16777217` stores as `16777216`) — so a numeric `_neq`/`_gt`/`_lt` filter on such a column compares the pre-narrowing payload, and can deliver an event whose stored row lands on the other side of the comparison: a `_gt: "1.004"` on a `Decimal(10, 2)` column delivers a payload of `1.005` on the stream, while the query path compares the stored (truncated) `1.00` and hides the row. `_eq`/`_in` cannot fail open here: ClickHouse narrows the bound filter constant to the column's declared type just as insert narrowed the payload, so an equality that matches on the stream matches the stored row too. Columns that store exactly (integer IDs, `String` tenants) are unaffected under every operator. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Split the numeric narrowing caveat into shorter sentences.
Line 390 combines the stored-row exception, Decimal truncation, Float32 rounding, and the _gt example in one sentence. Split the paragraph into separate statements so the fail-open boundary is easier to review and less likely to be misread.
🧰 Tools
🪛 LanguageTool
[style] ~390-~390: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...self** — this one edge can fail open. The guarantee holds for the payload value, and the one place that differs from the stored row is a payload with more precision than the column's declared type, which the insert narrows — a Decimal truncates at its scale (1.005, 1.006 and 1.009 all store as 1.00 in a Decimal(10, 2)), while a Float32 rounds to its nearest representable value (16777217 stores as 16777216) — so a numeric _neq/_gt/_lt filter on such a column compares the pre-narrowing payload, and can deliver an event whose stored row lands on the other side of the comparison: a _gt: "1.004" on a Decimal(10, 2) column delivers a payload of 1.005 on the stream, while the query path compares the stored (truncated) 1.00 and hides the row. _eq/_in cannot fail open here: Clic...
(TOO_LONG_SENTENCE)
Source: Linters/SAST tools
| ::: | ||
|
|
||
| **The canonical form, precisely.** This is the one strict timestamp spelling in WaveHouse — the same one `/v1/query` and `/v1/pipes/{name}` render for top-level timestamp columns and the SSE stream carries (the raw-SQL proxy `/v1/admin/query` instead renders server-side via `date_time_output_format=iso`, which keeps trailing fraction zeros), and the form the stream row-filter will require for timestamp comparisons once row-level enforcement lands ([#381](https://github.com/Wave-RF/WaveHouse/issues/381)): | ||
| **The canonical form, precisely.** This is the one strict timestamp spelling in WaveHouse — the same one `/v1/query` and `/v1/pipes/{name}` render for top-level timestamp columns and the SSE stream carries (the raw-SQL proxy `/v1/admin/query` instead renders server-side via `date_time_output_format=iso`, which keeps trailing fraction zeros). The stream row-filter doesn't require it: row-level enforcement compares timestamp operands as **instants** under this same input grammar, so a filter constant in any accepted spelling — zone-less, RFC 3339, Unix seconds — matches the canonical payload denoting the same instant, and an operand the grammar can't read withholds the row (see [the enforcement caution](/access-control#where-each-rule-is-enforced) for per-type comparison rules and the spelling that also works in query-path SQL): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(rowfilter\.go|access-control\.mdx|api\.md|CHANGELOG\.md)$'
printf '%s\n' '--- row-filter implementation symbols ---'
rg -n -C 8 'ParseTime|schema|byte equality|timestamp|instant|Compare|row filter|withhold' internal docs/src/content/docs
printf '%s\n' '--- target documentation sections ---'
sed -n '260,295p' docs/src/content/docs/api.md
sed -n '365,400p' docs/src/content/docs/access-control.mdx
sed -n '560,595p' docs/src/content/docs/api.mdRepository: Wave-RF/WaveHouse
Length of output: 50374
🏁 Script executed:
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(rowfilter\.go|access-control\.mdx|api\.md|CHANGELOG\.md)$'
printf '%s\n' '--- relevant implementation and documentation references ---'
rg -n -C 8 'ParseTime|schema|byte equality|timestamp|instant|Compare|row.?filter|withhold' internal docs/src/content/docs
printf '%s\n' '--- target sections ---'
sed -n '260,295p' docs/src/content/docs/api.md
sed -n '365,400p' docs/src/content/docs/access-control.mdx
sed -n '560,595p' docs/src/content/docs/api.mdRepository: Wave-RF/WaveHouse
Length of output: 50376
🏁 Script executed:
set -eu
rowfilter="$(git ls-files | rg '(^|/)rowfilter\.go$' | head -n 1)"
printf '%s\n' "--- row-filter file: $rowfilter ---"
wc -l "$rowfilter"
cat -n "$rowfilter" | sed -n '1,240p'
printf '%s\n' '--- timestamp-spec and canonicalization implementation ---'
rg -n -C 12 'type timestampSpec|ParseTime|CanonicalizeTimestamps|resolveTimestampSpecs|parse.*time|byte equality|bytes.Equal' internal/discovery internal/policy internal/stream
printf '%s\n' '--- cited API and access-control passages ---'
cat -n docs/src/content/docs/api.md | sed -n '276,288p'
cat -n docs/src/content/docs/access-control.mdx | sed -n '376,392p'
cat -n docs/src/content/docs/api.md | sed -n '574,586p'Repository: Wave-RF/WaveHouse
Length of output: 50373
🏁 Script executed:
set -eu
printf '%s\n' '--- API timestamp statement ---'
cat -n docs/src/content/docs/api.md | sed -n '280,284p'
printf '%s\n' '--- access-control enforcement caution ---'
cat -n docs/src/content/docs/access-control.mdx | sed -n '378,390p'
printf '%s\n' '--- stream schema-spec construction and projector call sites ---'
cat -n internal/stream/hub.go | sed -n '150,220p'
rg -n -C 8 'columnSpecs\(|RowVisible\(|ReplayProjector|Projector' internal/stream internal/api
printf '%s\n' '--- schema refresh failure and cache semantics ---'
cat -n internal/discovery/discovery.go | sed -n '69,148p'
rg -n -C 6 'Refresh\(|StartAutoRefresh|schema discovery|Get\(table\)' internal/api internal/discovery internal/streamRepository: Wave-RF/WaveHouse
Length of output: 50373
🏁 Script executed:
python3 - <<'PY'
from datetime import datetime, timezone
def opaque_compare(event, constant):
# Equivalent to ColumnOpaque in internal/policy/rowfilter.go:
# only identical renderings are provably equal.
return 0 if event == constant else None
def time_compare(event, constant):
# Minimal probe for the documented instant-comparison distinction.
def parse(value):
if value.endswith("Z"):
value = value[:-1] + "+00:00"
return datetime.fromisoformat(value).astimezone(timezone.utc)
return (parse(event) > parse(constant)) - (parse(event) < parse(constant))
canonical = "2026-06-21T04:00:00Z"
zone_less = "2026-06-21 04:00:00"
assert time_compare(canonical, zone_less) == 0, "usable timestamp parser compares instants"
assert opaque_compare(canonical, zone_less) is None, "no parser does not compare alternate spellings"
assert opaque_compare(canonical, canonical) == 0, "opaque equality still admits identical bytes"
print("usable parser: alternate spelling compares equal by instant")
print("no usable parser: alternate spelling is not byte-equal and is withheld")
PYRepository: Wave-RF/WaveHouse
Length of output: 286
Qualify instant comparison by parser availability.
Line 282 must state that instant comparison requires a usable schema and ColumnSpec.ParseTime. Without them, the stream uses byte equality, so an alternate spelling can be withheld instead of matching the canonical payload.
This was referenced Aug 12, 2026
Open
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
The Server-Sent Events stream applied a role's column allowlist but never its row-level
filter, so a subscriber could receive rows the structured-query path would have hidden for that same role — a row-level-security bypass on the streaming surface. This applies the same row-filter on the stream:ResolvedPermissions.RowVisible) on the stream — so the two can't drift._gt/_lt) are schema-informed — numeric columns compare numerically via the schema registry now wired into the hub — while equality/set (_eq/_neq/_in) stay exact. Ambiguous values fail closed; the best-effort ordering boundary is documented in access-control docs.Related Issues
Closes #319