feat(ingest)!: require a declared Content-Type and honor it by EricAndrechek · Pull Request #552 · Wave-RF/WaveHouse

EricAndrechek · 2026-09-02T05:44:44Z

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

make ci green locally on this branch's exact tree (verify, unit, integration against live ClickHouse, e2e, all coverage gates). Now that feat(policy)!: role-first policies.json with split permission types #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.
The branch descends from its base and carries only this layer's change (plus any follow-up commits answering review)
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.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ

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

`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 something ingest does not read, 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. Parameters are ignored (`application/json; charset=utf-8` is `application/json`), matching `mime.ParseMediaType`. The TS SDK already sent `application/json` on every ingest call, so `.insert()` is unaffected. A hand-rolled client that relied on sniffing must now declare the type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ

coderabbitai · 2026-09-02T05:44:50Z

📝 Summary

Summary by CodeRabbit

Breaking Changes
- POST /v1/ingest now requires a supported, authoritative Content-Type header.
- Missing, unsupported, malformed, or conflicting declarations return HTTP 415.
- Declared types determine JSON or NDJSON parsing; JSON object-versus-array handling remains body-driven.
Documentation
- Updated API, architecture, SDK, changelog, and reverse-proxy guidance.
- Documented supported NDJSON aliases and content-type forwarding requirements.
SDK
- Single-object ingest requests now explicitly send application/json.

Walkthrough

/v1/ingest now requires a supported Content-Type. JSON and NDJSON declarations select parsing behavior, JSON body shape selects arity, and invalid declarations return HTTP 415. The TypeScript SDK and documentation now declare and describe these rules.

Changes

Ingest content-type contract

Layer / File(s)	Summary
Declared format selection `internal/api/record_reader.go`	The reader resolves supported JSON and NDJSON declarations, checks repeated headers, bounds echoed declarations, and selects the parsing mode. JSON arity remains body-driven.
Handler validation and coverage `internal/api/ingest.go`, `internal/api/ingest_test.go`, `tests/e2e/sdk/ingest.test.ts`	The handler resolves content types before body parsing and returns 415 for missing, unsupported, or conflicting declarations. Unit and end-to-end tests cover resolution, duplicate headers, bounded error output, and accepted types.
Client and documentation alignment `clients/ts/src/table.ts`, `clients/ts/src/table.test.ts`, `docs/src/content/docs/api.md`, `docs/src/content/docs/architecture.md`, `docs/src/content/docs/sdk/queries.md`, `docs/src/content/docs/reverse-proxy.mdx`, `AGENTS.md`, `CHANGELOG.md`	The SDK sets and tests `application/json` for single-object inserts. Documentation, design guidance, proxy guidance, and the changelog describe the required content-type contract and accepted aliases.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟡 Moderate · up to 074d5

Ingest now requires an authoritative Content-Type, but requests containing an empty header line alongside a valid declaration may incorrectly receive 415 instead of being accepted. This can reject otherwise valid traffic forwarded through systems that preserve blank header values.

Suggested reviewers: taitelee

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Handle
  participant resolveContentType
  participant newRecordReader
  participant IngestPublisher
  Client->>Handle: POST /v1/ingest with Content-Type
  Handle->>resolveContentType: resolve declared content type
  resolveContentType-->>Handle: return JSON or NDJSON format
  Handle->>newRecordReader: pass format and body
  newRecordReader-->>Handle: return parsed record reader
  Handle->>IngestPublisher: publish parsed records
  IngestPublisher-->>Handle: return publish result
  Handle-->>Client: return ingest result or HTTP 415

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 65.63% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 6 files. (3 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the main change: requiring and honoring the declared Content-Type for ingest requests.
Description check	✅ Passed	The description is detailed and directly explains the Content-Type negotiation changes, rejection behavior, SDK updates, tests, and stack context.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 65.63% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 6 files. (3 skipped: 3 unsupported.)

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch stack/3-content-type

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch stack/3-content-type

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

EricAndrechek · 2026-09-02T06:33:50Z

@coderabbitai review

Auto-review is off for PRs whose base isn't the default branch, and this one sits in a stack (base stack/2-policy). Requesting a review explicitly.

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.

coderabbitai · 2026-09-02T06:34:06Z

@EricAndrechek I will review PR #552 against stack/2-policy. I will limit the review to this stack layer.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

`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

