---
phase: 65.1-phase-65-code-review-remediation
fixed_at: 2026-05-26T00:00:00Z
review_path: .planning/phases/65.1-phase-65-code-review-remediation/65.1-REVIEW.md
iteration: 2
findings_in_scope: 12
fixed: 12
skipped: 0
status: all_fixed
---

# Phase 65.1: Code Review Fix Report

**Fixed at:** 2026-05-26 (iteration 1: CR/WR; iteration 2: IN)
**Source review:** .planning/phases/65.1-phase-65-code-review-remediation/65.1-REVIEW.md
**Iteration:** 2

**Summary:**
- Findings in scope: 12 (CR-01 + WR-01..07 + IN-01..04)
- Fixed: 12 (iteration 1: 8 CR+WR fixes; iteration 2: 4 IN fixes)
- Skipped: 0

**Verification (iteration 1 — CR-01 + WR-01..07):**
- `cargo test` — 850 unit + 5 doc + integration suites all pass (zero failures across all test bins)
- `cargo build --features extension` — clean
- `cargo clippy -- -D warnings` (default features, matches `just lint`) — clean
- `cargo fmt --check` — clean
- `just test-all` — not run in the worktree (the worktree omits the
  extension-ci-tools submodule + Python venv `just` expects); deferred
  to the orchestrator's post-cleanup gate on `milestone/v0.10.0`.

**Verification (iteration 2 — IN-01..04):**
- `cargo build` (default features) — clean
- `cargo build --features extension` — clean (compiles modified shim.cpp
  including the new diagnostic, saturation guard, and 16 MiB pre-check)
- `cargo test` (default features) — 850 unit + 5 doc + 95 integration
  tests across all bins pass; `tests/no_long_lived_conn.rs` (the file
  the IN-03 fix lives in) reports `2 passed; 0 failed`, confirming the
  new allow-list-coverage assertion fires correctly on the single
  legitimate entry.
- `cargo clippy -- -D warnings` (default features, matches `just lint`) — clean
- `cargo fmt --check` — clean
- `just test-all` — same deferral as iteration 1: not runnable in the
  worktree, deferred to the orchestrator's post-cleanup gate. The
  per-IN-fix changes are syntactically and unit-test verified; the
  sqllogictest + ducklake CI legs in `just test-all` cover paths the
  IN-* fixes don't touch (IN-01/02 are diagnostic-only; IN-03 is a
  Rust-only structural test; IN-04 is a guard on a hostile path).

## Fixed Issues

### CR-01: `sv_register_parser_hooks` leaks `OverrideContext` on idempotent re-LOAD

**Files modified:** `cpp/src/shim.cpp`, `tests/parser_hook_idempotent.rs`
**Commits:** 7a9fc24 (primary), 4055196 (structural-test anchor refinement)
**Applied fix:**

Moved the `void *rust_state = sv_make_override_context();` allocation
from above the dedup loop into the `if (!already_registered) { ... }`
block. The original Plan 12 patch had moved the
`ParserExtension::Register` call and the `SemanticViewsParserInfo`
allocation behind the guard but left `sv_make_override_context()`
unconditional — so every redundant `LOAD semantic_views` still leaked
one heap-allocated `Box<OverrideContext>`. The struct is empty so the
per-LOAD cost is small, but the growth is unbounded across a long-lived
host process — the precise leak shape WR-09 was meant to fix.

Updated `tests/parser_hook_idempotent.rs` Assertion 5 to pin BOTH
allocations (`new SemanticViewsParserInfo(` AND
`void *rust_state = sv_make_override_context(`) behind the guard. The
prior assertion only checked `SemanticViewsParserInfo`, which is why
the regression escaped structurally. The follow-up commit 4055196
anchored the test on the assignment-prefixed form
(`void *rust_state = sv_make_override_context(`) rather than the bare
function name, so the find() can't be confused by comment references
to the function name appearing before the guard.

### WR-01: `sv_register_table_function` dereferences `arg_types` without null guard

**Files modified:** `cpp/src/shim.cpp`
**Commit:** 2354706
**Applied fix:**

Added the symmetric `(arg_count > 0 && arg_types == nullptr)` null
guard to both `sv_register_table_function` (line 631) and
`sv_register_scalar_function` (line 721), alongside the existing
null guards for `db_handle / name / exec_cb / init_cb /
DatabaseWrapper`. Diagnostic message surfaces via the same
`write_err` snprintf channel and ABI-stable `error_buf`, matching the
D-05/D-08/D-09 hardening pattern Phase 65.1 Plan 02a already applied.

The sister helpers `sv_register_explain_semantic_view_impl` and
`sv_register_semantic_view_impl` construct their args vectors from
stack literals, so they're not affected (matches the finding's note).

### WR-02: `sv_semantic_view_function` silently truncates column-count mismatch

**Files modified:** `cpp/src/shim.cpp`
**Commit:** 68d8187
**Applied fix:**

