fix(ingest): canonicalize DateTime column values to RFC 3339 UTC - #402
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
WalkthroughWaveHouse adds ingest-time canonicalization for ChangesTimestamp Canonicalization
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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://cdcc8bc4-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall coverage in commit 437ad23 in the Show a code coverage summary of the most impacted files.
Updated |
github-actions
Bot
removed
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 22fa45c5-fbb2-4b42-be1f-3e2fc17b161f
📒 Files selected for processing (14)
CHANGELOG.mddocs/src/content/docs/api.mdgo.modinternal/api/boot_chain_test.gointernal/api/ingest.gointernal/api/ingest_test.gointernal/discovery/discovery.gointernal/discovery/discovery_test.gointernal/discovery/timestamp.gointernal/discovery/timestamp_test.gointernal/discovery/validation.gointernal/ingest/worker.gointernal/ingest/worker_test.gotests/e2e/sdk/streaming.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Docs preview
- GitHub Check: Coverage
- GitHub Check: E2E tests
- GitHub Check: Integration tests
🧰 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/ingest/worker_test.gointernal/discovery/discovery_test.gointernal/discovery/timestamp_test.gointernal/api/ingest_test.gointernal/api/boot_chain_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/api.md
🧠 Learnings (5)
📚 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/ingest/worker_test.gointernal/discovery/discovery_test.gointernal/discovery/timestamp_test.gointernal/api/ingest_test.gointernal/api/boot_chain_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/ingest/worker_test.gointernal/ingest/worker.gointernal/api/ingest.gointernal/discovery/validation.gointernal/discovery/discovery_test.gointernal/discovery/discovery.gointernal/discovery/timestamp_test.gointernal/discovery/timestamp.gointernal/api/ingest_test.gointernal/api/boot_chain_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:
CHANGELOG.mddocs/src/content/docs/api.md
📚 Learning: 2026-05-20T01:02:00.784Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:00.784Z
Learning: In WaveHouse’s internal API tests (files matching internal/api/**/*_test.go), follow the existing separation-of-concerns convention for testing the RequireRole middleware: inject `ContextKeyRole` directly into the request `context.Context` instead of using `testutil.MakeJWT`/JWT-driven flows. Do not refactor role-gate tests to use JWT tokens—JWT parsing and token handling are covered separately in `middleware_test.go` (the dedicated JWT parsing tests), and mixing those concerns would expand the failure surface and reduce isolation.
Applied to files:
internal/api/ingest_test.gointernal/api/boot_chain_test.go
📚 Learning: 2026-05-23T01:23:59.268Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:23:59.268Z
Learning: In WaveHouse Go tests in internal/api/**/*_test.go, use internal/testutil.AssertJSONErrorResponse(t, w) for HTTP error-path JSON assertions. Do not use (or reintroduce) package-local assertJSONErrorResponse helpers. AssertJSONErrorResponse verifies the response Content-Type is application/json, includes the X-Content-Type-Options: nosniff header, and that the JSON body contains an "error" field.
Applied to files:
internal/api/ingest_test.gointernal/api/boot_chain_test.go
🔇 Additional comments (14)
internal/discovery/discovery.go (1)
71-77: LGTM!Also applies to: 118-135
internal/discovery/discovery_test.go (1)
112-128: LGTM!internal/api/boot_chain_test.go (1)
41-57: LGTM!CHANGELOG.md (1)
49-50: LGTM!docs/src/content/docs/api.md (1)
220-221: LGTM!Also applies to: 240-240, 545-546, 776-776
tests/e2e/sdk/streaming.test.ts (1)
80-128: LGTM!go.mod (1)
3-3: 📐 Maintainability & Code QualityKeep
go 1.26.5if the toolchain bump is intended
1.26.5is a valid Go patch release and the current 1.26.x latest, so the version itself looks fine. The only remaining question is whether this minimum-version bump is deliberate for contributors or should be reverted.internal/discovery/validation.go (1)
51-69: LGTM!internal/discovery/timestamp_test.go (1)
12-133: LGTM!internal/api/ingest.go (1)
404-413: LGTM!internal/api/ingest_test.go (1)
1383-1432: LGTM!Also applies to: 1450-1482
internal/ingest/worker.go (1)
434-438: LGTM!internal/ingest/worker_test.go (1)
326-328: LGTM!internal/discovery/timestamp.go (1)
26-159: Confirm the Go toolchain bump covers these APIs
strings.SplitSeqrequires Go 1.24, andfor range precisionrequires Go 1.22+. Make surego.modtargets at least Go 1.24 before merging.
github-project-automation
Bot
moved this from In progress
to In review
in WaveHouse Task Board
…nical wire-form invariant Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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: fc33ad16-18ec-4fcd-9f2a-a02e315de790
📒 Files selected for processing (10)
AGENTS.mdCHANGELOG.mdcmd/wavehouse/main.godocs/src/content/docs/api.mddocs/src/content/docs/architecture.mdinternal/api/ingest.gointernal/api/ingest_test.gointernal/discovery/discovery.gointernal/discovery/timestamp.gointernal/discovery/timestamp_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Docs preview
- GitHub Check: Coverage
- GitHub Check: Integration tests
- GitHub Check: E2E tests
- GitHub Check: Unit tests
- GitHub Check: Lint
🧰 Additional context used
📓 Path-based instructions (4)
docs/src/content/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Keep documentation accurate, runnable, and synchronized with code changes; update architecture, API, configuration, deployment, or development docs as relevant.
Files:
docs/src/content/docs/architecture.mddocs/src/content/docs/api.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Record notable changes under
[Unreleased]when a change affects user-visible behavior or project conventions.
Files:
CHANGELOG.md
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Use shared mocks from
internal/testutil/instead of ad-hoc mocks, and prefer the provided JWT, schema, policy, pipes, and JSON response helpers in tests.
Files:
internal/api/ingest_test.gointernal/discovery/timestamp_test.go
internal/discovery/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Preserve schema discovery and timestamp canonicalization behavior: ingest timestamps must be rewritten to canonical RFC 3339 UTC before publish.
Files:
internal/discovery/discovery.gointernal/discovery/timestamp_test.gointernal/discovery/timestamp.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: Validate locally before every push by running `make ci` the documented way; do not rely on CI as the first feedback loop.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: For PR-branch pushes, run `/prepush` and ensure every required pre-push reviewer from `scripts/pre-push-reviewers.sh` reaches `ship_it` or is deliberately skipped on the record.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: Every code change must update its corresponding documentation and `CHANGELOG.md` in the same PR.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: Address and resolve every review finding; reply substantively, fix it or track it in an issue, and never silently drop a thread.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: Agents must create draft PRs only, and PR titles must pass the Conventional Commits lint rules (≤72 chars, lowercase-first subject, no trailing period).
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: Never force-push or rebase a PR branch; merge upstream main instead with `git merge origin/main`.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: Never hand-write review markers or use `--no-verify`; use the documented gates and skip tooling instead.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: Comment the why, not the what; only add comments when the reason is not obvious from the code, and keep them short.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: Prefer DRY, with one source of truth for shared logic and canonical scripts for repeated repo rules.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: Leave touched code neater than you found it by fixing small safe issues in the same scope, but avoid risky cleanup.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-09T17:18:37.453Z
Learning: Use table-driven tests with `t.Run(tt.name, ...)` for multiple scenarios.
📚 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:
cmd/wavehouse/main.gointernal/api/ingest.gointernal/api/ingest_test.gointernal/discovery/discovery.gointernal/discovery/timestamp_test.gointernal/discovery/timestamp.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.mdAGENTS.mddocs/src/content/docs/api.md
📚 Learning: 2026-05-20T01:02:00.784Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:00.784Z
Learning: In WaveHouse’s internal API tests (files matching internal/api/**/*_test.go), follow the existing separation-of-concerns convention for testing the RequireRole middleware: inject `ContextKeyRole` directly into the request `context.Context` instead of using `testutil.MakeJWT`/JWT-driven flows. Do not refactor role-gate tests to use JWT tokens—JWT parsing and token handling are covered separately in `middleware_test.go` (the dedicated JWT parsing tests), and mixing those concerns would expand the failure surface and reduce isolation.
Applied to files:
internal/api/ingest_test.go
📚 Learning: 2026-05-23T01:23:59.268Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:23:59.268Z
Learning: In WaveHouse Go tests in internal/api/**/*_test.go, use internal/testutil.AssertJSONErrorResponse(t, w) for HTTP error-path JSON assertions. Do not use (or reintroduce) package-local assertJSONErrorResponse helpers. AssertJSONErrorResponse verifies the response Content-Type is application/json, includes the X-Content-Type-Options: nosniff header, and that the JSON body contains an "error" field.
Applied to files:
internal/api/ingest_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/api/ingest_test.gointernal/discovery/timestamp_test.go
📚 Learning: 2026-07-09T16:20:50.620Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 402
File: internal/api/ingest_test.go:1434-1448
Timestamp: 2026-07-09T16:20:50.620Z
Learning: In WaveHouse’s Go schema/timestamp handling (internal/discovery), resolve any timestamp column zone/precision requirements exactly once during SchemaRegistry.Refresh() and cache the resolved specs on the column. Do not resolve zones per-record on the hot path. Ensure timezone resolution uses embedded tzdata (e.g., via cmd/wavehouse’s time.LoadLocation + embedded tzdata) so minimal containers don’t silently fall back to UTC for non-UTC ClickHouse servers, which would corrupt stored instants. Treat timezone-resolution failures during refresh as non-fatal: keep boot alive, RetryRefresh should continue retrying, and readiness/health (/livez) should surface a degraded state. If a column’s timezone cannot be resolved after refresh, emit a warning and reject only that column’s timestamp values per-record rather than failing the entire refresh.
Applied to files:
internal/discovery/discovery.gointernal/discovery/timestamp_test.gointernal/discovery/timestamp.go
🔇 Additional comments (11)
internal/discovery/discovery.go (1)
22-28: LGTM!Also applies to: 71-96, 130-137, 236-247
internal/api/ingest_test.go (1)
1411-1432: LGTM!Also applies to: 1434-1448, 1450-1482
docs/src/content/docs/api.md (1)
220-221: LGTM!Also applies to: 240-240, 545-546, 776-776
CHANGELOG.md (1)
53-54: LGTM!cmd/wavehouse/main.go (1)
17-23: LGTM!internal/discovery/timestamp.go (2)
44-71: LGTM!Also applies to: 73-80, 82-105, 107-147, 196-207
119-119: 🎯 Functional CorrectnessNo Go version compatibility issue here.
go 1.26.5already supportsstrings.SplitSeqand range-over-function syntax.> Likely an incorrect or invalid review comment.internal/discovery/timestamp_test.go (1)
47-91: LGTM!Also applies to: 93-101, 105-114, 116-140, 142-171, 173-182
internal/api/ingest.go (1)
404-414: LGTM!AGENTS.md (1)
37-37: LGTM!docs/src/content/docs/architecture.md (1)
116-117: LGTM!Also applies to: 171-173
github-project-automation
Bot
moved this from Ready
to In review
in WaveHouse Task Board
…/best_effort divergence, DLQ traps, Nullable offset exception)
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 43be325f-aa48-47bc-9eb5-825d8dad788a
📒 Files selected for processing (8)
AGENTS.mdCHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/ingest-pipeline.mddocs/src/content/docs/sdk/streaming.mdinternal/api/clickhouse_exec.gointernal/api/clickhouse_exec_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: E2E tests
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (6)
docs/src/content/docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep documentation synchronized with code changes, including API, configuration, architecture, ingest/event formats, deployment, and development documentation as applicable.
Files:
docs/src/content/docs/sdk/streaming.mddocs/src/content/docs/architecture.mddocs/src/content/docs/ingest-pipeline.mddocs/src/content/docs/api.md
**/*.{go,ts,tsx,md,mdx,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Every notable change must update
CHANGELOG.mdunder[Unreleased]in the same PR.
Files:
docs/src/content/docs/sdk/streaming.mdinternal/api/clickhouse_exec.gointernal/api/clickhouse_exec_test.godocs/src/content/docs/architecture.mdCHANGELOG.mddocs/src/content/docs/ingest-pipeline.mdAGENTS.mddocs/src/content/docs/api.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Comment code only to explain non-obvious reasons, keep comments to one or two lines, avoid restating code, and prefer rationale in commit messages, PRs, or the changelog.
Files:
docs/src/content/docs/sdk/streaming.mddocs/src/content/docs/architecture.mdCHANGELOG.mddocs/src/content/docs/ingest-pipeline.mdAGENTS.mddocs/src/content/docs/api.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use Go 1.26, format code strictly with gofumpt, return errors instead of panicking, wrap errors withfmt.Errorf("context: %w", err), inject dependencies explicitly, and use lowercase single-word package names.
Use structured logging withlog/slogand JSON handlers.
Files:
internal/api/clickhouse_exec.gointernal/api/clickhouse_exec_test.go
internal/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/api/**/*.go: Validate flat JSON ingest payloads against the discovered ClickHouse schema before publishing; unknown fields, invalid types, and nullability violations must be rejected.
Use bearer-token authentication without cookies or sessions; CORS must never emitAccess-Control-Allow-Credentials.
Use/livezand/readyzfor liveness and readiness, retain/healthzas the liveness alias, and use/v1/healthfor the SDK public ping; do not use deprecated/healthor/readyaliases.
Files:
internal/api/clickhouse_exec.gointernal/api/clickhouse_exec_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Use table-driven tests withtests := []struct{...}andt.Run(tt.name, ...)for multiple scenarios.
Use shared helpers frominternal/testutil/, including shared mocks, JWT helpers, schema registries, response assertions, and policy/pipe memory stores; do not create ad-hoc equivalents.
Every new function should have corresponding test cases; runmake lintandmake testbefore considering work complete.
Maintain at least 80% project-wide coverage, with enforced per-suite minima of unit 80%, integration 20%, E2E 60%, and SDK 50%.
Files:
internal/api/clickhouse_exec_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-11T18:03:35.065Z
Learning: Use one source of truth: reuse existing helpers, types, and constants instead of duplicating logic or rules.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-11T18:03:35.065Z
Learning: Run `make ci` locally before every push, using the documented background execution method with Docker running and output redirected to `tmp/ci.log`; do not use CI as the first feedback loop.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-11T18:03:35.065Z
Learning: On PR branches, run `/prepush` and ensure every reviewer listed by `scripts/pre-push-reviewers.sh` reaches `ship_it` or is deliberately skipped through the skip script; never hand-write markers.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-11T18:03:35.065Z
Learning: Create agent PRs as drafts with `gh pr create --draft`; titles must use Conventional Commits syntax, be at most 72 characters, start lowercase in the subject, and have no trailing period.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-11T18:03:35.065Z
Learning: Never force-push or rebase PR branches; merge `origin/main` instead. Do not use `--no-verify` or bypass repository gates.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-08-11T18:03:35.065Z
Learning: Address every review finding with a substantive reply, a fix or linked tracking issue, an appropriate bot mention, and resolved review threads before merge.
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
docs/src/content/docs/sdk/streaming.mddocs/src/content/docs/architecture.mdCHANGELOG.mddocs/src/content/docs/ingest-pipeline.mdAGENTS.mddocs/src/content/docs/api.md
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
internal/api/clickhouse_exec.gointernal/api/clickhouse_exec_test.go
📚 Learning: 2026-05-20T01:02:00.784Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:00.784Z
Learning: In WaveHouse’s internal API tests (files matching internal/api/**/*_test.go), follow the existing separation-of-concerns convention for testing the RequireRole middleware: inject `ContextKeyRole` directly into the request `context.Context` instead of using `testutil.MakeJWT`/JWT-driven flows. Do not refactor role-gate tests to use JWT tokens—JWT parsing and token handling are covered separately in `middleware_test.go` (the dedicated JWT parsing tests), and mixing those concerns would expand the failure surface and reduce isolation.
Applied to files:
internal/api/clickhouse_exec_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/clickhouse_exec_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/clickhouse_exec_test.go
🪛 LanguageTool
docs/src/content/docs/sdk/streaming.md
[style] ~92-~92: Consider an alternative for the overused word “exactly”.
Context: ... spelling — a zone-less one of those is exactly what new Date() reads as local time...
(EXACTLY_PRECISELY)
docs/src/content/docs/api.md
[style] ~238-~238: Consider using the typographical ellipsis character here instead.
Context: ...n"}| Malformed request body | | 400 |{"error":"unknown column ... for table ..."}(also:missing requir...
(ELLIPSIS)
[style] ~238-~238: Consider using the typographical ellipsis character here instead.
Context: ...nown column ... for table ..."}(also:missing required column ..., type mismatch for column ..., null...
(ELLIPSIS)
[style] ~238-~238: Consider using the typographical ellipsis character here instead.
Context: ...(also:missing required column ..., type mismatch for column ..., null value for non-nullable column ....
(ELLIPSIS)
[style] ~238-~238: Consider using the typographical ellipsis character here instead.
Context: ...n ..., type mismatch for column ..., null value for non-nullable column ...`) | Schema validation failure (unknown ...
(ELLIPSIS)
[style] ~269-~269: Consider using a more formal/concise alternative here.
Context: ...edge cases] - Digit-strings of lengths other than 9–10 are ClickHouse's own forms — calen...
(OTHER_THAN)
🔇 Additional comments (6)
internal/api/clickhouse_exec.go (1)
342-346: LGTM!internal/api/clickhouse_exec_test.go (1)
164-168: LGTM!Also applies to: 178-191
docs/src/content/docs/architecture.md (1)
77-77: LGTM!Also applies to: 171-173, 184-193
docs/src/content/docs/ingest-pipeline.md (1)
18-35: LGTM!Also applies to: 70-88
docs/src/content/docs/api.md (1)
214-219: LGTM!Also applies to: 238-238, 256-271, 273-290, 809-809
AGENTS.md (1)
68-68: 🎯 Functional CorrectnessUse one verified contract for timezone-resolution failures. The affected documents do not distinguish the discovered server default from a declared column zone. Verify the current implementation and document the exact behavior for each path.
AGENTS.md#L68-L68: remove the blanket “never a failed refresh” claim unless the server-default path is verified as non-fatal.docs/src/content/docs/architecture.md#L116-L117: describe server-default resolution and column-zone resolution as separate paths.docs/src/content/docs/api.md#L272-L272: state whether an unresolved server default fails refresh, skips canonicalization, or passes through.CHANGELOG.md#L53-L53: make the historical behavior precise and consistent with the current discovery implementation.
…-parser and DLQ-spelling notes
…lete project tree
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/src/content/docs/architecture.md (1)
116-117: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLimit the canonicalization claim to top-level values.
docs/src/content/docs/sdk/streaming.mdexcludes temporal values nested inArray,Map, andTuplecolumns. These bullets say everyDateTime/DateTime64value receives one canonical spelling. State that only parseable top-level column values are canonicalized.Suggested wording
- rewrites every DateTime/DateTime64 value to the canonical RFC 3339 UTC wire form + rewrites parseable values in top-level DateTime/DateTime64 columns to the canonical RFC 3339 UTC wire formAlso applies to: 171-173
docs/src/content/docs/api.md (1)
217-219: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
DateTime64in the schema compatibility list.Line 217 lists
DateTime,Date, andEnumas accepting strings or numbers, but Line 219 and the canonicalization section documentDateTime64as a supported timestamp type. AddDateTime64to keep the validation contract complete.Suggested wording
- `DateTime`/`Date`/`Enum` accept JSON strings or numbers + `DateTime`/`DateTime64`/`Date`/`Enum` accept JSON strings or numbers
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e3ed2fe-9f38-4c6b-8d2c-8c43c2162692
📒 Files selected for processing (18)
.claude/agents/docs-reviewer.mdAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdSUPPORT.mddeployments/compose/dev-policy.yamldocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.mddocs/src/content/docs/development.mddocs/src/content/docs/ingest-pipeline.mddocs/src/content/docs/sdk/streaming.mdinternal/config/config.gointernal/discovery/discovery.gointernal/discovery/discovery_test.gointernal/discovery/timestamp.gointernal/policy/policy.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Docs preview
- GitHub Check: Coverage
- GitHub Check: E2E tests
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
Third-party GitHub Actions must be pinned to full commit SHAs with version comments; never use floating tags or
@main. Preserve themerge_groupCI trigger and fail-closed change classification.
Files:
deployments/compose/dev-policy.yaml
deployments/compose/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Keep deployment changes synchronized with deployment documentation and configuration environment blocks.
Files:
deployments/compose/dev-policy.yaml
**/*
📄 CodeRabbit inference engine (AGENTS.md)
Never force-push or rebase PR branches; merge
origin/maininstead. Do not hand-write review markers or bypass verification with--no-verify.
Files:
deployments/compose/dev-policy.yamlinternal/config/config.goCONTRIBUTING.mdSUPPORT.mdinternal/policy/policy.godocs/src/content/docs/deployment.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/development.mddocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/architecture.mdinternal/discovery/discovery.godocs/src/content/docs/ingest-pipeline.mdAGENTS.mddocs/src/content/docs/api.mdinternal/discovery/timestamp.goCHANGELOG.mdinternal/discovery/discovery_test.go
**/*.{go,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{go,ts,tsx,js,jsx}: Return errors instead of panicking; wrap errors withfmt.Errorf("context: %w", err).
Every code change must update corresponding documentation andCHANGELOG.mdin the same PR; public API changes must also update the canonical TypeScript SDK where users' code would be affected.
Files:
internal/config/config.gointernal/policy/policy.gointernal/discovery/discovery.gointernal/discovery/timestamp.gointernal/discovery/discovery_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use Go 1.26 with strictgofumptformatting, structuredlog/sloglogging, Chi v5 for HTTP routing, explicit constructor dependency injection, and lowercase single-word package names.
Before every push, validate locally with the documentedmake cipipeline; do not use CI as the first feedback loop.
Files:
internal/config/config.gointernal/policy/policy.gointernal/discovery/discovery.gointernal/discovery/timestamp.gointernal/discovery/discovery_test.go
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep dependencies explicit; do not use global state. Define interfaces for core behaviors with multiple implementations.
Files:
internal/config/config.gointernal/policy/policy.gointernal/discovery/discovery.gointernal/discovery/timestamp.gointernal/discovery/discovery_test.go
**/*.{go,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Every new function must have corresponding test cases; use table-driven tests with
t.Runfor multiple scenarios and target at least 80% coverage for new code.
Files:
internal/config/config.gointernal/policy/policy.gointernal/discovery/discovery.gointernal/discovery/timestamp.gointernal/discovery/discovery_test.go
internal/config/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Configuration fields must have synchronized YAML, environment-variable, and default tags, and corresponding documentation in
configuration.mdx,config.yaml, compose environment blocks, and deployment documentation.
Files:
internal/config/config.go
**/*.md{,x}
📄 CodeRabbit inference engine (AGENTS.md)
Documentation prose must remain accurate against code, contain runnable examples, and be clear and complete. Mermaid diagrams should default to top-down layouts, avoid side-by-side large diagrams, and keep labels short.
Files:
CONTRIBUTING.mdSUPPORT.mddocs/src/content/docs/deployment.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/development.mddocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/architecture.mddocs/src/content/docs/ingest-pipeline.mdAGENTS.mddocs/src/content/docs/api.mdCHANGELOG.md
internal/policy/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Authorization must fail closed:
policy.IsAdminis the single exact, case-sensitive admin check; empty roles and wildcard entries authorize nothing; nil policies deny everyone except the documented operator-key exception.
Files:
internal/policy/policy.go
internal/discovery/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Canonicalize parseable
DateTimeandDateTime64values to RFC 3339 UTC using the column precision and zone; unparseable values or unresolved zones must be published verbatim rather than rejected or silently reinterpreted.
Files:
internal/discovery/discovery.gointernal/discovery/timestamp.gointernal/discovery/discovery_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Use shared helpers from
internal/testutil/, including mock implementations, JWT builders, schema registries, JSON response assertions, and policy or pipe memory stores, instead of ad-hoc equivalents.
Files:
internal/discovery/discovery_test.go
🧠 Learnings (4)
📚 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/config/config.gointernal/policy/policy.gointernal/discovery/discovery.gointernal/discovery/timestamp.gointernal/discovery/discovery_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:
CONTRIBUTING.mdSUPPORT.mddocs/src/content/docs/deployment.mddocs/src/content/docs/development.mddocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/architecture.mddocs/src/content/docs/ingest-pipeline.mdAGENTS.mddocs/src/content/docs/api.mdCHANGELOG.md
📚 Learning: 2026-07-09T16:20:50.620Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 402
File: internal/api/ingest_test.go:1434-1448
Timestamp: 2026-07-09T16:20:50.620Z
Learning: In WaveHouse’s Go schema/timestamp handling (internal/discovery), resolve any timestamp column zone/precision requirements exactly once during SchemaRegistry.Refresh() and cache the resolved specs on the column. Do not resolve zones per-record on the hot path. Ensure timezone resolution uses embedded tzdata (e.g., via cmd/wavehouse’s time.LoadLocation + embedded tzdata) so minimal containers don’t silently fall back to UTC for non-UTC ClickHouse servers, which would corrupt stored instants. Treat timezone-resolution failures during refresh as non-fatal: keep boot alive, RetryRefresh should continue retrying, and readiness/health (/livez) should surface a degraded state. If a column’s timezone cannot be resolved after refresh, emit a warning and reject only that column’s timestamp values per-record rather than failing the entire refresh.
Applied to files:
internal/discovery/discovery.gointernal/discovery/timestamp.gointernal/discovery/discovery_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/discovery/discovery_test.go
🪛 LanguageTool
docs/src/content/docs/deployment.md
[typographical] ~362-~362: The word ‘When’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...NATS stream under subjects dlq.{table}. This prevents infinite retry loops. Mon...
(WRB_QUESTION_MARK)
docs/src/content/docs/sdk/streaming.md
[style] ~94-~94: Consider an alternative for the overused word “exactly”.
Context: ...e table at all), and a zone-less one is exactly what new Date() reads as local time...
(EXACTLY_PRECISELY)
🔇 Additional comments (17)
docs/src/content/docs/configuration.mdx (1)
165-165: LGTM!docs/src/content/docs/deployment.md (1)
362-362: LGTM!internal/discovery/discovery.go (1)
124-138: LGTM!internal/discovery/discovery_test.go (1)
105-115: LGTM!Also applies to: 160-184, 202-216
internal/discovery/timestamp.go (1)
139-149: LGTM!docs/src/content/docs/architecture.md (1)
77-77: LGTM!Also applies to: 123-123, 184-187
docs/src/content/docs/ingest-pipeline.md (1)
30-35: LGTM!Also applies to: 70-89
docs/src/content/docs/sdk/streaming.md (1)
87-93: LGTM!Also applies to: 96-97
AGENTS.md (1)
308-308: LGTM!Also applies to: 367-367, 382-382
internal/config/config.go (1)
54-54: LGTM!internal/policy/policy.go (1)
345-345: LGTM!CHANGELOG.md (1)
374-374: LGTM!CONTRIBUTING.md (1)
46-46: LGTM!SUPPORT.md (1)
26-26: LGTM!deployments/compose/dev-policy.yaml (1)
19-19: LGTM!docs/src/content/docs/api.md (1)
214-216: LGTM!Also applies to: 238-238, 256-290, 378-378, 497-497, 578-579, 809-809, 829-829
docs/src/content/docs/development.md (1)
432-444: LGTM!
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: d3173e0d-00a6-4e24-bc3c-00ca1f6643e6
📒 Files selected for processing (5)
.claude/agents/docs-reviewer.mdAGENTS.mdCHANGELOG.mddocs/src/content/docs/architecture.mddocs/src/content/docs/sdk/streaming.md
📜 Review details
⏰ Context from checks skipped due to timeout. (8)
- GitHub Check: Integration tests
- GitHub Check: Docs build
- GitHub Check: Coverage
- GitHub Check: Unit tests
- GitHub Check: E2E tests
- GitHub Check: Lint
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
🧠 Learnings (38)
📓 Common learnings
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 402
File: internal/api/ingest_test.go:1434-1448
Timestamp: 2026-07-09T16:20:57.165Z
Learning: In WaveHouse's internal/discovery package (Go), timestamp column zone/precision specs are resolved once during SchemaRegistry.Refresh() and cached on the column (not resolved per-record on the hot path). cmd/wavehouse embeds time/tzdata (~450 KB) so time.LoadLocation resolves timezones even on minimal container images lacking a system zone database, avoiding a design where zone-less timestamp strings would silently fall back to UTC on non-UTC ClickHouse servers (which would corrupt the stored instant). A refresh failure due to zone resolution is only reachable when ClickHouse reports a timezone newer than the binary's embedded tzdata; in that case boot stays non-fatal, RetryRefresh keeps retrying, and /livez surfaces the degraded state. An unresolvable column zone (post-refresh) causes a warning and per-record rejection for that column's timestamp values, rather than failing the whole refresh.
📚 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 README.md : Keep the repository README in sync with user-facing API, SDK, or governance changes when the README is part of the documented source-of-truth set.
Applied to files:
.claude/agents/docs-reviewer.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 docs/src/content/docs/**/*.{md,mdx} : 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.
Applied to files:
.claude/agents/docs-reviewer.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 docs/src/content/docs/configuration.md : Document every new or changed configuration option in the configuration docs.
Applied to files:
.claude/agents/docs-reviewer.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 clients/ts/README.md : Keep the TypeScript SDK README in sync with SDK-facing changes and public client behavior.
Applied to files:
.claude/agents/docs-reviewer.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:
.claude/agents/docs-reviewer.mdAGENTS.md
📚 Learning: 2026-07-09T16:20:57.165Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 402
File: internal/api/ingest_test.go:1434-1448
Timestamp: 2026-07-09T16:20:57.165Z
Learning: In WaveHouse's internal/discovery package (Go), timestamp column zone/precision specs are resolved once during SchemaRegistry.Refresh() and cached on the column (not resolved per-record on the hot path). cmd/wavehouse embeds time/tzdata (~450 KB) so time.LoadLocation resolves timezones even on minimal container images lacking a system zone database, avoiding a design where zone-less timestamp strings would silently fall back to UTC on non-UTC ClickHouse servers (which would corrupt the stored instant). A refresh failure due to zone resolution is only reachable when ClickHouse reports a timezone newer than the binary's embedded tzdata; in that case boot stays non-fatal, RetryRefresh keeps retrying, and /livez surfaces the degraded state. An unresolvable column zone (post-refresh) causes a warning and per-record rejection for that column's timestamp values, rather than failing the whole refresh.
Applied to files:
docs/src/content/docs/sdk/streaming.mddocs/src/content/docs/architecture.mdCHANGELOG.mdAGENTS.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:
docs/src/content/docs/sdk/streaming.mddocs/src/content/docs/architecture.mdCHANGELOG.mdAGENTS.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/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: 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.md
📚 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/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/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.mdCHANGELOG.mdAGENTS.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.mdAGENTS.md
📚 Learning: 2026-08-11T21:56:03.599Z
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:03.599Z
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:
CHANGELOG.mdAGENTS.md
📚 Learning: 2026-08-11T12:41:01.909Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 446
File: .github/actions/setup-env/action.yml:112-127
Timestamp: 2026-08-11T12:41:01.909Z
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-08-11T12:41:02.656Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 446
File: .github/workflows/README.md:0-0
Timestamp: 2026-08-11T12:41:02.656Z
Learning: In the GitHub Actions cache design, `.github/actions/setup-env/action.yml` owns the shared `gomod-v1` module cache and per-suffix `gobuild-v3` build-object caches for `ci.yml` Go jobs. `.github/workflows/publish-dev.yml` saves only the `gobuild-v3-...-release-...` build cache and uses `actions/cache/restore` to read `gomod-v1` without saving it. `.github/workflows/release.yml` does not save these caches.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-11T12:41:01.909Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 446
File: .github/actions/setup-env/action.yml:112-127
Timestamp: 2026-08-11T12:41:01.909Z
Learning: In `.github/workflows/publish-dev.yml`, restore the shared `gomod-v1` cache with `actions/cache/restore` rather than `actions/cache`. The workflow does not run a full `go mod download`, so it must not save a potentially partial shared module cache.
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-08-11T11:20:19.556Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 446
File: .github/workflows/publish-dev.yml:0-0
Timestamp: 2026-08-11T11:20:19.556Z
Learning: For GitHub Actions workflows that use `actions/cache` or `actions/setup-go` caching, a cache archive is saved only after an exact-key miss. A `go.sum`-keyed cache therefore creates a new archive when dependencies change, while runs with an existing exact key restore the archive and skip the post-job save.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-11T21:55:41.895Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/go.mod:3-6
Timestamp: 2026-08-11T21:55:41.895Z
Learning: In the WaveHouse repository, `clients/go/go.mod` declares `go 1.24` as the deliberate minimum supported Go version for the published Go SDK. This SDK compatibility floor is independent of the server build toolchain declared by the root `go.mod` and referenced in `AGENTS.md`; do not require the Go SDK module to use the server toolchain version.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-11T12:41:03.490Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 446
File: .github/actions/setup-env/action.yml:0-0
Timestamp: 2026-08-11T12:41:03.490Z
Learning: In `.github/actions/setup-env/action.yml`, `go` defaults to `"true"`. Every caller that does not need Go must explicitly pass `go: "false"`. When `go` is `"true"`, `go-cache-suffix` is required and the composite action fails before its Go cache steps if the suffix is empty. The guard must use the same `inputs.go == 'true'` condition as the Go cache steps.
Applied to files:
CHANGELOG.md
📚 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: AGENTS.md is the single source of truth for project conventions and AI agent instructions
Applied to files:
AGENTS.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.md
📚 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:
AGENTS.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: Maintain .github/copilot-instructions.md as a pointer to AGENTS.md for GitHub Copilot integration
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: Every code change must update its documentation and `CHANGELOG.md` in the same PR.
Applied to files:
AGENTS.md
📚 Learning: 2026-07-24T18:23:07.472Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 418
File: internal/observability/metrics_test.go:108-240
Timestamp: 2026-07-24T18:23:07.472Z
Learning: In `internal/observability/metrics_test.go`, tests in package `observability` cannot import shared `internal/testutil/` mocks because `internal/testutil/` imports `mq`, which imports `observability` and would create an import cycle. Keep minimal local test stubs (such as `stubDeduplicator`, `stubCHConn`, and `stubPartsRows`) in this package unless the dependency structure changes.
Applied to files:
AGENTS.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:
AGENTS.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-08-11T21:55:38.841Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/cmd/wavehouse-codegen/main.go:96-97
Timestamp: 2026-08-11T21:55:38.841Z
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:
AGENTS.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:
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 **/*.{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:
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 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:
AGENTS.md
📚 Learning: 2026-08-11T15:22:43.300Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 448
File: docs/src/content/docs/sdk/index.mdx:330-334
Timestamp: 2026-08-11T15:22:43.300Z
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:
AGENTS.md
📚 Learning: 2026-05-13T20:41:09.256Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 125
File: internal/api/health_test.go:100-163
Timestamp: 2026-05-13T20:41:09.256Z
Learning: In `internal/api/health_test.go` (WaveHouse), every handler test explicitly asserts `Content-Type: application/json` and `X-Content-Type-Options: nosniff` headers, including on 503 responses. This is deliberate regression coverage: the comment in `TestHealth_Readiness_PingFails` explains that without the 503-path header test, a future refactor moving header setup into the success branch would silently drop headers on error responses. New boot-degraded tests should follow the same pattern.
Applied to files:
AGENTS.md
📚 Learning: 2026-05-25T11:24:24.022Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 180
File: internal/cache/local.go:0-0
Timestamp: 2026-05-25T11:24:24.022Z
Learning: In the WaveHouse codebase (`internal/cache/local.go` and related packages), package-level `var` declarations of immutable `metric.MeasurementOption` / OTel attribute sets (e.g., `cacheL1Attrs = metric.WithAttributes(attribute.String("tier", "L1"))`) are intentional and acceptable. These are pre-allocated constants analogous to `regexp.MustCompile(...)`, not mutable global state. The AGENTS.md "no global state / constructor injection" rule applies to application dependencies (Cache, Publisher, Deduplicator), not to stateless OTel metric attribute options. Do not flag these as violations of the constructor-injection guideline.
Applied to files:
AGENTS.md
📚 Learning: 2026-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/api/router.go : Route registrations in `router.go` must stay in sync with the API docs and middleware/dependency wiring.
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 internal/config/config.go : Configuration structs must include matching `yaml`, `env`, and `env-default` tags for new config fields.
Applied to files:
AGENTS.md
🔇 Additional comments (5)
docs/src/content/docs/sdk/streaming.md (1)
94-96: LGTM!docs/src/content/docs/architecture.md (1)
77-77: LGTM!Also applies to: 123-123, 171-173, 184-187
.claude/agents/docs-reviewer.md (1)
54-54: LGTM!AGENTS.md (1)
37-37: LGTM!Also applies to: 124-124, 308-308, 317-317, 367-367, 382-382
CHANGELOG.md (1)
374-374: LGTM!
github-project-automation
Bot
moved this from Ready
to In progress
in WaveHouse Task Board
github-project-automation
Bot
moved this from In progress
to Done
in WaveHouse Task Board
2 participants
Summary
The SSE stream fans out the pre-insert payload verbatim, so a row
DateTimereached subscribers in whatever spelling the producer sent (typically zone-less), while/v1/queryrenders the stored value as RFC 3339Z— JavaScript parses a zone-less string as local time, landing backfill and live on different clocks.Ingest now canonicalizes every
DateTime/DateTime64column value to RFC 3339 UTC before the NATS publish, so the one payload every consumer shares (SSE, the ClickHouse insert, the DLQ) matches the query path:YYYY-MM-DD[ T]HH:MM:SS[.fff],YYYY-MM-DD, and Unix seconds (number or digit-string) all convert. Zone-less strings are interpreted in the column's time zone, else the server's (discovered viaSELECT timezone()) — the same rule ClickHouse applies, so the stored instant never changes. Per-column specs (precision + resolved zone) are precomputed at schema refresh, so the per-record path parses no type strings and loads no zones.Etc/UTCmaps to UTC without it) — publishes verbatim, and ClickHouse's more liberal parser stays the arbiter of insertability, with failures landing in the DLQ as before. Ingest never rejects a record over its timestamp spelling; fail-closed enforcement of the canonical form is the stream row-filter's (fix(stream): apply policy row-filter per subscriber on SSE #381).date_time_input_format=best_effort: the defaultbasicparser rejects the canonical form's zone suffix (verified live);best_effortis a superset, so pre-upgrade messages still in NATS parse unchanged.Date/Date32pass through untouched (day precision, no zone ambiguity on this path).This is the data-plane foundation for the row-filter timestamp enforcement landing in #381.
Related Issues
Closes #372