feat(config): refuse unbound WH_* env and unusable data_dir at boot - #569
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:
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 (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🔇 Additional comments (1)
📝 SummarySummary by CodeRabbit
WalkthroughBoot configuration now rejects unbound ChangesBoot Configuration Validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant main
participant config.Load
participant config.CheckDataDir
participant Startup
main->>config.Load: Load boot configuration
config.Load-->>main: Return Config or validation error
main->>config.CheckDataDir: Check cfg.DataDir
config.CheckDataDir-->>main: Return validation result
main->>Startup: Continue only when validation succeeds
Suggested reviewers: Merge Risk: 🟡 Moderate · up to Boot now validates data_dir before startup, but a dangling symlink may still be accepted even though the configured path cannot be created or used. This creates a material startup-availability risk that should be resolved before merge. 🚥 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 |
|
📚 Docs preview is live → https://6298e464-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit 48ffc10 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: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 61793bce-c51f-433c-845e-6de232bbb2af
📒 Files selected for processing (10)
AGENTS.mdCHANGELOG.mdcmd/wavehouse/main.godocs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/settings-directory.mdxinternal/config/check.gointernal/config/check_test.gointernal/config/config.gointernal/config/persistence.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 (2)
**WH001 applies to every tracked Markdown file, with no carve-out**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/settings-directory.mdxCHANGELOG.mddocs/src/content/docs/architecture.mdAGENTS.mddocs/src/content/docs/configuration.mdx
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/configuration.mdx
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-08T19:18:47.827Z
Learning: Run `make lint` and `make test` before considering work complete.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-08T19:18:47.827Z
Learning: Every new function should have corresponding test cases.
📚 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/configuration.mdx
📚 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/config/persistence.go
🔇 Additional comments (3)
internal/config/persistence.go (1)
27-27: LGTM!cmd/wavehouse/main.go (1)
153-153: LGTM!Also applies to: 163-168, 170-171, 208-208
AGENTS.md (1)
36-36: LGTM!
github-project-automation
Bot
moved this from Backlog
to In review
in WaveHouse Task Board
|
@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)
internal/config/check.go (1)
97-104: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject dangling symlinks during the ancestor walk.
When
diror an intermediate component is a dangling symlink,os.Statreturnsfs.ErrNotExist. The loop then probes an unrelated existing ancestor withos.CreateTemp, soCheckDataDircan returnnil.Pebble.Openand the embedded NATS server later receive paths under the danglingcfg.DataDirand can fail during storage initialization.Use
os.Lstatduring the walk so the symlink itself is detected. Add regression tests for final and intermediate dangling symlinks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 2b141d14-b229-4ed2-b83f-0953c054e976
📒 Files selected for processing (7)
CHANGELOG.mdcmd/wavehouse/main.godocs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxinternal/config/check.gointernal/config/check_test.gointernal/config/persistence.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 (2)
**WH001 applies to every tracked Markdown file, with no carve-out**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxCHANGELOG.md
In MDX, leave a blank line between a JSX tag and a code fence.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/configuration.mdx
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-08T20:20:40.183Z
Learning: **Go 1.26**, strict formatting (`gofumpt`, enforced by CI)
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-08T20:20:40.183Z
Learning: **Every new function should have corresponding test cases.**
📚 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
🔇 Additional comments (6)
internal/config/persistence.go (1)
13-13: LGTM!internal/config/check_test.go (1)
119-126: LGTM!CHANGELOG.md (1)
13-13: LGTM!cmd/wavehouse/main.go (1)
153-153: LGTM!Also applies to: 163-169, 208-208
docs/src/content/docs/architecture.md (1)
111-111: LGTM!docs/src/content/docs/configuration.mdx (1)
20-20: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: ce55f954-a133-465d-9e14-54a1247a07bf
📒 Files selected for processing (5)
CHANGELOG.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.mdinternal/config/check.gointernal/config/check_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. (7)
- GitHub Check: Coverage
- GitHub Check: Integration tests
- GitHub Check: E2E tests
- GitHub Check: Docs build
- GitHub Check: Lint
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-09T22:42:50.425Z
Learning: Every code change should update the corresponding docs in the same PR.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-09T22:42:50.425Z
Learning: Run `make lint` and `make test` before considering work complete.
📚 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
🪛 LanguageTool
docs/src/content/docs/configuration.mdx
[style] ~19-~19: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ... carries names that aren't WaveHouse's. One outside source does share the prefix: Kubernetes injects {SERVICE}_SERVICE_HOST, {SERVICE}_PORT, and similar link variables into every pod for each Service that existed before it, so a Service named wh or wh-* produces WH_SERVICE_HOST, WH_PORT, … and the pod refuses to boot on its next restart. Set enableServiceLinks: false on the ...
(TOO_LONG_SENTENCE)
🔇 Additional comments (4)
internal/config/check.go (1)
30-30: LGTM!Also applies to: 56-60, 64-67
internal/config/check_test.go (1)
7-7: LGTM!Also applies to: 10-10, 34-53, 56-65
docs/src/content/docs/deployment.md (1)
189-189: LGTM!docs/src/content/docs/configuration.mdx (1)
20-20: LGTM!Also applies to: 22-22
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Good stuff. My only real find is that the permission-denied path returns without the permission hint which is what it exists for and there's no test that would have caught that I don't think.
A couple tiny other nitpick things I left in check.go, and then everything else is just stale comment/doc things, as per usual.
…v, sync docs to review
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 6fb12e14-3325-4bf3-bd3c-e9b2e2eeef4c
📒 Files selected for processing (11)
CHANGELOG.mdconfig.yamldeployments/Dockerfiledeployments/Dockerfile.goreleaserdocs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.mddocs/src/integrations/diagram-png.mjsinternal/config/check.gointernal/config/check_test.gointernal/config/config.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. (4)
- GitHub Check: Docs preview
- GitHub Check: E2E tests
- GitHub Check: Coverage
- GitHub Check: Integration tests
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-10T13:41:57.966Z
Learning: Run `make lint` and `make test` before considering work complete.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-10T13:41:57.966Z
Learning: Every code change updates its docs + `CHANGELOG.md` in the same PR
📚 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
🪛 LanguageTool
docs/src/content/docs/deployment.md
[locale-violation] ~343-~343: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ..., because none of them announces itself afterwards: - **Nullable(T) DEFAULT … columns n...
(AFTERWARDS_US)
[style] ~347-~347: Consider using a more formal/concise alternative here.
Context: ...ry this release — the ClickHouse wiring other than the password, dedupe.*, dlq.*, `str...
(OTHER_THAN)
CHANGELOG.md
[typographical] ~23-~23: Consider using an em dash in dialogues and enumerations.
Context: - **Boot refuses an unbound WH_* enviro...
(DASH_RULE)
🔇 Additional comments (11)
internal/config/config.go (1)
38-38: LGTM!internal/config/check.go (1)
20-20: LGTM!Also applies to: 26-26, 33-33, 39-39, 80-87, 96-100
internal/config/check_test.go (1)
30-31: LGTM!Also applies to: 53-53, 150-163
docs/src/integrations/diagram-png.mjs (1)
112-113: 🎯 Functional CorrectnessNo in-repository caller still uses
WH_SKIP_DIAGRAM_PNG.The repository contains only
DOCS_SKIP_DIAGRAM_PNGand its changelog entry. No compatibility alias is required for an in-repository caller.CHANGELOG.md (1)
23-24: LGTM!config.yaml (1)
1-4: LGTM!docs/src/content/docs/architecture.md (1)
110-111: LGTM!docs/src/content/docs/configuration.mdx (1)
19-20: LGTM!Also applies to: 22-22
docs/src/content/docs/deployment.md (1)
189-189: LGTM!Also applies to: 209-215, 343-343
deployments/Dockerfile (1)
32-32: LGTM!deployments/Dockerfile.goreleaser (1)
10-10: LGTM!
|
@coderabbitai review |
✅ Action performedReview finished.
|
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Ok approving it cause the rest is all docs. I don't know enough about k8s to understand what/why its getting nit-picky at comments and docs so I had it try to help explain it to me, which I thought you may find helpful too. It also flagged our pm-triage skill which I left out as I already know I need to fix that skill up and think I have an issue to track it's general fixes already. Here's what it said:
Kubernetes service links
Worth spelling out, because we describe this in three places and no copy is
quite right.
kubelet injects a set of environment variables into every container for each
Service it can see. It's a Docker-links compatibility feature, on by default
through enableServiceLinks: true on the PodSpec. Three rules decide what a
pod actually receives:
- Namespace. Only Services in the pod's own namespace, plus the
kubernetesAPI Service fromdefault. AwhService in some other
namespace is harmless. - Timing. Only Services that already existed when the pod started. A
Service created afterwards injects nothing until the pod restarts. - ClusterIP. Services with
clusterIP: Noneare skipped, so a headless
whService injects nothing at all.
Names are the Service name uppercased with - mapped to _, then a fixed set
per Service (port numbers here are illustrative — they follow the Service's
own ports):
Service "wh" → WH_SERVICE_HOST, WH_SERVICE_PORT, WH_PORT,
WH_PORT_8080_TCP, WH_PORT_8080_TCP_ADDR, …
Service "wh-foo" → WH_FOO_SERVICE_HOST, WH_FOO_SERVICE_PORT, WH_FOO_PORT,
WH_FOO_PORT_8080_TCP, …
Every one of those starts with WH_, so every one trips the new check. That
part of the feature is working as intended. What's off is how we describe it:
configuration.mdx:19says "into every pod". It's every pod in the
Service's namespace, and never for a headless Service.check.go:30says a Service namedwhorwh-*injects
WH_SERVICE_HOST, WH_PORT. Only an exactwhproduces those —wh-foo
producesWH_FOO_SERVICE_HOSTandWH_FOO_PORT. This is the copy that
matters most: it's what an operator reads when the pod won't start, and it
sends them grepping for a variable that isn't there while the real offender
is already named in the list printed directly above it. No inline comment on
it, but it wants the same edit as the doc.deployment.md:347is already correct as of 26d9076.
Two consequences worth putting in the docs, because they're the shapes this
will actually take in the field:
- The failure is a restart, not a deploy. The Service almost always
pre-exists, so a cluster runs fine until something recycles the pod — a
rollout, a node drain, an OOM kill — and then it doesn't come back. Bad thing
to learn at 3am. - The likeliest instance is self-inflicted. If WaveHouse's own Service is
namedwh, WaveHouse's own pods getWH_SERVICE_HOSTand refuse to boot.
Our deployment docs should probably say don't name it that.
I'm reasoning from kubelet's behaviour rather than from a cluster in front of
me, so before rewriting anything: kubectl exec <pod> -- env | grep ^WH_ on a
pod in a namespace that has a wh-* Service. If that comes back empty, I've
got something wrong and I'd rather find out first.
Upgrade audit
Two comments on the same bullet at deployment.md:347. It's presented as the
complete list of what to strip before upgrading, so gaps in it are the
difference between a clean upgrade and a pod that won't start — WH_AUTH_JWKS_URL
in particular, since JWKS is the standard production auth posture.
pm-triage — follow-up, not this PR
Renaming the diagram knob was the right call, and the same argument covers nine
more names we own: .claude/skills/pm-triage/scripts/board.sh and state.sh
read WH_REPO, WH_OWNER, WH_PROJECT_ID, WH_PROJECT_NUM,
WH_PRIORITY_FIELD, WH_STATUS_FIELD, and WH_PM_STATE_{BRANCH,AUTHOR,EMAIL},
and SKILL.md:93 documents them as the supported override mechanism.
export WH_PROJECT_ID=… to point the routine at a different board and every
make dev in that shell then dies at boot blaming the settings-directory move.
Lower odds than the docs knob — it's an occasional local routine — so this is a
follow-up issue rather than something to hold #530 for. Happy to file it.
…ariable list, map config files
|
Added a short note in the k8s section of deployment.md not to name our own Service pm-triage is tracked in #582, since #526 only covers the rubric. @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 5680fd1f-1043-456c-9aab-44a3de0bb97e
📒 Files selected for processing (4)
docs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/deployment.mdinternal/config/check.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 (2)
Never hard-wrap prose.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/architecture.mddocs/src/content/docs/deployment.mddocs/src/content/docs/configuration.mdx
In MDX, leave a blank line between a JSX tag and a code fence.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/configuration.mdx
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-10T15:55:34.178Z
Learning: Run `make lint` and `make test` before considering work complete.
📚 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
🔇 Additional comments (3)
docs/src/content/docs/architecture.md (1)
110-113: LGTM!docs/src/content/docs/configuration.mdx (1)
19-20: LGTM!Also applies to: 22-22
docs/src/content/docs/deployment.md (1)
309-309: LGTM!Also applies to: 349-349
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
github-project-automation
Bot
moved this from In review
to Done
in WaveHouse Task Board
2 participants
Summary
Boot is now the validator for the non-hot-reloadable half of configuration; there is no dry-run subcommand (decided on #530).
config.Loadrefuses to start on aWH_*environment variable that noConfigfield binds and the binary doesn't otherwise read, naming every offender — the environment half of the strict YAML loader, soWH_DEDUPE_ENABLED=trueleft over from before the settings-directory move can no longer be set, ignored, and believed. Only theWH_prefix is checked, since the environment always carries unrelated names.Right after
Load, before the settings directory or ClickHouse are touched,config.CheckDataDirprobes thatdata_diris a writable directory (or absent under a writable parent, so boot can create it), so a root-owned bind mount refuses boot in the first second of the log with the UID-65532 remediation attached. Docs state that boot is the validator and thatwavehouse validatestays the settings-directory dry run only.Related Issues
Closes #530