Repository navigation
fix(policy): reject no-op rules, drop the policy HTTP surface - #541
Conversation
|
📚 Docs preview is live → https://20b178ba-wavehouse-docs.wave-rf.workers.dev |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2026-06-26T12:23:22.696ZApplied to files:
📚 Learning: 2026-08-13T12:17:52.620ZApplied to files:
🪛 LanguageToolCHANGELOG.md[typographical] ~39-~39: Consider using an em dash in dialogues and enumerations. (DASH_RULE) [style] ~39-~39: The word ‘caveat’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “caution” or “warning”. (CAVEAT) 🔇 Additional comments (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR removes policy inspection and dry-run validation HTTP endpoints and the TypeScript policy namespace. Policy validation now rejects operator-less filter and check entries. Documentation and end-to-end tests use ChangesPolicy surface and validation updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to This change removes the policy HTTP endpoints and SDK namespace, so existing consumers that still use them will fail after upgrading and must migrate to file-based policy management and CLI validation. The change is otherwise mergeable, with explicit owner awareness required for that compatibility impact. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy the linked objectives in [ Full details: Docstring CoverageExplanation Docstring coverage is 70.59% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 18 files. (3 skipped: 3 unsupported.)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: 5a0efc59-4d97-4dc2-be1a-f7dc90884ee6
📒 Files selected for processing (10)
CHANGELOG.mdclients/ts/src/policy.tsdocs/src/content/docs/access-control.mdxdocs/src/content/docs/api.mdinternal/api/policy.gointernal/api/policy_test.gointernal/policy/policy.gointernal/policy/policy_test.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. (5)
- GitHub Check: Coverage
- GitHub Check: E2E tests
- GitHub Check: Integration tests
- GitHub Check: Docs build
- GitHub Check: Lint
🧰 Additional context used
📓 Path-based instructions (6)
**Opt a page into the Cloud CTA with `cloudCta` frontmatter**, not by importing the component.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/access-control.mdx
Every code change updates its docs + `CHANGELOG.md` in the same PR
📄 CodeRabbit inference engine (AGENTS.md)
Files:
clients/ts/src/policy.tsinternal/settings/validate.gointernal/policy/policy.goCHANGELOG.mdinternal/api/policy_test.gointernal/settings/validate_test.godocs/src/content/docs/access-control.mdxdocs/src/content/docs/api.mdinternal/policy/policy_test.gointernal/api/policy.go
Every new function should have corresponding test cases.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/api/policy_test.gointernal/settings/validate_test.gointernal/policy/policy_test.go
Exactly one runtime dependency — `eventsource-parser` (SSE framing, itself dependency-free); adding a second needs the same scrutiny the first got.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
clients/ts/src/policy.ts
**In MDX, leave a blank line between a JSX tag and a code fence.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/access-control.mdx
**Never hard-wrap prose. One paragraph is one line.** No wrapping at 72/80 columns, no "semantic linefeeds" splitting a paragraph at sentence boundaries.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
CHANGELOG.mddocs/src/content/docs/api.md
🧠 Learnings (3)
📚 Learning: 2026-06-26T12:23:22.696Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).
Applied to files:
internal/api/policy_test.gointernal/settings/validate_test.gointernal/policy/policy_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/policy_test.go
📚 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/access-control.mdx
🪛 LanguageTool
CHANGELOG.md
[style] ~37-~37: The word ‘caveat’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “caution” or “warning”.
Context: ...icy/validateat once.#460`'s migration caveat (a stored policy hard-failing at boot) ...
(CAVEAT)
🔇 Additional comments (10)
internal/policy/policy.go (1)
63-69: LGTM!Also applies to: 661-663, 673-675, 685-687
internal/policy/policy_test.go (1)
613-664: LGTM!internal/api/policy.go (1)
5-10: LGTM!Also applies to: 43-78
internal/api/policy_test.go (1)
87-89: LGTM!Also applies to: 91-133, 135-147
docs/src/content/docs/api.md (1)
763-763: LGTM!internal/settings/validate.go (1)
56-62: LGTM!Also applies to: 64-72
internal/settings/validate_test.go (1)
454-456: LGTM!Also applies to: 458-482
CHANGELOG.md (1)
33-35: LGTM!Also applies to: 37-38
clients/ts/src/policy.ts (1)
32-38: LGTM!docs/src/content/docs/access-control.mdx (1)
185-186: LGTM!Also applies to: 204-204
|
@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: Team
Run ID: 47a30fcc-aa98-42ae-99d8-11e7b47bb931
📒 Files selected for processing (29)
AGENTS.mdCHANGELOG.mdclients/ts/src/client.test.tsclients/ts/src/client.tsclients/ts/src/index.tsclients/ts/src/namespaces.test.tsclients/ts/src/policy.tsclients/ts/src/types.tscmd/wavehouse/main.godocs/src/content/docs/access-control.mdxdocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/development.mddocs/src/content/docs/reverse-proxy.mdxdocs/src/content/docs/sdk/admin.mddocs/src/content/docs/sdk/reference.mddocs/src/content/docs/settings-directory.mdxinternal/api/policy.gointernal/api/policy_test.gointernal/api/router.gointernal/api/router_test.gointernal/policy/scalars.gointernal/settings/validate_test.gotests/e2e/sdk/admin.test.tstests/e2e/sdk/ingest.test.tstests/e2e/sdk/query.test.tstests/e2e/sdk/settings.tstests/e2e/sdk/streaming.test.ts
💤 Files with no reviewable changes (11)
- cmd/wavehouse/main.go
- clients/ts/src/client.ts
- clients/ts/src/namespaces.test.ts
- docs/src/content/docs/sdk/reference.md
- internal/api/policy.go
- clients/ts/src/types.ts
- internal/api/router.go
- clients/ts/src/policy.ts
- clients/ts/src/index.ts
- clients/ts/src/client.test.ts
- internal/api/policy_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: Docs build
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (2)
Never hard-wrap prose. One paragraph is one line.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/configuration.mdxCHANGELOG.mdAGENTS.mddocs/src/content/docs/settings-directory.mdxdocs/src/content/docs/access-control.mdxdocs/src/content/docs/development.mddocs/src/content/docs/api.mddocs/src/content/docs/architecture.mddocs/src/content/docs/sdk/admin.mddocs/src/content/docs/reverse-proxy.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.mdxdocs/src/content/docs/settings-directory.mdxdocs/src/content/docs/access-control.mdxdocs/src/content/docs/reverse-proxy.mdx
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-01T16:34:19.270Z
Learning: Run `make lint` and `make test` before considering work complete.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-01T16:34:19.270Z
Learning: Use table-driven tests with `t.Run(tt.name, ...)` for multiple scenarios.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-01T16:34:19.270Z
Learning: Validate locally before every push
📚 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/access-control.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/settings/validate_test.go
🪛 Betterleaks (1.8.1)
docs/src/content/docs/development.md
[high] 208-210: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
CHANGELOG.md
[style] ~37-~37: The word ‘caveat’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “caution” or “warning”.
Context: ...use validateat once.#460`'s migration caveat (a stored policy hard-failing at boot) ...
(CAVEAT)
🔇 Additional comments (17)
internal/settings/validate_test.go (1)
249-251: LGTM!internal/api/router_test.go (1)
371-378: LGTM!tests/e2e/sdk/admin.test.ts (1)
4-4: LGTM!Also applies to: 20-20, 92-92, 102-105
tests/e2e/sdk/ingest.test.ts (1)
12-12: LGTM!Also applies to: 177-177, 180-180, 233-233, 236-236, 292-292, 296-298, 347-347, 360-360, 364-364, 366-366, 380-380
tests/e2e/sdk/query.test.ts (1)
11-11: LGTM!Also applies to: 188-191, 219-222, 255-262, 276-276, 296-297, 307-307, 420-427, 465-465
tests/e2e/sdk/settings.ts (1)
10-10: LGTM!Also applies to: 89-89
tests/e2e/sdk/streaming.test.ts (1)
3-4: LGTM!Also applies to: 11-14
docs/src/content/docs/architecture.md (1)
76-76: LGTM!docs/src/content/docs/development.md (1)
203-210: LGTM!docs/src/content/docs/reverse-proxy.mdx (1)
93-93: LGTM!Also applies to: 203-203
docs/src/content/docs/sdk/admin.md (1)
3-3: LGTM!AGENTS.md (1)
31-31: LGTM!Also applies to: 62-62, 371-377, 424-424
CHANGELOG.md (1)
25-26: LGTM!Also applies to: 37-37
docs/src/content/docs/access-control.mdx (1)
65-77: LGTM!Also applies to: 90-90, 185-185, 198-198, 204-204, 327-340
docs/src/content/docs/api.md (1)
8-8: LGTM!Also applies to: 32-32, 42-42, 617-619, 726-728
docs/src/content/docs/settings-directory.mdx (1)
52-52: LGTM!internal/policy/scalars.go (1)
17-19: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: 5c184d7c-811e-474d-89f2-3788efa003cc
📒 Files selected for processing (11)
CHANGELOG.mdclients/ts/src/client.tscmd/wavehouse/main.godocs/src/content/docs/access-control.mdxdocs/src/content/docs/architecture.mddocs/src/content/docs/configuration.mdxdocs/src/content/docs/sdk/index.mdxinternal/api/router.gointernal/api/router_test.gointernal/auth/auth.gointernal/auth/context.go
💤 Files with no reviewable changes (1)
- clients/ts/src/client.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-01T18:35:09.517Z
Learning: DRY — one source of truth.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-01T18:35:09.517Z
Learning: NO_COLOR=1 make ci > tmp/ci.log 2>&1
📚 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-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
🪛 LanguageTool
CHANGELOG.md
[typographical] ~39-~39: Consider using an em dash in dialogues and enumerations.
Context: - **Policy validation now rejects the fai...
(DASH_RULE)
[style] ~39-~39: The word ‘caveat’ is a legal term. To make your text as clear as possible to all readers, do not use this foreign term unless it is used with its legal meaning. Possible alternatives are “caution” or “warning”.
Context: ...use validateat once.#460`'s migration caveat (a stored policy hard-failing at boot) ...
(CAVEAT)
🔇 Additional comments (10)
CHANGELOG.md (1)
39-39: LGTM!docs/src/content/docs/access-control.mdx (1)
12-12: LGTM!Also applies to: 329-329, 350-350, 398-398
internal/api/router.go (1)
245-245: LGTM!Also applies to: 261-261
internal/api/router_test.go (1)
112-112: LGTM!docs/src/content/docs/sdk/index.mdx (1)
550-550: LGTM!cmd/wavehouse/main.go (1)
570-570: LGTM!internal/auth/auth.go (1)
235-236: LGTM!internal/auth/context.go (1)
74-74: LGTM!docs/src/content/docs/architecture.md (2)
76-77: LGTM!
234-234: LGTM!
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
The policy's entire HTTP surface is removed:
GET /v1/ops/policy,POST /v1/ops/policy/validate, and the SDK'swh.policynamespace. Both endpoints were born alongsidePUT /v1/ops/policyand outlived it when #508 deleted the policy write API — with files as the only write path, the policy is read, edited, and validated where it lives. The dry run had also kept its original lenient decoder while adoption became strict, so it certified{"valid": true}for documents a reload would refuse — including #460's"eq"-for-"_eq"typo, which silently decodes to a filter that disables row security; deleting it removes the last non-strict policy decode site (#514's other five were deleted or made strict by #508). Its replacement iswavehouse validate, which enforces strictly more (the cross-file role references a single-document dry run could never see). The break-glass story narrows to match: the operator key still triggersPOST /v1/ops/settings/reload, whose findings report exactly why a rejected directory was refused; a bad edit still never breaks a running server. The e2e read-modify-write pattern moves fromwh.policy.get()to reading the harness-ownedpolicies.json(readPolicyFile()— the file is the adopted policy there, sincesetPolicyfails unless the reload reports adoption). The SDK keeps the policy document types (Policy,TablePolicy,RolePermissions) — they describepolicies.json;GET /v1/ops/pipes[/{name}]is untouched.validateRolePermsgains three rejections for well-formed rules the engine would silently not honor, the fail-open shapes strict decoding can't see (#460): afilterentry with no operator ("tenant_id": {}— zero predicates, row security off), an operator-lesscheckentry, andfilter:under aninsert:grant. Every adoption — boot, watch,SIGHUP, reload,wavehouse validate— funnels through the onepolicy.Validatepath, so the checks land everywhere at once. No shipped seed, compose, or fixture policy carries any rejected shape. Docs are synced across the API, access-control, settings-directory, configuration, development, architecture, reverse-proxy, and SDK pages, plusAGENTS.mdandCHANGELOG.md.Related Issues
Closes #514
Closes #460