Repository navigation
fix(pipes): escape non-scalar param values - #330
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:
📝 WalkthroughWalkthroughThis PR hardens named pipe parameter binding: it enforces declared ParamDef.Type, rewrites formatParamValue to escape strings and render arrays as safe IN lists (rejecting objects and empty arrays), updates BindParams to validate and abort on errors, and adds unit, handler, E2E tests plus docs and changelog updates. ChangesPipe parameter security and type enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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://f9cefb02-wavehouse-docs.wave-rf.workers.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 95638e1b-fa68-45ef-a5f1-71e7be4cd78d
📒 Files selected for processing (8)
CHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/pipes.mdinternal/api/pipes_test.gointernal/pipes/pipes.gointernal/pipes/pipes_test.gotests/e2e/sdk/admin.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (9)
tests/e2e/sdk/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
tests/e2e/sdk/**/*.test.ts: E2E tests via SDK: The TypeScript SDK is the primary E2E test harness. Tests intests/e2e/sdk/exercise the full pipeline and validate backend + SDK correctness viamake test-e2e
Per-suite table isolation in E2E: Each test file owns its own ClickHouse tables —<table>_<suite>, generated fromtests/e2e/sdk/tables.ts. Get names viaconst T = suiteTables("<suite>")
Files:
tests/e2e/sdk/admin.test.ts
docs/src/content/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/**/*.{md,mdx}: Author Mermaid diagrams vertically so they fit the page at legible size; default to top-down (flowchart TB/TD); reserveLR/RLfor short chains
Keep Mermaid node labels short; use<br/>for a second line; lean on semantic node classes (wh,win,pain,fail,infra,neutral,store,client) and CSS variables
Diagrams are click-to-zoom on the site; fine detail is recoverable, but ship legible inline diagrams
Never sit two large diagrams side-by-side; wrap comparisons in<div class="diagram-pair">…</div>for vertical stacking
Files:
docs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/pipes.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use Go 1.26 with strict formatting viagofumpt, enforced by CI
Use structured logging withlog/slog(JSON handler)
Error handling: Return errors, don't panic. Wrap withfmt.Errorf("context: %w", err)
No global state: Dependencies are passed explicitly (constructor injection)
Files:
internal/api/pipes_test.gointernal/pipes/pipes.gointernal/pipes/pipes_test.go
internal/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/api/**/*.go: Use Chi v5 for HTTP routing
Handler error responses must stay in sync with docs (api.mderror tables)
Files:
internal/api/pipes_test.go
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Package naming: Lowercase, single word (or abbreviated).
internal/enforces module privacy
Files:
internal/api/pipes_test.gointernal/pipes/pipes.gointernal/pipes/pipes_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Table-driven tests: Usetests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases
Shared mocks ininternal/testutil/: UseMockPublisher,MockCache,MockDeduplicator,MockSubscriberinstead of ad-hoc mocks
Schema helpers: Usetestutil.NewTestSchemaRegistry(tables)ordiscovery.NewSchemaRegistryFromMap(tables)for schema-aware tests
Files:
internal/api/pipes_test.gointernal/pipes/pipes_test.go
internal/api/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Response assertions: Use
testutil.AssertJSONResponse(t, rec, status, expected)andtestutil.AssertJSONContains(t, rec, status, substring)for HTTP handlers
Files:
internal/api/pipes_test.go
internal/pipes/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Named query pipes: fail-closed (security). Per-pipe
allowed_rolesis the only execute-path gate viapolicy.RoleAllowed: exact allowlist (no"*"), admin always passes, empty role authorizes nobody, noallowed_roles→ admin-only
Files:
internal/pipes/pipes.gointernal/pipes/pipes_test.go
internal/pipes/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Pipes helpers: Use
pipes.NewMemoryStore(queries...)for in-memory pipes testing without NATS
Files:
internal/pipes/pipes_test.go
🧠 Learnings (4)
📚 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/api.mddocs/src/content/docs/architecture.mdCHANGELOG.mddocs/src/content/docs/pipes.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/pipes_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/pipes_test.go
📚 Learning: 2026-05-20T20:30:15.808Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:15.808Z
Learning: For WaveHouse pipes authorization allowlist checks, fix the empty-role fail-open behavior by (1) removing any outer guard that prevents allowlist evaluation when the incoming `role` is `""` (e.g., don’t short-circuit with `if role != "" { ... }`), and (2) during allowlist scanning, ensure only non-empty allowlist entries can match—e.g., require `ar != "" && ar == role` (so a malformed allowlist like `["" ]` cannot grant access to an empty incoming role via `"" == ""`).
Applied to files:
internal/api/pipes_test.go
🔇 Additional comments (3)
docs/src/content/docs/architecture.md (1)
138-138: LGTM!docs/src/content/docs/pipes.md (2)
49-49: LGTM!
175-176: LGTM!
github-project-automation
Bot
moved this from Backlog
to In review
in WaveHouse Task Board
Code Coverage OverviewLanguages: Go GoThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/pipes/pipes.go (1)
242-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBoolean query-string values are accepted but rendered with string quoting.
validateParamTypeaccepts"true"/"false"spellings for declared booleans, butformatParamValue’s string branch only special-cases declared"string"and falls back to quoting for booleans ('true'/'false'). This breaks the declared boolean contract for query-string inputs and can produce incorrect predicates.Suggested fix
case string: + if declaredType == "boolean" { + b, err := strconv.ParseBool(val) + if err != nil { + return "", fmt.Errorf("expected boolean, got string") + } + if b { + return "1", nil + } + return "0", nil + } // A numeric-looking string renders bare (so `?limit=100` from a query // string works as a number), unless the parameter is declared a string — // then it is always quoted, honoring the declared type. if declaredType != "string" { if _, err := strconv.ParseFloat(val, 64); err == nil { return val, nil } }docs/src/content/docs/pipes.mdx (1)
179-180:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a separate row for the empty array error message.
The error table shows two rows, but line 180 conflates two distinct error cases in its description: "a JSON object, or an empty array." These produce different error messages:
- Objects →
{"error":"parameter \"x\": unsupported parameter type object"}- Empty arrays →
{"error":"parameter \"x\": array parameter must not be empty"}The
api.mderror table correctly separates these into three rows (lines 490–492). This table should match.📝 Suggested addition
| 400 | `{"error":"parameter \"x\": expected number, got array"}` | A value violates the parameter's declared `type` | -| 400 | `{"error":"parameter \"x\": unsupported parameter type object"}` | A non-scalar with no SQL form (a JSON object, or an empty array) | +| 400 | `{"error":"parameter \"x\": unsupported parameter type object"}` | A JSON object has no SQL literal form | +| 400 | `{"error":"parameter \"x\": array parameter must not be empty"}` | An empty array — it would render as the invalid `IN ()` |
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ce378aa-99f0-48bd-9b7f-b0684f65b2d8
📒 Files selected for processing (6)
CHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/pipes.mdxinternal/pipes/pipes.gointernal/pipes/pipes_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Docs preview
- GitHub Check: E2E tests
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (6)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Markdown files must pass markdownlint style checks (enforce via
make lint)
Files:
docs/src/content/docs/architecture.mddocs/src/content/docs/api.mdCHANGELOG.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Markdown documentation files must pass docs-prose review (accuracy vs. code, examples, clarity, completeness); scope is canonical set from
scripts/docs-prose.shexcluding.claude/**,.github/**,CHANGELOG.md,AGENTS.md,CLAUDE.md,*.draft.md,*.old.md
Files:
docs/src/content/docs/architecture.mddocs/src/content/docs/api.mdCHANGELOG.mddocs/src/content/docs/pipes.mdx
docs/src/content/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
When authoring Mermaid diagrams in Markdown: author vertically (top-down default,
flowchart TB/TD) to fit page column (~46–58rem); keep node labels short with<br/>for multi-line; use semantic node classes (wh,win,pain,fail,infra,neutral,store,client); never sit two large diagrams side-by-side without<div class="diagram-pair">wrapper
Files:
docs/src/content/docs/architecture.mddocs/src/content/docs/api.mddocs/src/content/docs/pipes.mdx
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Go code must use strict formatting enforced bygofumpt
Use structured logging withlog/slog(JSON handler) in Go code
Use Chi v5 for HTTP routing in Go code
Return errors from Go functions, never panic; wrap withfmt.Errorf("context: %w", err)
Pass dependencies explicitly via constructor injection in Go; no global state
Go code must passgo fmt(viagofumpt) andgoimports(enforced by CI'smake verify)
Go code must passgolangci-lintwith v2.11.4 (auto-installed to.bin/on first run; enforced by CI'smake verify)
Go code must passgovulncheck ./...for vulnerability scanning (enforced by CI'smake verify)
Gointernal/packages must not be imported outsidecmd/or otherinternal/packages (module privacy enforced by the filesystem)
Files:
internal/pipes/pipes.gointernal/pipes/pipes_test.go
internal/*/**.go
📄 CodeRabbit inference engine (AGENTS.md)
Use lowercase, single-word package names in Go;
internal/enforces module privacy
Files:
internal/pipes/pipes.gointernal/pipes/pipes_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Use table-driven tests in Go withtests := []struct{ name string; ... }andt.Run(tt.name, ...)for multiple scenarios
Use shared mocks frominternal/testutil/(MockPublisher, MockCache, MockDeduplicator, MockSubscriber) instead of ad-hoc mocks in Go tests
Usetestutil.MakeJWT(t, claims)andtestutil.MakeExpiredJWT(t, claims)for auth tests in Go
Usetestutil.NewTestSchemaRegistry(tables)ordiscovery.NewSchemaRegistryFromMap(tables)for schema-aware tests in Go
Usepolicy.NewMemoryStore(p)for in-memory policy testing without NATS in Go
Usepipes.NewMemoryStore(queries...)for in-memory pipes testing without NATS in Go
Usetestutil.AssertJSONResponse(t, rec, status, expected)andtestutil.AssertJSONContains(t, rec, status, substring)for HTTP response assertions in Go tests
Go test files must be named*_test.goand placed in the same package as the code under test
Files:
internal/pipes/pipes_test.go
🧠 Learnings (1)
📚 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.mddocs/src/content/docs/api.mdCHANGELOG.md
🪛 LanguageTool
docs/src/content/docs/api.md
[style] ~362-~362: Consider using the typographical ellipsis character here instead.
Context: ...e upstream URL, and the request body is {"sql": "..."} with no escape hatch for query-strin...
(ELLIPSIS)
🔇 Additional comments (3)
docs/src/content/docs/api.md (1)
490-492: LGTM!docs/src/content/docs/architecture.md (1)
138-138: LGTM!docs/src/content/docs/pipes.mdx (1)
8-9: LGTM!Also applies to: 38-40, 53-53, 87-93, 110-110, 184-198, 205-207
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: 13432d47-a6a2-43d5-b986-05deccee080c
📒 Files selected for processing (4)
CHANGELOG.mddocs/src/content/docs/api.mdinternal/pipes/pipes.gointernal/pipes/pipes_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Markdown files must pass markdownlint style checks (enforce via
make lint)
Files:
CHANGELOG.mddocs/src/content/docs/api.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Markdown documentation files must pass docs-prose review (accuracy vs. code, examples, clarity, completeness); scope is canonical set from
scripts/docs-prose.shexcluding.claude/**,.github/**,CHANGELOG.md,AGENTS.md,CLAUDE.md,*.draft.md,*.old.md
Files:
CHANGELOG.mddocs/src/content/docs/api.md
docs/src/content/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
When authoring Mermaid diagrams in Markdown: author vertically (top-down default,
flowchart TB/TD) to fit page column (~46–58rem); keep node labels short with<br/>for multi-line; use semantic node classes (wh,win,pain,fail,infra,neutral,store,client); never sit two large diagrams side-by-side without<div class="diagram-pair">wrapper
Files:
docs/src/content/docs/api.md
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Go code must use strict formatting enforced bygofumpt
Use structured logging withlog/slog(JSON handler) in Go code
Use Chi v5 for HTTP routing in Go code
Return errors from Go functions, never panic; wrap withfmt.Errorf("context: %w", err)
Pass dependencies explicitly via constructor injection in Go; no global state
Go code must passgo fmt(viagofumpt) andgoimports(enforced by CI'smake verify)
Go code must passgolangci-lintwith v2.11.4 (auto-installed to.bin/on first run; enforced by CI'smake verify)
Go code must passgovulncheck ./...for vulnerability scanning (enforced by CI'smake verify)
Gointernal/packages must not be imported outsidecmd/or otherinternal/packages (module privacy enforced by the filesystem)
Files:
internal/pipes/pipes.gointernal/pipes/pipes_test.go
internal/*/**.go
📄 CodeRabbit inference engine (AGENTS.md)
Use lowercase, single-word package names in Go;
internal/enforces module privacy
Files:
internal/pipes/pipes.gointernal/pipes/pipes_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Use table-driven tests in Go withtests := []struct{ name string; ... }andt.Run(tt.name, ...)for multiple scenarios
Use shared mocks frominternal/testutil/(MockPublisher, MockCache, MockDeduplicator, MockSubscriber) instead of ad-hoc mocks in Go tests
Usetestutil.MakeJWT(t, claims)andtestutil.MakeExpiredJWT(t, claims)for auth tests in Go
Usetestutil.NewTestSchemaRegistry(tables)ordiscovery.NewSchemaRegistryFromMap(tables)for schema-aware tests in Go
Usepolicy.NewMemoryStore(p)for in-memory policy testing without NATS in Go
Usepipes.NewMemoryStore(queries...)for in-memory pipes testing without NATS in Go
Usetestutil.AssertJSONResponse(t, rec, status, expected)andtestutil.AssertJSONContains(t, rec, status, substring)for HTTP response assertions in Go tests
Go test files must be named*_test.goand placed in the same package as the code under test
Files:
internal/pipes/pipes_test.go
🧠 Learnings (1)
📚 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
🔇 Additional comments (3)
internal/pipes/pipes.go (1)
142-156: LGTM!Also applies to: 206-227, 251-277, 343-344
internal/pipes/pipes_test.go (1)
292-296: LGTM!Also applies to: 347-348, 396-405, 481-498
docs/src/content/docs/api.md (1)
490-492: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/pipes/pipes.go (1)
33-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReinstate declared parameter-type enforcement before literal formatting.
At Line 33-35,
ParamDef.Typeis documented as advisory-only, and at Line 204 binding is driven only by runtime value formatting. This re-opens a contract gap where callers can supply broader shapes than the pipe definition declares (notably on publicly executable pipes), weakening the intended fail-closed behavior.Suggested fix (shape)
func BindParams(q *NamedQuery, supplied map[string]any) (string, []any, error) { @@ - lit, err := formatParamValue(val) + if p, ok := formal[name]; ok && p.Type != "" { + if err := validateParamType(p.Type, val); err != nil { + bindErr = fmt.Errorf("parameter %q: %w", name, err) + return match + } + } + lit, err := formatParamValue(val) if err != nil { bindErr = fmt.Errorf("parameter %q: %w", name, err) return match } return lit })Also applies to: 204-207
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c1a9c2fd-f9e6-4f76-8374-2972c7dacebc
📒 Files selected for processing (7)
CHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/pipes.mdxinternal/api/pipes_test.gointernal/pipes/pipes.gointernal/pipes/pipes_test.go
💤 Files with no reviewable changes (2)
- docs/src/content/docs/api.md
- internal/api/pipes_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Docs preview
- GitHub Check: Coverage
- GitHub Check: E2E tests
🧰 Additional context used
📓 Path-based instructions (4)
docs/src/content/docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/docs/**/*.{md,mdx}: Author Mermaid diagrams vertically (flowchart TB/TD) to fit page column width (~46–58rem); reserve LR for genuinely short chains (≤3–4 nodes)
Keep Mermaid node labels short; use
for a second line rather than one long line; lean on semantic node classes (wh, win, pain, fail, infra, neutral, store, client)
Never sit two large diagrams side-by-side; wrap comparisons in…to stack them vertically
Files:
docs/src/content/docs/architecture.mddocs/src/content/docs/pipes.mdx
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use Go 1.26 with strict formatting enforced by gofumpt
Use structured logging with log/slog (JSON handler)
Use Chi v5 for HTTP routing
Return errors, don't panic. Wrap with fmt.Errorf("context: %w", err)
Use package naming: lowercase, single word (or abbreviated). internal/ enforces module privacy
No global state: Dependencies are passed explicitly (constructor injection)
Comment the why, not the what. Add a comment only when the reason isn't obvious from the code; a line that matches the surrounding pattern needs none. Keep comments to 1–2 lines
DRY — one source of truth. Before adding logic, look for an existing helper, type, or constant to reuse; before duplicating a rule, factor it into one place every caller reads
Leave it neater than you found it — within reason. Fix small, safe things in passing: a stale comment, an obvious typo, a misnamed local, dead code on your path
Files:
internal/pipes/pipes.gointernal/pipes/pipes_test.go
internal/pipes/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Named query pipes: NamedQuery type + NATS KV store (WAVEHOUSE_PIPES) + .sql file bootstrap. Pre-defined SQL templates with param binding + caching; per-pipe allowed_roles is the only execute-path gate via policy.RoleAllowed
Files:
internal/pipes/pipes.gointernal/pipes/pipes_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Use table-driven tests with tests := []struct{ name string; ... } and t.Run(tt.name, ...)
Use shared mocks from internal/testutil/ (MockPublisher, MockCache, MockDeduplicator, MockSubscriber) instead of creating ad-hoc mocks
Use testutil.MakeJWT(t, claims) and testutil.MakeExpiredJWT(t, claims) for auth tests
Use testutil.NewTestSchemaRegistry(tables) or discovery.NewSchemaRegistryFromMap(tables) for schema-aware tests
Use policy.NewMemoryStore(p) for in-memory policy testing without NATS
Use pipes.NewMemoryStore(queries...) for in-memory pipes testing without NATS
Use testutil.AssertJSONResponse(t, rec, status, expected) and testutil.AssertJSONContains(t, rec, status, substring) for response assertions
Files:
internal/pipes/pipes_test.go
🧠 Learnings (1)
📚 Learning: 2026-06-10T15:01:09.027Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: docs/src/content/docs/development.md:0-0
Timestamp: 2026-06-10T15:01:09.027Z
Learning: In this repo’s Markdown review (all .md files), do not flag capitalization/style issues for literal paths starting with ".github/" (or any substring that is a path beginning with ".github/"). Treat ".github" as the correct lowercase dotfile directory name, even when it appears inside prose or code spans; automated checks such as LanguageTool’s "(GITHUB)" rule commonly produce false positives for this literal filesystem path.
Applied to files:
docs/src/content/docs/architecture.mdCHANGELOG.md
🔇 Additional comments (3)
docs/src/content/docs/pipes.mdx (3)
84-93: LGTM!
95-110: LGTM!
179-179: LGTM!
taitelee
changed the title
type enforcement intentionally dropped
github-project-automation
Bot
moved this from In review
to Done
in WaveHouse Task Board
Summary
Pipe parameters are inlined into the SQL template as escaped literals, but formatParamValue's fallback branch emitted any non-scalar value via fmt.Sprintf("%v") — so a JSON array or object passed as a parameter reached ClickHouse raw and unescaped. An array element's inner ' could close a string literal and inject SQL (a UNION/boolean break-out past the per-column allowlist). Pipe execution is authorized only by the per-pipe allowed_roles (commonly [public]), so this was reachable unauthenticated.
formatParamValueis now recursive and fails closed:The declared parameter type stays advisory metadata documenting intent for callers and the SDK — binding keys off the runtime value, consistent with how the rest of WaveHouse leaves value typing to ClickHouse. (Stricter param typing can be its own PR if wanted.)
Related Issues
Closes #317