## Code Review Results **Scope:** PR #1 (local-aligned), merge-base `24c789e` -> working tree at `78891dd` (3 files, 464 insertions / 1 deletion; 57 executable non-test lines) **Intent:** Stop a hostile or buggy Cassandra/Scylla peer from panicking the goroutine that called `Scan` while decoding a CQL `vector<...>` value. `info.Dimensions` is the peer's number and previously reached `make` / `reflect.MakeSlice` unbounded; a new bound rejects a count that cannot fit in the payload, dividing payload length by a new recursive per-element minimum (`vectorElemMinSize`). A second guard rejects a nil `reflect.TypeOf(info.Zero())` for element types with no Go representation. Accepted behaviour change: short or zero-length non-NULL payloads that decoded to zero values now error. **Mode:** markdown report-only (read-only review: nothing was edited, committed, or posted to the PR; the tree was verified clean at `78891dd` before and after) **Reviewers:** correctness, security, reliability, testing, api-contract, performance, plus a cross-model adversarial pass through Codex - security -- a peer-controlled dimension count and element type drive allocation and reflection on the `Scan` goroutine - reliability -- the change converts a process-killing panic into an error and deliberately makes a lenient decode path strict - testing -- 464 new test lines, one asserting where rejection happens via a process-global allocation counter - api-contract -- payloads that used to decode to zero values now error, and `getCassandraLongType` changes the `TypeInfo` returned for unknown types, in a library - performance -- a new recursive per-element size computation runs on every vector decode, ahead of the float fast paths ### Triage Groups | Group | Findings | Kind | Context | Preferred Resolution | Why | |-------|----------|------|---------|----------------------|-----| | The one-byte floor for variable-length elements | #1, #2 | decision-gate | Both trace to `vectorElemMinSize` returning 1 for every variable-length element (`marshal.go:1131`), which lets the bound at `marshal.go:1000` admit `Dimensions == len(data)` and hand that count to `reflect.MakeSlice` over a much larger Go element | Decide once how the bound should relate to the Go-side allocation, then apply both parts. Handle #2 first: an allocation-proportional bound (or incremental `reflect.Append`) is the only form that covers `text`/`blob`, where one wire byte per element is the genuine minimum and no per-type floor helps. Then #1: raise the floor for variable-wrapper types with fixed bodies | Shared root cause and one partly-shared fix path, but neither fix subsumes the other. A decision-gate because the allocation ratio a driver should tolerate is a product call -- too tight a cap rejects legitimate large vector reads | ### P1 -- High | # | File | Issue | Reviewer | Confidence | |---|------|-------|----------|------------| | 1 | `marshal.go:1131` | Zero-length `date`/`time` elements pass the bound and still allocate one Go value per claimed element, decoding to `minDate` with a nil error | adversarial-codex | 100 | | 2 | `marshal.go:1000` | The bound counts wire bytes while the allocation is sized in Go bytes, leaving ~16-24x memory amplification | security | 75 | | 3 | `marshal.go:1136` | The recursive floor's overflow-saturation branch, the one guarding a nested peer `Dimensions`, has no test | testing | 75 | - **#1** -- `isVectorVariableLengthType` lists `TypeDate` (`marshal.go:1185`), so `vectorElemMinSize` hands back the 1-byte floor even though a well-formed `date` element is a 1-byte vint length plus a 4-byte body. A peer sending N `0x00` bytes for a `vector` column satisfies the bound, reaches `reflect.MakeSlice` of N 24-byte `time.Time` values (`marshal.go:1083`), and every element decodes to `minDate` with a nil error (`serialization/date/unmarshal_utils.go:171-173`). Two consequences: the allocation still scales with the claimed count, and the application gets a silently wrong slice instead of a rejection. **Response (mechanical, with one check first):** compute true minimum wire sizes for variable-wrapper types with fixed bodies -- `date`=5, `time`=9, `counter`=9, `smallint`=3, `tinyint`=2 -- after confirming each against Cassandra's `VectorType` serializers, and add a regression test for `vector` over zero-length prefixes. **How sure:** confidence 100, found by the cross-model peer, confirmed by an independent validator that re-read both cited lines. Validation noted that incidence against a real server was not measured -- the code permits this, but nobody has shown a server producing it. - **#2** -- `elemMin` is 1 for every variable-length element type (text, blob, varchar, date, time, smallint, tinyint, counter, decimal, varint, inet, collections), so `info.Dimensions > len(data)/elemMin` admits `Dimensions == len(data)`. A peer body may be 256 MiB (`internal/frame/frames.go:47`), so a `vector` value passes the guard and `reflect.MakeSlice(t, info.Dimensions, info.Dimensions)` asks the allocator for roughly 4 GiB of string headers before a single element is decoded; scanning into `any` is worse, since `Zero()` yields a pointer slice plus a heap object per claimed element. The wrap-to-zero panic the guard was written for is closed; the OOM the allocation permits is not, and `unmarshalList` -- the precedent the new comment cites (`marshal.go:835`) -- tolerates only 4x. **Response (decision first, then code):** bound the allocation as well as the count. Either reject when `Dimensions * t.Elem().Size()` exceeds a small multiple of `len(data)` (a constant near 8 matches what `unmarshalList`'s `/4` already tolerates for a 24-byte element), or allocate empty and grow with `reflect.Append` in the decode loop so peak memory tracks elements actually present. The second form has no constant to tune; the first needs a product call on the ratio, which is why the group is a gate. **How sure:** confidence 75, confirmed by an independent validator that re-read the bound, the allocation and the frame cap. **Reviewer disagreement, kept visible:** the correctness reviewer measured the same amplification and deliberately kept it as a residual risk rather than a finding, on the grounds that `unmarshalList` applies the same shape of bound so this matches the established convention rather than regressing it. Both reviewers agree the diff strictly improves on base, where the count was unbounded. The open question is whether an incomplete-but-conventional bound is a defect. - **#3** -- `vectorElemMinSize` recurses into a nested vector's `SubType`, and `marshal.go:1136` exists to keep that recursion from wrapping when a peer sends an enormous inner `Dimensions` -- the same attack class this PR targets, one level deeper. No test reaches it: every nested case in `marshal_vector_test.go` uses an inner dimension of 3 (lines 663, 670, 752, 775, 812, 839), and the large dimensions at lines 636 and 839 are outer, overflowing the fast-path multiplication rather than this floor. If the comparison direction, the boundary, or the `int64`-to-`int` narrowing at the return is wrong, a crafted nested type can make the floor small and defeat the outer bound, reopening the crash, with nothing in the suite to catch it. **Response (mechanical):** add a `TestVectorElemMinSize` case whose inner `VectorType` has `Dimensions` large enough to trip the guard (for example `1<<29` as the `SubType` of an outer vector), asserting saturation to `math.MaxInt32` rather than a wrapped or negative value; pair it with an `unmarshalVector` case using a small payload and that same type, asserting the outer bound still rejects with a "do not fit" error instead of allocating. **How sure:** confidence 75, confirmed by an independent validator that re-ran the grep over the test file. ### P3 -- Low | # | File | Issue | Reviewer | Confidence | |---|------|-------|----------|------------| | 6 | `marshal_vector_test.go:695` | The PR body's claim that every new test fails against pristine code is measurably wrong for 3 of 7 test functions | testing | 100 | - **#6** -- The reviewer built a scratch tree with base `24c789e`'s `marshal.go`/`helpers.go` and head's test file and ran each new top-level test. `TestUnmarshalVectorAcceptsWellFormedPayloads` (line 695) and `TestUnmarshalVectorZeroDimensionsDoesNotTouchSubType` (line 937) pass unchanged against base -- they are regression guards, which is fine -- and `TestVectorElemMinSize` (line 797) does not compile against base at all (`undefined: vectorElemMinSize`), which is not one of the four failure modes the body lists. A reader using the literal claim to judge how much of the suite would have caught the bug overcounts by 3 of 7. Nothing about the fix is affected. **Response (documentation, human):** scope the claim in the PR body to the three tests that assert new rejections, and say the rest are regression guards. Report-only; it is not in the actionable queue. ### Actionable Findings | # | File | Issue | Route | Notes | |---|------|-------|-------|-------| | 1 | `marshal.go:1131` | Raise the per-type wire floor for variable-wrapper types with fixed bodies | `manual -> downstream-resolver` | `suggested_fix` present; verify the per-type minimums against Cassandra's serializers before applying, and add the `vector` test | | 2 | `marshal.go:1000` | Bound the allocation, not just the claimed count | `manual -> downstream-resolver` | `suggested_fix` present; needs the ratio decision (or the incremental-append form, which needs none). Do this one first | | 3 | `marshal.go:1136` | Cover the nested-dimension saturation branch | `gated_auto -> downstream-resolver` | `suggested_fix` present; test-only change, no production code involved | ### Pre-existing (does not count toward the verdict) | # | File | Issue | Reviewer | |---|------|-------|----------| | PE1 | `marshal.go:1097` | An oversized VInt element length becomes a silent empty value | adversarial-codex | - **PE1** -- With the one-byte floor letting variable-length vectors through, three max-width VInts in a `vector` pass the bound; `elemSize = int(m)` (`marshal.go:1097`) turns `math.MaxUint64` into -1, so no payload is consumed and each element decodes as an empty string with a nil error. This is the PR body's deferred issue #1106 and the new bound does not depend on that path for its own correctness, so it stays pre-existing. The fix, when it is taken up, is to reject `m > uint64(len(data))` after consuming the VInt prefix and before the conversion to `int`. ### Out of Scope and Deferred (not blockers) The PR body defers these explicitly, and this review treats them as out of scope. None of them blocks the merge. - **#1085** -- making a nested vector actually decode into `interface{}`. It now errors rather than crashing, which is the improvement this PR claims; full support is deferred. - **#1086** -- surplus trailing bytes in a generic vector payload are still accepted, left exactly as found. `marshal_vector_test.go:724` pins that behaviour, while the float fast paths reject the same malformed payload. Raised independently by the cross-model peer, and out of scope. - **#1106** -- a variable-length element's vint length narrowing to a negative and decoding to empty with a nil error, which this bound does not catch. This is PE1 above. - **#1093**, the companion change rejecting `dim < 1` in the metadata parsers, is not in this tree. Finding #2's `vector,N>` shape is claimed by it; the variable-length shapes in #1 and #2 are well-formed with `dim >= 1` and are not covered by it. ### Coverage **Validation (Stage 5b).** One validator batch, four findings (#1, #2, #3, #4), verdicts landed within the wait. Confirmed 3, rejected 1, unresolved 0, malformed 0, failed 0, shortcut-skipped 0. - #1 confirmed on protected subject `memory-safety`; its reason states incidence against a real server was not measured, so it carries `validation_status: confirmed` with that caveat -- a defect the code permits that nobody has shown to occur. - #2 confirmed on protected subject `memory-safety`; likewise, exploit incidence was not measured. - #3 confirmed on inspection of the test file; no protected subject. - #4 **rejected and dropped**: reliability's P2, `marshal.go:1001`, "the dimension-bound error omits the element type, unlike the fast-path errors it preempts". The validator confirmed by inspection that the message omits `info.SubType` and that `scanColumn` (`session.go:2973-2992`) returns `Unmarshal` errors unwrapped, but judged the request an unsupported wording preference rather than a defect in the diff's behaviour, since the message still names the operation and prints both peer-controlled numbers. Recorded here rather than in the findings. - Protected-subject classification was done independently of the validator before its verdicts were read: #1 and #2 both allege an allocation sized by a peer-controlled count, so both are `memory-safety`; #3 alleges absent test coverage and #4 alleges error-message wording with no evidenced external consumer of the text, so neither is protected. That matches the validator's own classification, so no reclassification was needed and no rejection had to clear the protected-subject evidence bar. - No validator was skipped. The shortcut needs a finding corroborated by both an ordinary reviewer and the independence-verified peer inside one merged group, and no finding merged that way. **Cross-model.** The adversarial pass ran through Codex over route `codex`; `gpt-5.6-luna` at xhigh was requested, and the serving model and effort are unverified on this route (`receipt_supported` false). `independence_verified` is true, so agreement with a local reviewer could promote confidence. Two peer findings were folded in: #1 entered the primary set on its own anchor, and the peer's pre-existing one became PE1. No merged group held both an ordinary reviewer and the peer, so the one-step agreement promotion never applied (0 promotions). **Synthesis mechanics.** 1 finding suppressed at anchor 50, 0 at anchor 25. 1 mode-aware demotion: testing's P2 "no test for the PR's headline behaviour change" moved to testing gaps, because #3 is the umbrella coverage finding for this subsystem. 5 findings had `first_evidence` backfilled from `evidence[0]`; the quote-the-line check demoted 0 findings. 0 malformed findings, 0 malformed returns. 2 residual risks and 1 testing gap were dropped as duplicates of primary or pre-existing findings, and two grep-only callsite notes were merged into one. No finding proposed deleting anything, so there is no removable-surface line and the protected-artifact rule never fired. **What did not run.** - Scope: local-aligned (PR #1; local tree diff against merge-base `24c789e`). - Project standards: not run -- no applicable standards files (no `CODING_STANDARDS.md`, `CLAUDE.md` or `AGENTS.md` anywhere in the repo). - No plan document found (`docs/plans` does not exist), so requirements completeness was not assessed and settlement suppression was not evaluated. - No learnings corpus and no declared Compound Packs; the learnings-researcher did not run. - Previous-comments review not run: the PR has no prior review comments. - No untracked files were excluded. - The adversarial lens ran cross-model through Codex, so the in-process adversarial-reviewer was not dispatched. - Maintainability not run: 57 executable non-test changed lines, no structural or abstraction change. - agent-native, data-migration, swift-ios and julik-frontend-races not run: no such surface in the diff. - The correctness reviewer was re-engaged once after its first return, to settle by execution rather than inspection whether the `vector` / nil-pointer-element round-trip break is a finding. It measured both revisions and kept it as a residual risk because Cassandra's `FixedLengthSerializer` rejects such a write, so the read can never occur. - Reviewers ran `go build`, `go vet` and the unit-tagged test suite; the repository working tree was verified clean at `78891dd` before and after the batch. - **No live-node or integration run was performed by any reviewer.** Four pre-existing `TestShardAwarePort*` TLS fixture failures were observed and are unrelated to this diff. **Residual risks (informational; none is a finding).** - **Measured, and kept as a risk rather than raised to a finding (correctness).** The new bound rejects every payload this driver's own `marshalVector` produces for a fixed-size element it encodes in zero bytes -- but every such payload is one no server would return. Executed on both revisions from temp trees built with `git archive`, repo left clean at `78891dd`. Verdicts that changed from base `24c789e` to head, all from "nil error, zero-filled result" to `unmarshal vector: 3 dimensions do not fit in 0 bytes`: `vector` over `[]time.Time{{},{},{}}`, over `[]*time.Time{nil,nil,nil}`, and over three `UnsetValue`; and, not timestamp-specific, the nil-pointer element slice for every fixed-size type -- `[]*bool`, `[]*int32`, `[]*float32`, `[]*int64`, `[]*float64`, `[]*UUID` all marshal to 0 bytes and all flipped from a silent zero-filled decode to an error. Unchanged: `vector` over `[]time.Time{now,{},now}` encodes 16 bytes and already failed on base; only the message changed. All eight fixed-size types over genuine non-pointer values encode exactly `vectorFixedElemSize*N` and round-trip on both revisions, so `vectorFixedElemSize` over-estimates only for a nil pointer, a zero `time.Time`, or `UnsetValue`. - **Why that stayed a risk (correctness).** A short payload for a fixed-size vector column cannot come back from a server: Cassandra's `VectorType` uses a `FixedLengthSerializer` whose validate rejects a buffer that is not `dimensions*elementSize` bytes, so a write carrying the 0-byte or 16-byte value measured above fails with `InvalidRequest` and the read never happens. The only way to observe the changed verdicts is a local Marshal-then-Unmarshal with no server in between. In every changed case the base behaviour was exactly the silent corruption this PR set out to end, and the PR body names the same shape as an accepted change. Not verified against a live node; the PR makes the same disclaimer. - **Pre-existing, surfaced by that measurement (correctness).** `marshalVector` emits a wire-invalid vector value rather than erroring whenever an element marshals to fewer bytes than `vectorFixedElemSize` and carries no vint prefix -- a nil pointer element of any of the eight fixed-size types, `UnsetValue`, or a zero `time.Time` (`serialization/timestamp/marshal_utils.go:30` returns `make([]byte, 0)` for `v.IsZero()`). `marshal.go:916-932` writes the bytes with no prefix and no length check, so the caller learns about it only from the server's `InvalidRequest`. This diff does not touch `marshalVector`. The symmetric fix is a length check against `vectorFixedElemSize` on the marshal side. - **`vectorElemMinSize(nil)` still panics (correctness).** With `Dimensions > 0` and `data != nil`, a caller-constructed `VectorType` with a nil `SubType` reaches `vectorFixedElemSize(nil)` and calls `Type()` on a nil interface. Not a regression -- the pre-diff fast-path switch at `marshal.go:1010` sat behind the identical guard -- and nothing from the wire produces a nil `SubType`, so it is reachable only through the exported struct. The diff's `TestUnmarshalVectorZeroDimensionsDoesNotTouchSubType` pins only the zero-dimension case. - **Peer-controlled text in an error (security, CWE-117).** The new error at `marshal.go:1048` formats the peer's type descriptor with `%v`, and `NativeType.String` (`marshal.go:2016`) prints the raw custom name the server sent, now preserved verbatim by the `helpers.go` change -- up to 65535 bytes of arbitrary peer text, including newlines and terminal escapes, reaching an error applications routinely log. Not filed as a finding: the driver already prints `info` this way in neighbouring unmarshal errors, so the change does not widen an existing exposure. - **Malformed `VectorType` name parsing (security).** `frame.go:855-858` does `spec[1 : len(spec)-1]` and `spec[:idx]` where `idx` may be -1, and `helpers.go:264` indexes `names[1]` unguarded; a malformed name from the wire panics there. Pre-existing and unchanged, and the frame-parse entry sits inside `parseFrame`'s recover. The reviewer could not construct, within budget, a custom string whose frame-time parse survives while the `Scan`-goroutine parse hits the same panic -- the two parsers differ -- so the possibility is not closed. - **Recursion depth (security).** `vectorElemMinSize` recursion follows the peer's type-descriptor nesting, but the descriptor arrives via `framer.readString` (at most 65535 bytes) and each level costs 43 characters, capping depth near 1500, far below a Go stack overflow. Checked; not a finding. - **Nested-vector error cannot name the element type (correctness, the claim suppressed at anchor 50).** For a nested vector the new guard prints `no Go type for element type custom(org.apache.cassandra.db.marshal.VectorType)` -- the bare prefix -- because `getCassandraLongType` builds the nested `VectorType`'s `NativeType` from the constant prefix (`helpers.go:271`) rather than the spec it just parsed. The half of this PR written to let an error name what the server sent achieves that for an unknown leaf type but not for a nested vector. Suggested fix, if taken up: `NewCustomType(protoVer, TypeCustom, name)` in that branch. Held at anchor 50 because the reviewer could not rule out a consumer relying on the bare-prefix value. - **Callsite completeness is grep-only (correctness and security).** The repo exposes no symbol-aware search. Every non-test reader of the `TypeInfo` returned by the changed `getCassandraLongType` was traced by hand -- `Custom()`, `String()`, `goType()`, `NewWithError()`, `asVectorType()`, `recreate.go`, `metadata_scylla.go` -- and none depends on the previous empty-string `Custom()`, but a consumer reached through an interface or reflection would not appear. External consumers comparing `TypeInfo` values for equality would see `NativeType.custom` go from `""` to the server's class name for unknown types. - **The fallback change is wider than the stated intent (api-contract).** `getCassandraLongType`'s basic-type fallback (`helpers.go:279-290`) is not scoped to vector elements: it changes `Custom()`/`String()` for any unrecognized type resolved through this function, including elements nested inside list, set, map, tuple and UDT definitions, since the function is called recursively for those. Every non-test consumer was traced and the targeted unit tests pass, so it looks like a net improvement -- but it is a wider change to exported `TypeInfo` output than the description describes. - **Undocumented exported behaviour (api-contract).** `Unmarshal`'s doc comment (`marshal.go:260-300`) has never mentioned vector decoding -- no `vector` row in the CQL-to-Go table, and `VectorType`'s own comment covers only `Zero()`. That gap predates this diff, but the PR adds two new error conditions on top of an already-undocumented path, so a consumer reading only pkg.go.dev has no authoritative description of how vector `Scan` can fail. - **No release-notes channel (api-contract).** The repository removed upstream's `CHANGELOG.md`, and neither `README.md` nor `CONTRIBUTING.md` states a SemVer or breaking-change policy, though `go.mod` tags releases as v1.x.y. There is no established channel through which a decode-behaviour change like this one reaches a consumer pinning a released tag. A pre-existing gap in project practice, not something this PR was positioned to close. - **A test helper builds a descriptor the wire never produces (testing).** `makeVectorType(TypeText, "UTF8Type", dim)` (`marshal_vector_test.go:557`, used for every `vector` case) sets `typ=TypeText` for a wire name the real parser (`getApacheCassandraType`, `helpers.go:363`) maps to `TypeVarchar`. Harmless today, since `isVectorVariableLengthType`, `vectorFixedElemSize`, and the marshal/unmarshal dispatch treat the two identically, so no assertion is weakened. Flagged because a future divergence between `TypeText` and `TypeVarchar` would not be caught by these tests. - **No measurable performance regression (performance).** `unmarshalVector` now calls `vectorElemMinSize(info.SubType)` unconditionally, ahead of the `*[]float64`/`*[]float32` fast paths that exist to skip reflect dispatch, and recomputes the same result for every row since nothing caches it on `VectorType` -- in principle hoistable to column-metadata construction. Benchmarked against base at `-benchtime 200000x -count 3` for dims 128/384/768/1536: head 130.9-134ns to 1236-1278ns, base 108-193ns to 1236-1266ns, indistinguishable within noise, 0 allocs/op on both. The float fast path resolves in one type-switch, and the generic path's O(1) cost is negligible next to the `reflect.MakeSlice` plus per-element dispatch it already performs. Not actionable. **Testing gaps (informational; #3 is the one coverage item carried as a finding).** - No test for the PR's headline behaviour change: an empty non-NULL payload now errors. Verified against base that `unmarshalVector(vector, []byte{}, &dst)` returned `err=nil, dst=[0 0 0]`, and against head that it returns `unmarshal vector: 3 dimensions do not fit in 0 bytes`. The closest case (`marshal_vector_test.go:621`) uses a 2-byte payload and the zero-dimension test (line 944) uses `Dimensions==0`, so neither pins the case that produced the silent corruption. Add a case with `data: []byte{}` and `Dimensions > 0`. - The accepted behaviour change is asserted only for the `vector` shape the PR body names. Measurement shows it also flips the nil-pointer element slice of every fixed-size type and the zero `time.Time` / `UnsetValue` cases. One table test round-tripping Marshal then Unmarshal over those shapes would pin the new verdict deliberately. - No test covers a well-formed payload whose variable-length element has a real minimum above the 1-byte floor (`vector`, `vector`), confirming the bound stays permissive rather than rejecting a real server payload. - No test asserts the bound leaves the float fast paths alone for a payload longer than the minimum (`vector` over 32 bytes must still give "expected 24 bytes, got 32"), which is the boundary where the documented message substitution stops applying. - `TestUnmarshalVectorRejectsNestedCountBeforeAllocating` measures peak allocation only for the nested fixed-size shape. There is no allocation assertion for a variable-length element type (`vector` into `[]string` or into `any`), which is where the 1-byte floor permits the largest Go-side allocation. - No test covers a zero-dimension inner vector as an element type (`vector,N>`), the shape reaching the `return 1` fallback at `marshal.go:1142`; `TestVectorElemMinSize` pins the floor but nothing pins what `unmarshalVector` then allocates for it. - Every new dimension-bound and no-Go-type test calls `unmarshalVector` or `Unmarshal` directly. None exercises the real `Scan` path (`iterScanner.Scan` -> `scanColumn` -> `Unmarshal`, `session.go:3012-3016`) to pin that a hostile payload's error surfaces cleanly to a `Scan` caller with no panic in the intermediate plumbing -- which is the goroutine this PR exists to protect. - No test drives the new bound through a type nested three or more vector levels deep, confirming the recursion terminates with a sane floor. Minor, since `Dimensions` is bounded before depth could matter in practice. - `TestUnmarshalVectorRejectsAnElementTypeWithNoGoType/nested_vector_into_an_interface` (`marshal_vector_test.go:519-530`) asserts only that the error contains "no Go type for element type", not what identifier follows. Today it renders as the bare `custom(org.apache.cassandra.db.marshal.VectorType)` prefix, unlike the unknown-leaf case in the same test, so a further regression there would go unnoticed. - No unit test exercises `getCassandraLongType`'s basic-type fallback outside the vector path -- a `List`, an unknown map value, a UDT field -- to confirm the widened `Custom()`/`String()` population behaves for those positions, since the same function backs all of them. - `vector_bench_test.go` benchmarks only flat `vector`. There is no benchmark for a nested vector or a variable-length element vector, both of which now exercise the recursive prologue on every value, so a future regression in that recursion would not show up. --- ### Verdict > **Verdict:** Ready with fixes > > **Reasoning:** The PR does what it set out to do -- the wrap-to-zero panic and the nil `reflect.TypeOf` dereference are both closed, and the new tests fail against pristine code for the rejections that matter. Three confirmed P1s keep it short of ready to merge. #1 and #2 share one root cause: the per-element floor is 1 byte for every variable-length element, so a peer-controlled count still reaches an allocation sized by the Go representation (up to ~4 GiB from a 256 MiB body), and zero-length `date` elements decode to `minDate` with a nil error instead of being rejected. Neither was shown to occur against a real server -- validation confirmed both on the code and recorded that incidence was not measured -- so weigh them as defects the code permits, not as observed incidents. #3 is a test-only gap on the saturation branch that guards the same attack class one level deeper. Nothing here is a P0 and nothing regresses the base behaviour. The deferred items (#1085, #1086, #1106, and PE1, which is #1106) and every residual risk above are informational and do not block. > > **Fix order:** decide the allocation bound for #2 first (an incremental `reflect.Append` needs no ratio decision at all), then #1's per-type floors on top of that decision, then #3's saturation test. #6 is a one-line correction to the PR body. ### Actionable Findings (summary) - **#1** -- P1, `marshal.go:1131`, zero-length `date`/`time` elements still drive a per-element allocation and decode to `minDate` silently. `manual`, `suggested_fix` present, confidence 100. Mechanical once the per-type minimums are checked against Cassandra's serializers. - **#2** -- P1, `marshal.go:1000`, the bound counts wire bytes while the allocation is sized in Go bytes; ~16-24x amplification remains. `manual`, `suggested_fix` present, confidence 75. Decision first: the tolerable ratio, or the incremental-append form that needs none. - **#3** -- P1, `marshal.go:1136`, the nested-dimension saturation branch has no test. `gated_auto`, `suggested_fix` present, confidence 75. Mechanical, test-only. Report-only, not in the queue: **#6** (P3, `marshal_vector_test.go:695`, the PR body's verification claim overstates coverage -- reword the body) and **PE1** (pre-existing, `marshal.go:1097`, deferred as #1106). Run artifacts: `/tmp/compound-engineering-1003/ce-code-review/20260920-152325-1146cc5c/report.md`