Replaced `idx_t n_cols = std::min<idx_t>(chunk->ColumnCount(),
bd.columns.size());` with an explicit `throw InternalException(...)`
that names both counts and includes a "(please report this bug)"
trailer. The pattern matches the existing bind-time
`InternalException` paths in `sv_register_explain_semantic_view_impl`.
This restores the legacy Rust impl's fail-loud semantics (which had a
`Mutex + col_count` check) without re-introducing the long-lived-state
concern the Phase 65 migration was retiring.

### WR-03: ENUM-related stale comment in `sv_resolve_output_logical_types`

**Files modified:** `cpp/src/shim.cpp`
**Commit:** de05dd0 (folded into WR-07 commit)
**Applied fix:**

Updated both the function docstring and the `needs_logical_probe`
comment to remove the misleading reference to ENUM. ENUM is
intentionally declared as VARCHAR via
`sv_logical_type_from_c_type_id`; the LIMIT-0 probe is NOT and never
was required for ENUM. The prior comment was a maintenance trap — a
contributor reading it might assume ENUM probe handling was missing
and "fix" it. Folded into the WR-07 commit because both touched the
same function header/body region; the docstring updates the comment
contract changed by WR-07.

### WR-04: `extract_quoted_string` / `extract_view_comment` / `extract_single_quoted` corrupt multi-byte UTF-8

**Files modified:** `src/parse.rs`
**Commit:** 7a2ea28
**Applied fix:**

Rewrote all three helpers to walk the input as a `char` stream via
`char_indices()` rather than as `&[u8]` with the `bytes[pos] as char`
cast. The cast interprets each byte as a Unicode scalar value (Latin-1
supplement region U+0080..U+00FF for non-ASCII) which silently
mangled Cyrillic, CJK, emoji, em-dash, smart-quote, and accented Latin
payloads. Preserved the SQL `''` escape semantics via iterator clone
+ `matches!(peek.next(), Some((_, '\'')))` peek pattern. Byte-offset
arithmetic adjusted so the consumed-length return values remain
correct (the closing quote is always one ASCII byte, so the offset
arithmetic stays simple).

Added 11 unit tests covering Cyrillic, CJK, emoji, em-dash, smart
quotes, and accented Latin payloads across all three helpers, plus
the `''` escape interaction with non-ASCII surroundings. Tests pass:
`test result: ok. 11 passed; 0 failed`.

The finding's note about this being a pre-existing bug surfaced by
Phase 65.1 Plan 07's FileSystem-direct YAML read path is accurate
and the fix protects the FROM YAML FILE Unicode-path case that the
review specifically called out.

### WR-05: `parse_string_list` returns flat `None` with no FFI-shape detail

**Files modified:** `src/query/explain.rs`, `src/query/table_function.rs`
**Commit:** 711a44a
**Applied fix:**

Changed both per-file `parse_string_list` helpers from
`Option<Vec<String>>` to `Result<Vec<String>, String>` with detailed
diagnostics:
- `"null buffer but len=N (FFI shape drift)"`
- `"buffer too short for count prefix: len=N (expected >= 4)"`
- `"expected u32 at offset N of M (truncated)"`
- `"element I of N declares length L but only K bytes remain at offset O"`
- `"trailing N bytes after count C (consumed K of L)"`

Updated all six call sites in the two dispatchers to render the
detail inline: `"malformed `dimensions` payload: {detail}"`, etc.
This matches the diagnostic shape of the C++ `sv_parse_varchar_payload`
helpers, eliminating the diagnostic asymmetry the finding flagged.

### WR-06: `sv_parser_override` discards Rust error on Err path

**Files modified:** `src/parse.rs`
**Commit:** e040ddb
**Applied fix:**

Renamed `Err(_err)` to `Err(err)` and added
`write_error_to_buffer(error_out, error_out_len, &err.message)` on the
rc=2 (defer-to-default-parser) path. The buffer is heap-allocated and
the write cost is sub-microsecond; the existing `write_error_to_buffer`
helper respects the caller's capacity and truncates at a char
boundary (proptest-verified by
`write_error_to_buffer_truncates_at_char_boundary`), so no new safety
concern. Future tooling (debug-build flag, tracing hook) can now
surface both errors without another ABI change.

The C++ side currently ignores the rc=2 channel per the Phase 62
design; this fix keeps the channel populated for the case the finding
describes (non-deterministic `rewrite_to_native_sql` failure paths
producing different errors on the second sv_parse_stub invocation).

### WR-07: Three Connection allocations per `semantic_view(...)` invocation

**Files modified:** `cpp/src/shim.cpp`
**Commit:** de05dd0
**Applied fix:**

Changed `sv_resolve_output_logical_types`'s signature from
`(ClientContext &context, ...)` to `(Connection &probe, ...)` and
updated `sv_semantic_view_bind` to pass its already-open `probe`
Connection down. One Connection now covers both the Rust dispatcher's
catalog reads AND the bind-time LIMIT-0 DECIMAL/LIST probe — same
transaction/catalog snapshot for both, and one fewer ctor/dtor pair
through `ConnectionManager::AddConnection`.

