feat(tenant): resolve X-Tenant-ID before auth and thread the tenant - #593
Conversation
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (73)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**WH001 applies to every tracked Markdown file, with no carve-out**📄 CodeRabbit inference engine (AGENTS.md) Files:
**`.mdx` is never auto-fixed by the generic markdownlint rules** In MDX, leave a blank line between a JSX tag and a code fence.📄 CodeRabbit inference engine (AGENTS.md) Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: 2026-06-26T12:23:22.696ZApplied to files:
📚 Learning: 2026-06-10T15:01:09.027ZApplied to files:
📚 Learning: 2026-08-13T12:17:52.620ZApplied to files:
📚 Learning: 2026-05-23T01:23:59.268ZApplied to files:
🪛 ast-grep (0.45.3)internal/config/persistence.go[warning] 28-28: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first. (log-injection-request-data-concat-go) 🪛 LanguageTooldocs/src/content/docs/sdk/reference.md[style] ~59-~59: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional. (EN_REPEATEDWORDS_USUALLY) [style] ~59-~59: Consider an alternative for the overused word “exactly”. (EXACTLY_PRECISELY) docs/src/content/docs/sdk/streaming.md[style] ~132-~132: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) AGENTS.md[grammar] ~48-~48: Please add a punctuation mark at the end of paragraph. (PUNCTUATION_PARAGRAPH_END) CHANGELOG.md[typographical] ~13-~13: Consider using an em dash in dialogues and enumerations. (DASH_RULE) [style] ~13-~13: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read. (TOO_LONG_SENTENCE) docs/src/content/docs/deployment.md[style] ~335-~335: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~360-~360: Consider using a more formal/concise alternative here. (OTHER_THAN) docs/src/content/docs/settings-directory.mdx[typographical] ~192-~192: Consider using an em dash in dialogues and enumerations. (DASH_RULE) docs/src/content/docs/architecture.md[style] ~99-~99: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) 🔇 Additional comments (73)
📝 SummarySummary by CodeRabbit
WalkthroughChangesThe change adds validated tenant identifiers, registry-backed tenant resolution before authentication, tenant-aware API and asynchronous paths, default Tenant routing and application wiring
Default logging migration
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Client
participant TenantMW
participant Registry
participant AuthMW
participant TenantAwareHandler
Client->>TenantMW: Send X-Tenant-ID
TenantMW->>Registry: Resolve tenant store
Registry-->>TenantMW: Return store or error
TenantMW->>AuthMW: Attach store to request context
AuthMW->>TenantAwareHandler: Authenticate and invoke route
TenantAwareHandler->>Registry: Read tenant-specific policy and settings
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 58.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 271 functions across 50 files. (23 skipped: 13 unsupported, 10 over the file limit.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
documentation
|
📚 Docs preview is live → https://79b7db19-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit dc0b3a3 in the Show a line coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Resolve policy for each replayed event. · hub.go:454
internal/stream/hub.go:454
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | ⚡ Quick winAuthorization Bypass
Reachability: External
Exploitability: Difficult
CWE: CWE-863 — Incorrect AuthorizationResolve policy for each replayed event.
PolicySourcerequires reads per event so a settings reload applies to the next event.ReplayProjectorcaptures one policy before creating its reusable closure, so a revocation during gap-fill does not affect subsequent replayed events.Proposed fix
func (h *Hub) ReplayProjector(role string, sub *Subscriber) func(raw []byte) []Frame { - p, filter := h.snapshotPolicy() var colSpecs map[string]policy.ColumnSpec specsFor := "" lastSig := "" return func(raw []byte) []Frame { + p, filter := h.snapshotPolicy() ev := newEventView(raw)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: b8d9e5d6-9cea-4f5b-b25f-f3aeb746dfce
📒 Files selected for processing (67)
.github/labeler.ymlAGENTS.mdCHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/development.mddocs/src/content/docs/sdk/index.mdxinternal/api/boot_chain_test.gointernal/api/dlq.gointernal/api/dlq_test.gointernal/api/errors.gointernal/api/errors_test.gointernal/api/ingest.gointernal/api/ingest_seams_test.gointernal/api/ingest_test.gointernal/api/main_test.gointernal/api/pipes.gointernal/api/pipes_test.gointernal/api/router.gointernal/api/router_test.gointernal/api/settings.gointernal/api/settings_test.gointernal/api/stream_test.gointernal/api/structured_query.gointernal/api/structured_query_test.gointernal/api/tenant.gointernal/api/tenant_helpers_test.gointernal/api/tenant_test.gointernal/app/app.gointernal/app/app_test.gointernal/app/wire.gointernal/auth/auth.gointernal/auth/auth_test.gointernal/auth/main_test.gointernal/chconn/chconn.gointernal/config/persistence.gointernal/config/persistence_test.gointernal/discovery/discovery.gointernal/discovery/discovery_test.gointernal/discovery/main_test.gointernal/discovery/timestamp.gointernal/discovery/timestamp_test.gointernal/ingest/main_test.gointernal/ingest/sweeper.gointernal/ingest/sweeper_test.gointernal/ingest/worker.gointernal/ingest/worker_test.gointernal/mq/embedded.gointernal/mq/embedded_test.gointernal/mq/main_test.gointernal/settings/main_test.gointernal/settings/registry.gointernal/settings/registry_test.gointernal/settings/store.gointernal/settings/store_test.gointernal/settings/watch.gointernal/stream/hub.gointernal/stream/hub_test.gointernal/stream/roweval_test.gointernal/stream/tenant_test.gointernal/tenant/tenant.gointernal/tenant/tenant_test.gointernal/testutil/logtest/logtest.gointernal/testutil/logtest/logtest_test.gointernal/testutil/testutil.gotests/integration/boot_resilience_test.gotests/integration/query_limits_test.go
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Coverage
- GitHub Check: E2E tests
🧰 Additional context used
📓 Path-based instructions (3)
only `internal/mq` imports NATS/JetStream (`github.com/nats-io/…`)
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/mq/main_test.gointernal/mq/embedded.gointernal/mq/embedded_test.go
WH001 applies to every tracked Markdown file, with no carve-out
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/development.mddocs/src/content/docs/architecture.mddocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/api.mdAGENTS.mdCHANGELOG.md
**`.mdx` is never auto-fixed by the generic markdownlint rules**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/sdk/index.mdx
🧠 Learnings (4)
📚 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/tenant/tenant_test.gointernal/app/app_test.gointernal/api/tenant_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/tenant_test.go
📚 Learning: 2026-08-11T21:55:53.726Z
Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/stream_test.go:159-176
Timestamp: 2026-08-11T21:55:53.726Z
Learning: For Go files in this repository, do not report direct type assertions solely because the `forcetypeassert` rule is commented out in `.golangci.yml`. Only flag a type assertion when there is an independent correctness, safety, or maintainability issue.
Applied to files:
internal/api/tenant.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.md
🪛 ast-grep (0.45.3)
internal/config/persistence.go
[warning] 28-28: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: slog.Error(kind+" init failed", fields...)
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🪛 LanguageTool
AGENTS.md
[grammar] ~48-~48: Please add a punctuation mark at the end of paragraph.
Context: ...th a tenant.ID and their getters take it ## Key Design Decisions The invariant...
(PUNCTUATION_PARAGRAPH_END)
CHANGELOG.md
[style] ~13-~13: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...ons. --> ## Unreleased ### Added - Requests resolve to a tenant before authentication, and the tenant is threaded through every settings read (internal/tenant/ (new, + tests), internal/settings/registry.go (new, + tests), internal/api/tenant.go (new, + tests), internal/api/{router,ingest,structured_query,pipes}.go, internal/ingest/{worker,sweeper}.go, internal/stream/hub.go, internal/discovery/discovery.go, internal/app/{app,wire}.go): story 1 of the multi-tenant epic (#583), with no behavior change for a deployment that sends no tenant header. internal/tenant defines the id — a va...
(TOO_LONG_SENTENCE)
🔇 Additional comments (38)
internal/api/errors.go (1)
44-44: LGTM!Also applies to: 59-59, 82-82, 103-103
internal/auth/auth.go (1)
69-70: LGTM!Also applies to: 79-81, 116-117, 121-121, 145-147, 159-159, 203-209, 243-256, 276-279
internal/config/persistence.go (1)
24-24: LGTM!Also applies to: 29-29, 42-42, 50-50, 56-56, 58-58, 64-64
internal/settings/watch.go (1)
6-6: LGTM!Also applies to: 54-55, 93-93
internal/config/persistence_test.go (1)
13-13: LGTM!Also applies to: 18-23, 26-26, 41-44, 55-58, 67-72, 81-83, 89-95, 106-108
internal/testutil/logtest/logtest.go (1)
1-59: LGTM!internal/testutil/logtest/logtest_test.go (1)
1-29: LGTM!tests/integration/query_limits_test.go (1)
20-20: LGTM!Also applies to: 95-102, 108-110
internal/auth/auth_test.go (1)
17-17: LGTM!Also applies to: 41-45, 48-53, 321-321, 333-333, 490-490, 508-508, 518-524, 541-542, 554-555, 563-564, 571-572, 631-631, 662-662
internal/stream/roweval_test.go (1)
10-10: LGTM!Also applies to: 61-61, 89-89, 105-105
internal/chconn/chconn.go (1)
79-80: LGTM!internal/discovery/discovery.go (1)
11-11: LGTM!Also applies to: 181-186, 194-196, 200-200, 243-243, 302-302, 310-310, 435-435, 444-446
internal/discovery/timestamp.go (1)
4-4: LGTM!Also applies to: 99-99, 107-107
internal/ingest/sweeper.go (1)
10-10: LGTM!Also applies to: 24-28, 31-32, 34-34, 37-37, 57-57, 61-61, 64-64
internal/ingest/worker.go (1)
22-22: LGTM!Also applies to: 67-74, 134-135, 183-183, 240-240, 279-279, 464-464, 469-470, 476-477, 490-491, 497-498, 569-569, 577-581, 683-683, 696-696, 711-711, 726-727, 735-736, 766-766, 776-777
internal/mq/embedded.go (1)
19-24: LGTM!Also applies to: 27-28, 31-32, 35-36, 39-40, 43-44, 82-85, 102-102, 133-133, 336-336, 445-451
internal/settings/store.go (1)
28-28: LGTM!Also applies to: 44-45, 73-90
internal/discovery/discovery_test.go (1)
16-18: LGTM!Also applies to: 53-56, 81-81, 114-114, 134-134, 155-155, 357-357, 365-365, 379-379, 578-578, 613-614, 668-668, 741-741
internal/ingest/worker_test.go (1)
31-31: LGTM!Also applies to: 38-38, 123-123, 137-137, 155-155, 272-272, 299-300, 306-306, 889-889, 1053-1053, 1317-1317, 1485-1485, 1506-1506, 1599-1600, 1651-1652
internal/testutil/testutil.go (1)
17-17: LGTM!Also applies to: 31-31
tests/integration/boot_resilience_test.go (1)
20-20: LGTM!Also applies to: 69-69, 91-91
internal/api/main_test.go (1)
1-15: LGTM!internal/discovery/main_test.go (1)
1-15: LGTM!internal/discovery/timestamp_test.go (1)
11-12: LGTM!Also applies to: 97-97, 134-134, 159-159, 190-190, 256-256, 278-278, 304-304
internal/ingest/main_test.go (1)
1-15: LGTM!internal/ingest/sweeper_test.go (1)
10-10: LGTM!Also applies to: 23-23, 40-40, 54-54, 66-66
internal/mq/embedded_test.go (1)
15-19: LGTM!Also applies to: 293-293, 511-511
internal/mq/main_test.go (1)
1-14: LGTM!internal/settings/main_test.go (1)
1-13: LGTM!internal/settings/store_test.go (1)
21-21: LGTM!Also applies to: 91-91, 95-95, 125-125
internal/auth/main_test.go (1)
1-15: LGTM!docs/src/content/docs/development.md (1)
350-350: LGTM!docs/src/content/docs/sdk/index.mdx (1)
380-380: LGTM!internal/settings/registry_test.go (1)
1-24: LGTM!internal/tenant/tenant.go (1)
1-50: LGTM!internal/tenant/tenant_test.go (1)
1-53: LGTM!internal/app/app_test.go (1)
27-27: LGTM!Also applies to: 147-175
internal/app/wire.go (1)
28-32: LGTM!Also applies to: 57-62, 78-92, 157-157, 184-184, 227-230, 253-257, 306-306, 319-319, 357-361, 421-421, 480-483, 496-498, 509-517
github-project-automation
Bot
moved this from Backlog
to In review
in WaveHouse Task Board
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Fix the malformed go:embed wording. · AGENTS.md:46
AGENTS.md:46
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the malformed
go:embedwording.The text renders as "
go:embedded seed". Use “embedded seed” or "go:embed-embedded seed”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 9b449254-5034-401c-b03b-345219fc455d
📒 Files selected for processing (17)
AGENTS.mdCHANGELOG.mddocs/src/content/docs/access-control.mdxdocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/development.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/index.mdxdocs/src/content/docs/sdk/reference.mddocs/src/content/docs/sdk/streaming.mddocs/src/content/docs/settings-directory.mdxinternal/api/structured_query.gointernal/api/tenant.gointernal/api/tenant_test.gointernal/app/wire.gointernal/stream/hub.gointernal/stream/hub_test.go
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
In MDX, leave a blank line between a JSX tag and a code fence.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/settings-directory.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/access-control.mdxdocs/src/content/docs/sdk/index.mdx
🧠 Learnings (4)
📓 Common learnings
Learnt from: taitelee
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-18T14:35:32.276Z
Learning: In `internal/api/tenant.go`, `opsStore` must strictly parse the complete `r.URL.RawQuery` with `url.ParseQuery`. If parsing fails, it returns `400` with `invalid ?tenant: malformed query string`. This intentional behavior applies to admin-only operations pipe reads, even when the malformed pair is not identifiable as the `tenant` parameter.
📚 Learning: 2026-08-13T12:17:52.620Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 470
File: docs/src/content/docs/reverse-proxy.mdx:137-144
Timestamp: 2026-08-13T12:17:52.620Z
Learning: For Wave-RF/WaveHouse documentation, verify claims about implementation control flow against the authoritative implementation source (for example, internal/auth/auth.go) rather than relying solely on docs/** content. Documentation may lag behind or paraphrase behavior, so control-flow claims should be confirmed in source code.
Applied to files:
docs/src/content/docs/settings-directory.mdxdocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/access-control.mdx
📚 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/tenant_test.go
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/stream/hub_test.go
🪛 LanguageTool
docs/src/content/docs/sdk/streaming.md
[style] ~132-~132: Since ownership is already implied, this phrasing may be redundant.
Context: ...ed more of on this path; see Supplying your own fetch. ...
(PRP_OWN)
docs/src/content/docs/sdk/reference.md
[style] ~59-~59: Consider an alternative for the overused word “exactly”.
Context: ...roxy. That silent-downgrade behavior is exactly why auth is re-read on every connecti...
(EXACTLY_PRECISELY)
CHANGELOG.md
[style] ~13-~13: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ...ons. --> ## Unreleased ### Added - Requests resolve to a tenant before authentication, and the tenant is threaded through every settings read (internal/tenant/ (new, + tests), internal/settings/registry.go (new, + tests), internal/api/tenant.go (new, + tests), internal/api/{router,ingest,structured_query,pipes}.go, internal/ingest/{worker,sweeper}.go, internal/stream/hub.go, internal/discovery/discovery.go, internal/app/{app,wire}.go): story 1 of the multi-tenant epic (#583), with no behavior change for a deployment that sends no tenant header. internal/tenant defines the id — a va...
(TOO_LONG_SENTENCE)
docs/src/content/docs/architecture.md
[typographical] ~99-~99: Consider using an em dash in dialogues and enumerations.
Context: - hub.go — Hub, the event fan-out. ...
(DASH_RULE)
[style] ~99-~99: Since ownership is already implied, this phrasing may be redundant.
Context: ...th's: ReplayProjector tracks drift in its own state and the two are not reconciled ([...
(PRP_OWN)
🔇 Additional comments (16)
AGENTS.md (1)
32-32: LGTM!Also applies to: 47-48, 78-81, 444-445
docs/src/content/docs/development.md (1)
350-350: LGTM!Also applies to: 471-471, 498-498
CHANGELOG.md (1)
25-26: LGTM!Also applies to: 65-66
docs/src/content/docs/sdk/reference.md (1)
33-33: LGTM!Also applies to: 59-59
docs/src/content/docs/access-control.mdx (1)
363-363: LGTM!docs/src/content/docs/sdk/streaming.md (1)
130-130: LGTM!Also applies to: 136-136
internal/api/tenant.go (1)
7-7: LGTM!Also applies to: 98-108
internal/api/tenant_test.go (1)
58-58: LGTM!Also applies to: 182-183, 194-196, 202-202
docs/src/content/docs/api.md (1)
61-61: LGTM!Also applies to: 176-176, 642-642, 784-784, 788-788
internal/stream/hub.go (1)
446-450: LGTM!Also applies to: 478-480
internal/stream/hub_test.go (1)
13-13: LGTM!Also applies to: 866-886
docs/src/content/docs/architecture.md (1)
71-71: LGTM!Also applies to: 80-80, 93-93, 99-99, 107-107, 167-167, 185-192, 207-208, 301-302
docs/src/content/docs/reverse-proxy.mdx (1)
197-197: LGTM!docs/src/content/docs/sdk/index.mdx (1)
338-338: LGTM!Also applies to: 380-380
internal/api/structured_query.go (1)
38-40: LGTM!internal/app/wire.go (1)
73-74: LGTM!Also applies to: 79-79, 87-94, 360-364
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
EricAndrechek
added
the
area/tenant
|
@coderabbitai review |
|
…c to the pipe reads
|
@coderabbitai full review |
✅ Action performedFull review finished. |
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Reviewed the full diff at f1bbc279 against epic #583's story 1, plus both CodeRabbit rounds and CI.
Scope first, because that's where the defects cluster. Story 1's checklist is delivered essentially in full: internal/tenant, the middleware (validate → default 0 → resolve → 404 → context), running before auth, handlers taking the store as an explicit argument, the getters threaded through api/ingest/discovery/async, wired to the default tenant in wire.go, and the slog cleanup owed from #586. Nothing material is missing.
Three things went beyond it, and every merge-relevant defect is in one of them:
?tenant=on the ops pipe reads — story 2 scope per the epic, and the PR body says as much. The one MUST-level bug (bearerTokendefeatingopsStore's strict parse) lives entirely here. Worth considering dropping it so story 2 lands it together with the reload route, thebearerTokenfix, and a router-level regression test.Vary: X-Tenant-ID— unlisted scope from round two. The code is right —AddnotSet, and the new test genuinely pins the composition — but the sentence documenting it overstates the guarantee.settings.Registry— story 2 scope, but unavoidable, since story 1's middleware must resolve against something. Shipping it as a stub is why no test can catch a wrong-store derivation.
One epic gap I'd like settled before merge: no story in #583 owns the admin-authorization axis. wire.go:63 binds RequireAdmin's policy source to tenant 0's store, so tenant 0's policies.json becomes a fleet-wide authorization authority the moment a second tenant exists. Story 9 covers the JWKS verifier only, not the policy source. Detail on that line.
Everything else is latent until story 3/5/6, or a docs fix. Full context on each comment, including which story owns it.
…iss, move tenant docs to deployment
|
@coderabbitai full review |
✅ Action performedFull review finished. |
github-project-automation
Bot
moved this from In review
to Done
in WaveHouse Task Board
…598) ## Summary Story 2 of the multi-tenant epic: a settings directory can hold one folder per tenant, and the registry owns every reload. No behavior change for a directory that holds the four files — the same findings byte for byte, the same boot refusal, the same keep-previous on a rejected reload, the same watcher and `SIGHUP`, the same `wavehouse validate` exit codes. - `settings.Validate` reads the shape off the root: any of the four file names makes it flat (`ValidateDir`, the old `Validate`, untouched); otherwise a folder makes it nested — folder names through the tenant-id grammar, each folder through `ValidateDir`, findings led by the folder (`acme/policies.json`). The shapes never mix. A folder whose name is not a tenant id is reported and skipped; an entry that cannot be stat'ed (a dangling symlink) is a finding about the root. - `settings.Open` returns the `Registry`, which owns `Reload`, `ReloadTenant`, the `AfterAdopt` hooks and the watcher; `Store` is a passive holder. A nested directory fails closed per tenant, at boot and on reload alike: a rejected folder stops its tenant (a bare `503` on tenant routes, which resolve before authentication), the rest carry on, and there is no previous-snapshot fallback. A whole-tree reload mirrors the folders and names a removed tenant in the log. A finding about the directory itself refuses boot and rejects a reload whole. A nested directory gets no watcher (`Registry.Watch` refuses one); `SIGHUP` reloads the whole tree in both shapes. - `POST /v1/ops/settings/reload` and `GET /v1/ops/pipes[/{name}]` take an optional `?tenant=`, parsed strictly (`400` for a query that does not parse or an empty, repeated or malformed id; `404` unknown; absent means the whole tree on the reload and tenant `0` on the reads). It lands with the `auth.bearerToken` fix deferred from #593 — the `?token=` strip no longer repairs a query that does not parse — pinned through `NewRouter` with the real authenticator. - Over a nested directory the `/v1/ops/*` gate admits the operator key alone and a token admin gets `403`; `NewRouter` decides that from the registry's shape, whatever policy source was wired. Boot warns when a nested directory has no operator key, since `SIGHUP` is then the only reload. - The resources a process still has one of (ClickHouse connection, dedupe store, MQ budget, verifier, CORS list) follow the settings tenant `0` last adopted, and their hooks run only when tenant `0` is adopted, so a rejected or removed `0` folder leaves them as they were. The keepalive wheel runs at the shortest `keepalive_interval` among the tenants being served (#597). - SDK: `wh.pipes.list()`, `wh.pipes.get()` and `wh.settings.reload()` take a `tenant` option (`OpsRequestOptions`), sent as `?tenant=`. - Decided with Eric on 2026-09-21, not yet in the #583 body: the rejected tenant's `503` is generic; the reload body is unchanged and a `422` can mean adopted in part; a failure about the directory itself rejects the reload whole; one shared keepalive wheel, with #597 tracking per-tenant keepalive; a badly named folder is skipped; a whole-tree reload mirrors the folders; `Dependencies.PolicySource` stays and is ignored when the registry is nested. ## Test plan - [x] `make ci` passes locally (unit, integration, e2e, coverage gates) - [x] Flat root: `Validate` equals `ValidateDir` across six root shapes, findings and document alike - [x] Nested: fail closed per tenant at boot and on reload; recovery by reload into the same `*Store`; folder mirroring; a root-level failure changing nothing (four shapes); a shape change refused in both directions; symlinked and dangling tenant folders; an unreadable folder - [x] `TestNewRouter_HandlersReceiveTheRequestTenantsStore`: `assert.Same` on the store each handler's getters received, two tenants alternating through one router (fails under two wrong-store mutations) - [x] Nested ops gate matrix (2 shapes × 4 callers × 3 routes); the strict `?tenant=` table on all three routes; `TestNewRouter_MalformedTenantSurvivesTheTokenStrip` (fails against the pre-fix `bearerToken`) - [x] `internal/app`: a nested boot without a `0` folder; the operator reloading one tenant by name; the process-wide resources following tenant `0` through another tenant's reload, a rejected `0` folder and a removed one (dedupe, MQ budget, CORS); the no-operator-key boot warning - [x] SDK unit tests, and e2e through the real server (`tenant: "0"`, unknown `404`, malformed and empty `400`) - [ ] Manual: over `acme/` + `globex/`, `curl -H 'X-Tenant-ID: acme' /v1/pipes/x` → `404 pipe not found`; no header → `404 unknown tenant: 0` ## Follow-ups - A second tenant is not isolation yet (stories 5, 6, 9): one ClickHouse, one verifier (a token is accepted under any tenant's header), and MQ topics that carry no tenant — so `/v1/stream` delivery is by table alone and is authorized by tenant `0`'s policy, and the most permissive tenant's `default_role` is the effective floor. - A nested directory that serves no tenant `0` boots unconfigured: no ClickHouse address, `/livez` degraded, the MQ byte budget uncapped (inert, since nothing publishes). Until story 6. - A lost tenant `0` (its folder rejected or removed by a reload): the five settings read through `defaultSetting` keep their last adopted values, but the async paths read tenant `0` as unset — the hub reads no policy (every stream subscriber's rows withheld), the sweeper's gap window is zero (gap-fill history purged), the DLQ switch reads on — and `perTenant` logs an error per event. Story 3 owns the async paths' miss semantics; left exactly as story 1 left them. - A whole-tree reload can catch a folder halfway through being written and reject its tenant until a later reload (story 3, where whole-tree reloads become routine). - #597: per-tenant keepalive. The wheel is also recomputed only after an adoption. - `GET /v1/ops/schema`, `POST /v1/ops/schema/refresh`, `GET /v1/ops/dlq/stats` and `POST /v1/ops/query` ignore `?tenant=` (stories 5 and 6). - The operator key is stamped with tenant `0`'s admin role name (story 9). The dedupe store keys on the event id alone (story 7). The pre-auth `400`/`404`/`503` split distinguishes tenant ids, `/v1/health` included (story 3). - A `settings.dir` pointed at a directory of unrelated subfolders boots as nested with nothing served; `wavehouse validate` exits 1 for a nested directory with one bad folder where boot would still serve the rest. - Docs for story 11: creating a nested directory by hand, a runnable control-plane loop, that a non-`0` folder must still carry every required key, and `config.yaml`'s flat-only `settings:` comment. - #583 body: amend story 3's first sentence (a whole-tree reload already mirrors the folders) and add the decisions above to the Decisions section. ## Related Issues Part of #583 (story 2). Lands the `?tenant=` and `bearerToken` work deferred from #593. <!-- Checklist for the author (not kept in the squash commit message): - `make ci` passes locally - Docs updated per AGENTS.md "Documentation & Consistency Sync" rules - CHANGELOG.md [Unreleased] entry added - Tests cover new / changed behavior (70 % minimum, 80 %+ preferred) The PR title is the squash commit subject — use Conventional Commits (`feat:`, `fix:`, `docs:`, `refactor:`, `test:`, `chore:`, `ci:`, `deps:`, `build:`, `perf:`, `revert:`, `style:`). The PR body below is the squash commit message, so keep it tight. -->
2 participants
Summary
Story 1 of the multi-tenant epic: every request resolves to a tenant before authentication, and the tenant is threaded through every settings read. No behavior change for a deployment that sends no tenant header — only tenant
0exists.internal/tenant:ID(a validated string — letters, digits,_,-, ≤ 64 bytes, safe as a folder name and an MQ subject token),Default = "0",Header = "X-Tenant-ID". Imports nothing from the repo.settings.Registry(For(id)) over the one storesettings.Openadopts, keyed0. Open, reload and the watcher are untouched.api.TenantMWruns ahead of auth on every/v1route outside/v1/ops/*: absent or empty header is tenant0, malformed or repeated is400, unknown is404; the resolved*settings.Storerides the request context. Handlers read it once and pass it down — the ingest, structured-query and pipe getters take the store as a parameter, and a tenant route reached without one answers500rather than fall back to a tenant. The async paths (worker, sweeper, hub, schema registry) are constructed with atenant.IDand their getters take it, wired withtenant.Defaultininternal/app./version, the metrics path and/v1/ops/*stay tenant-exempt; the admin pipe reads serve the default tenant.X-Tenant-IDjoins the CORS allow-headers list so browser clients can send it.*slog.Loggeranymore (mq, the handlers, auth, discovery, sweeper, worker, plussettings.Open,chconn.Openand theconfigdata-dir helpers); call sites use the context-aware calls. Tests reach log output through the newinternal/testutil/logtest(SilenceinTestMain,Capturefor log-asserting tests, which run serially).testutil.NopLoggeris removed.The tenant work and the logger cleanup are separate commits; the rest are review rounds.
Test plan
make cipasses locally (unit, integration, e2e, coverage gates; Go total 92.8%)internal/tenant: grammar table test (default, 19-digit id, length cap, dots, slashes, wildcards, non-ASCII)api.TenantMW: no header / empty /0/ unknown404/ malformed400/ repeated400; tenant resolves beforeAuthMW; exempt routes ignore the header; tenant-route handlers500without a resolved storeinternal/app: the wired registry answers404/400end to end; ops ignores the headerlogtest.Capturecurl -H 'X-Tenant-ID: acme' /v1/health→404; no header → as beforeFollow-ups
?tenant=on the reload route and the pipe reads, with thebearerTokenquery-rewrite fix, the per-tenant admin gate, and the SDK option to send it.0exists; story 3 defines a removed tenant's semantics.chconn-backed getters (story 6) are not threaded.Related Issues
Part of #583 (story 1). Finishes the logger cleanup deferred from #586.