---
phase: 65.1-phase-65-code-review-remediation
reviewed: 2026-05-26T00:00:00Z
depth: standard
files_reviewed: 28
files_reviewed_list:
  - cpp/include/parser_extension_compat.hpp
  - cpp/src/shim.cpp
  - cpp/src/shim.hpp
  - src/catalog.rs
  - src/ddl/describe.rs
  - src/ddl/get_ddl.rs
  - src/ddl/list.rs
  - src/ddl/read_ffi.rs
  - src/ddl/read_yaml.rs
  - src/ddl/show_columns.rs
  - src/ddl/show_dims.rs
  - src/ddl/show_dims_for_metric.rs
  - src/ddl/show_facts.rs
  - src/ddl/show_materializations.rs
  - src/ddl/show_metrics.rs
  - src/lib.rs
  - src/parse.rs
  - src/query/explain.rs
  - src/query/table_function.rs
  - test/integration/test_concurrent_writes_per_call_conn.py
  - test/integration/test_drop_on_fresh_readonly_clear_error.py
  - test/integration/test_load_extension_twice_idempotent.py
  - test/sql/phase651_bind_probe_hard_errors.test
  - test/sql/phase651_null_name_inputs.test
  - test/sql/phase651_yaml_filesystem_access_gating.test
  - tests/no_long_lived_conn.rs
  - tests/parser_hook_idempotent.rs
  - tests/registration_error_surfaces.rs
findings:
  critical: 1
  warning: 7
  info: 4
  total: 12
status: issues_found
---

# Phase 65.1: Code Review Report

**Reviewed:** 2026-05-26
**Depth:** standard
**Files Reviewed:** 28
**Status:** issues_found

## Summary

Phase 65.1 is the remediation pass landing 14 plans against 8 BLOCKER + 4 WARNING findings raised against the Phase 65 long-lived-connection retirement. The remediations are, overall, soundly engineered: the `BorrowedConnection` newtype + AST guard combination at `src/ddl/read_ffi.rs` + `tests/no_long_lived_conn.rs` is a strong defence-in-depth design; the WR-02 error_buf ABI cascade is consistently applied across 18 registration sites; the `FileSystem`-direct YAML read closes the CR-01 SQL-injection surface cleanly; the structural verification tests for parser-hook idempotence and registration error surfaces are honest workarounds for the `duckdb/loadable-extension`-stub constraint and document the decision well.

That said, one defect in the WR-09 idempotence fix itself defeats the intent of the leak guard it introduces: every redundant `LOAD semantic_views` now leaks a fresh `Box<OverrideContext>` instead of (as Phase 65 did) a fresh `parser_extension` list entry. The leak is smaller but still unbounded across re-LOAD cycles, and the test suite cannot observe it because both leaks present identically to the user (no exception, no functional regression). Several other warnings concern null-pointer defensive gaps and unfailing fallbacks that the remediation overlooked.

## Critical Issues

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

**File:** `cpp/src/shim.cpp:2648-2729`
**Issue:** The Plan 12 WR-09 D-21 idempotence guard added at line 2693-2699 short-circuits the `ParserExtension` build + `Register` block when `already_registered == true`. However, the `rust_state = sv_make_override_context()` allocation happens UNCONDITIONALLY at line 2648 — _before_ the dedup check. The `SemanticViewsParserInfo` constructor (line 2720) is what takes ownership of `rust_state` and would later free it via `sv_drop_override_context` in the dtor. On the `already_registered` path that ctor never runs, the function returns `true` at line 2780, and `rust_state` is permanently leaked.

This precisely re-introduces the leak shape WR-09 was supposed to fix: instead of growing the parser_extensions list by one entry per redundant LOAD, we now grow the heap by one `Box<OverrideContext>` per redundant LOAD. The struct is empty today so the leak is small (≈ one allocation per repeat-LOAD), but it is unbounded over the lifetime of a long-lived host process — exactly the regression mode flagged by the original WR-09 finding.