**Not addressed (documented in commit body):** the third Connection in
`sv_semantic_view_init_global` (line 2463). That call site is invoked
AFTER bind completes, so the bind's Connection is already destroyed;
consolidating those two would require stashing a Connection in
BindData and reasoning about its lifetime across the bind →
init_global → exec boundary, which the DuckDB extension API doesn't
make easy. The WR-07 finding's broader "three Connection" framing is
reduced to two; the most impactful consolidation (the two Connections
that should agree on transaction/catalog state) is achieved.

The finding's deeper concern about visibility of uncommitted writes
remains documented at the call site — `Connection(*context.db)`
doesn't satisfy a "see caller's uncommitted writes" contract regardless
of whether we open one or two, so that's a separate follow-up.

### IN-01: bridge-contract probe diagnostic improvement

**Files modified:** `cpp/src/shim.cpp`
**Commit:** bc0d94d
**Applied fix:**

In `sv_register_parser_hooks` the Plan 10 D-12 runtime probe ran
`duckdb_query(handle, "SELECT 1", &r)` against a freshly-bridged
`duckdb_connection` and on rc != DuckDBSuccess produced the fixed
message `"bridge contract broken — duckdb_connection layout assumption
failed at load time"`. The finding noted this would mislead an
operator hitting a non-bridge cause (transient DB error at LOAD time,
OOM, shutdown in progress) toward chasing an ABI regression that
isn't there.

Read `duckdb_result_error(&r)` BEFORE the `duckdb_destroy_result(&r)`
call, include the rc value and the underlying error text in the
diagnostic, and rephrase to "bridge contract probe failed ... Possible
causes: duckdb_connection ABI drift OR transient DB error at LOAD
time". The snprintf copies the error text into `error_buf` before the
result is destroyed, so the pointer lifetime is preserved.

### IN-02: saturate `sv_count_parser_extensions` counter at INT32_MAX

**Files modified:** `cpp/src/shim.cpp`
**Commit:** 869c818
**Applied fix:**

The structural-verification helper `sv_count_parser_extensions`
declares its return contract as ">= 0 on success; -1 on failure".
The inner `int32_t count = 0` loop incremented unconditionally; if
the parser_extensions list ever grew past 2^31-1 entries the counter
would wrap to a negative value and the structural test would
misinterpret it as a registration failure.

Added a `count < INT32_MAX` guard before the increment to saturate at
INT32_MAX. `INT32_MAX` comes from `<cstdint>` which is already
included in shim.cpp. Reaching this point is unreachable in practice
(the WR-09 dedup guard bounds the list to 1), but the saturation is
cheap defence-in-depth against a hypothetical dedup failure.

### IN-03: assert ALLOWED_DISCONNECT_SITES entries match a real call site

**Files modified:** `tests/no_long_lived_conn.rs`
**Commit:** e489e8a
**Applied fix:**

The `DisconnectFinder` AST walker filtered matches against an
allow-list of `(file_path, full_call_path)` pairs but did not track
whether each allow-list entry actually corresponded to a present
call site. The finding flagged that a stale entry (e.g. the `RawDb`
test fixture moved out of `src/lib.rs`, or its
`ffi::duckdb_disconnect` call restructured) would silently become a
free pass for any future code legitimately added at that coordinate.

Added a `matched: Vec<bool>` shadow array tracking which allow-list
entries matched during the retain() pass. After the existing
"finder.found_call_sites.is_empty()" assertion, a second loop
asserts every allow-list entry was used at least once, with a
diagnostic that explicitly tells the maintainer to remove the stale
exception or update its coordinate. Test ran clean: 2 passed; 0 failed.

### IN-04: pre-check YAML file size against 16 MiB cap before allocating

**Files modified:** `cpp/src/shim.cpp`
**Commit:** 5a35d2a
**Applied fix:**

`sv_create_from_yaml_bind` called `fs.GetFileSize(*handle)` and
immediately allocated `std::string(static_cast<size_t>(size), '\0')`
before handing the buffer to the Rust YAML parser. A hostile multi-GB
YAML path would allocate that many bytes of std::string storage,
perform the file read, hand the buffer to Rust, and ONLY THEN be
rejected by the Rust `from_yaml_with_size_cap` (1 MiB). On a
memory-constrained host the practical outcome is OOM kill before
Rust ever sees the buffer.

Added a `constexpr int64_t MAX_YAML_BYTES = 16 * 1024 * 1024;` pre-check
that throws `BinderException` with the pinned `"FROM YAML FILE failed: "`
prefix if size > 16 MiB. The C++ cap is deliberately LOOSER than the
1 MiB Rust cap, so the Rust limit remains the single source of truth
for legitimate size decisions; the C++ pre-check exists purely as
defence-in-depth on a hostile path. Updated the surrounding comment to
explain the asymmetry (was: "Rust enforces the cap, we deliberately do
NOT pre-check"; now: "C++ pre-check is fail-fast on hostile sizes;
Rust cap remains the legitimate-size source of truth").

## Skipped Issues

None — all 12 in-scope findings (CR-01 + WR-01..07 + IN-01..04) were fixed
across the two iterations.

---

_Fixed: 2026-05-26 (iteration 1: CR/WR; iteration 2: IN)_
_Fixer: Claude (gsd-code-fixer)_
_Iteration: 2_
