refactor(ingest): buffer the body, add type-layer seams and encoder by EricAndrechek · Pull Request #553 · Wave-RF/WaveHouse

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

Summary

This is the seam work the native type layer lands against, plus the memory
trade that comes with it.

Responses are unchanged except at the body cap, where buffering makes the
413 atomic — and that is a real behavior change, not a refactor artifact:

An over-cap batch used to publish the records it had already decoded
before the cap surfaced mid-iteration. A client retrying that 413 re-sent
them. Now nothing is published, so a 413 is safe to retry once split.
An over-cap single object followed by an oversized tail used to answer
200, having ingested that first object and silently discarded the rest. Now
it is a 413.
One new response shape: 400 {"error":"invalid request body"} when the body
cannot be read at all (malformed transfer encoding, truncated upload), which
previously surfaced through the decoder as invalid json.

Both cap changes are improvements, and the CHANGELOG and api.md error tables
carry them.

The handler now reads the whole (already MaxBytesReader-capped) body into a
pooled *bytes.Buffer and runs the record readers over those bytes rather than
the live connection, so the 413 surfaces at that read instead of
mid-iteration and the 415 is decided from the header before a byte is read.

The memory profile is NOT unchanged, and that is the deliberate part. Streaming
meant peak resident bytes on the order of one record — NDJSON scanned line by
line, the array path let json.Decoder compact after each element. Peak is now
O(body) per in-flight request, and bytes.Buffer doubles, so peak allocation
can exceed the cap before MaxBytesReader errors. maxPooledBufferBytes (1
MiB) caps what a request hands back to the pool, not its peak, and nothing
bounds total in-flight bytes — the ceiling is concurrency × the 16 MiB
data-plane cap, which has no operator knob, so the outer limit is the proxy's.
Kept because the type layer needs the body addressable rather than consumed; a
server-side bound is tracked in #544, and the reverse-proxy guide now says to
size the container for concurrency rather than for one request.

Three decision points became interfaces with default implementations that
delegate to today's code unchanged: RecordValidator (schema validation +
timestamp canonicalization — the two calls stay where they are, with the
check-clause block between them, since merging them would move checks onto
canonicalized values), InsertChecker (the _eq and _in comparisons), and
stream.RowEvaluator (row visibility, reached by both the live fan-out and
replay through the one shared admission step).

ingest.EncodeCompactRow lands here unused — the positional encoder the wire
change ahead of it will call.

Stacked PR