EricAndrechek · 2026-09-02T13:03:08Z

@coderabbitai review

coderabbitai · 2026-09-02T13:03:14Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: b60a1610-ea03-443c-ada9-c8d35ebd5d88

📥 Commits

Reviewing files that changed from the base of the PR and between 1e92574 and 3b36e89.

📒 Files selected for processing (9)

CHANGELOG.md
clients/ts/src/table.ts
docs/src/content/docs/api.md
docs/src/content/docs/architecture.md
docs/src/content/docs/sdk/queries.md
internal/api/ingest.go
internal/api/ingest_test.go
internal/api/record_reader.go
tests/e2e/sdk/ingest.test.ts

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)

Default to top-down Never sit two large diagrams side-by-side.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/architecture.md

Never hard-wrap prose.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/architecture.md
CHANGELOG.md
docs/src/content/docs/sdk/queries.md
docs/src/content/docs/api.md

🧠 Learnings (3)

📚 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/api/record_reader.go

📚 Learning: 2026-06-26T12:23:22.696Z

Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 346
File: internal/stream/subscriber_test.go:9-28
Timestamp: 2026-06-26T12:23:22.696Z
Learning: In this Go repository, prefer table-driven tests (e.g., `[]struct{...}` with `t.Run(...)`) only for tests that cover multiple scenarios/inputs and can be cleanly enumerated. Do not artificially rewrite a clear single-scenario sequential behavioral-flow test into a table-driven form just to fit the pattern; if there’s only one meaningful scenario, keep the test as a straightforward linear flow (as in `TestSubscriber_SendDeliversThenDropsWhenFull`).

Applied to files:

internal/api/ingest_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/ingest_test.go

🔇 Additional comments (3)

clients/ts/src/table.ts (1)

17-19: LGTM!

Also applies to: 125-127

docs/src/content/docs/architecture.md (1)

80-80: LGTM!

tests/e2e/sdk/ingest.test.ts (1)

281-291: LGTM!

Also applies to: 293-315

The unsupported-media-type test checked the status and two of the five accepted types. It now asserts the JSON error envelope and its security headers via testutil.AssertJSONErrorResponse — which this branch was missing, though the same assertion exists further up the stack, so the split dropped it onto the wrong layer — and iterates supportedContentTypes. On what that loop pins: it cannot catch an alias being dropped from supportedContentTypes, because the message is built from the same slice and the loop would simply check one fewer. Verified. What it does catch is the message diverging from the list, so a client is never told to use a type the server accepts but never names; verified by truncating the Join and watching it fail. Also rewords the documented 415 cause in three places: "or one ingest doesn't read" was not a condition a reader could act on. Now "or an unsupported one". Raised by CodeRabbit on #552. 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

…policy

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

Pre-push review findings on the previous commit: - The `CheckClauses` call-site comment I just added claimed "Evaluate only returns Allowed with both sides resolved". That is false, and false in the direction that matters: `evaluateSelect` returns `Allowed: true` with `Insert` marked `unresolved`, which is the whole point of the marker. A maintainer trusting that sentence would conclude a cross-side bare read is safe anywhere. The real reason this line is safe is narrower — the handler resolved this grant for `insert` — so say that. - `ResolvedPredicate`/`ResolvePredicates` were exported by this PR only because `ResolvedInsert.CheckPredicates` needed a public type. That field is gone and nothing outside `internal/policy` names either identifier, so they go back to main's unexported spelling, along with the two prose mentions that moved with them. - `architecture.md`'s rowfilter bullet lost its third list item to a spliced-in clause: `ColumnSpec` read as a coordinate of the `HasRowFilter` gate explanation with no verb attaching it. The gate explanation moves to the end of the bullet. - The CHANGELOG's "without it" attached the `RowVisible`-true consequence to the insert-side bind-unsafe deny; that consequence belongs to the select-side operator-less deny. Check clauses never reach `RowVisible`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ

Three CodeRabbit threads on #551: - `access-control.mdx` claimed an old-layout document is always reported as one clear error. An *empty* operation block (`"select": {}`) carries no role names to give it away, so it reads as a grant for a role named `select` and either trips the undeclared-role error or — if `select` is declared — warns and adopts. State the exception. - `settings-directory.mdx` described only the migration rejection. The ambiguity refusal (a role named after the *other* operation) is a second, distinct one with its own message, and the operator who hits it was being told their file uses the pre-v2 layout. Both spellings are now stated, including that the same-operation form is accepted. - Pin the mixed-key classification in `validate_test.go`: a pre-v2 block carrying one operation-named role beside a real one. The real name settles the reading, so it is pre-v2 and gets the migration message, not the ambiguity refusal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ

`ingestFormat` discarded the parsed media type on any `ParseMediaType` error. Go returns `ErrInvalidMediaParameter` *together with* a usable media type, so headers that this endpoint read fine before the tightening started answering 415: application/json; charset → 415, was ingested application/json; boundary= → 415, was ingested application/json;; → 415, was ingested application/json; charset="unterm → 415, was ingested application/x-ndjson;charset → 415, was ingested Over-rejecting is not what this PR is for — the intended tightening is "an absent type, or one ingest does not read", not "a syntactically perfect parameter list". Now only an error that leaves no usable media type refuses. That distinction is narrower than it looks, and worth stating because a review raised the joined-header case as if this fix covered it: a proxy that merges two `Content-Type` headers into `application/json, application/json` produces a *different* error ("unexpected content after media subtype") and an empty media type, so it is still refused — correctly, since there are two declarations and no basis to pick one. Verified each case against the stdlib rather than reasoned about. Reverting the guard fails exactly the five new cases and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: b93963f7-c212-46a1-86f6-2150fe07f384

📥 Commits

Reviewing files that changed from the base of the PR and between 3c7462f and a275821.

📒 Files selected for processing (6)

CHANGELOG.md
docs/src/content/docs/api.md
docs/src/content/docs/architecture.md
internal/api/ingest.go
internal/api/ingest_test.go
internal/api/record_reader.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: Docs build
GitHub Check: Coverage
GitHub Check: Integration tests
GitHub Check: E2E tests
GitHub Check: Lint

🧰 Additional context used

📓 Path-based instructions (4)

Create `*_test.go` files in the same package as the code under test.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/api/ingest_test.go

Every code change updates its docs + `CHANGELOG.md` in the same PR Never hard-wrap prose.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

CHANGELOG.md

Create the package under `internal/`.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/api/ingest.go
internal/api/record_reader.go
internal/api/ingest_test.go

Never hard-wrap prose.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/architecture.md
docs/src/content/docs/api.md

🧠 Learnings (2)

📓 Common learnings

Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse

Timestamp: 2026-09-02T21:18:54.681Z
Learning: In the Go ingest API for `POST /v1/ingest`, all `Content-Type` declarations, whether repeated header lines or comma-joined values, are resolved individually and must agree on both the resolved `IngestFormat` and supportedness (`err == nil`). A supported-plus-unsupported pair such as `application/json` and `text/csv` returns the conflicting-header HTTP 415 response. This check cannot compare `IngestFormat` alone because `ingestFormatOne` returns `FormatJSON` on its error path.

Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse

Timestamp: 2026-09-03T11:23:06.429Z
Learning: In `internal/api/record_reader.go`, `Content-Type` is a singleton `media-type` field. Do not comma-split a `Content-Type` header line. Resolve each repeated header line with `mime.ParseMediaType`. Tolerate `mime.ErrInvalidMediaParameter` because parameters do not select `IngestFormat`, except when the uncleanly parsed header line contains a comma. In that case, reject it as `errUnsupportedContentType`; otherwise a comma-joined second declaration can be ignored and cause an NDJSON body to be read as a single JSON object.

📚 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/ingest_test.go

🪛 GitHub Check: CodeQL

internal/api/ingest.go

[failure] 221-221: Clear-text logging of sensitive information
Sensitive data returned by HTTP request headers flows to a logging call.

[failure] 229-229: Clear-text logging of sensitive information
Sensitive data returned by HTTP request headers flows to a logging call.

🪛 LanguageTool

docs/src/content/docs/architecture.md

[style] ~147-~147: Consider using “who” when you are referring to a person instead of an object.
Context: ...rite-side accessor for the one consumer that iterates a side's map instead of asking...

(THAT_WHO)

CHANGELOG.md

[typographical] ~25-~25: Consider using an em dash in dialogues and enumerations.
Context: - **Ingest now requires a declared `Conte...

(DASH_RULE)

🔇 Additional comments (6)

internal/api/record_reader.go (1)

112-121: LGTM!

Also applies to: 276-320, 380-445, 451-463, 484-489

internal/api/ingest.go (1)

80-82: LGTM!

Also applies to: 111-123, 201-233, 299-303, 344-345, 436-459

internal/api/ingest_test.go (1)

8-15: LGTM!

Also applies to: 1210-1236, 1250-1253, 1484-1491, 1514-1526, 1843-1843, 2164-2210, 2212-2285, 2287-2379

CHANGELOG.md (1)

25-25: LGTM!

docs/src/content/docs/api.md (1)

191-199: LGTM!

Also applies to: 266-266, 376-376

docs/src/content/docs/architecture.md (1)

80-80: LGTM!

Also applies to: 147-147

CodeRabbit: a rejected request resolved its own headers THREE times — disagreeingIndex ran inside resolveContentType, again in Handle to compute the log's pin, and a third time inside contentTypeMessage. Each pass re-parses every declaration through mime.ParseMediaType. This is mine, and it accumulated in two steps that each looked reasonable alone. 548f10e added the pin, computed inside contentTypeMessage. a275821 then hoisted a pin in Handle for the log but LEFT contentTypeMessage recomputing, and separately made resolveContentType delegate to disagreeingIndex while fixing the duplicated-predicate finding. Two computations became three, in the commit written to stop this invariant drifting. I took that shape because the review offered both options — thread it through, or leave contentTypeMessage recomputing at a measured 1.6 ms — and I chose the smaller diff. Wrong trade on an invariant that had ALREADY broken once: the response was pinned and the log was not, so the operator's record buried the declaration that caused the refusal. Adding a third computation papered over the drift instead of removing the conditions for it. resolveContentType now returns the index, and Handle threads the one value to both consumers. Three call sites become one, and the log/response agreement is structural rather than three independent computations trusted to match. Verified by mutation: forcing the log's pin back to -1 — the exact bug that shipped — fails TestIngest_ConflictLogNamesTheDisagreement. Worth recording why nine review rounds missed it: every round was scoped to its delta, so none asked what the accumulated shape had become. CodeRabbit read the file cold and saw it immediately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw

Three shaping items from the shippability pass. No behavior change — the 415 body and the WARN log name exactly what they named before. `echoSafe` was still computed twice per rejected request: once for the log, once inside `contentTypeMessage`. Pure function, same arguments, so not a live bug — but it is the residue of the same shape the previous commit removed for the pin: an invariant ("the log and the body name the same declarations") upheld by two call sites passing matching arguments rather than by structure. That is exactly how they diverged before. `Handle` now computes `decls` once and both consumers read it; `contentTypeMessage` takes the already-bounded slice. One echoSafe call site in production code, down from two. Mutation-verified: making the body re-bound with its own pin — the old divergence — fails two tests. `disagreeingIndex`'s doc still said "Same predicate as resolveContentType's". Since d517212 there is no second predicate to be the same as; resolveContentType calls this function. A reader chasing the comparison found nothing. It now says it IS that predicate. And errUnsupportedContentType's doc enumerated three causes and omitted the one this PR exists for: a comma-bearing line that did not parse cleanly. That case is easy to miss precisely because its media type may BE in the accepted list — `application/json; charset=utf-8, application/x-ndjson` resolves to application/json and is still refused. A reader who stopped at the error variable would conclude an accepted media type cannot produce it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw

taitelee

Just a few things Claude found. Core code changes LGTM.

Two findings from taitelee's review. AGENTS.md invariant #3 still described ingest as "takes flat JSON, validated against the discovered schema. No envelope." — nothing about the declared Content-Type choosing the format, or the 415 landing before the body is read. That file is the index an agent reads before touching internal/api, and §Doc Sync pairs it with architecture.md for exactly this kind of change. A fail-closed precondition belongs there. One clause added. The second is the better catch, and it is an operational one I had missed entirely: the comma-join case is explained only in api.md, but the people who hit it are operators with a proxy, not callers. reverse-proxy.mdx enumerates every other header whose forwarding matters — Authorization, X-Operator-Key, the X-Forwarded-* family, CORS — and never mentioned Content-Type. The failure mode is specific and quiet. A proxy appending a second header LINE is fine: both are resolved and accepted when they agree. A proxy that MERGES duplicates into one comma-joined value — Envoy's `append: true`, some WAF rewrite rules — produces `application/json, application/json`, which is a 415 on every ingest even though both halves are identical. Someone flipping that setting would see ingest fail fleet-wide with nothing on the proxy page to point at. Verified rather than repeated: two agreeing LINES resolve to json with no error; the MERGED value returns errUnsupportedContentType. The bullet says forward it verbatim, explains the line-vs-merge distinction, and links back to the api.md section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: d96365e4-a37c-401f-8ea6-80d1e8a6984f

📥 Commits

Reviewing files that changed from the base of the PR and between a275821 and 44759fa.

📒 Files selected for processing (3)

internal/api/ingest.go
internal/api/ingest_test.go
internal/api/record_reader.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 build
GitHub Check: E2E tests
GitHub Check: Integration tests
GitHub Check: Coverage

🧰 Additional context used

📓 Path-based instructions (2)

Create `*_test.go` files in the same package as the code under test.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/api/ingest_test.go

Create the package under `internal/`.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

internal/api/record_reader.go
internal/api/ingest_test.go
internal/api/ingest.go

🧠 Learnings (1)

📓 Common learnings

Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse

Timestamp: 2026-09-02T21:18:54.681Z
Learning: In the Go ingest API for `POST /v1/ingest`, all `Content-Type` declarations, whether repeated header lines or comma-joined values, are resolved individually and must agree on both the resolved `IngestFormat` and supportedness (`err == nil`). A supported-plus-unsupported pair such as `application/json` and `text/csv` returns the conflicting-header HTTP 415 response. This check cannot compare `IngestFormat` alone because `ingestFormatOne` returns `FormatJSON` on its error path.

Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse

Timestamp: 2026-09-03T11:23:06.429Z
Learning: In `internal/api/record_reader.go`, `Content-Type` is a singleton `media-type` field. Do not comma-split a `Content-Type` header line. Resolve each repeated header line with `mime.ParseMediaType`. Tolerate `mime.ErrInvalidMediaParameter` because parameters do not select `IngestFormat`, except when the uncleanly parsed header line contains a comma. In that case, reject it as `errUnsupportedContentType`; otherwise a comma-joined second declaration can be ignored and cause an NDJSON body to be read as a single JSON object.

🪛 GitHub Check: CodeQL

internal/api/ingest.go

[failure] 218-218: Clear-text logging of sensitive information
Sensitive data returned by HTTP request headers flows to a logging call.

[failure] 226-226: Clear-text logging of sensitive information
Sensitive data returned by HTTP request headers flows to a logging call.

The AGENTS.md clause I added one commit ago stated two things the code does not do, and both were contradicted by documentation in this same PR — one of them by the bullet three lines away in the same commit. "an absent, unreadable or DUPLICATED declaration is a 415" is false for duplicates. Repeated header lines that AGREE are accepted; only disagreement refuses. api.md, architecture.md and the new reverse-proxy bullet all say so correctly. The harm is specific to where it sat: an agent reading the invariant index before touching internal/api would take "duplicated implies 415" as the thing to preserve, and could "fix" the accommodation #563 deliberately kept. "so a mis-declared body fails per-record rather than being silently re-framed" holds in one direction only. Declared-NDJSON with a bad line does fail per-record. The reverse — NDJSON bytes sent as application/json — is NOT caught: objectReader takes the first object and ignores the rest, answering 200 for one record. That is #561, and it is exactly what commit 32c5bf8 was written to disclaim. I reasserted it unqualified while compressing the rule into an index entry. Both corrected, and verified by running resolveContentType rather than by trusting the review: agreeing repeated lines return no error, disagreeing ones return the conflict sentinel, comma-joined returns unsupported. Also from the review, on reverse-proxy.mdx: the bullet documented the LOUD proxy failure (merge, 415 fleet-wide) and not the silent one, which is the case where "forward it verbatim" matters most. A layer that REPLACES the header rather than appending — a WAF normalizing to application/json, an API-gateway mapping, or the BFF this page itself suggests re-issuing the request — turns every NDJSON batch into a 200 with only the first record ingested, and the operator's only stated cue was a 415 that never comes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw

I asked the docs reviewer to falsify the wording IT had supplied last round, on the grounds that "the reviewer wrote it" is authority, not verification — which is what cost rounds 10 and 11 on this branch. Both halves were false. "unparseable ... is a 415" inverts the change this PR deliberately made. On a comma-free line ingestFormatOne re-parses the media-type prefix and accepts it. Verified by running all four: `; charset=a; charset=b`, `;;`, `; charset` and a mid-quote value are ALL accepted, and ingest_test.go pins them with "refusing them would be a regression, not the intended tightening". The index would have told an agent to reject exactly what 4d18506 was written to keep. "repeated lines that agree are accepted" is false unqualified: two agreeing `text/csv` lines are a 415. They agree — both resolve to (JSON, error) — so they take the unsupported path. Verified. Rewritten from the probe output rather than from the suggestion: the media type must be supported and parseable, a comma-bearing value must parse whole, repeated lines must resolve to the same SUPPORTED format, and a malformed parameter on a comma-free line never costs the request. Swept the same claim through the three api.md sites that carried it — :197's "one that doesn't parse", which contradicted :195 two sentences earlier, and both 415 table rows, which are standalone and are where a caller lands after being refused. Fix-one-copy has been this branch's recurring defect; moving the claim with its copies is the whole lesson. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw

Accuracy bucket empty this round — every claim in the rewritten invariant and the three api.md edits verified against the real resolver, including that different spellings of one format are accepted, which is why "same supported FORMAT" is right and "same media type" would have been a new false claim. Two mechanical items. My previous commit's replacement left `media type,,` in BOTH 415 rows. Neither misspell nor markdownlint owns a doubled comma, so it would have rendered on wavehouse.dev in the table a caller lands on after being refused. That is the second time a scripted substitution has left damage the linters do not catch; the first was the api.md row edit that silently never applied at all. And AGENTS.md had dropped "as a whole" from the comma clause — the qualifier 32c5bf8 deliberately restored to the same claim in api.md. The argument is stronger here than there: AGENTS.md's readers are the agents editing internal/api, which makes them exactly the people who know mime.ParseMediaType("application/json; charset=utf-8, application/x-ndjson") hands back ("application/json", ErrInvalidMediaParameter). Such a reader can take "media type unsupported or unparseable" (it is neither) together with "does not parse as one media type" (Go handed one back) and conclude the value is accepted, when it is a 415. All five copies of the rule now carry the qualifier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: d7d7112f-273f-4e90-a3e5-3ee3ff996297

📥 Commits

Reviewing files that changed from the base of the PR and between 44759fa and 074d551.

📒 Files selected for processing (3)

AGENTS.md
docs/src/content/docs/api.md
docs/src/content/docs/reverse-proxy.mdx

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: Unit tests
GitHub Check: Docs build
GitHub Check: E2E tests
GitHub Check: Coverage
GitHub Check: Integration tests
GitHub Check: Lint
GitHub Check: Analyze (go)

🧰 Additional context used

🧠 Learnings (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/reverse-proxy.mdx

🔇 Additional comments (2)

docs/src/content/docs/api.md (1)

197-197: LGTM!

Also applies to: 266-266, 376-376

docs/src/content/docs/reverse-proxy.mdx (1)

199-200: LGTM!

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

#552 landed on main as a single squashed commit (e203f3a), which carries no ancestry link to stack/3-content-type. Git therefore re-derives the whole stack diff against this branch and conflicts in four files, even though 6b095d5 already merged that exact content here. Resolved with -s ours because it discards nothing: main gained exactly one commit over stack/3-content-type, and `git diff stack/3-content-type main` is empty, so main's tree is identical to content this branch already carries. The tree is unchanged by this merge; only the ancestry is recorded, which is what makes every future merge from main clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw

#554's approval means this branch can finally sit on its real base — it was 102 commits behind, still carrying pre-#552 Content-Type handling (ingestFormat, unsupportedContentTypeMessage) that has since been rewritten twice. Merged with #554 preferred on every conflict, since its versions are the reviewed ones. That resolution is not sufficient on its own: -X theirs only governs CONFLICTING hunks, so #555's copies of work that MOVED into #554 survived wherever they landed at a different position. Three duplicates found and removed: - clients/ts/src/types.ts declared `default_kind` TWICE in one interface — #554's field and #555's, at different offsets so they never conflicted. TypeScript would have rejected it. Kept one, folding in the fact #555's wording had that #554's lacked (a record naming a computed column is rejected) alongside the one #554 had (they remain queryable). - CHANGELOG.md carried two entries each for the insertable-envelope change and the check-clause guard — #554's in Changed, #555's older versions in Fixed. Removed #555's. - architecture.md's streaming flow stated the drift re-announcement twice. Kept #554's, which is followed by the #543 live-vs-replay detail. What genuinely remains from this branch is small and real: richer request and SSE examples in api.md, a Bring Your Own Schema section in deployment.md covering the computed-column consequences for DDL authors, the numbered batching and DLQ points in AGENTS.md that #554 left describing schema-column order, a codegen example showing default_kind, and dlq_test.go moving from map literals to the typed EventMessage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

#553 landed on main as a single squashed commit (f72c8c6) carrying no ancestry link to stack/4-seams, so git re-derives the whole stack diff against this branch and conflicts — even though 6b095d5 already merged that exact content here. Same pattern as when #552 squashed. Resolved with -s ours because the interlock says it discards nothing: main gained exactly one commit over stack/4-seams, and `git diff stack/4-seams main` is empty, so main's tree is content this branch already carries. The tree is unchanged by this merge; only the ancestry is recorded, which is what makes every later merge from main clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

) ## 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 Wave-RF#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 Wave-RF#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 Wave-RF#549 | | 1 | `stack/1-discovery` | `main` | ✅ merged as Wave-RF#550 | | 2 | `stack/2-policy` | `main` | ✅ merged as Wave-RF#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 Wave-RF#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> Upstream-Commit: e203f3a

EricAndrechek and others added 4 commits September 1, 2026 21:03

github-project-automation Bot added this to WaveHouse Task Board Sep 2, 2026

github-project-automation Bot moved this to Backlog in WaveHouse Task Board Sep 2, 2026

github-actions Bot added documentation Improvements or additions to documentation go Pull requests that update go code area/api HTTP handlers, routing, middleware area/sdk TypeScript SDK (clients/ts/) area/docs Documentation, site/, README labels Sep 2, 2026

EricAndrechek mentioned this pull request Sep 2, 2026

feat!: policy v2, positional wire shape, and declared ingest format #540

Closed

6 tasks

coderabbitai Bot requested changes Sep 2, 2026

View reviewed changes

Comment thread docs/src/content/docs/api.md Outdated

Comment thread internal/api/ingest_test.go

github-project-automation Bot moved this from Backlog to In review in WaveHouse Task Board Sep 2, 2026

EricAndrechek and others added 10 commits September 2, 2026 10:33

Merge remote-tracking branch 'origin/stack/1-discovery' into stack/2-…

191fb56

…policy

Merge remote-tracking branch 'origin/main' into stack/1-discovery

ac735f9

Merge stack/1-discovery into stack/2-policy

0188d42

Merge stack/2-policy into stack/3-content-type

6ca8fed

EricAndrechek marked this pull request as ready for review September 4, 2026 16:29

EricAndrechek requested review from a team and taitelee September 4, 2026 16:29

coderabbitai Bot requested changes Sep 4, 2026

View reviewed changes

Comment thread internal/api/record_reader.go Outdated

github-project-automation Bot moved this from In progress to In review in WaveHouse Task Board Sep 4, 2026

EricAndrechek and others added 2 commits September 4, 2026 12:52

taitelee requested changes Sep 4, 2026

View reviewed changes

Comment thread docs/src/content/docs/architecture.md

Comment thread docs/src/content/docs/api.md

github-advanced-security Bot found potential problems Sep 4, 2026

View reviewed changes

Comment thread internal/api/ingest.go Dismissed

Comment thread internal/api/ingest.go Dismissed

coderabbitai Bot requested changes Sep 4, 2026

View reviewed changes

Comment thread internal/api/record_reader.go

EricAndrechek and others added 3 commits September 4, 2026 14:08

coderabbitai Bot requested changes Sep 4, 2026

View reviewed changes

Comment thread docs/src/content/docs/api.md

coderabbitai Bot approved these changes Sep 4, 2026

View reviewed changes

taitelee approved these changes Sep 4, 2026

View reviewed changes

github-project-automation Bot moved this from In review to In progress in WaveHouse Task Board Sep 4, 2026

EricAndrechek added this pull request to the merge queue Sep 4, 2026

Merged via the queue into main with commit e203f3a Sep 4, 2026
19 checks passed

github-project-automation Bot moved this from In progress to Done in WaveHouse Task Board Sep 4, 2026

EricAndrechek deleted the stack/3-content-type branch September 4, 2026 19:05

This was referenced Sep 5, 2026

docs(api,deployment): examples and BYOS notes for the positional wire #555

Merged

feat(stream)!: positional rows on NATS and SSE, columns sent apart #554

Merged

Conversation

EricAndrechek commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Stacked PR

Response-size bound

Test plan

Review

Uh oh!

coderabbitai Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

EricAndrechek commented Sep 2, 2026

Uh oh!

coderabbitai Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EricAndrechek commented Sep 2, 2026

Uh oh!

coderabbitai Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taitelee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EricAndrechek commented Sep 2, 2026 •

edited

Loading

coderabbitai Bot commented Sep 2, 2026 •

edited

Loading

coderabbitai Bot commented Sep 2, 2026 •

edited

Loading

coderabbitai Bot commented Sep 2, 2026 •

edited

Loading