Worse, the behavioural test `test/integration/test_load_extension_twice_idempotent.py` cannot catch this: it only asserts the second LOAD doesn't throw and that subsequent DDL still flows. The structural test `tests/parser_hook_idempotent.rs` Assertion 5 verifies that `new SemanticViewsParserInfo(...)` appears AFTER the guard — which is the wrong invariant; what matters is that `rust_state` is not allocated until after the dedup decision (or is reclaimed on the skip path).

**Fix:** Move the `sv_make_override_context()` allocation _into_ the `if (!already_registered)` block so it is only performed when ownership will actually be transferred:

```cpp
auto &cbmgr = config.GetCallbackManager();
bool already_registered = false;
for (auto &existing : cbmgr.ParserExtensions()) {
    if (existing.parser_override == sv_parser_override) {
        already_registered = true;
        break;
    }
}

if (!already_registered) {
    void *rust_state = sv_make_override_context();
    if (!rust_state) {
        if (error_buf != nullptr && error_buf_len > 0) {
            snprintf(error_buf, error_buf_len,
                "sv_register_parser_hooks: sv_make_override_context returned null");
        }
        return false;
    }

    ParserExtension ext;
    ext.parser_override = sv_parser_override;
    ext.parse_function  = sv_parse_stub;
    ext.plan_function   = sv_plan_unreachable;
    std::shared_ptr<ParserExtensionInfo> info_std(
        new SemanticViewsParserInfo(rust_state));
    ext.parser_info = duckdb::shared_ptr<ParserExtensionInfo>(info_std);
    ParserExtension::Register(config, ext);
    config.SetOption("allow_parser_override_extension", Value("FALLBACK"));
}
```

Then add an explicit assertion to `tests/parser_hook_idempotent.rs` that the `sv_make_override_context(` call site appears AFTER the `if (!already_registered) {` line in the function body — this is the assertion that prevents regression.

## Warnings

### WR-01: `sv_register_table_function` dereferences `arg_types` without null guard when `arg_count > 0`

**File:** `cpp/src/shim.cpp:631-633`
**Issue:** Inside the registration helper, the loop `for (size_t i = 0; i < arg_count; ++i) { args.push_back(arg_types[i]); }` dereferences `arg_types` for every `i`. The header docstring at `cpp/src/shim.hpp:107-109` notes "May be null when arg_count == 0", but there is no defensive check enforcing that contract. A buggy internal caller passing `arg_types == nullptr` with `arg_count > 0` would segfault inside the registration helper — far from the bug site. The D-05 / D-08 / D-09 hardening pattern applied to other input arguments (db_handle, name, init_cb null-checked at lines 610-617) is missing here.

**Fix:** Add the symmetric check inside the top-of-function null-guard block:

```cpp
if (arg_count > 0 && arg_types == nullptr) {
    write_err(
        "sv_register_table_function('%s'): arg_count > 0 but arg_types is null",
        name);
    return false;
}
```

Same fix applies to `sv_register_scalar_function` at lines 721-723. The sister `sv_register_explain_semantic_view_impl` and `sv_register_semantic_view_impl` construct `args` from a local stack literal so they're not affected.

### WR-02: `sv_semantic_view_function` silently truncates column-count mismatch instead of failing loud

**File:** `cpp/src/shim.cpp:2498`
**Issue:** The exec callback uses `idx_t n_cols = std::min<idx_t>(chunk->ColumnCount(), bd.columns.size());` to pick the safe column count to copy. The inline comment claims "we rely on the bind-time schema agreement," but if a bug ever produces a chunk with FEWER columns than declared (e.g. a future planner inserting a projection that elides a column), the loop only copies the first `n_cols` and the remaining `bd.columns.size() - n_cols` slots in the output `DataChunk` are left uninitialized. DuckDB's output Vector slots default-construct to a valid state in most cases but are not guaranteed to be NULL-marked — readers downstream could see garbage data for the unfilled columns.