This is part 4 of 7 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`
1	`stack/1-discovery`	`stack/0-classify-paths`
2	`stack/2-policy`	`stack/1-discovery`
3	`stack/3-content-type`	`stack/2-policy`
4	`stack/4-seams`	`stack/3-content-type`	→ this PR
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.

Test plan

make ci green on this branch's exact tree (verify, unit, integration against live ClickHouse, e2e, all coverage gates)
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 gate the tip of the stack (#555), whose delta against main is the union of all seven branches. This branch carries a logged skip (scripts/skip-pre-push-review.sh) on that basis, recorded in tmp/review-skips-*.log.

🤖 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

…encoder Observable behavior is unchanged. This is the seam work the native type layer lands against, plus the memory trade that comes with it. The handler now reads the whole (already `MaxBytesReader`-capped) body into a pooled `*bytes.Buffer` and runs the record readers over those bytes rather than the live connection, so the `413` surfaces at that read instead of mid-iteration and the `415` is decided from the header before a byte is read. The memory profile is NOT unchanged, and that is the deliberate part. Streaming meant peak resident bytes on the order of one record — NDJSON scanned line by line, the array path let `json.Decoder` compact after each element. Peak is now O(body) per in-flight request, and `bytes.Buffer` doubles, so peak allocation can exceed the cap before `MaxBytesReader` errors. `maxPooledBufferBytes` (1 MiB) caps what a request hands back to the pool, not its peak, and nothing bounds total in-flight bytes — the ceiling is concurrency × the 16 MiB data-plane cap, which has no operator knob, so the outer limit is the proxy's. Kept because the type layer needs the body addressable rather than consumed; a server-side bound is tracked in #544, and the reverse-proxy guide now says to size the container for concurrency rather than for one request. Three decision points became interfaces with default implementations that delegate to today's code unchanged: `RecordValidator` (schema validation + timestamp canonicalization — the two calls stay where they are, with the check-clause block between them, since merging them would move checks onto canonicalized values), `InsertChecker` (the `_eq` and `_in` comparisons), and `stream.RowEvaluator` (row visibility, reached by both the live fan-out and replay through the one shared admission step). `ingest.EncodeCompactRow` lands here unused — the positional encoder the wire change ahead of it will call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: cae672d8-46e0-405d-a7dc-62dac4198761

📥 Commits

Reviewing files that changed from the base of the PR and between 5be3dd7 and c807b46.

📒 Files selected for processing (14)

CHANGELOG.md
clients/ts/src/table.ts
docs/src/content/docs/api.md
docs/src/content/docs/architecture.md
docs/src/content/docs/reverse-proxy.mdx
docs/src/content/docs/sdk/queries.md
internal/api/ingest.go
internal/api/ingest_seams.go
internal/api/ingest_seams_test.go
internal/api/ingest_test.go
internal/api/record_reader.go
internal/ingest/compact.go
internal/stream/hub.go
internal/stream/roweval_test.go

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. (8)

GitHub Check: Coverage
GitHub Check: Integration tests
GitHub Check: Docs build
GitHub Check: Unit tests
GitHub Check: E2E tests
GitHub Check: Lint
GitHub Check: Analyze (go)
GitHub Check: Analyze (go)

🧰 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_seams_test.go
internal/api/ingest_seams.go
internal/ingest/compact.go
internal/api/ingest.go
internal/stream/roweval_test.go
internal/api/ingest_test.go
internal/api/record_reader.go
internal/stream/hub.go

Never hard-wrap prose.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/reverse-proxy.mdx
docs/src/content/docs/sdk/queries.md
docs/src/content/docs/architecture.md
docs/src/content/docs/api.md

🧠 Learnings (5)

📓 Common learnings

Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 553
File: internal/api/bufpool.go:18-24
Timestamp: 2026-09-02T16:08:40.177Z
Learning: In WaveHouse, ingest intentionally buffers the complete `http.MaxBytesReader`-capped request body because later native type-layer work requires an addressable body. `internal/api/bufpool.go` limits only idle buffer retention through `maxPooledBufferBytes`; it does not limit aggregate in-flight request memory. The aggregate buffered-ingest memory limit is tracked in GitHub issue `#544` and is intentionally out of scope for PR `#553` because it would add a new request-rejection behavior. The planned mitigation is a configurable server-side in-flight byte budget acquired before the body read and released on every exit path.

📚 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

📚 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_seams_test.go
internal/stream/roweval_test.go
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_seams_test.go
internal/api/ingest_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/api/record_reader.go

🪛 LanguageTool

docs/src/content/docs/reverse-proxy.mdx

