fix(ingest): canonicalize DateTime column values to RFC 3339 UTC#402
fix(ingest): canonicalize DateTime column values to RFC 3339 UTC#402taitelee wants to merge 16 commits into
Conversation
github-actions
Bot
added
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📜 Recent review details⏰ Context from checks skipped due to timeout. (6)
🧰 Additional context used📓 Path-based instructions (4)**/*.md📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-06-10T15:01:09.027ZApplied to files:
📚 Learning: 2026-06-26T12:23:22.696ZApplied to files:
📚 Learning: 2026-07-07T12:38:12.052ZApplied to files:
🔇 Additional comments (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughWaveHouse adds ingest-time canonicalization for ChangesTimestamp Canonicalization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant IngestAPI
participant SchemaRegistry
participant CanonicalizeTimestamps
participant Publisher
participant ClickHouseWorker
Client->>IngestAPI: POST record with raw DateTime value
IngestAPI->>SchemaRegistry: load schema with cached timestamp spec
IngestAPI->>CanonicalizeTimestamps: rewrite timestamp fields
CanonicalizeTimestamps->>IngestAPI: canonical UTC value or original input
IngestAPI->>Publisher: publish canonicalized record
Publisher->>ClickHouseWorker: deliver record for insert
ClickHouseWorker->>ClickHouseWorker: insert with best-effort datetime parsing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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://6286a093-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall coverage in the branch remains at 90%, unchanged from the branch. 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
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Ok, I'm sending this back without detailed comments on much else because timezones and time parsing scares me, and I see exactly 0 tests that actually run e2e with a full clickhouse instance and various variations of datetime w/ and w/out timezone, varying degrees of precision, etc columns in their schema that we test against. For instance, what happens if you send "20260711", or "202607111500"? Does ClickHouse, with best_effort turned on now, interpret these as YYYYMMDD and YYYYMMDDHHMMSS, or as unix seconds?
Also, worth reading falsehoods programmers believe about time and more falsehoods programmers believe about time – basically, it scares me a lot to do this much, and I am POSITIVE that if we started fuzzing this end-to-end with random inputs, we wouldn't have to wait very long before the time we parsed it as differed from the time clickhouse parsed it as...
So in essence, I really think we need to figure out how to be FAR more strict about what the time coming in is when parsing it or its timezone, and only use that if we are ABSOLUTELY sure we know the format they used, etc. Good luck, times are hard. :)
github-project-automation
Bot
moved this from Ready
to In review
in WaveHouse Task Board
… 9-10 digit shape; reject ''/Local zones; add differential CH oracle test
…o-trimming, Z-only)
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: ee110756-6915-4e62-b4ff-69b5bdd25e2c
📒 Files selected for processing (20)
AGENTS.mdCHANGELOG.mdMakefiledocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/ingest-pipeline.mdinternal/api/errors_test.gointernal/api/ingest.gointernal/api/ingest_test.gointernal/api/router_test.gointernal/api/schema_test.gointernal/api/structured_query_test.gointernal/discovery/discovery.gointernal/discovery/discovery_test.gointernal/discovery/timestamp.gointernal/discovery/timestamp_test.gointernal/ingest/worker.gointernal/ingest/worker_test.gointernal/testutil/testutil.gotests/integration/timestamp_canonicalization_test.go
💤 Files with no reviewable changes (1)
- internal/ingest/worker.go
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: Docs preview
- GitHub Check: E2E tests
- GitHub Check: Unit tests
- GitHub Check: Coverage
- GitHub Check: Integration tests
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use Go 1.26 with strictgofumptformatting.
Use structured logging withlog/slogand JSON handlers.
Use Chi v5 for HTTP routing.
Return errors instead of panicking, and wrap errors withfmt.Errorf("context: %w", err).
Avoid global state; pass dependencies explicitly through constructor injection.
Comment the why, not the what; keep comments to one or two lines and omit comments that merely restate code.
Files:
internal/api/schema_test.gointernal/ingest/worker_test.gointernal/api/ingest.gotests/integration/timestamp_canonicalization_test.gointernal/api/errors_test.gointernal/api/router_test.gointernal/testutil/testutil.gointernal/discovery/discovery_test.gointernal/api/structured_query_test.gointernal/discovery/timestamp_test.gointernal/discovery/timestamp.gointernal/api/ingest_test.gointernal/discovery/discovery.go
internal/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Use lowercase, single-word or abbreviated package names; keep implementation packages under
internal/for module privacy.
Files:
internal/api/schema_test.gointernal/ingest/worker_test.gointernal/api/ingest.gointernal/api/errors_test.gointernal/api/router_test.gointernal/testutil/testutil.gointernal/discovery/discovery_test.gointernal/api/structured_query_test.gointernal/discovery/timestamp_test.gointernal/discovery/timestamp.gointernal/api/ingest_test.gointernal/discovery/discovery.go
**/*.{go,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer one source of truth and reuse existing helpers, types, and constants instead of duplicating logic.
Files:
internal/api/schema_test.gointernal/ingest/worker_test.gointernal/api/ingest.gotests/integration/timestamp_canonicalization_test.gointernal/api/errors_test.gointernal/api/router_test.gointernal/testutil/testutil.gointernal/discovery/discovery_test.gointernal/api/structured_query_test.gointernal/discovery/timestamp_test.gointernal/discovery/timestamp.gointernal/api/ingest_test.gointernal/discovery/discovery.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Use table-driven tests withtests := []struct{...}andt.Run(tt.name, ...)for multiple scenarios.
Every new function should have corresponding test cases; aim for at least 80% coverage on new code.
Files:
internal/api/schema_test.gointernal/ingest/worker_test.gotests/integration/timestamp_canonicalization_test.gointernal/api/errors_test.gointernal/api/router_test.gointernal/discovery/discovery_test.gointernal/api/structured_query_test.gointernal/discovery/timestamp_test.gointernal/api/ingest_test.go
internal/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/**/*_test.go: Use shared mocks frominternal/testutil/instead of creating ad-hoc mocks.
Usetestutil.MakeJWT,MakeExpiredJWT,NewTestSchemaRegistry,AssertJSONResponse, andAssertJSONContainsfor the corresponding tests.
Files:
internal/api/schema_test.gointernal/ingest/worker_test.gointernal/api/errors_test.gointernal/api/router_test.gointernal/discovery/discovery_test.gointernal/api/structured_query_test.gointernal/discovery/timestamp_test.gointernal/api/ingest_test.go
docs/src/content/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep documentation synchronized with code changes, including API, configuration, architecture, deployment, ingest/event-format, development-process, and notable-change documentation as applicable.
Files:
docs/src/content/docs/ingest-pipeline.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Record every notable code change under
[Unreleased].
Files:
CHANGELOG.md
internal/discovery/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Canonicalize parseable
DateTimeandDateTime64ingest values to RFC 3339 UTC before publishing, but fail open and publish unparseable or unresolvable values verbatim.
Files:
internal/discovery/discovery_test.gointernal/discovery/timestamp_test.gointernal/discovery/timestamp.gointernal/discovery/discovery.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-13T17:33:30.271Z
Learning: Run `make ci` locally before every push; for agents, run it in the background with output redirected to `tmp/ci.log` and no pipes.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-13T17:33:30.271Z
Learning: Never force-push or rebase a PR branch; merge `origin/main` instead.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-13T17:33:30.271Z
Learning: Create agent PRs as drafts with a Conventional Commits title of at most 72 characters, validated by `scripts/lint-pr-title.sh`.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-07-13T17:33:30.271Z
Learning: Never hand-write review or CI markers and never bypass gates with `--no-verify`; run or deliberately skip required reviewers through the provided tooling.
📚 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/schema_test.gointernal/api/errors_test.gointernal/api/router_test.gointernal/api/structured_query_test.gointernal/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/schema_test.gointernal/api/errors_test.gointernal/api/router_test.gointernal/api/structured_query_test.gointernal/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/schema_test.gointernal/ingest/worker_test.gotests/integration/timestamp_canonicalization_test.gointernal/api/errors_test.gointernal/api/router_test.gointernal/discovery/discovery_test.gointernal/api/structured_query_test.gointernal/discovery/timestamp_test.gointernal/api/ingest_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/schema_test.gointernal/ingest/worker_test.gointernal/api/ingest.gotests/integration/timestamp_canonicalization_test.gointernal/api/errors_test.gointernal/api/router_test.gointernal/testutil/testutil.gointernal/discovery/discovery_test.gointernal/api/structured_query_test.gointernal/discovery/timestamp_test.gointernal/discovery/timestamp.gointernal/api/ingest_test.gointernal/discovery/discovery.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/ingest-pipeline.mdCHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.mdAGENTS.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_test.gointernal/discovery/timestamp_test.gointernal/discovery/timestamp.gointernal/discovery/discovery.go
🪛 ast-grep (0.44.1)
tests/integration/timestamp_canonicalization_test.go
[error] 156-157: Detected a SQL query built with 'fmt.Sprintf' and passed directly to a 'database/sql' query method (Query, QueryRow, QueryContext, or QueryRowContext). Formatting user-controlled values into a query string enables SQL injection. Use parameterized queries with placeholders (e.g. 'db.Query("SELECT * FROM users WHERE id = ", id)') and pass arguments separately instead of interpolating them with 'fmt.Sprintf'.
Context: env(t).chConn.QueryRow(context.Background(),
fmt.Sprintf("SELECT v FROM %s WHERE id = ?", table), id)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-query-sprintf-go)
[warning] 92-92: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(i)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🔇 Additional comments (26)
internal/api/ingest_test.go (1)
23-36: LGTM!Also applies to: 1385-1489
internal/ingest/worker_test.go (1)
314-349: LGTM!internal/testutil/testutil.go (2)
4-13: LGTM!Also applies to: 27-39, 41-66, 70-110
68-69: 🎯 Functional CorrectnessPossible duplicate
utcRowdeclaration. The snippet showstype utcRow struct{ driver.Row }twice in a row; if that's in the source, it will fail to compile.internal/api/errors_test.go (1)
164-164: LGTM!Also applies to: 192-213
internal/api/router_test.go (1)
279-334: LGTM!Also applies to: 409-468, 470-505, 521-544, 546-570, 644-687
internal/api/schema_test.go (1)
16-33: LGTM!Also applies to: 35-56, 58-71
internal/api/structured_query_test.go (1)
32-44: LGTM!Also applies to: 76-99, 101-111, 113-123, 130-153, 155-181, 183-211, 213-247, 249-262, 282-296
docs/src/content/docs/architecture.md (3)
184-187: 🎯 Functional CorrectnessVerify the "server default since ClickHouse 26.5" claim.
See consolidated comment (anchored here) — this exact claim is repeated in
ingest-pipeline.mdandCHANGELOG.mdand could not be corroborated against current ClickHouse documentation.
116-117: LGTM!Also applies to: 171-173
1-1: 📐 Maintainability & Code Quality | ⚡ Quick winUnverified claim:
best_effortbecame ClickHouse's defaultdate_time_input_format"since 26.5". All three docs assert this specific version madebest_effortthe server default. A web search against ClickHouse's own current docs and blog turned up no confirmation of this change — multiple sources (including a ClickHouse blog post and third-party guides) still describebasicas the default with no mention of a 26.5 default flip, and the 26.5 release blog's own feature list doesn't mention this setting. Since the worker already pinsdate_time_input_format=best_effortexplicitly on every insert (confirmed intests/integration/timestamp_canonicalization_test.go'sinsertBestEffort), this claim has no functional impact, but it's a specific, falsifiable version claim repeated three times and worth double-checking before it ships as documentation fact.
docs/src/content/docs/architecture.md#L184-187: verify or soften the "server default since ClickHouse 26.5" claim (or drop the specific version if it can't be confirmed against ClickHouse's release notes).docs/src/content/docs/ingest-pipeline.md#L30-34: same claim, same fix.CHANGELOG.md#L53-54: same claim ("verified live") — if this was empirically verified against a specific ClickHouse build, consider citing that build/commit rather than a general version-default assertion that public docs don't corroborate.docs/src/content/docs/ingest-pipeline.md (1)
30-34: 🎯 Functional CorrectnessVerify the "server default since ClickHouse 26.5" claim.
See consolidated comment (anchored on
architecture.md) — this claim is repeated here and inCHANGELOG.md.CHANGELOG.md (2)
53-54: 🗄️ Data Integrity & IntegrationTwo claims here are shared with other files — see consolidated comments.
(1) The "server default since ClickHouse 26.5" claim, anchored on
architecture.md. (2) The "unresolvable zone... never a failed refresh" blanket wording, anchored onAGENTS.md.
372-372: LGTM!docs/src/content/docs/api.md (1)
220-230: LGTM!Also applies to: 553-554, 784-784
AGENTS.md (3)
68-68: 📐 Maintainability & Code QualityConfirm the fail-open wording still distinguishes the two zone-resolution tiers.
Per the earlier thread on this line, an unresolvable server-default zone deliberately fails
Refresh()(discovery.go:87-95, non-fatal only because boot wraps it inRetryRefresh), while an unresolvable column-declared zone only warns and defers to ingest. The current sentence — "an unparseable value or unresolvable zone … never a failed refresh" — still reads as one blanket claim rather than the two-tier split the maintainer said would be spelled out here. Worth a final check that this is intentionally scoped to ingest-time processing only, not a reintroduction of the earlier ambiguity. See consolidated comment (anchored here) for the matchingCHANGELOG.mdsentence.Based on learnings from the prior review thread on this file/paragraph.
124-124: LGTM!Also applies to: 382-382
1-1: 📐 Maintainability & Code QualityRe-confirm the fail-open zone wording still reflects the two-tier split from the prior review thread. A previous review on this repo established that an unresolvable server-default zone deliberately fails
SchemaRegistry.Refresh()(non-fatal only because boot wraps it inRetryRefresh), while an unresolvable column-declared zone only warns and defers to per-record ingest rejection — and the maintainer committed to rewording both AGENTS.md and the matching CHANGELOG.md sentence to state both tiers explicitly. Both sentences here still read as one blanket "never a failed refresh" claim rather than the two-tier split; worth a final check that this is intentionally scoped to ingest-time-only behavior (where no refresh occurs) and not a reintroduction of the earlier conflation.
AGENTS.md#L68-68: confirm "an unparseable value or unresolvable zone … never a failed refresh" doesn't need to carve out the server-default-zone-fails-refresh case.CHANGELOG.md#L53-54: confirm the matching "an unresolvable zone … never a failed refresh" sentence has the same scoping.Based on learnings from the prior review thread on this exact AGENTS.md paragraph and its CHANGELOG.md counterpart.
Makefile (1)
675-683: LGTM!tests/integration/timestamp_canonicalization_test.go (2)
1-153: LGTM!
154-160: 🔒 Security & PrivacyConfirm table-name provenance here
IfcreateTablecan ever incorporate caller-controlled input, thisfmt.Sprintf("SELECT v FROM %s WHERE id = ?", table)needs identifier binding likeinsertBestEffortuses for theINSERTpath.internal/discovery/discovery_test.go (2)
99-183: 📐 Maintainability & Code QualityAd-hoc
fakeConn/fakeRowsmocks likely duplicatetestutil's fake conn.
internal/testutil/testutil.goalready appears to implement an equivalent fakedriver.Conn(Query,Scan,Next,Close,Err) backingNewTestSchemaRegistry. A prior reviewer already asked to move this kind of mock into test utils. As per path instructions forinternal/**/*_test.go: "Use shared mocks frominternal/testutil/instead of creating ad-hoc mocks."This may be structurally blocked by an import cycle (
discovery_test.goispackage discovery, and iftestutilimportsdiscoveryforNewTestSchemaRegistry's signature,discoverycan't importtestutilback). Please confirm whether that cycle is real; if so, this duplication may be unavoidable without restructuringtestutil(e.g., splitting off a cycle-free sub-package), but that's worth an explicit decision rather than silently re-duplicating the mock.#!/bin/bash # Confirm testutil's fake-conn surface and whether it imports discovery (⇒ cycle risk for discovery_test.go). fd testutil.go rg -n 'package|^import|"Wave-RF/WaveHouse/internal/discovery"|func.*Query|func.*Scan|func.*Next|func.*Close|func.*Err' internal/testutil/testutil.go -A1 rg -n '^package' internal/discovery/discovery_test.goSources: Coding guidelines, Path instructions
391-411: 🩺 Stability & AvailabilityCheck StartAutoRefresh order — if it triggers an initial
Refreshbefore observingctx.Done(), this test will hit the nil conn path and panic.internal/discovery/discovery.go (1)
16-27: LGTM!Also applies to: 71-135
internal/discovery/timestamp.go (1)
36-52: LGTM!Also applies to: 89-136, 138-164, 166-223, 225-283
internal/discovery/timestamp_test.go (1)
48-103: LGTM!Also applies to: 130-194, 198-228, 230-241
|
@coderabbitai review |
✅ Action performedReview finished.
|
… bullet api.md input forms, six shapes
… bullet api.md input forms, six shapes
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