The previous Rust path had a `Mutex + col_count` check that errored on mismatch (referenced in the comment at line 2497). The migration retired the explicit check on the grounds that the bind-time agreement is authoritative; this is fine for the happy path but loses the loud-fail diagnostic when something does go wrong.

**Fix:** Replace the `min` with an explicit mismatch error:

```cpp
if (chunk->ColumnCount() != bd.columns.size()) {
    throw InternalException(
        "semantic_view: column count mismatch — chunk has " +
        std::to_string(chunk->ColumnCount()) + " columns, bind declared " +
        std::to_string(bd.columns.size()) + " (please report this bug)");
}
idx_t n_cols = chunk->ColumnCount();
```

### WR-03: `sv_resolve_output_logical_types` comment claims ENUM needs probe but check at line 2329-2333 only matches DECIMAL and LIST

**File:** `cpp/src/shim.cpp:2326-2333, 2368`
**Issue:** The block comment at line 2325-2328 reads: "These three need logical-type metadata to declare correctly: DECIMAL needs width+scale; LIST needs child type; ENUM declares as VARCHAR but the LIMIT-0 confirms presence." The actual `needs_logical_probe` check at line 2329-2333 only includes `DUCKDB_TYPE_DECIMAL` and `DUCKDB_TYPE_LIST` — ENUM is omitted. Similarly the per-column resolution at line 2368 only special-cases DECIMAL and LIST.

This isn't necessarily a bug — `sv_logical_type_from_c_type_id` returns `LogicalType::VARCHAR` for ENUM, which is the documented intent ("ENUM declares as VARCHAR"). So the LIMIT-0 probe genuinely isn't needed for ENUM. But the comment then contradicts the code, which is a maintenance trap: a future contributor reading the comment may assume ENUM needs probe handling and add it incorrectly, or wonder why it's missing and "fix" it.

**Fix:** Update the comment to match reality — ENUM does NOT need a logical-type probe because it is declared as VARCHAR unconditionally:

```cpp
// These two need logical-type metadata to declare correctly: DECIMAL
// needs width+scale; LIST needs child type. ENUM is intentionally
// declared as VARCHAR via sv_logical_type_from_c_type_id (see that
// helper), so no probe is needed for it.
```

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

**File:** `src/parse.rs:365, 1102, 1303`
**Issue:** Each of these helpers walks the input as `&[u8]` and pushes characters into a result `String` via `result.push(bytes[pos] as char)`. The `as char` cast interprets each byte as a Unicode scalar value, which only works for ASCII (0x00..0x7F). A multi-byte UTF-8 sequence (e.g. a Cyrillic, Greek, or CJK character, an em-dash, a smart quote, an emoji) is split into its constituent bytes and each byte becomes a wrong codepoint (U+0080..U+00FF — the Latin-1 supplement region).

This bug bites every code path that extracts a single-quoted SQL literal from semantic-view DDL:

- `extract_quoted_string` is called by `SHOW SEMANTIC ... LIKE '<pattern>' STARTS WITH '<prefix>'` clause parsing and by `ALTER SEMANTIC VIEW v SET COMMENT = '<text>'`. Comments containing non-ASCII characters become silently mangled in the stored definition.
- `extract_view_comment` handles the inline `COMMENT = '<text>'` between view name and AS-body. Same corruption.
- `extract_single_quoted` parses the `FROM YAML FILE '<path>'` literal. A path containing non-ASCII bytes is corrupted before reaching the FileSystem — the user sees "file not found" instead of being able to load YAML from a Unicode path.

This is technically a **pre-existing** bug (the helpers predate Phase 65.1), but Phase 65.1 Plan 07's switch to the `FileSystem`-direct YAML read path means the corrupted bytes now go through `LocalFileSystem::OpenFile` instead of through `read_text()`, so any prior tolerance the Rust SQL-quoting layer offered is gone. Worth surfacing as part of this review because the failure surface changed shape.

**Fix:** Walk the input as `&str` chars instead of `&[u8]` bytes, or convert each accepted byte run via `std::str::from_utf8`. Example for `extract_quoted_string`:

```rust
fn extract_quoted_string(input: &str) -> Result<(String, usize), String> {
    let mut chars = input.char_indices();
    if !matches!(chars.next(), Some((_, '\''))) {
        return Err("Expected single-quoted string".to_string());
    }
    let mut result = String::new();
    while let Some((i, ch)) = chars.next() {
        if ch == '\'' {
            // Lookahead via the iterator clone for the doubled-quote case.
            let mut peek = chars.clone();
            if matches!(peek.next(), Some((_, '\''))) {
                result.push('\'');
                chars.next();
            } else {
                return Ok((result, i + 1));
            }
        } else {
            result.push(ch);
        }
    }
    Err("Unterminated single-quoted string".to_string())
}
```

Sibling fixes for `extract_view_comment` and `extract_single_quoted`. Add a proptest covering non-ASCII payloads round-tripping through CREATE/DESCRIBE/SHOW.

### WR-05: `parse_string_list` accepts trailing-bytes payloads silently in `src/query/explain.rs` mirror

**File:** `src/query/explain.rs:84-86` versus `src/query/table_function.rs:81-83`
**Issue:** Both Wave 5 (explain) and Wave 6 (semantic_view) implement a per-file `parse_string_list` helper that decodes the LIST(VARCHAR) wire format. The C++ side relies on these to be strict — `sv_serialise_string_list` writes a count-prefixed payload that consumes the entire buffer, so trailing bytes indicate FFI shape drift. Both helpers correctly check `if off != len { return None; }` at the very end, so this concern is closed. **However**, in `sv_explain_semantic_view_bind_rust` and `sv_semantic_view_bind_rust`, when `parse_string_list` returns `None` the error message is "`malformed dimensions payload`" with no detail (`src/query/explain.rs:157-159`, `src/query/table_function.rs:149-152`). The C++ side ALSO does trailing-byte checks (`sv_parse_varchar_payload` raises `BinderException` with "FFI buffer has trailing bytes (consumed N of M)"). The asymmetric diagnostic quality means an FFI-shape regression in one of these three dispatchers will be diagnosed but the user message will be unactionable for two of them.

**Fix:** Return a `Result<Vec<String>, String>` from `parse_string_list` (or thread an `&mut String` for the error) so the dispatcher can surface "expected u32 at offset N of M" or "trailing N bytes after count C" details. Cheap to do; matches the C++ helpers' diagnostic shape.

### WR-06: `sv_parser_override` discards Rust error on `Err` path, deferring to default parser without preserving original diagnostic

**File:** `src/parse.rs:2620-2632`
**Issue:** The success-path handler at line 2620 catches `Err(_err)` from `rewrite_to_native_sql` and returns rc=2 (defer to default parser). The comment explains the design: the registered `parse_function` (sv_parse_stub) will re-run validation and produce its own error with caret position. This works for deterministic validation paths (`detect_near_miss`, `validate_and_rewrite` syntax checks).

However, `rewrite_to_native_sql` can also fail in non-deterministic ways — e.g., the metadata-via-SQL `now()` rendering, JSON serialisation failures (`serde_json::to_string` errors), or the (unreachable but coded) "internal error" dispatch paths. If the second invocation in `sv_parse_function_rust` produces a DIFFERENT error than the first (because catalog state changed mid-call, or a transient panic-caught error was reproduced differently), the user sees the second error which may be less actionable than the first.

The original `_err` is shadowed and discarded — there is no log, no trace, no way for an operator to know that two different errors fired.

**Fix:** This is a defensive concern, not an active bug. The cleanest fix is to write the rendered `err.message` to `error_out` on rc=1 path (which is documented but currently unused per the Phase 62 design):

```rust
Err(err) => {
    if !error_out.is_null() && error_out_len > 0 {
        write_error_to_buffer(error_out, error_out_len, &err.message);
    }
    2 // defer to parse_function for caret
}
```

Then a future debug-build flag could surface both errors. At minimum, write the message into the buffer even if the C++ side currently ignores it (the buffer is heap-allocated and the cost is sub-microsecond) so the channel stays alive for future tooling.