[typographical] ~101-~101: Insert a space between the numerical value and the unit symbol.
Context: ...lient_max_body_size 16m;- **Caddy** —request_body { max_size 16MB }` (Caddy has no body limit by defa...

(UNIT_SPACE)

[style] ~107-~107: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ... just for one request] Ingest reads the whole body into memory before parsing it, so ...

(EN_REPEATEDWORDS_WHOLE)

[style] ~107-~107: ‘On top of that’ might be wordy. Consider a shorter alternative.
Context: ...cates the same 32 MiB before the 413. On top of that sits the decoded Go value of the record...

(EN_WORDINESS_PREMIUM_ON_TOP_OF_THAT)

docs/src/content/docs/sdk/queries.md

[style] ~43-~43: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...imits); bounded-concurrency chunking of very large arrays is tracked in [#196](https://git...

(EN_WEAK_ADJECTIVE)

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)

[style] ~148-~148: 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

[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)

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

(DASH_RULE)

docs/src/content/docs/api.md

[style] ~322-~322: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...rror anywhere in a JSON array fails the whole request. Both forms report a wrong-type...

(EN_REPEATEDWORDS_WHOLE)

[style] ~374-~374: Consider using the typographical ellipsis character here instead.
Context: ...s mid-value is invalid json | | 400 | {"error":"invalid json: ..."} | A structural JSON syntax error, a ...

(ELLIPSIS)

🔇 Additional comments (1)

internal/ingest/compact.go (1)

19-22: 🗄️ Data Integrity & Integration

Do not flag input_format_null_as_default here.

EncodeCompactRow is not used by the production ingest path. insertToClickHouse sends envelope.Data with FORMAT JSONEachRow, so the encoder’s null placeholders do not reach ClickHouse.

📝 Summary

Summary by CodeRabbit

Changed
- Ingest requests now buffer the complete request body before parsing.
- The 16 MiB ingest limit applies consistently to all supported body formats, including NDJSON.
- Unsupported content types are rejected before the request body is read.
- Oversized requests return 413 without ingesting any records.
- Malformed or incomplete request bodies return 400 without publishing records.
- Ingest memory usage scales with concurrent upload size; large buffers are not retained.
- Row-level visibility filtering remains enforced for live updates and replayed data.

Walkthrough

The ingest handler now buffers request bodies before parsing and resolves content types before reading. Validator, insert-check, and stream row-visibility decisions use nil-safe interfaces with default implementations. Compact row encoding and related tests were added.

Changes

Ingest and stream seams

Layer / File(s)	Summary
Buffered ingest request path `internal/api/bufpool.go`, `internal/api/ingest.go`, `internal/api/record_reader.go`, `internal/api/ingest_test.go`, `docs/src/content/docs/...`, `clients/ts/src/table.ts`, `CHANGELOG.md`	Ingest resolves `Content-Type` before reading, buffers capped bodies, and creates readers from buffered bytes. Body-read failures return `400`, over-limit bodies return `413`, and unsupported content types return `415` before body reads. Documentation describes the full-body buffering and memory behavior.
Ingest validation and policy seams `internal/api/ingest_seams.go`, `internal/api/ingest.go`, `internal/api/ingest_seams_test.go`	`RecordValidator` and `InsertChecker` provide injectable validation, timestamp canonicalization, and policy comparisons. Nil fields use the existing default behavior. Tests cover ordering and fallback behavior.
Compact row encoding `internal/ingest/compact.go`, `internal/ingest/compact_test.go`, `docs/src/content/docs/architecture.md`	`EncodeCompactRow` emits schema-ordered JSON arrays with `null` for missing columns and no trailing newline. Tests cover numbers, structured values, Unicode, and errors.
Stream row visibility seam `internal/stream/hub.go`, `internal/stream/roweval_test.go`	`Hub` uses an injectable `RowEvaluator` for live and replay delivery. The default evaluator delegates to existing row-filter behavior.

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

Merge Risk: 🟡 Moderate · up to c807b

Parallel near-limit uploads can exhaust server memory and disrupt ingestion, so this should be addressed or explicitly accepted before merge.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant IngestHandler
  participant BodyBuffer
  participant RecordReader
  Client->>IngestHandler: Send request with Content-Type and body
  IngestHandler->>BodyBuffer: Read capped request body
  IngestHandler->>RecordReader: Create reader from resolved format and buffered bytes
  RecordReader-->>IngestHandler: Return records

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 91.18% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 12 files. (5 skipped: 5…
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.
Title check	✅ Passed	The title clearly summarizes the main changes: buffered ingest bodies, type-layer seams, and the encoder.
Description check	✅ Passed	The description directly explains the buffering behavior, atomic 413 responses, new seams, encoder, memory trade-offs, documentation, and test plan.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch stack/4-seams

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch stack/4-seams

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:51Z

@coderabbitai review

Auto-review is off for PRs whose base isn't the default branch, and this one sits in a stack (base stack/3-content-type). 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:08Z

@EricAndrechek I will review this PR against stack/3-content-type. I will limit the review to this layer's diff.

⚠️ 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

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

EricAndrechek · 2026-09-02T14:33:31Z

@coderabbitai review

coderabbitai · 2026-09-02T14:33:38Z

✅ 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: 4

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: be1c6d13-6c39-4e08-995c-2914c1e3d43a

📥 Commits

Reviewing files that changed from the base of the PR and between 3b36e89 and 5be3dd7.

📒 Files selected for processing (12)

CHANGELOG.md
docs/src/content/docs/reverse-proxy.mdx
internal/api/bufpool.go
internal/api/bufpool_test.go
internal/api/ingest.go
internal/api/ingest_seams.go
internal/api/ingest_seams_test.go
internal/api/record_reader.go
internal/ingest/compact.go
internal/ingest/compact_test.go
internal/stream/hub.go
internal/stream/roweval_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Review details

🧰 Additional context used

📓 Path-based instructions (2)

Never hard-wrap prose.

📄 CodeRabbit inference engine (AGENTS.md)

Files:

docs/src/content/docs/reverse-proxy.mdx
CHANGELOG.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/reverse-proxy.mdx

🧠 Learnings (4)

📚 Learning: 2026-08-11T21:55:53.726Z

Learnt from: jfwoods
Repo: Wave-RF/WaveHouse PR: 434
File: clients/go/stream_test.go:159-176
Timestamp: 2026-08-11T21:55:53.726Z
Learning: For Go files in this repository, do not report direct type assertions solely because the `forcetypeassert` rule is commented out in `.golangci.yml`. Only flag a type assertion when there is an independent correctness, safety, or maintainability issue.

Applied to files:

internal/api/bufpool.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_seams_test.go
internal/stream/roweval_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_seams_test.go

📚 Learning: 2026-08-13T12:17:52.620Z

Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 470
File: docs/src/content/docs/reverse-proxy.mdx:137-144
Timestamp: 2026-08-13T12:17:52.620Z
Learning: For Wave-RF/WaveHouse documentation, verify claims about implementation control flow against the authoritative implementation source (for example, internal/auth/auth.go) rather than relying solely on docs/** content. Documentation may lag behind or paraphrase behavior, so control-flow claims should be confirmed in source code.

Applied to files:

docs/src/content/docs/reverse-proxy.mdx

🪛 LanguageTool

docs/src/content/docs/reverse-proxy.mdx

[grammar] ~104-~104: Ensure spelling is correct
Context: ...ler of the proxy's and WaveHouse's. For ingest, WaveHouse's 16 MiB is the ceiling — ra...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

[style] ~107-~107: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ... just for one request] Ingest reads the whole body into memory before parsing it, so ...

(EN_REPEATEDWORDS_WHOLE)

🔇 Additional comments (2)

internal/ingest/compact.go (1)

30-49: LGTM!

internal/ingest/compact_test.go (1)

14-130: LGTM!

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

The reordering this branch exists to establish had no test defending it. Every 415 case passes a small, readable body, so moving resolveContentType back below body.ReadFrom left the entire suite green while making an unsupported request pay for a full 16 MiB buffer — and AGENTS.md Key Design Decision #3 ("a 415 decided *before* the body is read") would have become false with nothing to say so. Verified by mutation: hoisting the buffer read above resolution fails both new subtests (415 -> 400, 415 -> 413) and no pre-existing test at all. The body-cap consequence reached table.ts's insertNDJSON docstring, api.md and reverse-proxy.mdx but not the SDK doc page for that same method, whose example uploads a whole .ndjson file. That page is where a reader forms the "NDJSON is the big-upload path" belief this branch retires. The adjacent array-insert paragraph had the same gap. architecture.md's ingest flow skipped from the Content-Type step straight to schema validation, so the step both neighbours point at ("before the body is parsed", "before the body is read") was invisible. 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

…racy Three gaps found by mutation-testing the seams rather than reading them. Replacing h.checker().InSet with the canonical valueInSet left the entire package green: the existing _in tests reach that arm only through the default checker, so the wiring was undefended. A swapped-in type-aware checker would have taken effect for _eq and silently not for _in, inside insert-check authorization. The new subtests fail under that mutation and nothing else does. TestIngest_BodyCap_413 pinned only half the cap change. All four existing cases put the over-cap bytes inside the first value, so every one answers 413 on main too. The case that distinguishes the trees is a COMPLETE first object followed by an oversized tail — 200 with one record published on main, 413 with none here. api.md states that contract explicitly; nothing defended it. Docs accuracy, all in prose this branch introduced: - api.md called per-record error reporting an NDJSON advantage. Arrays report per-record too (arrayReader turns an UnmarshalTypeError into a recordSyntaxError and keeps going). The real differentiator is syntax tolerance: a bad line is skipped where a syntax error kills the array. - The at-least-once caution exempted 413 but not the 400 of an upload cut off in transit, which is the same case — decided before any record is processed. It also omitted the oversized NDJSON line, which IS an abort-partway: bufio.ErrTooLong is fatal and the earlier lines are already published. - The reverse-proxy sizing note said "the amplification above" with no clear referent. The reading that makes it meaningful is the order-of-magnitude figure three paragraphs up, which is attributed to an array of many small values — precisely what a flat single-object row cannot be, its key count being bounded by the table's columns. Named the referent and scoped it rather than telling operators to size for a blow-up that does not apply. - insert()'s TSDoc was the fourth copy of the body-cap claim, unswept. - architecture.md's ingest/ section is a complete per-file map; compact.go made it four files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw

The parenthetical I added last commit was wrong, and wrong in the unsafe direction. It claimed a flat ingest row cannot show the order-of-magnitude decode blow-up because its key count is bounded by the table's columns. That bound is real for an ACCEPTED row and irrelevant to peak memory: objectReader decodes into a map[string]any carrying whatever keys arrived (record_reader.go:158), and h.validator().Validate — the only thing that knows the table's columns — is not reached until ingest.go:448, with the whole map already materialized. Measured with the handler's own decoder settings (UseNumber, map[string]any): a 16 MiB flat object of ~1.4M one-byte values decodes to 183 MiB of heap, 11.5x the body. So peak for one request is the ~32 MiB buffer plus that map, and the "size for 3x" rule undercounts by roughly a factor of three in the one paragraph an operator sizes a container from. Under the shipped compose policy (default_role public, insert allow_columns ["*"]) that request needs no credential. Replaced the exemption with the ordering that causes it and raised the sizing guidance to ~10x, noting a single oversized array element or NDJSON line costs the same. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw

…ionable Three follow-ons to the previous commit, all in the same paragraph. The 10x rule was itself ~2x optimistic for a shape it did not cover: an array-VALUED key, not a top-level array element. {"c":[1,1,1,...]} at 16 MiB decodes to 285.9 MiB live heap — 17.9x, and 61x in total allocation churn — because objectReader builds the map and materializes the []any value before discovery.Validate ever looks at the column type. An operator sizing at a flat 10x is OOM'd by one uncredentialled request. The tail is unbounded (nesting goes higher), so the fix is a directional hedge rather than a bigger number. "Cap object bodies at the proxy" named a distinction no proxy can make: a single object and an array both arrive as application/json, arity being chosen by the first non-whitespace byte, and nginx cannot set client_max_body_size from a variable. Replaced with lowering the body cap for /v1/ingest, which is what the surrounding examples actually support. Line 96's parenthetical still named only arrays, so a reader following the new cross-reference up from line 107 found an example that did not match the shape they had just been shown to be reachable. Objects measure 7.1x, so they belong there on the merits. Measurement note: the 11.5x in the previous commit message was taken without a post-decode GC and so counted garbage. Settled figures, live heap after GC with the handler's decoder settings: flat object of tiny keys 7.1x, one array-valued key 17.9x, one 16 MiB string value 1.0x. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZqHaWmHxxNLRUEQy5ZYRw

github-actions · 2026-09-04T19:52:56Z

📚 Docs preview is live → https://cc6af349-wavehouse-docs.wave-rf.workers.dev

Commit — c807b46: docs(reverse-proxy): hedge the sizing rule, make the proxy advice actionable
Author — @EricAndrechek, Claude Opus 5
Committed — 2026-09-04 15:41 (UTC-04:00)
Deployed — 2026-09-04 16:11 EDT

github-code-quality · 2026-09-04T19:53:47Z

Code Coverage Overview

Languages: Go

Go

The overall line coverage in commit c807b46 in the stack/4-seams branch remains at 91%, unchanged from commit e203f3a in the main branch.

Show a line coverage summary of the most impacted files.

File	`main` e203f3a	`stack/4-seams` c807b46	+/-
`internal/api/record_reader.go`	93%	90%	-3%
`internal/api/ingest.go`	98%	97%	-1%
`internal/stream/hub.go`	97%	97%	0%
`internal/api/bufpool.go`	0%	83%	+83%
`internal/api/ingest_seams.go`	0%	100%	+100%
`internal/ingest/compact.go`	0%	100%	+100%

_{Updated September 04, 2026 20:11 UTC}

This branch was 87 commits behind its own base and 5 behind main, which also meant it still carried golang.org/x/crypto v0.55.0 and failed vulncheck (GO-2026-6354/6355); main already has v0.56.0. Two conflicts: AGENTS.md — two independent renames of the same sentence. #554 renamed stream.filterColumns to stream.projectIndices; #553 renamed ResolvePredicates to resolvePredicates. Kept both, verified against the merged tree rather than the diff. internal/stream/filter_test.go — #554 rewrote these cases from filterColumns (map in, map out) to projectIndices (indices + names), and filterColumns no longer exists, so #554's structure had to win. Applied #553's change of Select to a pointer, and carried its unresolved-select-side case across in the new shape. That surfaced a real semantic interaction rather than a textual one: #553 makes a nil Select mean "never resolved for reads" and deny, but #554's "no lists keeps every column" case built ResolvedPermissions{Allowed: true} with no Select at all, which was unrestricted when Select was a value type and is now denied. Corrected to the explicit-empty form, matching how IsAdmin builds an unrestricted grant, and the nil case is now covered separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

taitelee

LGTM

#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>

#554 gained five commits since this branch last took it: the -s ours merge of main after #553 squashed, the deterministic policy-check guard, the unpairable counter on both delivery paths, and the gap-fill qualifications. One conflict, in api.md's schema-frame paragraph, and it is the good kind — both branches had independently added the same "not guaranteed after a gap-fill" qualification, at different points in the same paragraph. Kept this branch's, which places it earlier and also carries the received_timestamp name-collision note; #554's copy would have been a second statement of the same fact three sentences later. Verified rather than assumed, because the last merge from #554 left eight paraphrased duplicates that -X theirs could not see: the qualification appears once, the collision note once, #554's other two sites (api.md's envelope section and sdk/streaming.md) survived intact, and the context-aware sweep reports zero unqualified schema-frame claims across AGENTS.md and the whole docs tree. The eight claims that were duplicated last time are all still single-copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…ave-RF#553) ## Summary This is the seam work the native type layer lands against, plus the memory trade that comes with it. Responses are unchanged **except at the body cap**, where buffering makes the `413` atomic — and that is a real behavior change, not a refactor artifact: - An over-cap **batch** used to publish the records it had already decoded before the cap surfaced mid-iteration. A client retrying that `413` re-sent them. Now nothing is published, so a `413` is safe to retry once split. - An over-cap **single object** followed by an oversized tail used to answer `200`, having ingested that first object and silently discarded the rest. Now it is a `413`. - One new response shape: `400 {"error":"invalid request body"}` when the body cannot be read at all (malformed transfer encoding, truncated upload), which previously surfaced through the decoder as `invalid json`. Both cap changes are improvements, and the CHANGELOG and `api.md` error tables carry them. The handler now reads the whole (already `MaxBytesReader`-capped) body into a pooled `*bytes.Buffer` and runs the record readers over those bytes rather than the live connection, so the `413` surfaces at that read instead of mid-iteration and the `415` is decided from the header before a byte is read. The memory profile is NOT unchanged, and that is the deliberate part. Streaming meant peak resident bytes on the order of one record — NDJSON scanned line by line, the array path let `json.Decoder` compact after each element. Peak is now O(body) per in-flight request, and `bytes.Buffer` doubles, so peak allocation can exceed the cap before `MaxBytesReader` errors. `maxPooledBufferBytes` (1 MiB) caps what a request hands back to the pool, not its peak, and nothing bounds total in-flight bytes — the ceiling is concurrency × the 16 MiB data-plane cap, which has no operator knob, so the outer limit is the proxy's. Kept because the type layer needs the body addressable rather than consumed; a server-side bound is tracked in Wave-RF#544, and the reverse-proxy guide now says to size the container for concurrency rather than for one request. Three decision points became interfaces with default implementations that delegate to today's code unchanged: `RecordValidator` (schema validation + timestamp canonicalization — the two calls stay where they are, with the check-clause block between them, since merging them would move checks onto canonicalized values), `InsertChecker` (the `_eq` and `_in` comparisons), and `stream.RowEvaluator` (row visibility, reached by both the live fan-out and replay through the one shared admission step). `ingest.EncodeCompactRow` lands here unused — the positional encoder the wire change ahead of it will call. ## Stacked PR This is **part 4 of 7** 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` | | | 1 | `stack/1-discovery` | `stack/0-classify-paths` | | | 2 | `stack/2-policy` | `stack/1-discovery` | | | 3 | `stack/3-content-type` | `stack/2-policy` | | | 4 | `stack/4-seams` | `stack/3-content-type` | **→ this PR** | | 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. ## Test plan - [x] `make ci` green on this branch's exact tree (verify, unit, integration against live ClickHouse, e2e, all coverage gates) - [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 gate the tip of the stack (Wave-RF#555), whose delta against `main` is the union of all seven branches. This branch carries a logged skip (`scripts/skip-pre-push-review.sh`) on that basis, recorded in `tmp/review-skips-*.log`. 🤖 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: f72c8c6

