feat(settings): add settings-directory validation and validate CLI - #500
Conversation
github-actions
Bot
added
go
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a four-file settings-directory model and validator with strict JSON checks, content and cross-file validation, warnings, and structured findings. Adds ChangesSettings validation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR adds settings-directory validation and a CLI gate, but the current implementation can accept a null JSON document as valid and may hang indefinitely when an expected settings file is a FIFO; documentation also describes deferred settings behavior as active and omits SHA validation. These bounded correctness, availability, and operator-facing issues should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant Operator
participant wavehouse
participant runValidate
participant settings.Validate
participant SettingsDirectory
Operator->>wavehouse: invoke validate [dir]
wavehouse->>runValidate: pass command arguments
runValidate->>settings.Validate: validate resolved directory
settings.Validate->>SettingsDirectory: read and validate JSON files
SettingsDirectory-->>settings.Validate: return findings
settings.Validate-->>runValidate: return findings
runValidate-->>Operator: print findings and exit status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 4ba382de-4b02-4385-96ce-e2525afbeea1
📒 Files selected for processing (12)
.testcoverage.ymlCHANGELOG.mdcmd/wavehouse/main.gocmd/wavehouse/validate.gocmd/wavehouse/validate_test.goconfig.yamlinternal/config/config.gointernal/settings/decode.gointernal/settings/finding.gointernal/settings/settings.gointernal/settings/validate.gointernal/settings/validate_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 (5)
**/*.{go,ts,tsx,js,jsx,md,mdx,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
Every code change updates its docs +
CHANGELOG.mdin the same PR
Files:
cmd/wavehouse/main.goconfig.yamlcmd/wavehouse/validate_test.gocmd/wavehouse/validate.gointernal/settings/decode.gointernal/config/config.gointernal/settings/settings.goCHANGELOG.mdinternal/settings/validate.gointernal/settings/finding.gointernal/settings/validate_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Go 1.26, strict formatting (gofumpt, enforced by CI)
- Table-driven tests: Use
tests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
Files:
cmd/wavehouse/main.gocmd/wavehouse/validate_test.gocmd/wavehouse/validate.gointernal/settings/decode.gointernal/config/config.gointernal/settings/settings.gointernal/settings/validate.gointernal/settings/finding.gointernal/settings/validate_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
Files:
cmd/wavehouse/validate_test.gointernal/settings/validate_test.go
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- No global state: Dependencies are passed explicitly (constructor injection).
Files:
internal/settings/decode.gointernal/config/config.gointernal/settings/settings.gointernal/settings/validate.gointernal/settings/finding.gointernal/settings/validate_test.go
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
- Never hard-wrap prose. One paragraph is one line. No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
Files:
CHANGELOG.md
🧠 Learnings (4)
📚 Learning: 2026-07-07T12:38:12.052Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 378
File: internal/auth/auth.go:119-132
Timestamp: 2026-07-07T12:38:12.052Z
Learning: In this repo, do not add or recommend logging/tracing client IP addresses using naive or untrusted sources (e.g., `r.RemoteAddr` or directly trusting/deriving `X-Forwarded-For`) anywhere in the Go codebase. `middleware.RealIP` was removed due to IP-spoofing risks, and proper trusted-proxy-aware client-IP handling is intentionally deferred to issue `#333`. During code review, if proposed changes would record client IPs (including in audit paths such as `internal/auth/auth.go`), reject/redirect until `#333` lands with correct trusted-proxy configuration and safeguards.
Applied to files:
cmd/wavehouse/main.gocmd/wavehouse/validate_test.gocmd/wavehouse/validate.gointernal/settings/decode.gointernal/config/config.gointernal/settings/settings.gointernal/settings/validate.gointernal/settings/finding.gointernal/settings/validate_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:
cmd/wavehouse/main.gocmd/wavehouse/validate_test.gocmd/wavehouse/validate.gointernal/settings/decode.gointernal/config/config.gointernal/settings/settings.gointernal/settings/validate.gointernal/settings/finding.gointernal/settings/validate_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:
cmd/wavehouse/validate_test.gointernal/settings/validate_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.md
🔇 Additional comments (6)
internal/config/config.go (1)
34-44: LGTM!config.yaml (1)
106-111: LGTM!internal/settings/finding.go (1)
5-48: LGTM!.testcoverage.yml (1)
74-82: LGTM!cmd/wavehouse/main.go (1)
40-49: LGTM!cmd/wavehouse/validate.go (1)
16-46: LGTM!
github-project-automation
Bot
moved this from In progress
to In review
in WaveHouse Task Board
github-actions
Bot
added
the
documentation
|
@coderabbitai review |
|
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: 6687ff7b-8d43-448f-bbe2-19473a6d7771
📒 Files selected for processing (4)
CHANGELOG.mddocs/src/content/docs/configuration.mdxinternal/settings/validate.gointernal/settings/validate_test.go
💤 Files with no reviewable changes (1)
- CHANGELOG.md
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: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{go,ts,tsx,js,jsx,md,mdx,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
Every code change updates its docs +
CHANGELOG.mdin the same PR
Files:
docs/src/content/docs/configuration.mdxinternal/settings/validate_test.gointernal/settings/validate.go
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
- Never hard-wrap prose. One paragraph is one line. No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
Files:
docs/src/content/docs/configuration.mdx
**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
- In MDX, leave a blank line between a JSX tag and a code fence.
Files:
docs/src/content/docs/configuration.mdx
docs/src/content/docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/docs/**/*.mdx: - Opt a page into the Cloud CTA withcloudCtafrontmatter, not by importing the component.
- Never hand-write
®or™in prose.- Never hand-write
utm_*params orrelon a link towavehouse.cloudorwave-rf.com. UsecloudLink()/relFor()fromdocs/src/config/outbound.ts.
Files:
docs/src/content/docs/configuration.mdx
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Go 1.26, strict formatting (gofumpt, enforced by CI)
- Table-driven tests: Use
tests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
Files:
internal/settings/validate_test.gointernal/settings/validate.go
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- No global state: Dependencies are passed explicitly (constructor injection).
Files:
internal/settings/validate_test.gointernal/settings/validate.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
Files:
internal/settings/validate_test.go
🧠 Learnings (6)
📚 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/configuration.mdx
📚 Learning: 2026-08-19T15:23:52.908Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-19T15:23:52.908Z
Learning: Applies to **/*.go : - **Table-driven tests**: Use `tests := []struct{ name string; ... }` with `t.Run(tt.name, ...)` for test cases.
Applied to files:
internal/settings/validate_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/settings/validate_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/settings/validate_test.gointernal/settings/validate.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/settings/validate_test.gointernal/settings/validate.go
📚 Learning: 2026-08-19T15:44:27.183Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 500
File: internal/settings/settings.go:31-31
Timestamp: 2026-08-19T15:44:27.183Z
Learning: In the WaveHouse Go codebase, do not flag package-level lookup tables or precomputed stateless values when they are immutable and read-only, including settings.Files, validate.pipeParamTypes, mutationVerbs, nonMutationVerbs, identEscaper, intBounds, and package-level regular expressions. The no-global-state guideline applies to injected application dependencies and mutable singletons, not immutable lookup data.
Applied to files:
internal/settings/validate_test.gointernal/settings/validate.go
🔇 Additional comments (3)
internal/settings/validate.go (1)
129-151: LGTM!internal/settings/validate_test.go (1)
123-153: LGTM!Also applies to: 230-255
docs/src/content/docs/configuration.mdx (1)
4-5: LGTM!Also applies to: 359-360
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit 49d881d in the Show a line coverage summary of the most impacted files.
Updated |
|
📚 Docs preview is live → https://8f80cf3a-wavehouse-docs.wave-rf.workers.dev |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 (1)
docs/src/content/docs/configuration.mdx (1)
183-185: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument per-file SHA validation.
Line 185 lists JSON, shape, and reference checks, but it omits the per-file SHA validation in this PR. State that validation checks per-file SHA values so operators can prepare and diagnose settings directories correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a68de3f7-e8d3-4992-a37d-9c1b94f7ab21
📒 Files selected for processing (3)
CHANGELOG.mdcmd/wavehouse/main.godocs/src/content/docs/configuration.mdx
💤 Files with no reviewable changes (1)
- CHANGELOG.md
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 (5)
**/*.{go,ts,tsx,js,jsx,md,mdx,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
Every code change updates its docs +
CHANGELOG.mdin the same PR
Files:
docs/src/content/docs/configuration.mdxcmd/wavehouse/main.go
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
- Never hard-wrap prose. One paragraph is one line. No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
Files:
docs/src/content/docs/configuration.mdx
**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
- In MDX, leave a blank line between a JSX tag and a code fence.
Files:
docs/src/content/docs/configuration.mdx
docs/src/content/docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/docs/**/*.mdx: - Opt a page into the Cloud CTA withcloudCtafrontmatter, not by importing the component.
- Never hand-write
®or™in prose.- Never hand-write
utm_*params orrelon a link towavehouse.cloudorwave-rf.com. UsecloudLink()/relFor()fromdocs/src/config/outbound.ts.
Files:
docs/src/content/docs/configuration.mdx
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Go 1.26, strict formatting (gofumpt, enforced by CI)
- Table-driven tests: Use
tests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
Files:
cmd/wavehouse/main.go
🧠 Learnings (5)
📓 Common learnings
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 479
File: docs/src/content/docs/sdk/admin.md:7-12
Timestamp: 2026-08-18T03:06:23.785Z
Learning: For WaveHouse pull request reviews, keep findings within the stated scope of the current PR. Defer broader documentation review feedback to a separately scoped documentation PR when the author requests it.
📚 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/configuration.mdx
📚 Learning: 2026-08-18T19:26:46.940Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 485
File: docs/src/content/docs/api.md:178-178
Timestamp: 2026-08-18T19:26:46.940Z
Learning: In WaveHouse `cmd/wavehouse/main.go`, a checkout `go build` with VCS metadata on Go 1.24+ can expose a synthesized module pseudo-version through `debug.ReadBuildInfo().Main.Version`, plus `vcs.revision` and `vcs.time`. `buildInfoFallback` uses `"dev"` and `"unknown"` only when VCS metadata is unavailable, such as with `-buildvcs=false` or outside a repository.
Applied to files:
cmd/wavehouse/main.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:
cmd/wavehouse/main.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:
cmd/wavehouse/main.go
🔇 Additional comments (1)
cmd/wavehouse/main.go (1)
13-13: LGTM!Also applies to: 33-95
…mpty policies.json lockout
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: 8acd656b-a5b5-46ec-8e74-e75a62be7d38
📒 Files selected for processing (9)
CHANGELOG.mdcmd/wavehouse/main.gocmd/wavehouse/validate.godocs/src/content/docs/configuration.mdxinternal/config/config.gointernal/config/config_test.gointernal/settings/settings.gointernal/settings/validate.gointernal/settings/validate_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. (3)
- GitHub Check: Docs build
- GitHub Check: E2E tests
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{go,ts,tsx,js,jsx,md,mdx,yaml,yml,json}
📄 CodeRabbit inference engine (AGENTS.md)
Every code change updates its docs +
CHANGELOG.mdin the same PR
Files:
docs/src/content/docs/configuration.mdxinternal/config/config_test.gocmd/wavehouse/main.gocmd/wavehouse/validate.goCHANGELOG.mdinternal/config/config.gointernal/settings/settings.gointernal/settings/validate.gointernal/settings/validate_test.go
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
- Never hard-wrap prose. One paragraph is one line. No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
Files:
docs/src/content/docs/configuration.mdxCHANGELOG.md
**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
- In MDX, leave a blank line between a JSX tag and a code fence.
Files:
docs/src/content/docs/configuration.mdx
docs/src/content/docs/**/*.mdx
📄 CodeRabbit inference engine (AGENTS.md)
docs/src/content/docs/**/*.mdx: - Opt a page into the Cloud CTA withcloudCtafrontmatter, not by importing the component.
- Never hand-write
®or™in prose.- Never hand-write
utm_*params orrelon a link towavehouse.cloudorwave-rf.com. UsecloudLink()/relFor()fromdocs/src/config/outbound.ts.
Files:
docs/src/content/docs/configuration.mdx
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Go 1.26, strict formatting (gofumpt, enforced by CI)
- Table-driven tests: Use
tests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
Files:
internal/config/config_test.gocmd/wavehouse/main.gocmd/wavehouse/validate.gointernal/config/config.gointernal/settings/settings.gointernal/settings/validate.gointernal/settings/validate_test.go
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
- No global state: Dependencies are passed explicitly (constructor injection).
Files:
internal/config/config_test.gointernal/config/config.gointernal/settings/settings.gointernal/settings/validate.gointernal/settings/validate_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
Files:
internal/config/config_test.gointernal/settings/validate_test.go
🧠 Learnings (5)
📚 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/configuration.mdx
📚 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/config/config_test.go
📚 Learning: 2026-06-10T19:54:03.032Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 312
File: CHANGELOG.md:0-0
Timestamp: 2026-06-10T19:54:03.032Z
Learning: In the Wave-RF/WaveHouse repository, CHANGELOG.md entries under `[Unreleased]` use descriptive Keep-a-Changelog leads (e.g. "The structured-query column allowlist is now a hard cap…"), NOT the Conventional Commit PR title verbatim. Do not flag CHANGELOG entry leads for not matching the PR title — that is not a rule in this repo. There is no `.coderabbit.yaml`, and neither `AGENTS.md` nor `CONTRIBUTING.md` requires CHANGELOG leads to match PR titles.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-08-19T15:44:27.183Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 500
File: internal/settings/settings.go:31-31
Timestamp: 2026-08-19T15:44:27.183Z
Learning: In the WaveHouse Go codebase, do not flag package-level lookup tables or precomputed stateless values when they are immutable and read-only, including settings.Files, validate.pipeParamTypes, mutationVerbs, nonMutationVerbs, identEscaper, intBounds, and package-level regular expressions. The no-global-state guideline applies to injected application dependencies and mutable singletons, not immutable lookup data.
Applied to files:
internal/settings/settings.go
📚 Learning: 2026-08-19T15:23:52.908Z
Learnt from: CR
Repo: Wave-RF/WaveHouse PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-08-19T15:23:52.908Z
Learning: Applies to internal/**/*.go : - **No global state**: Dependencies are passed explicitly (constructor injection).
Applied to files:
internal/settings/settings.go
🔇 Additional comments (9)
internal/settings/settings.go (1)
19-36: LGTM!Also applies to: 77-80
internal/config/config.go (1)
37-42: LGTM!internal/config/config_test.go (1)
6-6: LGTM!Also applies to: 525-535
internal/settings/validate.go (1)
4-4: LGTM!Also applies to: 148-176, 201-227, 258-258, 269-330, 360-385
internal/settings/validate_test.go (1)
67-72: LGTM!Also applies to: 107-168, 181-182, 218-228, 312-365
cmd/wavehouse/main.go (1)
7-7: LGTM!Also applies to: 101-135
cmd/wavehouse/validate.go (1)
7-7: LGTM!Also applies to: 22-27
docs/src/content/docs/configuration.mdx (1)
183-189: LGTM!CHANGELOG.md (1)
17-17: LGTM!
There was a problem hiding this comment.
Thanks for the explanations on things, I resolved a bunch of old threads and followed up on others. I think the main remaining points are:
- when it was just run with no commands (and the config loader instead), and then one jerry-rigged
healthcommand for docker healthchecks it wasn't a huge deal, but as we introduce more actual subcommands we recommend people run, likevalidatewith its own options and help output ideally, we need to actually use a proper cli tool, whether it be stdlib'sflagpackage, or the industry standardcobraone that k8s and docker and stuff use, or something likekong– whatever it is, we should add something NOW before the problem continues cascading and growing. - I don't know if you saw it in my last review comment as it was seemingly unaddressed or I just missed it, but re-pasting below:
Finally, I thought we were trying to split our config into two types: those that need to be set prior to running the binary by the operator (and thus stay as yaml, json, or env variables with the existing config mechanism/path) and do NOT need to be hot-reloadable, and those that DO get set mid-run and should be hot-reloadable, with the latter being moved into the config.json file this PR is adding – but I see none of them actually moved over in this PR. Is that intentional and will be a separate PR, or what is the plan there?
Would love those two things addressed, and a new issue opened for PR work to begin on either a json schema or zod or something, and then this is ready to merge. Thanks!
|
Ok so my reasoning for not using cobra yet is that we have only four commands and zero real flags so far, and the FlagSet-per-subcommand is exactly what a cobra port would be useful for and use, so we can just easily get that in when that does happen. I think this is marked as a commet in main.go. But again I also don't have a strong opinion on whether or not we should just integrate cobra now. On the split you mention for the config stuff, Opened #506 for the JSON Schema work. |
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 platform limitations.
⚠️ Outside diff range comments (1)
cmd/wavehouse/main.go (1)
129-139: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd direct tests for
printUsage.
printUsageis a new function. Add a table-drivenTestPrintUsagethat writes to abytes.Bufferand verifies the documented commands andconfig.EnvSettingsDir.As per coding guidelines: “Every new function should have corresponding test cases.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3dc564be-9605-47dd-8faf-aabea3aa6768
📒 Files selected for processing (10)
.testcoverage.ymlCHANGELOG.mdcmd/wavehouse/health.gocmd/wavehouse/health_test.gocmd/wavehouse/main.gocmd/wavehouse/validate.gocmd/wavehouse/validate_test.gointernal/settings/decode.gointernal/settings/validate.gointernal/settings/validate_unix_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. (3)
- GitHub Check: Docs preview
- GitHub Check: E2E tests
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Go 1.26, strict formatting (gofumpt, enforced by CI)
- No global state: Dependencies are passed explicitly (constructor injection).
Files:
internal/settings/validate_unix_test.gocmd/wavehouse/health.gocmd/wavehouse/main.gocmd/wavehouse/validate_test.gointernal/settings/decode.gocmd/wavehouse/validate.gocmd/wavehouse/health_test.gointernal/settings/validate.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: - Table-driven tests: Usetests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
Files:
internal/settings/validate_unix_test.gocmd/wavehouse/validate_test.gocmd/wavehouse/health_test.go
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
- Never hard-wrap prose. One paragraph is one line. No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
Files:
CHANGELOG.md
🔇 Additional comments (6)
internal/settings/decode.go (1)
15-21: LGTM!internal/settings/validate.go (1)
75-78: LGTM!Also applies to: 130-147
cmd/wavehouse/validate.go (1)
20-68: LGTM!CHANGELOG.md (1)
17-17: LGTM!.testcoverage.yml (1)
74-84: LGTM!cmd/wavehouse/health.go (1)
36-57: LGTM!
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Looks good, and gotcha I see so we will move the things out of the current config and into this new file config in the next PR then, not all at once that makes sense.
My only remaining note would be that I think we can probably move many more settings away and into this new dir/config.json approach, but I think it will be easier to audit which can get moved or not in the PR where you actually do that so we can say what remains.
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 (1)
cmd/wavehouse/main.go (1)
129-138: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a focused test for
printUsage.
printUsageis a new function. Add a test with abytes.Bufferthat verifies the command list andWH_SETTINGS_DIRfallback text.As per coding guidelines: “Every new function should have corresponding test cases.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 344c408e-103b-4ee9-94c4-465955a2d7c2
📒 Files selected for processing (10)
.testcoverage.ymlCHANGELOG.mdcmd/wavehouse/health.gocmd/wavehouse/health_test.gocmd/wavehouse/main.gocmd/wavehouse/validate.gocmd/wavehouse/validate_test.gointernal/settings/decode.gointernal/settings/validate.gointernal/settings/validate_unix_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 (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: - Go 1.26, strict formatting (gofumpt, enforced by CI)
- No global state: Dependencies are passed explicitly (constructor injection).
Files:
cmd/wavehouse/validate.gointernal/settings/validate_unix_test.gocmd/wavehouse/validate_test.gocmd/wavehouse/health.gocmd/wavehouse/health_test.gocmd/wavehouse/main.gointernal/settings/validate.gointernal/settings/decode.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: - Table-driven tests: Usetests := []struct{ name string; ... }witht.Run(tt.name, ...)for test cases.
- Every new function should have corresponding test cases. Run
make lintandmake testbefore considering work complete.
Files:
internal/settings/validate_unix_test.gocmd/wavehouse/validate_test.gocmd/wavehouse/health_test.go
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
- Never hard-wrap prose. One paragraph is one line. No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
Files:
CHANGELOG.md
🧠 Learnings (1)
📚 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:
cmd/wavehouse/health_test.go
🪛 LanguageTool
CHANGELOG.md
[style] ~15-~15: Since ownership is already implied, this phrasing may be redundant.
Context: ...ter,LiveDemo}.astro`): the site tracked its own CTAs but nothing a reader did on the wa...
(PRP_OWN)
[style] ~15-~15: Since ownership is already implied, this phrasing may be redundant.
Context: ...docs area without each tracker carrying its own copy; it's stamped at capture time by a...
(PRP_OWN)
[style] ~15-~15: Since ownership is already implied, this phrasing may be redundant.
Context: ...ressive Code, and the 404 route all own their own markup — some of it created after page ...
(PRP_OWN)
🔇 Additional comments (10)
internal/settings/decode.go (1)
15-21: LGTM!internal/settings/validate.go (1)
75-78: LGTM!Also applies to: 130-147
internal/settings/validate_unix_test.go (1)
15-50: LGTM!.testcoverage.yml (1)
75-84: LGTM!cmd/wavehouse/main.go (1)
103-124: LGTM!cmd/wavehouse/validate.go (1)
20-68: LGTM!cmd/wavehouse/validate_test.go (1)
58-64: LGTM!CHANGELOG.md (1)
13-13: LGTM!cmd/wavehouse/health.go (1)
5-6: LGTM!Also applies to: 36-57
cmd/wavehouse/health_test.go (1)
48-48: LGTM!Also applies to: 64-64, 79-104
github-project-automation
Bot
moved this from In review
to In progress
in WaveHouse Task Board
github-project-automation
Bot
moved this from In progress
to Done
in WaveHouse Task Board
2 participants
Summary
First piece of the file-based control plane: settings move to a directory of JSON documents (
roles.json,policies.json,pipes.json,config.json), andsettings.Validate(dir)is the single gate every consumer of that directory runs — the CLI now, boot and live reload when they land.internal/settingspackage: pure validation (no network, no ClickHouse), collecting all findings in one pass — directory contents (exactly the four files, unknown*.jsonrejected), strict JSON decoding (unknown fields, duplicate keys, truncated files), per-file sha and cross-file role referential integrity. Warnings don't invalidate.wavehouse validate [dir]subcommand: directory from the argument orWH_SETTINGS_DIR, prints findings, exits 0/1/2 (valid/invalid/usage) operators and CI can gate a config change before it reaches a running instance.settings.dir/WH_SETTINGS_DIRadded to boot config as the reload machinery will follow.TODOs (follow-up PRs)
WH_SETTINGS_DIRat startup, refuse to start loudly on invalid settings, and repoint runtime consumers (roles, policy, pipes, tenant tunables) at the snapshot./v1/ops/reloadand/or SIGHUP and/or fsnotify with debounce; a failed reload keeps serving the previous good document./v1/opssettings write endpoints (reverses the Hot-Reloading Configuration #48-era runtime-settings direction).validatecommand once loader lands.NOTE: The e2e bullet reflects what the exclude actually says in .testcoverage.yml: the package is only reachable via the CLI today, the e2e suite is server-focused so it sat at 0% and pulled that suite's gate under its floor, unit coverage is thorough, the merged total is untouched, and the exclude is explicitly temporary until the server consumes the directory.
Related Issues
Advances #48 (validation gate only — reload wiring lands separate