### WR-07: Three Connection allocations per `semantic_view(...)` invocation when one is sufficient

**File:** `cpp/src/shim.cpp:2403, 2346, 2463`
**Issue:** `sv_semantic_view_bind` constructs `Connection probe(*context.db)` at line 2403, then calls `sv_resolve_output_logical_types` which constructs ANOTHER `Connection probe(*context.db)` at line 2346 when DECIMAL/LIST is present, then `sv_semantic_view_init_global` constructs a THIRD `Connection probe(*context.db)` at line 2463 to run the materialised query. Each ctor calls `ConnectionManager::AddConnection` (acquires `connections_lock`); each dtor releases it.

For a simple `SELECT * FROM semantic_view('v', metrics:=['cnt'])` with no DECIMAL/LIST columns this is 2 ctor/dtor pairs; with DECIMAL it becomes 3. The READ-PATH-SPIKE measured this at sub-millisecond per ctor, so the overhead is minimal — but the design implies a lifetime split between the bind connection's query result and a separate connection running the actual query, which is exactly the long-lived-handle pattern Phase 65 was retiring (just with a per-call lifecycle).

More importantly, the bind-time `probe` connection at line 2403 is used only for the FFI bridge to Rust (which itself opens the `probe.Query(limit0_sql)` separately at line 2348 in `sv_resolve_output_logical_types`). The Rust dispatcher accesses the catalog via the borrowed handle, but the `probe.Query(limit0_sql)` is a separate query on a separate Connection from the one Rust used to read the catalog. If the user's connection has uncommitted writes (e.g. they just did `CREATE TABLE` in the same transaction), `probe` does NOT see them — it's a fresh Connection on the DatabaseInstance.

**Fix:** This is a correctness/visibility concern, not just an efficiency one. Reuse a single per-bind Connection across all FFI dispatches and the LIMIT-0 probe — pass it down to `sv_resolve_output_logical_types` as a borrowed parameter instead of opening a fresh one. Document explicitly whether semantic_view bind-time queries are expected to see the caller's uncommitted writes; if yes, the per-call Connection on `*context.db` doesn't satisfy that contract and a deeper rework (binding to `context.client.GetCurrentTransaction()` somehow) is needed.

## Info

### IN-01: `sv_register_parser_hooks` runtime BORROW-contract probe runs `SELECT 1` before the helper-TF registration check

**File:** `cpp/src/shim.cpp:2630-2646`
**Issue:** The Plan 10 D-12 runtime probe at line 2630-2646 runs `duckdb_query(handle, "SELECT 1", &r)` BEFORE `sv_make_override_context()` — good ordering for the BORROW-contract check itself. However the probe runs on `db` even when the database is in a state where `SELECT 1` could surface a transient error (e.g. shutdown-in-progress). A transient probe failure refuses to LOAD the extension permanently for this DB.

Cost is bounded — `SELECT 1` is microseconds — and the failure mode is fail-closed (refuse to LOAD, which is correct). But the diagnostic message at line 2638-2641 always reads "bridge contract broken — duckdb_connection layout assumption failed at load time" even when the actual cause is something else (transient query failure, OOM, etc.). A non-bridge failure would mislead the diagnostic.

**Fix:** Inspect `duckdb_result_error` and include it in the diagnostic:

```cpp
if (rc != DuckDBSuccess) {
    const char *err_text = duckdb_result_error(&r);
    if (error_buf != nullptr && error_buf_len > 0) {
        snprintf(error_buf, error_buf_len,
            "bridge contract probe failed (duckdb_query SELECT 1 returned %d): %s. "
            "Possible causes: duckdb_connection ABI drift OR transient DB error at LOAD time.",
            (int)rc, err_text ? err_text : "(no detail)");
    }
    duckdb_destroy_result(&r);
    return false;
}
```

### IN-02: `sv_count_parser_extensions` returns `int32_t` but inner counter has no overflow guard