EricAndrechek and others added 5 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/ingest Ingest pipeline (Bento, batching, DLQ) 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

EricAndrechek and others added 2 commits September 2, 2026 07:13

coderabbitai Bot requested changes Sep 2, 2026

View reviewed changes

Comment thread docs/src/content/docs/reverse-proxy.mdx Outdated

Comment thread internal/api/bufpool.go

Comment thread internal/api/ingest_seams_test.go

Comment thread internal/stream/roweval_test.go Outdated

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

EricAndrechek and others added 8 commits September 2, 2026 11:12

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

github-actions Bot added area/sdk TypeScript SDK (clients/ts/) area/infra CI, build, deploy, Docker, release labels Sep 4, 2026

EricAndrechek and others added 5 commits September 4, 2026 15:10

github-actions Bot removed area/query Structured query AST, SQL builder area/policy Access control policies (Hasura-style) area/infra CI, build, deploy, Docker, release labels Sep 4, 2026

coderabbitai Bot approved these changes Sep 4, 2026

View reviewed changes

EricAndrechek marked this pull request as ready for review September 4, 2026 20:08

EricAndrechek requested review from a team and taitelee September 4, 2026 20:08

EricAndrechek mentioned this pull request Sep 4, 2026

api: ingest decodes the body before validating it, so one request can allocate ~20x the body cap #564

