Repository navigation
feat(policy)!: role-first policies.json with split permission types - #551
Merged
Merged
Conversation
Both decisions in classify-paths.sh were `if printf … | grep -qE …; then A; else B; fi`. grep exits 0 on match, 1 on no match, and 2 on error — can't fork/exec, read error, bad pattern — and the else branch collapsed 1 and 2 into the same answer. `set -euo pipefail` does not help: `set -e` is suppressed for a command used as an `if` condition. Observed twice while gating this stack, in runs whose static checks run at -j 14. A different single case failed each time — `mixed-docs-go` answering docs=false, then `dep-bump-go` answering code=false — while every other case passed. That is the signature of a transient grep failure under load, not a pattern bug; the script and its test are unchanged from main and both pass standalone. The test caught it because it asserts expected values. The production path has no such check: CI's `changes` job gates the docs pipeline on this answer, so a docs=false produced by an errored grep skips the docs build and still reports success. Both greps now go through a `matches` helper that aborts with a diagnostic on any exit above 1. The test suite stubs grep onto PATH to prove the abort fires — that case fails against the previous script, which answers confidently instead. Closes #545. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
Additive metadata the upcoming native type layer needs, captured on the same refresh as the columns so they can never describe different servers: - Column gains DefaultExpression and Position, both scanned from the widened system.columns select. TableSchema.Columns was already ordered by position, so declaration order needed no new structure. - TableSchema gains DDL from system.tables.create_table_query. It is json:"-": the schema endpoint marshals TableSchema straight to the client and an external-engine table (S3, MySQL, Kafka) carries its credentials in that statement. A table listed in system.tables with no system.columns rows is skipped, never published column-less. - SchemaRegistry gains ServerVersion(), from a SELECT version() probe next to the existing SELECT timezone(). Both new queries fail the refresh on error, matching timezone() and system.columns: callers keep the prior cache and retry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
`tables.<table>.select.<role>` becomes `tables.<table>.<role>.select`. Field
names and semantics are unchanged; only the nesting moves. `select` and
`insert` are now distinct types (`SelectPermissions`/`InsertPermissions`), so a
field on the wrong side is a validation error instead of being accepted and
ignored.
There is no automatic conversion — convert the file by hand and run
`wavehouse validate` before restarting. A pre-v2 document is reported as one
clear finding pointing at the migration note rather than a confusing
strict-decode error.
Three fail-closed fixes the new shape made visible or possible:
- `ResolvedPermissions` now marks the side `Evaluate` did not resolve. An
unresolved side is zero, and a zero side reads as an empty allow list plus an
empty deny list — which every accessor would answer as "unrestricted". Each
one now denies instead, `HasRowFilter` included: it is the reachable gate in
front of `RowVisible`, so without it the guard behind it was dead code and
both call sites took the whole-bucket fast path.
- `evaluateInsert` resolved a check using an operator it does not honor
(`_neq`/`_gt`/`_lt`, or the ambiguous `_eq`+`_in`) to no clause at all,
authorizing the insert with the rule silently gone — where `evaluateSelect`
denies outright in the mirror situation.
- A `filter` or `check` entry naming no operator (`"tenant_id": {}`) survived a
strict decode, passed `Validate`, and matched no case in either resolver.
Reproduced before fixing: `Allowed=true`, `HasRowFilter()=false`, and
`RowVisible` true for another tenant's row — a declared row-level restriction
applying on neither the query path nor the live stream.
Note on scope: an earlier revision of this work also made
`POST /v1/ops/policy/validate` agree with file adoption. #541 deleted the
policy HTTP surface entirely, so that fix is gone with it — the legacy-layout
detector it relied on is still reached through file adoption's own pipeline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
|
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 (14)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used📓 Path-based instructions (1)In MDX, leave a blank line between a JSX tag and a code fence.📄 CodeRabbit inference engine (AGENTS.md) Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-06-26T12:23:22.696ZApplied to files:
🪛 LanguageTooldocs/src/content/docs/settings-directory.mdx[style] ~69-~69: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) docs/src/content/docs/architecture.md[style] ~147-~147: Consider using “who” when you are referring to a person instead of an object. (THAT_WHO) docs/src/content/docs/access-control.mdx[style] ~170-~170: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~170-~170: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) 🔇 Additional comments (8)
📝 SummarySummary by CodeRabbit
WalkthroughThe policy system now uses role-first table grants with separate select and insert permission types. Validation rejects legacy layouts and unsupported rules. Runtime enforcement, SDK types, fixtures, and documentation were updated to match the new model. ChangesRole-first policy migration
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The role-first policy migration rejects unresolved insert access before batch processing and preserves the intended SDK authorization coverage. No actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 67.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 132 functions across 28 files. (4 skipped: 4 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 |
github-actions
Bot
added
documentation
6 tasks done
Member
Author
|
@coderabbitai review Auto-review is off for PRs whose base isn't the default branch, and this one sits in a stack (base Note for context: this PR's diff against its base is one layer of a seven-PR stack replacing #540. Reviewing it in isolation is the intent — the layers below it are already in its history. |
|
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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: Team
Run ID: cb111291-3274-4ed3-8a64-330a8e29b6ed
📒 Files selected for processing (34)
AGENTS.mdCHANGELOG.mdclients/ts/src/index.tsclients/ts/src/types.tsdeployments/compose/settings/policies.jsondocs/src/content/docs/access-control.mdxdocs/src/content/docs/architecture.mddocs/src/content/docs/settings-directory.mdxinternal/api/errors_test.gointernal/api/ingest.gointernal/api/ingest_test.gointernal/api/structured_query.gointernal/api/structured_query_test.gointernal/auth/auth_test.gointernal/policy/policy.gointernal/policy/policy_test.gointernal/policy/rowfilter.gointernal/policy/rowfilter_test.gointernal/query/builder.gointernal/query/builder_test.gointernal/settings/finding.gointernal/settings/validate.gointernal/settings/validate_test.gointernal/stream/filter_test.gointernal/stream/hub.gointernal/stream/hub_test.gotests/e2e/sdk/admin.test.tstests/e2e/sdk/ingest.test.tstests/e2e/sdk/query.test.tstests/e2e/sdk/settings.tstests/e2e/sdk/setup.tstests/e2e/sdk/streaming.test.tstests/integration/query_limits_test.gotests/integration/rowfilter_narrowing_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: E2E tests
- GitHub Check: Coverage
🧰 Additional context used
📓 Path-based instructions (1)
WH001 applies to every tracked Markdown file, with no carve-out
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/architecture.mdAGENTS.mddocs/src/content/docs/settings-directory.mdxCHANGELOG.mddocs/src/content/docs/access-control.mdx
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-02T10:54:30.110Z
Learning: Every new function should have corresponding test cases.
📚 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:
tests/integration/query_limits_test.gointernal/settings/validate_test.gointernal/policy/policy_test.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.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
docs/src/content/docs/access-control.mdx
[typographical] ~314-~314: Insert a space between the numerical value and the unit symbol.
Context: ...et max_memory_usage as a size string ("4GiB", "512MiB" — IEC vs SI is respected,...
(UNIT_SPACE)
[typographical] ~314-~314: Insert a space between the numerical value and the unit symbol.
Context: ..."512MiB" — IEC vs SI is respected, so "4GB" is 4×109 and "4GiB" is 4...
(UNIT_SPACE)
[typographical] ~314-~314: Insert a space between the numerical value and the unit symbol.
Context: ...ted, so "4GB" is 4×109 and "4GiB" is 4×230) or a bare number...
(UNIT_SPACE)
[typographical] ~461-~461: Insert a space between the numerical value and the unit symbol.
Context: ...(ClickHouse max_memory_usage). Set as "4GiB"/"512MiB" or a number of bytes. 0 ...
(UNIT_SPACE)
[typographical] ~461-~461: Insert a space between the numerical value and the unit symbol.
Context: ...se max_memory_usage). Set as "4GiB"/"512MiB" or a number of bytes. 0 = no role l...
(UNIT_SPACE)
[style] ~461-~461: Consider shortening this phrase to avoid wordiness.
Context: ..._usage). Set as "4GiB"/"512MiB"or a number of bytes.0` = no role limit. Must be non...
(A_NUMBER_OF)
🔇 Additional comments (23)
internal/policy/policy.go (3)
26-40: LGTM!Also applies to: 215-241
714-727: LGTM!Also applies to: 775-781, 819-825
103-106: 🩺 Stability & AvailabilityNo change required. Production callers read only the resolved operation fields:
structured_query.goevaluates"select"before readingperms.Select, andingest.goevaluates"insert"before readingperms.Insert. No mismatched bare field read exists.internal/policy/policy_test.go (2)
14-16: LGTM!Also applies to: 203-243, 613-751
1502-1545: LGTM!Also applies to: 1562-1580, 1582-1613, 1615-1648, 1650-1685
internal/api/ingest.go (1)
378-386: LGTM!internal/api/ingest_test.go (2)
239-239: LGTM!Also applies to: 264-264, 289-291, 729-729, 1043-1043
317-319: LGTM!Also applies to: 351-353, 395-397, 440-442, 473-475, 502-504, 534-534, 1010-1010, 1071-1071, 1601-1603
internal/auth/auth_test.go (1)
128-133: LGTM!Also applies to: 162-166
internal/query/builder_test.go (2)
184-184: LGTM!Also applies to: 287-287, 681-681, 710-710
743-763: LGTM!Also applies to: 788-788, 802-802, 819-819
internal/stream/filter_test.go (1)
31-37: LGTM!Also applies to: 53-53
internal/stream/hub_test.go (2)
123-124: LGTM!Also applies to: 185-186, 225-228
281-281: LGTM!Also applies to: 306-306, 379-379, 418-418, 569-569, 720-720, 760-764
tests/integration/rowfilter_narrowing_test.go (1)
203-203: LGTM!internal/policy/rowfilter.go (1)
14-29: LGTM!Also applies to: 132-137, 147-147
internal/api/errors_test.go (1)
167-167: LGTM!internal/api/structured_query.go (1)
185-186: LGTM!Also applies to: 200-204
internal/query/builder.go (1)
108-110: LGTM!Also applies to: 148-149, 192-192
internal/stream/hub.go (1)
373-373: LGTM!internal/policy/rowfilter_test.go (1)
21-25: LGTM!Also applies to: 350-351
tests/integration/query_limits_test.go (1)
56-63: LGTM!Also applies to: 73-73, 83-83, 98-98
internal/api/structured_query_test.go (1)
160-160: LGTM!Also applies to: 186-186, 214-217, 302-305, 317-317, 340-340, 366-366, 381-381, 397-397, 412-412, 458-458, 477-477, 493-493, 516-519
github-project-automation
Bot
moved this from Backlog
to In review
in WaveHouse Task Board
`database` is a live getter so a ClickHouse reconfigure is honored on the next refresh. Reading it once per query rather than once per refresh let a reconfigure land between the system.columns and system.tables scans, attaching DDL from the new database to same-named schemas discovered from the old one — silently, and it would survive into GET /v1/ops/schema. Refresh now snapshots the name before the first query and threads it to both. Both refresh-failure tests asserted emptiness after a single failed refresh, which is satisfied by the regression they exist to prevent — a failure that wipes an already-published registry. Both now seed a successful refresh first. TestRefresh_DatabaseSnapshottedForWholeRefresh flips the getter between the two scans and fails without the snapshot. CORRECTS TWO CLAIMS IN THIS BRANCH'S EARLIER COMMIT (e10eb04), whose message is already pushed and cannot be amended under the no-force-push rule: 1. "an external-engine table carries its credentials in that statement" is FALSE on the ClickHouse this repo ships. Verified on 26.7.3.19: MySQL, PostgreSQL, S3, Kafka SASL, MongoDB URIs and S3 presigned signatures all render the secret as `[HIDDEN]`. What create_table_query does leak, unconditionally, is topology — endpoint, bucket or host, database, username, S3 access key id. `json:"-"` is still right, for that stronger reason. The password is exposed only on a pre-masking server or one with the server-level display_secrets_in_show_and_select enabled. 2. "captured on the same refresh so they can never describe different servers" overstated the guarantee. chconn.Manager resolves the connection per call, so a reload changing clickhouse.addr mid-refresh can pair a version from one server with schemas from another. It is a publication guarantee, not a same-server one. The read side straddles too: ServerVersion() and Get() take separate RLocks. The rationale is corrected at every site it was replicated — discovery.go, AGENTS.md, architecture.md, api.md, the CHANGELOG — and the test fixture, which had been asserting a literal secret no real server emits, now uses masked DDL and asserts on the bucket and access key id instead. Adds tests/integration/discovery_metadata_test.go: position is 1-based and contiguous against live ClickHouse, DefaultExpression and HasDefault behave as documented for a MATERIALIZED column, DDL is captured, ServerVersion is set. Raised by CodeRabbit on #550 and by the pre-push reviewer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
The pre-v2 layout detector treated `tables.<table>.select` as an operation map whenever its value's keys were not permission fields, so a valid v2 grant for a role literally named `select` was rejected before the strict decode. Raised by CodeRabbit on #551. The first fix bailed out whenever every key was an operation name. That was wrong, and wrong in the dangerous direction — it made this document adopt: {"tables":{"clicks":{"select":{"select":{"allow_columns":["a"]}, "insert":{"allow_columns":["b"]}}}}} Pre-v2 that grants two read-only roles. Read as v2 it grants ONE role read and write: role `select` gained an INSERT the file never contained, and role `insert` silently lost its SELECT. No error, no warning. Caught by the pre-push reviewer; reproduced before this fix. The discriminator is narrower. When the inner key set is exactly the outer operation (`select.select`), the v1 and v2 readings describe identical access, so accepting the v2 decode is free. When it names the OTHER operation (`select.insert`) the readings diverge, and that is precisely the escalating case — so it is refused, with its own message asking for a rename rather than the migration message, which would tell an operator to convert a file that may already be v2. TestReportLegacyPolicyLayout_RoleNamedAfterAnOperation pins all three outcomes and asserts the divergent case does not get the migration message. The previous version of that test asserted the escalating shape as safe. Also corrects `settings-directory.mdx`, which said a leftover `"select": {}` block always fails the undeclared-role check first — that holds only when `roles.json` does not declare a role named `select` — and qualifies the "reported as one clear finding" claims in the CHANGELOG and access-control.mdx, which are not true of the ambiguous shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Team
Run ID: 18b5b81d-07d1-4a94-aa1d-f99386c5ff3d
📒 Files selected for processing (7)
AGENTS.mdCHANGELOG.mddocs/src/content/docs/access-control.mdxdocs/src/content/docs/architecture.mddocs/src/content/docs/settings-directory.mdxinternal/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. (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.mdxdocs/src/content/docs/access-control.mdxdocs/src/content/docs/architecture.mdCHANGELOG.mdAGENTS.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/settings-directory.mdxdocs/src/content/docs/access-control.mdx
🧠 Learnings (1)
📓 Common learnings
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 551
File: docs/src/content/docs/settings-directory.mdx:69-69
Timestamp: 2026-09-02T12:04:04.547Z
Learning: In Wave-RF/WaveHouse `internal/settings/validate.go`, an empty legacy policy block such as `"select": {}` is not detected as the pre-v2 operation-first layout because `looksLikeRoleMap` returns false for an empty object. It decodes as an empty `TablePolicy` grant for a role literally named `select`. If `roles.json` does not declare `select`, `checkRoleRefs` emits an undeclared-role error and skips the empty-grant warning. If `roles.json` declares `select`, validation emits the neither-select-nor-insert warning and adopts the policy.
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 551
File: internal/settings/validate.go:288-288
Timestamp: 2026-09-02T12:18:32.228Z
Learning: In Wave-RF/WaveHouse PR `#551`, `internal/settings/validate.go` policy-layout detection must treat an operation-named role carefully. For a `tables.<table>.<outer-operation>` object whose nested keys are operation names, the role-first and operation-first readings are semantically identical when the key set contains only the outer operation; it is safe to accept the role-first reading. If the nested key set includes the other operation, the readings diverge and can silently widen privileges, so validation must reject the document with an ambiguity error that asks the operator to rename the role, not with the legacy migration message. Do not return early while iterating a Go map to classify operation-name keys because mixed keys make the result nondeterministic.
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-02T15:34:49.365Z
Learning: Every new function should have corresponding test cases.
🪛 LanguageTool
docs/src/content/docs/access-control.mdx
[style] ~170-~170: Since ownership is already implied, this phrasing may be redundant.
Context: ...clicks.select.insert` — is refused with its own message asking you to rename the role, ...
(PRP_OWN)
[style] ~170-~170: Since ownership is already implied, this phrasing may be redundant.
Context: ...in either direction. A role named after its own operation (`tables.clicks.select.select...
(PRP_OWN)
docs/src/content/docs/architecture.md
[style] ~120-~120: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read.
Context: ... to discover ClickHouse table schemas. Each refresh also records the server version (SELECT version()), joins system.tablesfor each table'screate_table_query(kept in-process asTableSchema.DDLand markedjson:"-"— an external-engine table renders its wiring in that statement — endpoint, bucket/host, database, username, access key id — so it must never reach/v1/ops/schema; ClickHouse masks the password as [HIDDEN]from ~23.9, so what is withheld here is the topology), reads each column'sdefault_expressionand 1-basedposition alongside its type, discovers the server's default time zone (SELECT timezone()) and bakes every DateTime/DateTime64 column's canonicalization spec (precision + resolved zone) into the cached schema, so the per-record ingest path parses no type strings and loads no zones ([#372`](#372)). Supports periodic auto-refresh, on-d...
(TOO_LONG_SENTENCE)
CHANGELOG.md
[typographical] ~23-~23: Consider using an em dash in dialogues and enumerations.
Context: - **Policy format v2: policies.json is ...
(DASH_RULE)
[style] ~23-~23: Since ownership is already implied, this phrasing may be redundant.
Context: ...fferent operation, or both — so it gets its own error asking you to rename the role rat...
(PRP_OWN)
AGENTS.md
[style] ~42-~42: Consider using the typographical ellipsis character here instead.
Context: ...equest; settings.Store in production, Static(q...) in tests) - policy/ — Hasura-st...
(ELLIPSIS)
🔇 Additional comments (6)
internal/settings/validate.go (2)
264-278: LGTM!Also applies to: 304-312
322-355: LGTM!internal/settings/validate_test.go (1)
551-559: LGTM!AGENTS.md (1)
43-43: LGTM!Also applies to: 63-63
CHANGELOG.md (1)
23-23: LGTM!docs/src/content/docs/architecture.md (1)
120-120: LGTM!Also applies to: 147-148
Four items from the chtypes-side review of #551: - `looksLikeRoleMap`'s doc comment still described the withdrawn first attempt, where that function itself special-cased an operation-named key and the ambiguous case "lost" to the v2 reading. Neither is true: the body only rejects permission-field keys, and the ambiguous case is refused by `operationNamedRoles`, not resolved. Rewritten to match. - Re-add the two operator-less rows dropped in the role-first rewrite of `validate_test.go`. `policy_test.go` still pinned the rule, but the settings layer is where an operator actually hits it. - Drop `ResolvedInsert.CheckPredicates`. It had no reader and a rule that deliberately diverges from the one ingest enforces (fail-closed vs auto-inject ""), which is a trap to leave lying around unread. It can come back in the PR that reads it, reviewed against a real consumer. - Note at `ingest.go`'s bare `CheckClauses` read why the `unresolved` marker cannot defend that line, at the call site rather than only on the type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
EricAndrechek
dismissed
coderabbitai[bot]’s stale review
September 2, 2026 18:00
The base branch was changed.
# Conflicts: # internal/api/boot_chain_test.go
|
📚 Docs preview is live → https://b3f372c4-wavehouse-docs.wave-rf.workers.dev
|
EricAndrechek
marked this pull request as ready for review
September 2, 2026 18:08
Contributor
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit 91a448b in the Show a line coverage summary of the most impacted files.
Updated |
Member
Author
|
@coderabbitai review Head moved to The single conflict was The last commit that changed anything on this branch is 🤖 Generated with Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head="72663d81"
printf '%s\n' '=== Merge parents and commit metadata ==='
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$head"
parents=($(git show -s --format=%P "$head"))
if [ "${`#parents`[@]}" -ne 2 ]; then
echo "Expected a two-parent merge commit; found ${`#parents`[@]} parent(s)." >&2
exit 1
fi
first_parent="${parents[0]}"
main_parent="${parents[1]}"
printf '%s\n' '=== Files changed by merge relative to PR parent ==='
git diff --name-status "$first_parent" "$head"
printf '%s\n' '=== Files changed by merge relative to main parent ==='
git diff --name-status "$main_parent" "$head"
printf '%s\n' '=== Conflict-resolution file identity against main-side parent ==='
git diff --quiet "$main_parent" "$head" -- internal/api/boot_chain_test.go \
&& echo 'internal/api/boot_chain_test.go: identical to main-side parent' \
|| echo 'internal/api/boot_chain_test.go: differs from main-side parent'
printf '%s\n' '=== Files introduced since last reviewable commit ==='
git diff --name-status ff8bddfd "$head"
printf '%s\n' '=== Changelog headings and unreleased entries ==='
rg -n -C 2 '^## Unreleased$|^### |^- ' CHANGELOG.md
printf '%s\n' '=== Duplicate non-empty changelog lines ==='
awk '
/^## Unreleased$/ { in_unreleased=1; next }
/^## / && in_unreleased { exit }
in_unreleased && NF {
count[$0]++
if (count[$0] == 2) print NR ":" $0
}
' CHANGELOG.md
printf '%s\n' '=== PR comparison summary ==='
gh pr view 551 --repo Wave-RF/WaveHouse --json baseRefName,headRefOid,mergeable,state,statusCheckRollup \
--jq '{baseRefName, headRefOid, mergeable, state, checks: [.statusCheckRollup[] | {name, conclusion, status}]}'Length of output: 201214 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="72663d81"
read -r first_parent main_parent < <(git show -s --format='%P' "$head")
echo "commit: $(git rev-parse "$head")"
echo "parents: $first_parent $main_parent"
echo "subject: $(git show -s --format=%s "$head")"
echo '--- merge delta vs first parent ---'
git diff --name-status "$first_parent" "$head"
echo '--- merge delta vs main-side parent ---'
git diff --name-status "$main_parent" "$head"
echo '--- boot_chain_test identity ---'
printf 'merge: '
git rev-parse "$head:internal/api/boot_chain_test.go"
printf 'main-parent: '
git rev-parse "$main_parent:internal/api/boot_chain_test.go"
echo '--- delta after ff8bddfd ---'
git diff --name-status ff8bddfd "$head"
echo '--- unreleased heading count ---'
grep -c '^## Unreleased$' CHANGELOG.md
echo '--- duplicate non-empty Unreleased lines ---'
awk '
/^## Unreleased$/ { inside=1; next }
/^## / && inside { exit }
inside && NF { count[$0]++; if (count[$0] == 2) print NR ":" $0 }
' CHANGELOG.md
echo '--- discovery mention count in Unreleased ---'
awk '
/^## Unreleased$/ { inside=1; next }
/^## / && inside { exit }
inside { print }
' CHANGELOG.md | grep -ci 'discovery' || trueLength of output: 1552 🏁 Script executed: #!/bin/bash
set -euo pipefail
head="72663d81"
set -- $(git show -s --format='%P' "$head")
if [ "$#" -ne 2 ]; then
echo "Expected two merge parents, found $#" >&2
exit 1
fi
first_parent="$1"
main_parent="$2"
echo "commit: $(git rev-parse "$head")"
echo "parents: $first_parent $main_parent"
echo "subject: $(git show -s --format=%s "$head")"
echo '--- merge delta vs first parent ---'
git diff --name-status "$first_parent" "$head"
echo '--- merge delta vs main-side parent ---'
git diff --name-status "$main_parent" "$head"
echo '--- boot_chain_test identity ---'
printf 'merge: '
git rev-parse "$head:internal/api/boot_chain_test.go"
printf 'main-parent: '
git rev-parse "$main_parent:internal/api/boot_chain_test.go"
echo '--- delta after ff8bddfd ---'
git diff --name-status ff8bddfd "$head"
echo '--- unreleased heading count ---'
grep -c '^## Unreleased$' CHANGELOG.md
echo '--- duplicate non-empty Unreleased lines ---'
awk '
/^## Unreleased$/ { inside=1; next }
/^## / && inside { exit }
inside && NF { count[$0]++; if (count[$0] == 2) print NR ":" $0 }
' CHANGELOG.md
echo '--- discovery mention count in Unreleased ---'
awk '
/^## Unreleased$/ { inside=1; next }
/^## / && inside { exit }
inside { print }
' CHANGELOG.md | grep -ci 'discovery' || trueLength of output: 3134
I found no actionable issues. The merge changes only
PR You are interacting with an AI system. |
3 tasks done
taitelee
previously approved these changes
Sep 3, 2026
taitelee
left a comment
taitelee
left a comment
Member
There was a problem hiding this comment.
I do appreciate the reorganization of how policy is formatted with operations and permissions. Didn't find any potential issues in internal/settings/validate.go with roles being named select or insert. I just have some questions about the code structure on certain things, but they're more open ended so I'm approving for now.
taitelee's review question on #551: why an `unresolved` bool rather than pointers? Because the answer is that the bool is papering over a real ambiguity, and pointers remove it. Before, the ZERO value of a side meant two opposite things. `IsAdmin` returned `&ResolvedPermissions{Allowed: true}` with both sides zero, and that is what made an admin unrestricted. Evaluate's skipped side was ALSO zero, marked only by an unexported bool. So "no restrictions" and "you asked the wrong question" were the same value, and a bare field read could not tell them apart — an unresolved `Select.WhereClause` is `""`, which is an unfiltered query. Now `Select *ResolvedSelect` / `Insert *ResolvedInsert`, nil means unresolved, and the admin path constructs both sides explicitly. That ordering is deliberate: if a site were missed, an admin gets denied — loud and caught by tests — where nil-means-unrestricted would silently widen. Two consequences taitelee predicted, both real: - A hand-built value can now express "not resolved". It could not before: the flag was unexported, so every fixture outside this package read as permissive. TestHandBuiltPermissions_NilSideDenies is the new capability; the old KeepPlainReading test is split, since its premise inverted. - The cross-side isolation rows now set BOTH sides on purpose. With one side nil they would have passed for the wrong reason — denied for being unresolved rather than for the list not applying. On the nine bare field reads: I did NOT add nil guards. A guard on `perms.Select.WhereClause` skips the row filter, which is the exact silent widening this removes. Comments at each site say so, because the "fix" is tempting. What I got wrong three times while writing this, each corrected by running it rather than reading: those bare reads do NOT panic in current code. An accessor or validator denies first at every one of them — validateAndAuthorizeColumns in the query path, and schema validation plus the IsColumnAllowed loop on ingest. The panic is a backstop for a future ordering change, not the live guarantee. The comments now say that, and TestBuild_InsertResolvedGrantIsRejected pins the error rather than a panic that never fires. taitelee's second question mostly dissolves: moving ingest's CheckClauses loop into the policy package was to get it the guard the accessors have, and it now has one either way.
Review of 60e77a7 found that one of my "verified by running it" claims was verified on a single schema and is false in general. The comment on ingest's bare `perms.Insert.CheckClauses` read said schema validation rejects a record with no columns. It does not: discovery.Validate only requires a column that is neither nullable nor defaulted, so a table whose columns are all nullable or all defaulted accepts `{}`. The column loop above then runs zero times and the bare read IS reached. I ran the case against `clicks`, where `page` is required, and generalized from one schema — the same mistake I have now made four times on this class, this time despite having executed it. So this takes taitelee's second suggestion after all, in the form that does not widen: a `ResolvedPermissions.CheckClauses() (map[string]any, bool)` accessor that returns ok=false on an unresolved insert side, with the caller REJECTING the record. A plain nil guard would have skipped the checks — the vacuous pass this whole change exists to remove. Also from the review: - builder.go named one denier where there are three, which left the other two reading as unguarded. A query that names columns is stopped by validateAndAuthorizeColumns; `select_all` alone by resolveProjection; a bare aggregation by IsAggregationAllowed. All three are now named and all three are pinned — the test covered only the first, i.e. only the shape that does NOT skip validateAndAuthorizeColumns. - policy_test.go's TestColumnPrimitives_AgreeOnVisibility had one fixture left unconverted. It still passed, but for a different reason: it used to mean "no lists, unrestricted" and had silently become "read side never resolved", duplicating case 0 and dropping the unrestricted shape from the agreement check. Converted, and the nil-side reading added as its own case. - TestEvaluate_UnresolvedSideFailsClosed's doc comment described the zero side as the unresolved one, which is now exactly backwards. - architecture.md and AGENTS.md still documented the deleted `unresolved` marker. architecture.md also inverted the design: it said accessors deny "rather than reading an empty grant as unrestricted", but an empty grant IS unrestricted now — that is what the admin return builds. Note on the docs wording: the reviewer's suggested replacement asserted that a bare field read panics. Its own analysis had already shown the accessors deny first, so that would have reintroduced the claim this commit corrects. The prose says "would panic if that ordering ever changed" instead.
Review of d7b5e6c. No MUST; four SHOULDs, all real. The best one is an irony worth recording: the commit whose whole point was "builder.go named one denier of three" left the identical overclaim in the TEST's own doc comment, one line above the loop that fixed it. Two more inaccuracies sat with it — "the two column-free shapes skip validateAndAuthorizeColumns entirely" is false for aggregation-only, which ENTERS that function and is denied inside it, and builder.go's third bullet dropped the `validateAndAuthorizeColumns →` hop the other two show. More importantly the test only asserted `require.Error`, so it would have passed just as happily if all three shapes collapsed onto one denier — making "one shape per denier" a prose claim rather than a checked one. Each shape now asserts its OWN error: ForbiddenColumnError / ErrNoReadableColumns / ForbiddenAggregationError. All three pass, which is what actually establishes the routing. CheckClauses' nil receiver returned false, inverting the convention every sibling accessor documents ("a nil receiver means no policy applies — all columns are allowed"). Not a live bug, but the trap points the wrong way: a new call site written by someone who internalized the sibling convention would make a deployment with no policy store 403 every ingest. Now returns (nil, true) for a nil receiver, and keeps fail-closed for the case that matters — a grant that WAS resolved, for the other operation. The doc says which is which. The unresolved case was also returned as a per-record reject, but perms is resolved once per REQUEST, so it is true for every record or none: a 10k-record batch would have emitted 10k ERROR lines and reported 10k independent permission failures for one mis-wired grant. It is a requestAbort now, matching processRecord's own contract. And CheckClauses was the only fail-closed accessor with no test, in the commit that added it — every sibling is pinned on a nil side. Now pinned in three places, including that an EMPTY insert side answers (nil, true): "unrestricted", not "unresolved", which is the distinction this whole PR turns on. Docs: my own sentence stated the guard relation backwards — "reached only after an accessor has already denied" describes something that cannot happen, since a denial means the read never occurs. And rewriting AGENTS.md's Evaluate sentence split a noun list and stranded "with JWT claim templating" in front of Source, which does not do templating. Both fixed; CheckClauses added to the accessor enumeration it was missing from.
Review of 27fc4d2. No MUST; two SHOULDs and a MAY, all consequences of the abort conversion in that same commit. The first is the day's recurring defect in miniature. I changed policy.go's wording from "reject this record" to "refuse the write" IN THAT COMMIT, precisely because converting to requestAbort made "record" wrong — and missed the caller-side comment fifteen lines from the code, which still said "ok=false REJECTS the record". In this file `reject` is a named concept with the opposite semantics. The second matters more. requestAbort's doc enumerated its causes exhaustively and characterized them as transient — "can't accept this record right now", "so the caller can retry the batch". The new 403 is a caller/config bug: not in the list, not transient, not retryable. handleBatch's doc compounded it, still saying a record failing "a permission rule" is recorded and the batch continues — which now names the wrong bucket for this one. The type doc is where that contract lives, so a reader asking "can a batch return non-200?" was getting the wrong answer. Both now distinguish the transient causes, where abandoning the tail is what makes a retry safe, from this one, where it cannot help. The MAY was worth taking: the abort-not-reject decision was load-bearing prose in three comments and pinned by zero tests — the same argument I accepted for replacing require.Error with per-denier assertions one file over. TestProcessRecord_UnresolvedInsertSideAborts calls processRecord directly and asserts abort non-nil, reject nil, 403, and no Retry-After. Verified load-bearing: reverting the return to a recordReject fails it. It also does something the prose could not. Its `require.NoError(discovery.Validate(schema, map[string]any{}))` line PROVES the reachability claim rather than asserting it — all-nullable/defaulted columns accept `{}`, the column loop runs zero times, and the CheckClauses call is reached. That is the claim I got wrong two commits ago by testing only against a schema with a required column.
"...constrain visible rows with `filter` instead. The mirror of the `filter`-under-`insert` rejection." — the second sentence has no verb. It was a trailing clause before an earlier rewrite in this PR replaced the sentence in front of it, and it also left the two sibling paragraphs out of parallel: the `filter` one at :229 ends cleanly on "constrain inserted values with `check` instead." Pre-existing on this branch rather than from the pointer work, but it is in the same file this PR rewrote.
…can produce
Two SHOULDs from the final review, both one-token/one-sentence.
The abort taxonomy was stated in THREE places in this file and I fixed two.
batchResult's doc still carried the pre-403 enumeration — 30 lines above
requestAbort, on the response type, which is where a reader asking "can a batch
return non-200?" lands first.
Rather than update the third copy, it now defers: the list lives on
requestAbort and only there. That is the structural fix the reviewer named for
the drift this branch kept producing — both of today's late findings were the
same defect, a list stated in more places than one, and updating copy three of
three would only have reset the clock. Also reflowed the over-long line the
previous insertion left in handleBatch's doc, which both reviewers spotted.
The test fixture was worse than untidy. `{Name: "a", Type: "String",
IsNullable: true}` is a column state discovery CANNOT produce: IsNullable is
derived from the type string (a literal `Nullable(` prefix test), so a real
String column is never nullable. Every other fixture in the tree spells it
`Nullable(String)`.
That matters because this fixture backs the reachability claim I already got
wrong once. The doc comment justified it with "all-nullable/defaulted columns
accept an empty record", resting on a flag that cannot be set that way in
production — and the registry the test builds held a `loose` whose column `a`
was actually REQUIRED, disagreeing with the schema pointer the test passes in.
Harmless while processRecord is called directly; a wrong answer the moment
anyone re-points it at the HTTP path, where the registry's copy is the one
consulted.
The docs reviewer returned ship_it with zero findings, having independently
confirmed the five remaining bare reads and that the 403 correctly stays out of
api.md's error tables.
The previous commit's message said the list "lives there and only there". It did not. A fourth copy sat inside handleBatch, three lines upstream of the code that commit changed, surviving untouched from main. It was also the worst of the four. Not merely incomplete — its rationale contradicted requestAbort's doc outright: "stop and surface the status so the caller retries the batch instead of treating a system outage as per-record loss". Retrying cannot help a grant resolved for the wrong operation, and a config bug is not a system outage. This branch is what made it wrong, by adding a non-transient cause to a list that assumed they were all transient. Now defers like the other two. `grep backpressure` returns exactly one enumeration (requestAbort's doc) plus two mentions that only explain when Retry-After is set, which stay true. Worth stating plainly: a commit written specifically to collapse a duplicated list into one canonical place missed a copy of that list. Grepping for the concept rather than trusting the count is the habit; I asserted "only there" without running it.
taitelee
approved these changes
Sep 3, 2026
github-project-automation
Bot
moved this from In progress
to Done
in WaveHouse Task Board
EricAndrechek
added a commit
that referenced
this pull request
Sep 4, 2026
#551 landed on main as 27c22cb, carrying the ResolvedPermissions pointer refactor. This absorbs it. Twelve files conflicted. Nine were #551's alone — stack/3 held the pre-merge version of them and contributed nothing, so they take main's: policy.go, rowfilter.go, builder.go, builder_test.go, policy_test.go, structured_query.go, filter_test.go, access-control.mdx, AGENTS.md. Three were genuinely two-sided: - ingest.go — one hunk, and it was #551's too: stack/3 had the bare `perms.Insert.CheckClauses` read with the old `unresolved`-marker comment, main has the CheckClauses() accessor and the request-abort branch. Took main's. #552's Content-Type resolution in the same file merged cleanly and is intact. - CHANGELOG.md — kept #552's Content-Type entry; main contributed nothing to that hunk. Both entries are present. - architecture.md — the conflicting hunk was #551's policy.go bullet, so main's (rewritten for the nil-side shape). #552's ingest bullet and its Ingest Path flow step merged cleanly. Verified after resolution rather than assumed: go build, go vet and go test ./... all clean, make verify passes, and both sides' work is present — ingestFormatOne still resolves through mime.ParseMediaType with the hand-rolled splitter still deleted, and policy.go still has the pointer sides and CheckClauses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Sep 4, 2026
## Summary `POST /v1/ingest` used to sniff the body and treat the header as a hint: the first non-whitespace byte chose between a single object and an array, and an NDJSON body whose first line happened to start with `[` was re-framed as a JSON array — silently, as a whole-request reinterpretation rather than a per-record error. The header is now required and authoritative. A request declaring nothing, or a media type not in the accepted list, is `415` before the body is parsed, with the supported types named in the body. The declared type chooses the format *family* — `application/json` versus the four NDJSON spellings — and within the JSON family the first non-whitespace byte still picks array versus single object. The bytes never choose the family, so an NDJSON body is read as NDJSON whatever its first byte and a bad line fails as a per-record error. The header is parsed by `mime.ParseMediaType`, so the grammar is RFC 9110's §8.3 `media-type` rather than one of ours. Only the media type selects the format; parameters are ignored, so no malformed parameter costs the request — `; charset`, `;;`, a value left mid-quote, even a name repeated with different values all read as `application/json`. One exception, below: a malformed parameter on a line that also contains a comma. **Duplicate declarations are the part most worth reviewing.** `Content-Type` is a *singleton* field (§8.3), and §5.3 forbids repeating a field line unless the field allows comma-list recombination — `media-type` does not. So both duplicate spellings are malformed input, and §8.3 says so directly, warning that recipients who resolve the resulting pseudo-list "by using the last syntactically valid member" cause "interoperability and security issues". Ingest therefore takes no member: - **Repeated header lines** (what curl and many proxies send) are all resolved and must agree on the format *and* on whether ingest reads it at all. `application/x-ndjson` alongside `application/ndjson; charset=utf-8` reads as NDJSON, because once they agree, which one gets honored stops mattering. Disagreement is `415` rather than resolution to the first — honoring the first would let an NDJSON body be read as one JSON object, ingesting record one and discarding the rest behind a `200`. - **A comma-joined value** (what a proxy merging duplicates sends) is refused. Where it fails outright there is no media type to take. Where it does parse a media type — `application/json; charset=utf-8, application/x-ndjson` yields `application/json` — it is still refused, because the comma may be a second declaration joined on and the error cannot distinguish that from a comma inside data. The security-critical detail is in `ingestFormatOne`. `ParseMediaType` returns `ErrInvalidMediaParameter` both for a merely-malformed parameter *and* when a second declaration was comma-joined on after a parameter — `application/json; charset=utf-8, application/x-ndjson` yields mediatype `application/json`. Tolerating that error unconditionally silently resolves a joined disagreement to its first member, which is exactly the truncation above. The two are indistinguishable from the error alone, so a comma on a line that did not parse cleanly is refused. That guard fails closed, and it is where I would look first. **Four additional shapes 0.1.0 accepted now `415`** (beyond repeated header lines that disagree, which it also accepted; see the CHANGELOG): a present-but-empty header; a value with a leading or trailing comma; a comma-joined value that does not parse as a single media type; and a malformed parameter on a line that also carries a comma. The last is an over-rejection and is tracked in #563. Note a comma is not disqualifying on its own: inside a quoted parameter value it is legal data, so `application/json; a=", application/x-ndjson; b="` is one media type and is accepted, even though an intermediary may have built it by illegally joining two lines. The server cannot tell. The TS SDK sends `application/json` for a single object and `application/x-ndjson` for arrays and `insertNDJSON`, on every path, so no SDK caller is affected. A hand-rolled client that relied on sniffing must now declare the type. ## Stacked PR This is **part 3 of 7** (parts 0, 1 and 2 merged) in a stack that replaces #540. Each PR is based on the one above it, so review this PR's own diff against its base — GitHub shows only this layer's changes. | # | Branch | Base | | | - | ------ | ---- | - | | 0 | `stack/0-classify-paths` | `main` | ✅ merged as #549 | | 1 | `stack/1-discovery` | `main` | ✅ merged as #550 | | 2 | `stack/2-policy` | `main` | ✅ merged as #551 | | 3 | `stack/3-content-type` | `main` | **→ this PR** | | 4 | `stack/4-seams` | `stack/3-content-type` | | | 5 | `stack/5-positional-wire` | `stack/4-seams` | | | 6 | `stack/6-computed-columns` | `stack/5-positional-wire` | | Merge in order, top to bottom. Rebasing or squashing out of order will make the later PRs' diffs unreadable. ## Response-size bound The 415 echoes what the caller declared, and it is decided **before the body is read** — so a header-only request, needing no credentials under the shipped compose policy, could size the response. Measured before the fix: 1.03 MB of headers produced a 4.65 MB body (and far worse over HTTP/2, where HPACK indexes a repeated header line to about a byte on the wire). Both dimensions are caller-controlled and both are bounded now: at most **four distinct** declarations, each capped at 128 bytes, then `"…and N more"`. The declaration that actually disagreed is pinned into the echo, so four agreeing spellings cannot crowd it out. Pinned by tests that fail if either bound is removed. ## Test plan - [x] `make ci` green locally on this branch's exact tree (verify, unit, integration against live ClickHouse, e2e, all coverage gates). Now that #551 has merged and this PR's base is `main`, GitHub CI runs on `pull_request` automatically — earlier runs on this branch were manual dispatches, so check the SHA before relying on an old one. - [x] The branch descends from its base and carries only this layer's change (plus any follow-up commits answering review) - [x] `go.mod` / `go.sum` untouched; no new dependencies ## Review Both pre-push reviewers ran against this branch — no logged skip. They gated every push, over several rounds; the last rounds reviewed the `mime.ParseMediaType` rewrite as a fresh change rather than as a delta, since the earlier approvals covered code the rewrite deleted. Reviewer findings that shaped the result, rather than only polishing it: - The first cut of the `ErrInvalidMediaParameter` guard tolerated the error unconditionally, which reintroduced the silent-truncation hazard the agreement rule exists to prevent. - A fourth (now fifth) tightening was neither listed nor pinned, and the known-limit test could not have caught a regression in it. - "A duplicate parameter name is a 415" was false: Go only errors when the values differ. - The docs' RFC delegation was falsifiable by their own example with its closing quote dropped. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
EricAndrechek
added a commit
that referenced
this pull request
Sep 4, 2026
Absorbs the current #552, which now also carries #551 (squash-merged to main as 27c22cb) through its own main merge. Done ahead of #552 landing so the eventual main merge has little left to resolve — the #551 experience was that a branch holding the PRE-squash copy conflicts on every shared file. Five conflicts, all genuinely two-sided, because both branches rewrote the same ingest path for different reasons. - ingest.go — stack/4 moved Content-Type resolution BEFORE the body cap (no reason to buffer bytes for a request already known to be unreadable) and buffers the body into a pooled buffer; stack/3 rewrote what resolution does (mime.ParseMediaType, the pin, the bounded echo). Kept stack/4's ordering and buffering with stack/3's resolver and comments. One thing the compiler caught and I had wrong: I took stack/3's newRecordReader call along with its comment, which reads r.Body — on this branch the reader must read the pooled buffer. - record_reader.go — stack/3 adds mediaTypePrefix; stack/4 changed newRecordReader to take an already-buffered []byte. Both kept, with stack/3's reasoning for taking the format and stack/4's note about who bounds the body. - api.md — stack/3 retired "Parameters are ignored…" (the declaration note supersedes it); stack/4 rewrote the 413 paragraph for the atomic cap and the NDJSON bound. Kept stack/4's paragraph without the retired sentence. - architecture.md — stack/3 rewrote the Content-Type sentences; stack/4 appended the pooled-buffer/O(body) sentence. Spliced. - ingest_test.go — imports; both needed. Verified after resolving rather than assumed: build, vet and go test ./... clean, make verify passes, and all four contributions are present — the mime switch, echoSafe/disagreeingIndex, the pooled buffer and seams, and #551's pointer sides with CheckClauses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tables.<table>.select.<role>becomestables.<table>.<role>.select. Fieldnames and semantics are unchanged; only the nesting moves.
selectandinsertare now distinct types (SelectPermissions/InsertPermissions), so afield on the wrong side is a validation error instead of being accepted and
ignored.
There is no automatic conversion — convert the file by hand and run
wavehouse validatebefore restarting. A pre-v2 document is reported as oneclear finding pointing at the migration note rather than a confusing
strict-decode error.
Three fail-closed fixes the new shape made visible or possible:
ResolvedPermissionsnow marks the sideEvaluatedid not resolve. Anunresolved side is zero, and a zero side reads as an empty allow list plus an
empty deny list — which every accessor would answer as "unrestricted". Each
one now denies instead,
HasRowFilterincluded: it is the reachable gate infront of
RowVisible, so without it the guard behind it was dead code andboth call sites took the whole-bucket fast path.
evaluateInsertresolved a check using an operator it does not honor(
_neq/_gt/_lt, or the ambiguous_eq+_in) to no clause at all,authorizing the insert with the rule silently gone — where
evaluateSelectdenies outright in the mirror situation.
filterorcheckentry naming no operator ("tenant_id": {}) matched nocase in either resolver.
mainalready rejects this at validation — fix(policy): reject no-op rules, drop the policy HTTP surface #541added that guard — so file adoption was never affected; what this PR adds is
the matching deny in
evaluateSelect/evaluateInsert, becauseEvaluatedoesnot re-validate what it is handed and
policy.Staticis a Validate-freepolicy.Source. Reproduced through that path before fixing:Allowed=true,HasRowFilter()=false, andRowVisibletrue for another tenant's row.Note on scope: an earlier revision of this work also made
POST /v1/ops/policy/validateagree with file adoption. #541 deleted thepolicy HTTP surface entirely, so that fix is gone with it — the legacy-layout
detector it relied on is still reached through file adoption's own pipeline.
Stacked PR
This is part 2 of 7 (part 0 merged) in a stack that replaces #540. Each PR is based on the one above it, so review this PR's own diff against its base — GitHub shows only this layer's changes.
stack/0-classify-pathsmainstack/1-discoverymainstack/2-policystack/1-discoverystack/3-content-typestack/2-policystack/4-seamsstack/3-content-typestack/5-positional-wirestack/4-seamsstack/6-computed-columnsstack/5-positional-wireMerge in order, top to bottom. Rebasing or squashing out of order will make the later PRs' diffs unreadable.
Test plan
make cigreen on this branch's exact tree (verify, unit, integration against live ClickHouse, e2e, all coverage gates)go.mod/go.sumuntouched; no new dependenciesReview
Both pre-push reviewers gate the tip of the stack (#555), whose delta against
mainis the union of all seven branches. This branch was additionally reviewed on its own.🤖 Generated with Claude Code
https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