**File:** `cpp/src/shim.cpp:2834-2839`
**Issue:** The counter loop `int32_t count = 0; for (...) { if (...) ++count; }` could overflow if the parser_extensions list grew past 2^31-1 entries. In practice this is unreachable (the WR-09 dedup keeps the list bounded to 1), but the function's documented contract is "returns >= 0 on success; -1 on failure" — overflow would wrap to negative and the structural test would incorrectly interpret it as a registration failure.

**Fix:** Either use `uint32_t` (and document the sentinel differently) or saturate at INT32_MAX. Low priority — only a concern if WR-09 itself fails badly.

### IN-03: `tests/no_long_lived_conn.rs` `DisconnectFinder` allow-list does not validate the allowed path actually exists

**File:** `tests/no_long_lived_conn.rs:159-164`
**Issue:** `ALLOWED_DISCONNECT_SITES` lists `("src/lib.rs", "ffi::duckdb_disconnect")` as the canonical exception (the `RawDb::Drop` test fixture). The filter at line 227-231 drops matches whose `(file, path)` tuple matches an allow-list entry. However:

1. If the allow-listed call site is ever DELETED (e.g. the `RawDb` helper is moved out of `src/lib.rs`), the allow-list entry becomes stale — the test still passes (no matches to filter) but the stale entry silently weakens the guard for any future code legitimately added at that path. There's no "this allow-list entry matched zero call sites" warning.
2. The allow-list does not verify that the allowed call site is wrapped in a properly-paired `duckdb_open` + `duckdb_connect` + `duckdb_close` ownership pattern. Anyone adding `ffi::duckdb_disconnect(...)` to `src/lib.rs` (for any reason) gets a free pass.

**Fix:** Track which allow-list entries matched and assert at the end that every entry was used at least once:

```rust
let mut matched = vec![false; ALLOWED_DISCONNECT_SITES.len()];
finder.found_call_sites.retain(|(file, path)| {
    for (idx, (allowed_file, allowed_path)) in ALLOWED_DISCONNECT_SITES.iter().enumerate() {
        if file == allowed_file && path == allowed_path {
            matched[idx] = true;
            return false;
        }
    }
    true
});
for (idx, (file, path)) in ALLOWED_DISCONNECT_SITES.iter().enumerate() {
    assert!(matched[idx], "ALLOWED_DISCONNECT_SITES entry ({file:?}, {path:?}) matched zero call sites — remove the stale exception");
}
```

### IN-04: `sv_create_from_yaml_bind` reads file size into `int64_t size` and casts to `size_t` without overflow check

**File:** `cpp/src/shim.cpp:816-829`
**Issue:** `fs.GetFileSize(*handle)` returns `int64_t size`; the code checks `size < 0` (good) but then casts to `static_cast<size_t>(size)` for the string allocation. On a 32-bit target (theoretical — DuckDB requires 64-bit, but the extension may eventually port) a 5 GB file size could overflow `size_t`. On 64-bit this is safe.

The Rust side enforces a 1 MiB cap via `from_yaml_with_size_cap`, but the C++ side does NOT pre-check — a 10 GB YAML file would allocate 10 GB of `std::string` storage, run the read, hand it to Rust, and only then get rejected. OOM kill is the practical outcome on a host with insufficient memory.

**Fix:** Pre-check `size` against a generous cap (e.g. 16 MiB, well above the Rust 1 MiB cap but bounded enough to fail-fast on rogue inputs):

```cpp
constexpr int64_t MAX_YAML_BYTES = 16 * 1024 * 1024;
if (size > MAX_YAML_BYTES) {
    throw BinderException(
        "FROM YAML FILE failed: file '" + bd->file_path + "' is " +
        std::to_string(size) + " bytes (max " +
        std::to_string(MAX_YAML_BYTES) + ")");
}
```

This stays loose enough not to duplicate the Rust cap (single source of truth there) but tight enough to prevent an OOM on a hostile path.

---

_Reviewed: 2026-05-26_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_