Open

This was referenced Sep 4, 2026

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

Merged

review-marker.sh keys reviewer markers to the session worktree, not the reviewed one #565

Open

taitelee approved these changes Sep 7, 2026

View reviewed changes

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

Merged via the queue into main with commit f72c8c6 Sep 8, 2026
32 checks passed

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

EricAndrechek deleted the stack/4-seams branch September 8, 2026 11:23

EricAndrechek mentioned this pull request Sep 9, 2026

sdk(stream): wire-version skew has no programmatic signal — a new SDK on an older server reports live and delivers nothing #577

Open

Navigation Menu

refactor(ingest): buffer the body, add type-layer seams and encoder - #553

refactor(ingest): buffer the body, add type-layer seams and encoder#553
EricAndrechek merged 87 commits into
mainfrom
stack/4-seams

EricAndrechek commented Sep 2, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 2, 2026 •

edited

Loading

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

EricAndrechek commented Sep 2, 2026

Uh oh!

coderabbitai Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

EricAndrechek commented Sep 2, 2026

Uh oh!

coderabbitai Bot commented Sep 2, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

github-code-quality Bot commented Sep 4, 2026 •

edited

Loading

Uh oh!

taitelee left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

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)

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!

Uh oh!

Uh oh!

github-actions Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-code-quality Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

Go

Uh oh!

taitelee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 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

github-actions Bot commented Sep 4, 2026 •

edited

Loading

github-code-quality Bot commented Sep 4, 2026 •

edited

Loading